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): add (and fix) `evaluateHandle` types #6130

Merged
merged 1 commit into from Jul 1, 2020

Conversation

@jackfranklin
Copy link
Collaborator

jackfranklin commented Jun 30, 2020

This change started as a small change to pull types from DefinitelyTyped over to
Puppeteer for the evaluateHandle function but instead ended up also fixing
what looks to be a long standing issue with our existing documentation.
evaluateHandle can in fact return an ElementHandle rather than a JSElement
if you return something that is an HTML element:

const button = page.evaluateHandle(() => document.querySelector('button'));
// this is an ElementHandle, not a JSHandle

Therefore I've updated the original docs and added a large explanation to the
TSDoc for page.evaluateHandle.

In TypeScript land we'll assume the function will return a JSHandle but you
can tell TS otherwise via the generic argument, which can only be JSHandle
(the default) or ElementHandle:

const button = page.evaluateHandle<ElementHandle>(() => document.querySelector('button'));

This is part of #6124

@googlebot googlebot added the cla: yes label Jun 30, 2020
@jackfranklin jackfranklin requested review from hanselfmu and mathiasbynens Jun 30, 2020
@jackfranklin
Copy link
Collaborator Author

jackfranklin commented Jun 30, 2020

@hanselfmu heads up that this touches Page.ts - just in case you have any ongoing TSDoc work in that file.

// and should use evaluate<ElementHandle> or if the type of evaluateHandle
// should change to enable the user to tell us they are expecting an
// ElementHandle rather than the default JSHandle.
await (buttonHandle as ElementHandle).click();

This comment has been minimized.

Copy link
@jackfranklin

jackfranklin Jun 30, 2020

Author Collaborator

it's great that this PR enables this big TODO to go!

@mathiasbynens
Copy link
Member

mathiasbynens commented Jun 30, 2020

Thanks for doing this 👍 The docs you put together are much better!

Let’s be precise when describing this change: ElementHandle extends JSHandle, so it’s not “not a JSHandle”.

docs/api.md Outdated

The only difference between `worker.evaluate` and `worker.evaluateHandle` is that `worker.evaluateHandle` returns in-page object (JSHandle).

If the function passed to the `worker.evaluateHandle` returns a [Promise], then `worker.evaluateHandle` would wait for the promise to resolve and return its value.

If the function returns an element, the returned handle will be of type [ElementHandle].

This comment has been minimized.

Copy link
@mathiasbynens

mathiasbynens Jun 30, 2020

Member

Stoyan once taught me this trick, sharing in case you like it too: https://twitter.com/mathias/status/839160485324947456

If so, will beis throughout the doc

This comment has been minimized.

Copy link
@jackfranklin

jackfranklin Jun 30, 2020

Author Collaborator

Nice, updated.

@jackfranklin jackfranklin force-pushed the evaluate-handle-types branch 2 times, most recently from bde11f7 to 52422aa Jun 30, 2020
@jackfranklin
Copy link
Collaborator Author

jackfranklin commented Jun 30, 2020

I've updated the commit message to clarify that ElementHandle extends JSHandle. So the original docs were technically correct, if not a little misleading / lacking clarity.

@jackfranklin jackfranklin force-pushed the evaluate-handle-types branch 3 times, most recently from e58dcc1 to 3f585b5 Jun 30, 2020
This change started as a small change to pull types from DefinitelyTyped over to
Puppeteer for the `evaluateHandle` function but instead ended up also fixing
what looks to be a long standing issue with our existing documentation.

`evaluateHandle` can in fact return an `ElementHandle` rather than a `JSHandle`.
Note that `ElementHandle` extends `JSHandle` so whilst the docs are technically
correct (all ElementHandles are JSHandles) it's confusing because JSHandles
don't have methods like `click` on them, but ElementHandles do.

if you return something that is an HTML element:

```
const button = page.evaluateHandle(() => document.querySelector('button'));
// this is an ElementHandle, not a JSHandle
```

Therefore I've updated the original docs and added a large explanation to the
TSDoc for `page.evaluateHandle`.

In TypeScript land we'll assume the function will return a `JSHandle` but you
can tell TS otherwise via the generic argument, which can only be `JSHandle`
(the default) or `ElementHandle`:

```
const button = page.evaluateHandle<ElementHandle>(() => document.querySelector('button'));
```
@jackfranklin jackfranklin force-pushed the evaluate-handle-types branch from 3f585b5 to 9c9a59e Jul 1, 2020
@jackfranklin jackfranklin merged commit 8370ec8 into main Jul 1, 2020
2 checks passed
2 checks passed
Travis CI - Pull Request Build Passed
Details
cla/google All necessary CLAs are signed
@jackfranklin jackfranklin deleted the evaluate-handle-types branch Jul 1, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.