New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(types): improve typing of .evaluate()
#6096
Conversation
export interface JSONObject { | ||
[key: string]: Serializable; | ||
} | ||
export type SerializableOrJSHandle = Serializable | JSHandle; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
these are taken directly from the DefinitelyTyped repository. Do we need to acknowledge that / credit back?
@marvinhagemeister would you mind having a look if you have time, please? There's much more to do with the types but this is the first step. I'm open to any suggestions or changes to the types you'd like to make. |
(The build will fail until the docs are updated but I'm going to do that just before merging to keep the diff clean in the interim) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The changes look good to me 👍
It would be sure nice to not remove credits. Probably hard to do on a file by file basis. Maybe a small section on the README that acknowledges that the types are taken from DT and mentions the authors, is a way to do that globally?
I like this idea - very keen to acknowledge all the work that has gone in thus far. @mathiasbynens wdyt? |
08d2a0d
to
4271eca
Compare
All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the ℹ️ Googlers: Go here for more info. |
This is the start of the work to take the types from the `@types/puppeteer` repository and port them into our repo so we can ship our built-in types out the box. This change types the `evaluate` function properly. It takes a generic type which is the type of the function you're passing, and the arguments and the return that you get back from the `evaluate` call are typed correctly.
👍 Sounds good. |
4271eca
to
e9f2794
Compare
CLAs look good, thanks! ℹ️ Googlers: Go here for more info. |
* @public | ||
*/ | ||
export interface JSONObject { | ||
[key: string]: Serializable; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know these come from DT, but I honestly don't think these are good typings. This attempts to enforce that the passed argument must be deep-serializable, but that is impossible to express in TypeScript: microsoft/TypeScript#1897
What this line here declares is not "accept any object where all values have this type" (that is inherently impossible with structural typing), but "accept only objects with an index signature of this type". This means that if you pass an object typed with an interface it could be perfectly serializable but TypeScript will still fail with "Type is missing index signature", which makes these types less ergonomic too use for little benefit.
pageFunction: Function | string, | ||
...args: unknown[] | ||
pageFunction: EvaluateFn | string, | ||
...args: SerializableOrJSHandle[] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What would be really helpful here (more than trying to constrain the serializable types) would be to make the arguments and return type type-safe, like this:
$eval<A extends any[], R>(pageFunction: (...args: A) => R, ...args: A): Promise<R>
This is the start of the work to take the types from the
@types/puppeteer
repository and port them into our repo so we can shipour built-in types out the box.
This change types the
evaluate
function properly. It takes a generictype which is the type of the function you're passing, and the arguments
and the return that you get back from the
evaluate
call are typedcorrectly.