Skip to content

Commit

Permalink
chore: migrate src/ExecutionContext (#5705)
Browse files Browse the repository at this point in the history 
* 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<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.
  • Loading branch information
@jackfranklin
jackfranklin committed Apr 22, 2020
1 parent 69bfa80 commit 29ebd0b
Show file tree
Hide file tree
Showing 6 changed files with 65 additions and 96 deletions.
4 changes: 3 additions & 1 deletion .eslintrc.js
View file
Expand Up @@ -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
}
}
]
Expand Down
9 changes: 6 additions & 3 deletions src/DOMWorld.js
View file
Expand Up @@ -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
Expand All @@ -44,7 +47,7 @@ class DOMWorld {

/** @type {?Promise<!ElementHandle>} */
this._documentPromise = null;
/** @type {!Promise<!Puppeteer.ExecutionContext>} */
/** @type {!Promise<!ExecutionContext>} */
this._contextPromise;
this._contextResolveCallback = null;
this._setContext(null);
Expand All @@ -62,7 +65,7 @@ class DOMWorld {
}

/**
* @param {?Puppeteer.ExecutionContext} context
* @param {?ExecutionContext} context
*/
_setContext(context) {
if (context) {
Expand Down Expand Up @@ -92,7 +95,7 @@ class DOMWorld {
}

/**
* @return {!Promise<!Puppeteer.ExecutionContext>}
* @return {!Promise<!ExecutionContext>}
*/
executionContext() {
if (this._detached)
Expand Down
93 changes: 27 additions & 66 deletions src/ExecutionContext.js → src/ExecutionContext.ts
View file
Expand Up @@ -14,76 +14,55 @@
* 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<ReturnType extends any>(pageFunction: Function | string, ...args: unknown[]): Promise<ReturnType> {
return await this._evaluateInternal<ReturnType>(true, pageFunction, ...args);
}

/**
* @param {Function|string} pageFunction
* @param {...*} args
* @return {!Promise<!JSHandle>}
*/
async evaluateHandle(pageFunction, ...args) {
return this._evaluateInternal(false /* returnByValue */, pageFunction, ...args);
async evaluateHandle(pageFunction: Function | string, ...args: unknown[]): Promise<JSHandle> {
return this._evaluateInternal<JSHandle>(false, pageFunction, ...args);
}

/**
* @param {boolean} returnByValue
* @param {Function|string} pageFunction
* @param {...*} args
* @return {!Promise<*>}
*/
async _evaluateInternal(returnByValue, pageFunction, ...args) {
private async _evaluateInternal<ReturnType>(returnByValue, pageFunction: Function | string, ...args: unknown[]): Promise<ReturnType> {
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,
returnByValue,
awaitPromise: true,
userGesture: true
}).catch(rewriteError);

if (exceptionDetails)
throw new Error('Evaluation failed: ' + helper.getExceptionMessage(exceptionDetails));

return returnByValue ? helper.valueFromRemoteObject(remoteObject) : createJSHandle(this, remoteObject);
}

Expand Down Expand Up @@ -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))
Expand All @@ -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'))
Expand All @@ -174,11 +149,7 @@ class ExecutionContext {
}
}

/**
* @param {!JSHandle} prototypeHandle
* @return {!Promise<!JSHandle>}
*/
async queryObjects(prototypeHandle) {
async queryObjects(prototypeHandle: JSHandle): Promise<JSHandle> {
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', {
Expand All @@ -187,23 +158,15 @@ class ExecutionContext {
return createJSHandle(this, response.objects);
}

/**
* @param {Protocol.DOM.BackendNodeId} backendNodeId
* @return {Promise<ElementHandle>}
*/
async _adoptBackendNodeId(backendNodeId) {
async _adoptBackendNodeId(backendNodeId: Protocol.DOM.BackendNodeId): Promise<ElementHandle> {
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<ElementHandle>}
*/
async _adoptElementHandle(elementHandle) {
async _adoptElementHandle(elementHandle: ElementHandle): Promise<ElementHandle> {
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', {
Expand All @@ -212,5 +175,3 @@ class ExecutionContext {
return this._adoptBackendNodeId(nodeInfo.node.backendNodeId);
}
}

module.exports = {ExecutionContext, EVALUATION_SCRIPT_URL};
51 changes: 28 additions & 23 deletions src/JSHandle.ts
View file
Expand Up @@ -15,6 +15,7 @@
*/

import {helper, assert, debugError} from './helper';
import {ExecutionContext} from './ExecutionContext';
import {CDPSession} from './Connection';

interface BoxModel {
Expand All @@ -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;
Expand All @@ -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<T extends any>(pageFunction: Function | string, ...args: any[]): Promise<T | undefined> {
return await this.executionContext().evaluate(pageFunction, this, ...args);
async evaluate<ReturnType extends any>(pageFunction: Function | string, ...args: unknown[]): Promise<ReturnType> {
return await this.executionContext().evaluate<ReturnType>(pageFunction, this, ...args);
}

async evaluateHandle(pageFunction: Function | string, ...args: any[]): Promise<JSHandle> {
async evaluateHandle(pageFunction: Function | string, ...args: unknown[]): Promise<JSHandle> {
return await this.executionContext().evaluateHandle(pageFunction, this, ...args);
}

async getProperty(propertyName: string): Promise<JSHandle | undefined> {
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;
Expand Down Expand Up @@ -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;
Expand All @@ -151,13 +152,16 @@ export class ElementHandle extends JSHandle {
}

async _scrollIntoViewIfNeeded(): Promise<void> {
const error = await this.evaluate<string | false>(async(element, pageJavascriptEnabled) => {
const error = await this.evaluate<Promise<string | false>>(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;
}
Expand All @@ -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);

Expand Down Expand Up @@ -275,7 +283,7 @@ export class ElementHandle extends JSHandle {
}

async uploadFile(...filePaths: string[]): Promise<void> {
const isMultiple = await this.evaluate(element => element.multiple);
const isMultiple = await this.evaluate<boolean>((element: HTMLInputElement) => element.multiple);
assert(filePaths.length <= 1 || isMultiple, 'Multiple file uploads only work with <input type=file multiple>');

// This import is only needed for `uploadFile`, so keep it scoped here to avoid paying
Expand All @@ -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.
Expand Down Expand Up @@ -431,25 +439,22 @@ export class ElementHandle extends JSHandle {
return result;
}

async $eval<T extends any>(selector: string, pageFunction: Function|string, ...args: any[]): Promise<T | undefined> {
async $eval<ReturnType extends any>(selector: string, pageFunction: Function|string, ...args: unknown[]): Promise<ReturnType> {
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<ReturnType>(pageFunction, ...args);
await elementHandle.dispose();
return result;
}

// TODO(jacktfranklin@): consider the types here
// we might want $$eval<SelectorType> which returns Promise<SelectorType[]>?
// Once ExecutionContext.evaluate is properly typed we can improve this a bunch
async $$eval<T>(selector: string, pageFunction: Function | string, ...args: any[]): Promise<T | undefined> {
async $$eval<ReturnType extends any>(selector: string, pageFunction: Function | string, ...args: unknown[]): Promise<ReturnType> {
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<ReturnType>(pageFunction, ...args);
await arrayHandle.dispose();
return result;
}
Expand Down Expand Up @@ -478,8 +483,8 @@ export class ElementHandle extends JSHandle {
return result;
}

isIntersectingViewport(): Promise<boolean> {
return this.evaluate(async element => {
async isIntersectingViewport(): Promise<boolean> {
return await this.evaluate<Promise<boolean>>(async element => {
const visibleRatio = await new Promise(resolve => {
const observer = new IntersectionObserver(entries => {
resolve(entries[0].intersectionRatio);
Expand Down
2 changes: 0 additions & 2 deletions src/externs.d.ts
View file
Expand Up @@ -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 {
Expand All @@ -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 { }
Expand Down

0 comments on commit 29ebd0b

Please sign in to comment.