chore: improve type inference of evaluate (#7267)

This commit updates the JSHandle class to take a generic representing
the underlying object that it's wrapping. We can then define
`ElementHandle` as a class that extends `JSHandle<Element>` and begin
to get better type inference.

Prior to this commit the following code would have `d` set to `any`:

```
const div: page.$<HTMLDivElement>('div')
const text = await div.evaluate(d => d.innerText)
```

You could work around this in two ways:

```
const text = await div.evaluate<(d: HTMLDivElement) => string>(d => d.innerText)
const text = await div.evaluate((d: HTMLDivElement) => d.innerText)
```

But both of these have two issues:

1. Requires the user to type extra information.
2. There's no type checking: in the code above I could type `d` as
   `number` and TS would be happy.

With the change here to `evaluate` the user can now type the original
code:

```
const div: page.$<HTMLDivElement>('div')
const text = await div.evaluate(d => d.innerText)
```

And TypeScript will know that `d` is an `HTMLDivElement`.

This change brings us inline with the approach that @types/puppeteer
takes. If we land this and it works, we can do the same with
`evaluateHandle` to hopefully make a similar improvement there.

BREAKING: because this changes the types, which were previously `any`,
this is technically a breaking change as users using TS could start
getting errors after this change is released.
This commit is contained in:
Jack Franklin 2021-05-26 14:46:17 +01:00 committed by GitHub
parent 457651d0a3
commit ea2b0d1f62
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
12 changed files with 92 additions and 36 deletions

View File

@ -10,6 +10,10 @@ const EXPECTED_ERRORS = new Map<string, string[]>([
"bad.ts(6,35): error TS2551: Property 'launh' does not exist on type",
"bad.ts(8,29): error TS2551: Property 'devics' does not exist on type",
'bad.ts(12,39): error TS2554: Expected 0 arguments, but got 1.',
"bad.ts(20,5): error TS2345: Argument of type '(divElem: number) => any' is not assignable to parameter of type 'EvaluateFn<HTMLAnchorElement>",
"bad.ts(20,34): error TS2339: Property 'innerText' does not exist on type 'number'.",
"bad.ts(24,45): error TS2344: Type '(x: number) => string' does not satisfy the constraint 'EvaluateFn<HTMLAnchorElement>'.",
"bad.ts(27,34): error TS2339: Property 'innerText' does not exist on type 'number'.",
],
],
[
@ -35,6 +39,8 @@ const EXPECTED_ERRORS = new Map<string, string[]>([
"bad.js(7,29): error TS2551: Property 'devics' does not exist on type",
'bad.js(11,39): error TS2554: Expected 0 arguments, but got 1.',
"bad.js(15,9): error TS2322: Type 'ElementHandle<HTMLElement> | null' is not assignable to type 'ElementHandle<HTMLElement>'",
"bad.js(22,5): error TS2345: Argument of type '(divElem: number) => any' is not assignable to parameter of type 'EvaluateFn<HTMLElement>'.",
"bad.js(22,26): error TS2339: Property 'innerText' does not exist on type 'number'.",
],
],
[
@ -67,6 +73,13 @@ const EXPECTED_ERRORS = new Map<string, string[]>([
]);
const PROJECT_FOLDERS = [...EXPECTED_ERRORS.keys()];
if (!process.env.CI) {
console.log(`IMPORTANT: this script assumes you have compiled Puppeteer
and its types file before running. Make sure you have run:
=> npm run tsc && npm run generate-d-ts
before executing this script locally.`);
}
function packPuppeteer() {
console.log('Packing Puppeteer');
const result = spawnSync('npm', ['pack'], {

View File

@ -105,7 +105,7 @@ export function createJSHandle(
*
* @public
*/
export class JSHandle {
export class JSHandle<HandleObjectType = unknown> {
/**
* @internal
*/
@ -154,7 +154,7 @@ export class JSHandle {
* ```
*/
async evaluate<T extends EvaluateFn>(
async evaluate<T extends EvaluateFn<HandleObjectType>>(
pageFunction: T | string,
...args: SerializableOrJSHandle[]
): Promise<UnwrapPromiseLike<EvaluateFnReturnType<T>>> {
@ -193,7 +193,7 @@ export class JSHandle {
*/
async getProperty(propertyName: string): Promise<JSHandle | undefined> {
const objectHandle = await this.evaluateHandle(
(object: HTMLElement, propertyName: string) => {
(object: Element, propertyName: string) => {
const result = { __proto__: null };
result[propertyName] = object[propertyName];
return result;
@ -328,7 +328,7 @@ export class JSHandle {
*/
export class ElementHandle<
ElementType extends Element = Element
> extends JSHandle {
> extends JSHandle<ElementType> {
private _page: Page;
private _frameManager: FrameManager;
@ -521,24 +521,25 @@ export class ElementHandle<
'"'
);
return this.evaluate<
(element: HTMLSelectElement, values: string[]) => string[]
>((element, values) => {
if (element.nodeName.toLowerCase() !== 'select')
throw new Error('Element is not a <select> element.');
return this.evaluate<(element: Element, values: string[]) => string[]>(
(element, values) => {
if (!(element instanceof HTMLSelectElement))
throw new Error('Element is not a <select> element.');
const options = Array.from(element.options);
element.value = undefined;
for (const option of options) {
option.selected = values.includes(option.value);
if (option.selected && !element.multiple) break;
}
element.dispatchEvent(new Event('input', { bubbles: true }));
element.dispatchEvent(new Event('change', { bubbles: true }));
return options
.filter((option) => option.selected)
.map((option) => option.value);
}, values);
const options = Array.from(element.options);
element.value = undefined;
for (const option of options) {
option.selected = values.includes(option.value);
if (option.selected && !element.multiple) break;
}
element.dispatchEvent(new Event('input', { bubbles: true }));
element.dispatchEvent(new Event('change', { bubbles: true }));
return options
.filter((option) => option.selected)
.map((option) => option.value);
},
values
);
}
/**
@ -549,9 +550,14 @@ export class ElementHandle<
* relative to the {@link https://nodejs.org/api/process.html#process_process_cwd | current working directory}
*/
async uploadFile(...filePaths: string[]): Promise<void> {
const isMultiple = await this.evaluate<
(element: HTMLInputElement) => boolean
>((element) => element.multiple);
const isMultiple = await this.evaluate<(element: Element) => boolean>(
(element) => {
if (!(element instanceof HTMLInputElement)) {
throw new Error('uploadFile can only be called on an input element.');
}
return element.multiple;
}
);
assert(
filePaths.length <= 1 || isMultiple,
'Multiple file uploads only work with <input type=file multiple>'
@ -588,7 +594,7 @@ export class ElementHandle<
// not actually update the files in that case, so the solution is to eval the element
// value to a new FileList directly.
if (files.length === 0) {
await this.evaluate<(element: HTMLInputElement) => void>((element) => {
await (this as ElementHandle<HTMLInputElement>).evaluate((element) => {
element.files = new DataTransfer().files;
// Dispatch events for this case because it should behave akin to a user action.
@ -619,7 +625,7 @@ export class ElementHandle<
* Calls {@link https://developer.mozilla.org/en-US/docs/Web/API/HTMLElement/focus | focus} on the element.
*/
async focus(): Promise<void> {
await this.evaluate<(element: HTMLElement) => void>((element) =>
await (this as ElementHandle<HTMLElement>).evaluate((element) =>
element.focus()
);
}

View File

@ -7,7 +7,7 @@
"compile": "../../node_modules/.bin/tsc"
},
"devDependencies": {
"typescript": "^4.1.3"
"typescript": "4.2.4"
},
"dependencies": {
"puppeteer": "file:../../puppeteer.tgz"

View File

@ -14,5 +14,12 @@ async function run() {
*/
const div = await page.$('div');
console.log('got a div!', div);
const contentsOfDiv = await div.evaluate(
/**
* @param {number} divElem
* @returns number
*/
(divElem) => divElem.innerText
);
}
run();

View File

@ -7,7 +7,7 @@
"compile": "../../node_modules/.bin/tsc"
},
"devDependencies": {
"typescript": "^4.1.3"
"typescript": "4.2.4"
},
"dependencies": {
"puppeteer": "file:../../puppeteer.tgz"

View File

@ -7,7 +7,7 @@
"compile": "../../node_modules/.bin/tsc"
},
"devDependencies": {
"typescript": "^4.1.3"
"typescript": "4.2.4"
},
"dependencies": {
"puppeteer": "file:../../puppeteer.tgz"

View File

@ -7,7 +7,7 @@
"compile": "../../node_modules/.bin/tsc"
},
"devDependencies": {
"typescript": "^4.1.3"
"typescript": "4.2.4"
},
"dependencies": {
"puppeteer": "file:../../puppeteer.tgz"

View File

@ -7,7 +7,7 @@
"compile": "../../node_modules/.bin/tsc"
},
"devDependencies": {
"typescript": "^4.1.3"
"typescript": "4.2.4"
},
"dependencies": {
"puppeteer": "file:../../puppeteer.tgz"

View File

@ -10,9 +10,21 @@ async function run() {
const browser2 = await puppeteer.launch();
// 'foo' is invalid argument
const page = await browser2.newPage('foo');
const div = (await page.$('div')) as puppeteer.ElementHandle<
HTMLAnchorElement
>;
const div = (await page.$(
'div'
)) as puppeteer.ElementHandle<HTMLAnchorElement>;
console.log('got a div!', div);
const contentsOfDiv = await div.evaluate(
// Bad: the type system will know here that divElem is an HTMLAnchorElement
// and won't let me tell it it's a number
(divElem: number) => divElem.innerText
);
// Bad: the type system will know here that divElem is an HTMLAnchorElement
// and won't let me tell it it's a number via the generic
const contentsOfDiv2 = await div.evaluate<(x: number) => string>(
// Bad: now I've forced it to be a number (which is an error also)
// I can't call `innerText` on it.
(divElem: number) => divElem.innerText
);
}
run();

View File

@ -9,5 +9,7 @@ async function run() {
const page = await browser.newPage();
const div = (await page.$('div')) as ElementHandle<HTMLAnchorElement>;
console.log('got a div!', div);
const contentsOfDiv = await div.evaluate((divElem) => divElem.innerText);
}
run();

View File

@ -7,7 +7,7 @@
"compile": "../../node_modules/.bin/tsc"
},
"devDependencies": {
"typescript": "^4.1.3"
"typescript": "4.2.4"
},
"dependencies": {
"puppeteer": "file:../../puppeteer.tgz"

View File

@ -28,10 +28,12 @@ const EXCLUDE_PROPERTIES = new Set([
'Page.create',
'JSHandle.toString',
'TimeoutError.name',
/* This isn't an actual property, but a TypeScript generic.
/* These are not actual properties, but a TypeScript generic.
* DocLint incorrectly parses it as a property.
*/
'ElementHandle.ElementType',
'ElementHandle.HandleObjectType',
'JSHandle.HandleObjectType',
]);
/**
@ -885,6 +887,20 @@ function compareDocumentations(actual, expected) {
expectedName: 'unknown',
},
],
[
'Method Page.queryObjects() prototypeHandle',
{
actualName: 'JSHandle',
expectedName: 'JSHandle<unknown>',
},
],
[
'Method ExecutionContext.queryObjects() prototypeHandle',
{
actualName: 'JSHandle',
expectedName: 'JSHandle<unknown>',
},
],
]);
const expectedForSource = expectedNamingMismatches.get(source);