feature(Page): evaluate should return a special message in case of non serialisable returned value

[yanivefraim]

This PR is a followup for the following discussion: #2418 (comment)

I wasn't sure which message do we want for each scenario, and how exactly do we want to implement the entire feature (if we want it at all...). So it is a start - we can discuss it from here.

it will fix #2420

[benjamingr]

I don't think returning a predefined string is a good way to do this - puppeteer should probably reject on that.

[yanivefraim]

I don't think returning a predefined string is a good way to do this - puppeteer should probably reject on that.

@benjamingr - yes, we already had this discussion here. I also agree an error is better here

[aslushnikov]

I don't think returning a predefined string is a good way to do this - puppeteer should probably reject on that.

@benjamingr We were trying to reject before and got a pushback from our clients. Users shouldn't be afraid to evaluate any expression, regardless of its return value.

What would be the usecase to justify this change?

• There's always a page.evaluateHandle that returns a handle for the object, if needed.
• The "serializability" of the object could be checked easily, trying to JSON.stringify it manually.

cc @Janpot who had thoughts on this topic as well.

[aslushnikov]

instead of returning a string, this should be a Symbol exposed on the puppeteer namespace. This way you'll be able to do the following:

const result = await page.evaluate(() => window);
if (result === puppeteer.NON_SERIALIZABLE_OBJECT) {
  // ...
}

[Janpot]

who had thoughts on this topic as well.

w.r.t. API design, I have to agree with @benjamingr. But @aslushnikov explained to me that the current approach was a pragmatic choice as many users seem to want to do things like:

await page.evaluate(() => window.foo = window);

Personally, I have no idea what's the use-case behind that code, but I think we could educate people to instead do:

await page.evaluate(() => {
  window.foo = window;
});

Maybe the error message can include a link to a documentation page on github that explains this pattern?
I know my team has ran into bugs caused by the current behavior more than once. All in all, this approach solves it for us and if puppeteer devs want to take the pragmatic approach, I have no strong objections against that. We can always wrap the evaluate call by one that takes a, for us, more sensible approach.

[aslushnikov]

I know my team has ran into bugs caused by the current behavior more than once.

@Janpot can you please give a few examples?

[benjamingr]

the current approach was a pragmatic choice as many users seem to want to do things like:

That's an interesting point. In general I think seralization could use some love.

[Janpot]

can you please give a few examples?

We were running an external library in .evaluate that under some circumstances returned a nested object that had a cyclic reference somewhere.

await page.evaluate(() => {
  const obj = {};
  obj.a = obj;
  return obj;
});
// undefined

I think it's very reasonable to assume .evaluate either returns a result or throws an error. But there's a class of errors that are magically encoded in a third state, being undefined. There is also the case that undefined might be an expected result of .evaluate and then there is no way to differentiate between the legit undefined and the one that denotes an error.

And then there was also this issue #2021.

I think APIs should be intuitive and having magical third states is not intuitive I think.

[benjamingr]

@Janpot just so we're clear - there is no hard limitation meaning puppeteer can't deal with returning recursive references, object methods, complex values (like buffers) or other things - only the fact the code for it hasn't been written in

I suggest we fix that instead of the error messages

[Janpot]

I am familiar with the limitations 🙂. I only proposed the error message as a compromise since there seems to be concern users won't understand why their code is broken.
If it were my choice, the evaluate API would either, resolve with the expected result, or reject when it can't, including unserializable values.

[aslushnikov]

I think it's very reasonable to assume .evaluate either returns a result or throws an error. But there's a class of errors that are magically encoded in a third state, being undefined. There is also the case that undefined might be an expected result of .evaluate and then there is no way to differentiate between the legit undefined and the one that denotes an error.

This ambiguity in evaluation results is indeed uncomfortable; throwing an error would be more logical here.

Postponing this until 2.0 since this will be a breaking change for our clients.
Please, follow #2079 for the future updates.