From 29ebd0bb3eccbb14f90e6cdb959421ceeb00640d Mon Sep 17 00:00:00 2001 From: Jack Franklin Date: Wed, 22 Apr 2020 10:33:44 +0100 Subject: [PATCH] chore: migrate src/ExecutionContext (#5705) * chore: migrate src/ExecutionContext to TypeScript 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((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((node, x) => true, 5) ``` And `node` and `x` get typed as `any` which means you can tell TS yourself: ``` handle.evaluate((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. --- .eslintrc.js | 4 +- src/DOMWorld.js | 9 +- ...xecutionContext.js => ExecutionContext.ts} | 93 ++++++------------- src/JSHandle.ts | 51 +++++----- src/externs.d.ts | 2 - src/helper.ts | 2 +- 6 files changed, 65 insertions(+), 96 deletions(-) rename src/{ExecutionContext.js => ExecutionContext.ts} (74%) diff --git a/.eslintrc.js b/.eslintrc.js index 80cd46bab3db5..23f02e381fef1 100644 --- a/.eslintrc.js +++ b/.eslintrc.js @@ -114,7 +114,9 @@ module.exports = { "semi": 0, "@typescript-eslint/semi": 2, "@typescript-eslint/no-empty-function": 0, - "@typescript-eslint/no-use-before-define": 0 + "@typescript-eslint/no-use-before-define": 0, + // We know it's bad and use it very sparingly but it's needed :( + "@typescript-eslint/ban-ts-ignore": 0 } } ] diff --git a/src/DOMWorld.js b/src/DOMWorld.js index 1b4e175f81447..33c093d251aa2 100644 --- a/src/DOMWorld.js +++ b/src/DOMWorld.js @@ -21,6 +21,9 @@ const {TimeoutError} = require('./Errors'); // Used as a TypeDef // eslint-disable-next-line no-unused-vars const {JSHandle, ElementHandle} = require('./JSHandle'); +// Used as a TypeDef +// eslint-disable-next-line no-unused-vars +const {ExecutionContext} = require('./ExecutionContext'); // Used as a TypeDef // eslint-disable-next-line no-unused-vars @@ -44,7 +47,7 @@ class DOMWorld { /** @type {?Promise} */ this._documentPromise = null; - /** @type {!Promise} */ + /** @type {!Promise} */ this._contextPromise; this._contextResolveCallback = null; this._setContext(null); @@ -62,7 +65,7 @@ class DOMWorld { } /** - * @param {?Puppeteer.ExecutionContext} context + * @param {?ExecutionContext} context */ _setContext(context) { if (context) { @@ -92,7 +95,7 @@ class DOMWorld { } /** - * @return {!Promise} + * @return {!Promise} */ executionContext() { if (this._detached) diff --git a/src/ExecutionContext.js b/src/ExecutionContext.ts similarity index 74% rename from src/ExecutionContext.js rename to src/ExecutionContext.ts index 81c8a5249801d..d7e09676d0956 100644 --- a/src/ExecutionContext.js +++ b/src/ExecutionContext.ts @@ -14,67 +14,44 @@ * limitations under the License. */ -const {helper, assert} = require('./helper'); -// Used as a TypeDef -// eslint-disable-next-line no-unused-vars -const {CDPSession} = require('./Connection'); -// Used as a TypeDef -// eslint-disable-next-line no-unused-vars -const {createJSHandle, JSHandle, ElementHandle} = require('./JSHandle'); - -const EVALUATION_SCRIPT_URL = '__puppeteer_evaluation_script__'; +import {helper, assert} from './helper'; +import {createJSHandle, JSHandle, ElementHandle} from './JSHandle'; +import {CDPSession} from './Connection'; + +export const EVALUATION_SCRIPT_URL = '__puppeteer_evaluation_script__'; const SOURCE_URL_REGEX = /^[\040\t]*\/\/[@#] sourceURL=\s*(\S*?)\s*$/m; -class ExecutionContext { - /** - * @param {!CDPSession} client - * @param {!Protocol.Runtime.ExecutionContextDescription} contextPayload - * @param {?Puppeteer.DOMWorld} world - */ - constructor(client, contextPayload, world) { +export class ExecutionContext { + _client: CDPSession; + _world: Puppeteer.DOMWorld; + _contextId: number; + + constructor(client: CDPSession, contextPayload: Protocol.Runtime.ExecutionContextDescription, world: Puppeteer.DOMWorld) { this._client = client; this._world = world; this._contextId = contextPayload.id; } - /** - * @return {?Puppeteer.Frame} - */ - frame() { + frame(): Puppeteer.Frame | null { return this._world ? this._world.frame() : null; } - /** - * @param {Function|string} pageFunction - * @param {...*} args - * @return {!Promise<*>} - */ - async evaluate(pageFunction, ...args) { - return await this._evaluateInternal(true /* returnByValue */, pageFunction, ...args); + async evaluate(pageFunction: Function | string, ...args: unknown[]): Promise { + return await this._evaluateInternal(true, pageFunction, ...args); } - /** - * @param {Function|string} pageFunction - * @param {...*} args - * @return {!Promise} - */ - async evaluateHandle(pageFunction, ...args) { - return this._evaluateInternal(false /* returnByValue */, pageFunction, ...args); + async evaluateHandle(pageFunction: Function | string, ...args: unknown[]): Promise { + return this._evaluateInternal(false, pageFunction, ...args); } - /** - * @param {boolean} returnByValue - * @param {Function|string} pageFunction - * @param {...*} args - * @return {!Promise<*>} - */ - async _evaluateInternal(returnByValue, pageFunction, ...args) { + private async _evaluateInternal(returnByValue, pageFunction: Function | string, ...args: unknown[]): Promise { const suffix = `//# sourceURL=${EVALUATION_SCRIPT_URL}`; if (helper.isString(pageFunction)) { const contextId = this._contextId; - const expression = /** @type {string} */ (pageFunction); + const expression = pageFunction; const expressionWithSourceUrl = SOURCE_URL_REGEX.test(expression) ? expression : expression + '\n' + suffix; + const {exceptionDetails, result: remoteObject} = await this._client.send('Runtime.evaluate', { expression: expressionWithSourceUrl, contextId, @@ -82,8 +59,10 @@ class ExecutionContext { awaitPromise: true, userGesture: true }).catch(rewriteError); + if (exceptionDetails) throw new Error('Evaluation failed: ' + helper.getExceptionMessage(exceptionDetails)); + return returnByValue ? helper.valueFromRemoteObject(remoteObject) : createJSHandle(this, remoteObject); } @@ -132,7 +111,7 @@ class ExecutionContext { * @return {*} * @this {ExecutionContext} */ - function convertArgument(arg) { + function convertArgument(this: ExecutionContext, arg: unknown): unknown { if (typeof arg === 'bigint') // eslint-disable-line valid-typeof return {unserializableValue: `${arg.toString()}n`}; if (Object.is(arg, -0)) @@ -158,11 +137,7 @@ class ExecutionContext { return {value: arg}; } - /** - * @param {!Error} error - * @return {!Protocol.Runtime.evaluateReturnValue} - */ - function rewriteError(error) { + function rewriteError(error: Error): Protocol.Runtime.evaluateReturnValue { if (error.message.includes('Object reference chain is too long')) return {result: {type: 'undefined'}}; if (error.message.includes('Object couldn\'t be returned by value')) @@ -174,11 +149,7 @@ class ExecutionContext { } } - /** - * @param {!JSHandle} prototypeHandle - * @return {!Promise} - */ - async queryObjects(prototypeHandle) { + async queryObjects(prototypeHandle: JSHandle): Promise { assert(!prototypeHandle._disposed, 'Prototype JSHandle is disposed!'); assert(prototypeHandle._remoteObject.objectId, 'Prototype JSHandle must not be referencing primitive value'); const response = await this._client.send('Runtime.queryObjects', { @@ -187,23 +158,15 @@ class ExecutionContext { return createJSHandle(this, response.objects); } - /** - * @param {Protocol.DOM.BackendNodeId} backendNodeId - * @return {Promise} - */ - async _adoptBackendNodeId(backendNodeId) { + async _adoptBackendNodeId(backendNodeId: Protocol.DOM.BackendNodeId): Promise { const {object} = await this._client.send('DOM.resolveNode', { backendNodeId: backendNodeId, executionContextId: this._contextId, }); - return /** @type {ElementHandle}*/(createJSHandle(this, object)); + return createJSHandle(this, object) as ElementHandle; } - /** - * @param {ElementHandle} elementHandle - * @return {Promise} - */ - async _adoptElementHandle(elementHandle) { + async _adoptElementHandle(elementHandle: ElementHandle): Promise { assert(elementHandle.executionContext() !== this, 'Cannot adopt handle that already belongs to this execution context'); assert(this._world, 'Cannot adopt handle without DOMWorld'); const nodeInfo = await this._client.send('DOM.describeNode', { @@ -212,5 +175,3 @@ class ExecutionContext { return this._adoptBackendNodeId(nodeInfo.node.backendNodeId); } } - -module.exports = {ExecutionContext, EVALUATION_SCRIPT_URL}; diff --git a/src/JSHandle.ts b/src/JSHandle.ts index 5197501027b77..c727b372d061e 100644 --- a/src/JSHandle.ts +++ b/src/JSHandle.ts @@ -15,6 +15,7 @@ */ import {helper, assert, debugError} from './helper'; +import {ExecutionContext} from './ExecutionContext'; import {CDPSession} from './Connection'; interface BoxModel { @@ -26,7 +27,7 @@ interface BoxModel { height: number; } -export function createJSHandle(context: Puppeteer.ExecutionContext, remoteObject: Protocol.Runtime.RemoteObject): JSHandle { +export function createJSHandle(context: ExecutionContext, remoteObject: Protocol.Runtime.RemoteObject): JSHandle { const frame = context.frame(); if (remoteObject.subtype === 'node' && frame) { const frameManager = frame._frameManager; @@ -36,31 +37,31 @@ export function createJSHandle(context: Puppeteer.ExecutionContext, remoteObject } export class JSHandle { - _context: Puppeteer.ExecutionContext; + _context: ExecutionContext; _client: CDPSession; _remoteObject: Protocol.Runtime.RemoteObject; _disposed = false; - constructor(context: Puppeteer.ExecutionContext, client: CDPSession, remoteObject: Protocol.Runtime.RemoteObject) { + constructor(context: ExecutionContext, client: CDPSession, remoteObject: Protocol.Runtime.RemoteObject) { this._context = context; this._client = client; this._remoteObject = remoteObject; } - executionContext(): Puppeteer.ExecutionContext { + executionContext(): ExecutionContext { return this._context; } - async evaluate(pageFunction: Function | string, ...args: any[]): Promise { - return await this.executionContext().evaluate(pageFunction, this, ...args); + async evaluate(pageFunction: Function | string, ...args: unknown[]): Promise { + return await this.executionContext().evaluate(pageFunction, this, ...args); } - async evaluateHandle(pageFunction: Function | string, ...args: any[]): Promise { + async evaluateHandle(pageFunction: Function | string, ...args: unknown[]): Promise { return await this.executionContext().evaluateHandle(pageFunction, this, ...args); } async getProperty(propertyName: string): Promise { - const objectHandle = await this.evaluateHandle((object, propertyName) => { + const objectHandle = await this.evaluateHandle((object: HTMLElement, propertyName: string) => { const result = {__proto__: null}; result[propertyName] = object[propertyName]; return result; @@ -123,13 +124,13 @@ export class ElementHandle extends JSHandle { _page: Puppeteer.Page; _frameManager: Puppeteer.FrameManager; /** - * @param {!Puppeteer.ExecutionContext} context + * @param {!ExecutionContext} context * @param {!CDPSession} client * @param {!Protocol.Runtime.RemoteObject} remoteObject * @param {!Puppeteer.Page} page * @param {!Puppeteer.FrameManager} frameManager */ - constructor(context: Puppeteer.ExecutionContext, client: CDPSession, remoteObject: Protocol.Runtime.RemoteObject, page: Puppeteer.Page, frameManager: Puppeteer.FrameManager) { + constructor(context: ExecutionContext, client: CDPSession, remoteObject: Protocol.Runtime.RemoteObject, page: Puppeteer.Page, frameManager: Puppeteer.FrameManager) { super(context, client, remoteObject); this._client = client; this._remoteObject = remoteObject; @@ -151,13 +152,16 @@ export class ElementHandle extends JSHandle { } async _scrollIntoViewIfNeeded(): Promise { - const error = await this.evaluate(async(element, pageJavascriptEnabled) => { + const error = await this.evaluate>(async(element: HTMLElement, pageJavascriptEnabled: boolean) => { 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) { + // Chrome still supports behavior: instant but it's not in the spec so TS shouts + // We don't want to make this breaking change in Puppeteer yet so we'll ignore the line. + // @ts-ignore element.scrollIntoView({block: 'center', inline: 'center', behavior: 'instant'}); return false; } @@ -168,8 +172,12 @@ export class ElementHandle extends JSHandle { }); observer.observe(element); }); - if (visibleRatio !== 1.0) + if (visibleRatio !== 1.0) { + // Chrome still supports behavior: instant but it's not in the spec so TS shouts + // We don't want to make this breaking change in Puppeteer yet so we'll ignore the line. + // @ts-ignore element.scrollIntoView({block: 'center', inline: 'center', behavior: 'instant'}); + } return false; }, this._page._javascriptEnabled); @@ -275,7 +283,7 @@ export class ElementHandle extends JSHandle { } async uploadFile(...filePaths: string[]): Promise { - const isMultiple = await this.evaluate(element => element.multiple); + const isMultiple = await this.evaluate((element: HTMLInputElement) => element.multiple); assert(filePaths.length <= 1 || isMultiple, 'Multiple file uploads only work with '); // This import is only needed for `uploadFile`, so keep it scoped here to avoid paying @@ -291,7 +299,7 @@ export class ElementHandle extends JSHandle { // 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 => { + await this.evaluate((element: HTMLInputElement) => { element.files = new DataTransfer().files; // Dispatch events for this case because it should behave akin to a user action. @@ -431,25 +439,22 @@ export class ElementHandle extends JSHandle { return result; } - async $eval(selector: string, pageFunction: Function|string, ...args: any[]): Promise { + async $eval(selector: string, pageFunction: Function|string, ...args: unknown[]): Promise { const elementHandle = await this.$(selector); if (!elementHandle) throw new Error(`Error: failed to find element matching selector "${selector}"`); - const result = await elementHandle.evaluate(pageFunction, ...args); + const result = await elementHandle.evaluate(pageFunction, ...args); await elementHandle.dispose(); return result; } - // TODO(jacktfranklin@): consider the types here - // we might want $$eval which returns Promise? - // Once ExecutionContext.evaluate is properly typed we can improve this a bunch - async $$eval(selector: string, pageFunction: Function | string, ...args: any[]): Promise { + async $$eval(selector: string, pageFunction: Function | string, ...args: unknown[]): Promise { const arrayHandle = await this.evaluateHandle( (element, selector) => Array.from(element.querySelectorAll(selector)), selector ); - const result = await arrayHandle.evaluate(pageFunction, ...args); + const result = await arrayHandle.evaluate(pageFunction, ...args); await arrayHandle.dispose(); return result; } @@ -478,8 +483,8 @@ export class ElementHandle extends JSHandle { return result; } - isIntersectingViewport(): Promise { - return this.evaluate(async element => { + async isIntersectingViewport(): Promise { + return await this.evaluate>(async element => { const visibleRatio = await new Promise(resolve => { const observer = new IntersectionObserver(entries => { resolve(entries[0].intersectionRatio); diff --git a/src/externs.d.ts b/src/externs.d.ts index 468e1ae7f8e88..1a45f7a159785 100644 --- a/src/externs.d.ts +++ b/src/externs.d.ts @@ -4,7 +4,6 @@ import {Page as RealPage} from './Page.js'; import {Mouse as RealMouse, Keyboard as RealKeyboard, Touchscreen as RealTouchscreen} from './Input.js'; import {Frame as RealFrame, FrameManager as RealFrameManager} from './FrameManager.js'; import {DOMWorld as RealDOMWorld} from './DOMWorld.js'; -import {ExecutionContext as RealExecutionContext} from './ExecutionContext.js'; import { NetworkManager as RealNetworkManager, Request as RealRequest, Response as RealResponse } from './NetworkManager.js'; import * as child_process from 'child_process'; declare global { @@ -19,7 +18,6 @@ declare global { export class FrameManager extends RealFrameManager {} export class NetworkManager extends RealNetworkManager {} export class DOMWorld extends RealDOMWorld {} - export class ExecutionContext extends RealExecutionContext {} export class Page extends RealPage { } export class Response extends RealResponse { } export class Request extends RealRequest { } diff --git a/src/helper.ts b/src/helper.ts index 5d4ff998721ce..2cf851759391e 100644 --- a/src/helper.ts +++ b/src/helper.ts @@ -52,7 +52,7 @@ function getExceptionMessage(exceptionDetails: Protocol.Runtime.ExceptionDetails return message; } -function valueFromRemoteObject(remoteObject: Protocol.Runtime.RemoteObject): unknown { +function valueFromRemoteObject(remoteObject: Protocol.Runtime.RemoteObject): any { assert(!remoteObject.objectId, 'Cannot extract value when objectId is given'); if (remoteObject.unserializableValue) { if (remoteObject.type === 'bigint' && typeof BigInt !== 'undefined')