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
chore: migrate src/ExecutionContext #5705
Conversation
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'}); |
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.
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.
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.
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?
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.
@mathiasbynens this isn't a TypeScript bug, but fine, yes we can.
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.
(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 :])
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.
keyword there is "arguably" ;) 😂
baeb841
to
9ef96ad
Compare
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 @mathiasbynens do we support taking a string argument as the function for |
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.
9ef96ad
to
7b4f31c
Compare
@jackfranklin 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. |
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:
And have TypeScript know the arguments of
node
andx
based on thosegenerics. But I hit two problems with that:
evaluate
to cope for as many numberof arguments as you can be bothered too (e.g. we'd need an overload for
1 arg, 2 args, 3 args, etc)
what those generics actually map to.
So in the end I went with one generic which is the return type of the
function:
And
node
andx
get typed asany
which means you can tell TSyourself:
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 behonest 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.