puppeteer: safer args for page.eval and friends #32492
Conversation
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.
typescript-bot
added
Popular package
|
@bgschiller Thank you for submitting this PR! 🔔 @marvinhagemeister @cdeutsch @ksm2 @SimonSchick - please review this PR in the next few days. Be sure to explicitly select If no reviewer appears after a week, a DefinitelyTyped maintainer will review the PR instead. |
| @@ -106,7 +119,7 @@ export interface Evalable { | |||
| $eval<R>( | |||
| selector: string, | |||
| pageFunction: (element: Element, ...args: any[]) => R | Promise<R>, | |||
There was a problem hiding this comment.
args here should have value as well.
There was a problem hiding this comment.
Or rather, the same type but without JSHandle and Element instead.
There was a problem hiding this comment.
Hmm, I thought about that, but JSHandles are not only handles for Elements, right? I could pass a JSHandle to a Regex, and then my pageFunction would be given the regex itself. Is that right?
typescript-bot
moved this from Waiting for Reviewers
to Review
in Pull Request Status Board
typescript-bot
added
the
Unmerged
|
After 5 days, no one has reviewed the PR 😞. A maintainer will be reviewing the PR in the next few days and will either merge it or request revisions. Thank you for your patience! |
| export interface JSONObject { | ||
| [key: string]: Serializable; | ||
| } | ||
| export type SerializableOrJSHandle = Serializable | JSHandle; |
There was a problem hiding this comment.
It looks like you want to catch circular references but this actually won't do that. Here is an example.
There was a problem hiding this comment.
Catching circular references would be great, but I'm not sure if the type system is actually powerful enough for that (aka, I don't know how to do it).
The goal of this Serializable is more to warn when an arg will not be correctly translated to JSON and back. For example, RegExp, Date, Function, etc.
The original impetus for this change was when I tried to pass an options object like { elem: JSHandle } as an arg. This contains a JSHandle, so I thought it would work. But it actually fails because it's neither Serializable nor a JSHandle.
More info on exactly why JSHandles are allowed, but not nested JSHandles is in the source near this edit: puppeteer/puppeteer#3591
There was a problem hiding this comment.
Ahh ok I misread the linked github thread.
|
Congratulations on your first DefinitelyTyped contribution! Thank you for being a part of the community! |
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
types.
Please fill in this template.
npm test.)npm run lint package-name(ortscif notslint.jsonis present).Select one of these and delete the others:
If changing an existing definition:
tslint.jsoncontaining{ "extends": "dtslint/dt.json" }.