feat(executioncontext): warn on nested js handle #3591
Conversation
ExecutionContext.evaluateHandle accepts arguments that are either serializable, or JSHandles. A potential confusion is that it *does not* accept arguments that *contain* JSHandles. This patch adds a log message warning when it encounters that situation. Fixes puppeteer#3562
|
I don't know if that's a good thing to do, if anything this should re-throw as another error, emitting warnings on incorrect usage that should result in errors doesn't make much sense. I think it would be sufficient to alter the error message eg. |
|
I like @SimonSchick suggestion - that's the right amount of code/effort for the problem we're having. |
|
Sounds good, I'll make that change and push an update |
remove the code checking if args contain a nested JSHandle, and instead simply suggest the possibility.
There was a problem hiding this comment.
This is better but I'm still unsure if this sort of validation is the right way to do at all.
There are probably hundreds possible ways to pass bad data into puppeteer and make it give obscure errors, that's (to some degree) due to the nature of JS.
If you want actual type safety, you want to go with something like TypeScript.
lib/ExecutionContext.js
Outdated
| userGesture: true | ||
| }); | ||
| } catch (err) { | ||
| err.message += ' Are you passing a nested JSHandle?'; |
There was a problem hiding this comment.
You do want to check if it's a TypeError still.
There was a problem hiding this comment.
Ah, right. I'll add that back.
|
@SimonSchick, I'd absolutely prefer to lean on the type system for this. I actually am using Typescript, but I don't get an error in this case. Looking at evaluateHandle(
fn: EvaluateFn,
...args: any[]
): Promise<JSHandle>;Even though args is documented as being I'm pretty new to Typescript, but I'm not aware of a way to specify that arguments be serializable. Maybe you can point me in the right direction? Is there a built-in type I'm missing, or would it involve a definition something like this? type Serializable = number | boolean | string | { [key: string]: Serializable } | Serializable[]; |
| userGesture: true | ||
| }); | ||
| } catch (err) { | ||
| if (err instanceof TypeError && err.message === 'Converting circular structure to JSON') |
There was a problem hiding this comment.
this error happens due to convertArgument call. Let's scope it down to this:
try {
args = args.map(convertArgument.bind(this));
} catch (err) {
if (err instanceof TypeError && err.message === 'Converting circular structure to JSON')
err.message += ' Are you passing a nested JSHandle?';
throw err;
}There was a problem hiding this comment.
Unfortunately, the error actually happens after convertArgument, in CDPSession.send. It looks like convertArgument prepares the args for serialization, but the call to JSON.stringify is later on.
lib/ExecutionContext.js
Outdated
| @@ -162,6 +169,7 @@ class ExecutionContext { | |||
| throw new Error('Execution context was destroyed, most likely because of a navigation.'); | |||
| throw error; | |||
| } | |||
|
|
|||
Test that the error thrown includes the altered message
|
As far as I can tell, test failures are either unrelated ( Also, I learned how to write a "Serializable" type, so I'm intending to submit a PR to DefinitelyTyped as well. |
aslushnikov
changed the title
page.evaluate and similar functions are currently typed as if they accept 'any' args, but they will throw exceptions unless the args are either Serializable (with JSON.stringify) or JSHandles. In puppeteer/puppeteer#3591, @SimonSchick recommended that tighter type definitions was a preferable way to address this rather than runtime error checking. I've been using these updated types in my project for the last couple of months without problem. I suspect that some of the X1, X2 type parameters should also be changed to reflect that they can only be SerializableOrJSHandle, but I wasn't sure how to change the existing UnwrapElementHandle<X1> types.
ExecutionContext.evaluateHandle accepts arguments that are either
serializable, or JSHandles. A potential confusion is that it does not
accept arguments that contain JSHandles.
This patch adds a log message warning when it encounters that situation.
Fixes #3562