fix(evaluate): throw error when page reloads during page.evaluate. #2073
Conversation
| a.b = b; | ||
| return a; | ||
| }); | ||
| expect(result).toBe(undefined); |
There was a problem hiding this comment.
@aslushnikov Is there any reason on why this should return undefined? I think throwing an error would be more appropriate. Think of the consumer of this API. They will not realise their result is circular, or it might happen only in some occasions. Or maybe sometimes undefined is a legit output of their function. How will they be able to distinguish these cases from a bug? How will they be notified of bugs in their code?
There was a problem hiding this comment.
@Janpot the motivation behind this was to let people use page.evaluate safely without caring on the return value. Consider the following simple script:
await page.evaluate(() => window.foo = window);Technically, the page.evaluate should throw since the expression returns non-serializable value,
but most of the time its unintentional.
How will they be able to distinguish these cases from a bug? How will they be notified of bugs in their code?
I filed #2079 with a solution that will hopefully address your concerns and satisfy our usecase.
There was a problem hiding this comment.
@aslushnikov How did you arrive at this conclusion: but most of the time its unintentional.?
In my experience it's exactly the opposite, I've never had to evaluate and not care about the result.
There was a problem hiding this comment.
How did you arrive at this conclusion: but most of the time its unintentional.?
@Janpot once we started throwing errors, we instantly had a bunch of feedback on both bugtracker and internally about the unwanted errors caused by one-liners in page.evaluate.
There was a problem hiding this comment.
Ok, I don't really agree with API design like this but I see you want to be sort of pragmatic, and I don't have arguments against that that would actually change your mind 🙂. I'll settle for
async function evaluate (page, ...args) {
const result = await page.evaluate(...args);
if (result === puppeteer.UNSERIALIZABLE_VALUE) {
throw new Error('Unserializable value');
}
retun result;
}then.
Fixes #2021.