Skip to content
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

Merged
merged 5 commits into from Jun 25, 2020
Merged

Conversation

jackfranklin
Copy link
Collaborator

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.

export interface JSONObject {
[key: string]: Serializable;
}
export type SerializableOrJSHandle = Serializable | JSHandle;
Copy link
Collaborator Author

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?

@jackfranklin
Copy link
Collaborator Author

@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.

@jackfranklin
Copy link
Collaborator Author

(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)

Copy link
Contributor

@marvinhagemeister marvinhagemeister left a 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?

@jackfranklin
Copy link
Collaborator Author

It would be sure nice to not remove credits. Probably hard to do on a file by file bases. 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?

@googlebot
Copy link

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 @googlebot I consent. in this pull request.

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 cla label to yes (if enabled on your project).

ℹ️ 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.
@mathiasbynens
Copy link
Member

It would be sure nice to not remove credits. Probably hard to do on a file by file bases. 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?

👍 Sounds good.

@googlebot
Copy link

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

@jackfranklin jackfranklin merged commit 46fc6ca into main Jun 25, 2020
@jackfranklin jackfranklin deleted the better-evaluate-types branch June 25, 2020 12:38
* @public
*/
export interface JSONObject {
[key: string]: Serializable;

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[]

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>

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants