chore: migrate src/ExecutionContext #5705

jackfranklin · 2020-04-21T14:30:31Z

I spent a while trying to decide on the best course of action for
typing the evaluate function.

Ideally I wanted to use generics so that as a user you could type
something like:

handle.evaluate<HTMLElement, number, boolean>((node, x) => true, 5)

And have TypeScript know the arguments of node and x based on those
generics. But I hit two problems with that:

you have to have n overloads of evaluate to cope for as many number
of arguments as you can be bothered too (e.g. we'd need an overload for
1 arg, 2 args, 3 args, etc)
I decided it's actually confusing because you don't know as a user
what those generics actually map to.

So in the end I went with one generic which is the return type of the
function:

handle.evaluate<boolean>((node, x) => true, 5)

And node and x get typed as any which means you can tell TS
yourself:

handle.evaluate<boolean>((node: HTMLElement, x:  number) => true, 5)

I'd like to find a way to force that the arguments after the function do
match the arguments you've given (in the above example, TS would moan if
I swapped that 5 for "foo"), but I tried a few things and to be
honest the complexity of the types wasn't worth it, I don't think.

I'm very open to tweaking these but I'd rather ship this and tweak going
forwards rather than spend hours now tweaking. Once we ship these
typedefs and get feedback from the community I'm sure we can improve
them.

jackfranklin · 2020-04-21T14:32:43Z

src/JSHandle.ts

      if (!element.isConnected)
        return 'Node is detached from document';
      if (element.nodeType !== Node.ELEMENT_NODE)
        return 'Node is not of type HTMLElement';
      // force-scroll if page's javascript is disabled.
      if (!pageJavascriptEnabled) {
-        element.scrollIntoView({block: 'center', inline: 'center', behavior: 'instant'});


This is interesting: TS shouts here because I think instant was going to be in the spec but now isn't? So I've ditched this in favour of auto which is the default. @mathiasbynens WDYT? I'm not sure if Chrome does still support instant but if it's going/gone from the spec presumably we'll stop supporting it one day.

Woah, this is a breaking change. We shouldn't work around TypeScript bugs by changing observable behavior in Puppeteer. Chrome still supports behavior: 'instant'. Can we make TypeScript ignore this particular line?

@mathiasbynens this isn't a TypeScript bug, but fine, yes we can.

(It’s arguably a bug: it doesn’t match real-world browser behavior. In general, when the spec and implementations disagree, then implementations take precedence, IMHO. This is a separate discussion though :])

keyword there is "arguably" ;) 😂

jackfranklin · 2020-04-21T14:49:49Z

I also had a look at https://github.com/DefinitelyTyped/DefinitelyTyped/blob/master/types/puppeteer/index.d.ts which do some smart things to get good type support for $eval and the like and I think we can use that as inspiration to vastly improve upon the types in this PR. @mathiasbynens I'd like to merge this as the first step and then follow up with a PR that improves the types a bunch if that's OK with you? Else this PR will grow a lot.

@mathiasbynens do we support taking a string argument as the function for evaluate() ? Some of the JSDoc comments say we do but I can't find an example so I'm wondering if that's correct or not. If we didn't have to type pageFunction as Function | string that would help with better TypeScript types.

I spent a while trying to decide on the best course of action for typing the `evaluate` function. Ideally I wanted to use generics so that as a user you could type something like: ``` handle.evaluate<HTMLElement, number, boolean>((node, x) => true, 5) ``` And have TypeScript know the arguments of `node` and `x` based on those generics. But I hit two problems with that: * you have to have n overloads of `evaluate` to cope for as many number of arguments as you can be bothered too (e.g. we'd need an overload for 1 arg, 2 args, 3 args, etc) * I decided it's actually confusing because you don't know as a user what those generics actually map to. So in the end I went with one generic which is the return type of the function: ``` handle.evaluate<boolean>((node, x) => true, 5) ``` And `node` and `x` get typed as `any` which means you can tell TS yourself: ``` handle.evaluate<boolean>((node: HTMLElement, x: number) => true, 5) ``` I'd like to find a way to force that the arguments after the function do match the arguments you've given (in the above example, TS would moan if I swapped that `5` for `"foo"`), but I tried a few things and to be honest the complexity of the types wasn't worth it, I don't think. I'm very open to tweaking these but I'd rather ship this and tweak going forwards rather than spend hours now tweaking. Once we ship these typedefs and get feedback from the community I'm sure we can improve them.

mathiasbynens · 2020-04-22T08:43:34Z

@jackfranklin We support it but I don’t think the API docs capture that (and we probably shouldn't recommend that pattern).

jackfranklin · 2020-04-22T08:45:54Z

We support it but I don’t think the API docs capture that (and we probably shouldn't recommend that pattern).

Great - I'll continue to support it in TS land. Maybe one to consider deprecating in future releases.

googlebot added the cla: yes label Apr 21, 2020

jackfranklin commented Apr 21, 2020

View reviewed changes

jackfranklin force-pushed the migrate-execution-context branch from baeb841 to 9ef96ad Compare April 21, 2020 14:45

jackfranklin requested a review from mathiasbynens April 21, 2020 14:45

jackfranklin added the typescript-migration label Apr 21, 2020

jackfranklin added this to the TypeScript migration milestone Apr 21, 2020

jackfranklin changed the title ~~[WIP] chore: migrate src/ExecutionContext~~ chore: migrate src/ExecutionContext Apr 21, 2020

jackfranklin force-pushed the migrate-execution-context branch from 9ef96ad to 7b4f31c Compare April 22, 2020 08:37

whoops

f863119

linting

fd69197

mathiasbynens approved these changes Apr 22, 2020

View reviewed changes

jackfranklin merged commit 29ebd0b into master Apr 22, 2020

jackfranklin deleted the migrate-execution-context branch April 22, 2020 09:33

snyk-bot mentioned this pull request Apr 25, 2020

[Snyk] Upgrade puppeteer from 3.0.0 to 3.0.1 patheard/cucumber-puppeteer#143

Merged

This was referenced May 3, 2020

[Snyk] Upgrade puppeteer from 3.0.1 to 3.0.2 patheard/cucumber-puppeteer#152

Merged

[Snyk] Upgrade puppeteer from 3.0.1 to 3.0.2 CollageLabs/collagelabs.org#131

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: migrate src/ExecutionContext #5705

chore: migrate src/ExecutionContext #5705

jackfranklin commented Apr 21, 2020 •

edited

jackfranklin Apr 21, 2020

mathiasbynens Apr 21, 2020

jackfranklin Apr 22, 2020

mathiasbynens Apr 22, 2020

jackfranklin Apr 22, 2020

jackfranklin commented Apr 21, 2020 •

edited

mathiasbynens commented Apr 22, 2020

jackfranklin commented Apr 22, 2020

chore: migrate src/ExecutionContext #5705

chore: migrate src/ExecutionContext #5705

Conversation

jackfranklin commented Apr 21, 2020 • edited

jackfranklin Apr 21, 2020

Choose a reason for hiding this comment

mathiasbynens Apr 21, 2020

Choose a reason for hiding this comment

jackfranklin Apr 22, 2020

Choose a reason for hiding this comment

mathiasbynens Apr 22, 2020

Choose a reason for hiding this comment

jackfranklin Apr 22, 2020

Choose a reason for hiding this comment

jackfranklin commented Apr 21, 2020 • edited

mathiasbynens commented Apr 22, 2020

jackfranklin commented Apr 22, 2020

jackfranklin commented Apr 21, 2020 •

edited

jackfranklin commented Apr 21, 2020 •

edited