From 1d4d25a0f3e5533cfcf889b8d8c5377b43e69366 Mon Sep 17 00:00:00 2001 From: Jack Franklin Date: Fri, 29 May 2020 09:59:26 +0100 Subject: [PATCH] Use Mitt as the Event Emitter (#5907) * chore: migrate to Mitt as the EventEmitter This commit moves us to using Mitt [1] for the event emitter in Puppeteer. This removes our dependency to Node's EventEmitter which is part of a larger stream of work to enable a Puppeteer-web version that doesn't depend on Node. There are no large breaking changes as we support the main methods that EventEmitter had, but it also provides some methods that Puppeteer didn't use. Technically end users could depend on this but it's unlikely. [1]: https://github.com/developit/mitt --- docs/api.md | 37 +++++++++++ package.json | 1 + src/Browser.ts | 2 +- src/BrowserFetcher.ts | 8 +-- src/Connection.ts | 4 +- src/EventEmitter.ts | 61 ++++++++++++++++++ src/FrameManager.ts | 2 +- src/NetworkManager.ts | 2 +- src/Page.ts | 2 +- src/WebSocketTransport.ts | 2 +- src/Worker.ts | 2 +- src/api.ts | 1 + src/helper.ts | 11 ++-- src/launcher/BrowserRunner.ts | 4 +- tsconfig.json | 3 +- utils/doclint/check_public_api/index.js | 84 ++++++++++++++++++++++++- 16 files changed, 203 insertions(+), 23 deletions(-) create mode 100644 src/EventEmitter.ts diff --git a/docs/api.md b/docs/api.md index 7c18503e..f81289a7 100644 --- a/docs/api.md +++ b/docs/api.md @@ -335,6 +335,13 @@ * [coverage.stopCSSCoverage()](#coveragestopcsscoverage) * [coverage.stopJSCoverage()](#coveragestopjscoverage) - [class: TimeoutError](#class-timeouterror) +- [class: EventEmitter](#class-eventemitter) + * [eventEmitter.emit(event, [eventData])](#eventemitteremitevent-eventdata) + * [eventEmitter.listenerCount(event)](#eventemitterlistenercountevent) + * [eventEmitter.off(event, handler)](#eventemitteroffevent-handler) + * [eventEmitter.on(event, handler)](#eventemitteronevent-handler) + * [eventEmitter.once(event, handler)](#eventemitteronceevent-handler) + * [eventEmitter.removeListener(event, handler)](#eventemitterremovelistenerevent-handler) ### Overview @@ -3935,6 +3942,35 @@ reported. TimeoutError is emitted whenever certain operations are terminated due to timeout, e.g. [page.waitForSelector(selector[, options])](#pagewaitforselectorselector-options) or [puppeteer.launch([options])](#puppeteerlaunchoptions). +### class: EventEmitter + +A small EventEmitter class backed by [Mitt](https://github.com/developit/mitt/). + +#### eventEmitter.emit(event, [eventData]) +- `event` <[string]|[symbol]> the event to trigger. +- `eventData` <[Object]> additional data to send with the event. + +#### eventEmitter.listenerCount(event) +- `event` <[string]|[symbol]> the event to check for listeners. +- returns: <[number]> the number of listeners for the given event. + +#### eventEmitter.off(event, handler) +- `event` <[string]|[symbol]> the event to remove the handler from. +- `handler` <[Function]> the event listener that will be removed. + +#### eventEmitter.on(event, handler) +- `event` <[string]|[symbol]> the event to add the handler to. +- `handler` <[Function]> the event listener that will be added. + +#### eventEmitter.once(event, handler) +- `event` <[string]|[symbol]> the event to add the handler to. +- `handler` <[Function]> the event listener that will be added. + +#### eventEmitter.removeListener(event, handler) +- `event` <[string]|[symbol]> the event to remove the handler from. +- `handler` <[Function]> the event listener that will be removed. + +This method is identical to `off` and maintained for compatibility with Node's EventEmitter. We recommend using `off` by default. [AXNode]: #accessibilitysnapshotoptions "AXNode" @@ -3984,4 +4020,5 @@ TimeoutError is emitted whenever certain operations are terminated due to timeou [selector]: https://developer.mozilla.org/en-US/docs/Web/CSS/CSS_Selectors "selector" [stream.Readable]: https://nodejs.org/api/stream.html#stream_class_stream_readable "stream.Readable" [string]: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Data_structures#String_type "String" +[symbol]: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Data_structures#Symbol_type "Symbol" [xpath]: https://developer.mozilla.org/en-US/docs/Web/XPath "xpath" diff --git a/package.json b/package.json index 2049bc03..36cb6451 100644 --- a/package.json +++ b/package.json @@ -48,6 +48,7 @@ "extract-zip": "^2.0.0", "https-proxy-agent": "^4.0.0", "mime": "^2.0.3", + "mitt": "^2.0.1", "progress": "^2.0.1", "proxy-from-env": "^1.0.0", "rimraf": "^3.0.2", diff --git a/src/Browser.ts b/src/Browser.ts index 2b23e22e..358c0a69 100644 --- a/src/Browser.ts +++ b/src/Browser.ts @@ -16,7 +16,7 @@ import { helper, assert } from './helper'; import { Target } from './Target'; -import * as EventEmitter from 'events'; +import { EventEmitter } from './EventEmitter'; import { Events } from './Events'; import Protocol from './protocol'; import { Connection } from './Connection'; diff --git a/src/BrowserFetcher.ts b/src/BrowserFetcher.ts index b46fec88..dd145e95 100644 --- a/src/BrowserFetcher.ts +++ b/src/BrowserFetcher.ts @@ -22,11 +22,11 @@ import * as childProcess from 'child_process'; import * as https from 'https'; import * as http from 'http'; -import * as extractZip from 'extract-zip'; -import * as debug from 'debug'; -import * as removeRecursive from 'rimraf'; +import extractZip from 'extract-zip'; +import debug from 'debug'; +import removeRecursive from 'rimraf'; import * as URL from 'url'; -import * as ProxyAgent from 'https-proxy-agent'; +import ProxyAgent from 'https-proxy-agent'; import { getProxyForUrl } from 'proxy-from-env'; import { helper, assert } from './helper'; diff --git a/src/Connection.ts b/src/Connection.ts index e2e15b83..f19c697c 100644 --- a/src/Connection.ts +++ b/src/Connection.ts @@ -15,12 +15,12 @@ */ import { assert } from './helper'; import { Events } from './Events'; -import * as debug from 'debug'; +import debug from 'debug'; const debugProtocol = debug('puppeteer:protocol'); import Protocol from './protocol'; import type { ConnectionTransport } from './ConnectionTransport'; -import * as EventEmitter from 'events'; +import { EventEmitter } from './EventEmitter'; interface ConnectionCallback { resolve: Function; diff --git a/src/EventEmitter.ts b/src/EventEmitter.ts new file mode 100644 index 00000000..8e490ab0 --- /dev/null +++ b/src/EventEmitter.ts @@ -0,0 +1,61 @@ +import mitt, { Emitter, EventType, Handler } from 'mitt'; + +export interface CommonEventEmitter { + on(event: EventType, handler: Handler): void; + off(event: EventType, handler: Handler): void; + /* To maintain parity with the built in NodeJS event emitter which uses removeListener + * rather than `off`. + * If you're implementing new code you should use `off`. + */ + removeListener(event: EventType, handler: Handler): void; + emit(event: EventType, eventData?: any): void; + once(event: EventType, handler: Handler): void; + listenerCount(event: string): number; +} + +export class EventEmitter implements CommonEventEmitter { + private emitter: Emitter; + private listenerCounts = new Map(); + + constructor() { + this.emitter = mitt(new Map()); + } + + on(event: EventType, handler: Handler): void { + this.emitter.on(event, handler); + const existingCounts = this.listenerCounts.get(event); + if (existingCounts) { + this.listenerCounts.set(event, existingCounts + 1); + } else { + this.listenerCounts.set(event, 1); + } + } + + off(event: EventType, handler: Handler): void { + this.emitter.off(event, handler); + + const existingCounts = this.listenerCounts.get(event); + this.listenerCounts.set(event, existingCounts - 1); + } + + removeListener(event: EventType, handler: Handler): void { + this.off(event, handler); + } + + emit(event: EventType, eventData?: any): void { + this.emitter.emit(event, eventData); + } + + once(event: EventType, handler: Handler): void { + const onceHandler: Handler = (eventData) => { + handler(eventData); + this.off(event, onceHandler); + }; + + this.on(event, onceHandler); + } + + listenerCount(event: EventType): number { + return this.listenerCounts.get(event) || 0; + } +} diff --git a/src/FrameManager.ts b/src/FrameManager.ts index 9b012f87..760d1d58 100644 --- a/src/FrameManager.ts +++ b/src/FrameManager.ts @@ -14,7 +14,7 @@ * limitations under the License. */ -import * as EventEmitter from 'events'; +import { EventEmitter } from './EventEmitter'; import { helper, assert, debugError } from './helper'; import { Events } from './Events'; import { ExecutionContext, EVALUATION_SCRIPT_URL } from './ExecutionContext'; diff --git a/src/NetworkManager.ts b/src/NetworkManager.ts index 14e0dda8..450c88ac 100644 --- a/src/NetworkManager.ts +++ b/src/NetworkManager.ts @@ -13,7 +13,7 @@ * See the License for the specific language governing permissions and * limitations under the License. */ -import * as EventEmitter from 'events'; +import { EventEmitter } from './EventEmitter'; import { helper, assert, debugError } from './helper'; import Protocol from './protocol'; import { Events } from './Events'; diff --git a/src/Page.ts b/src/Page.ts index 2263aff3..224b2940 100644 --- a/src/Page.ts +++ b/src/Page.ts @@ -15,7 +15,7 @@ */ import * as fs from 'fs'; -import * as EventEmitter from 'events'; +import { EventEmitter } from './EventEmitter'; import * as mime from 'mime'; import { Events } from './Events'; import { Connection, CDPSession } from './Connection'; diff --git a/src/WebSocketTransport.ts b/src/WebSocketTransport.ts index f1305599..04cb6ca6 100644 --- a/src/WebSocketTransport.ts +++ b/src/WebSocketTransport.ts @@ -13,7 +13,7 @@ * See the License for the specific language governing permissions and * limitations under the License. */ -import * as NodeWebSocket from 'ws'; +import NodeWebSocket from 'ws'; import type { ConnectionTransport } from './ConnectionTransport'; export class WebSocketTransport implements ConnectionTransport { diff --git a/src/Worker.ts b/src/Worker.ts index 9109ff7c..fd21da31 100644 --- a/src/Worker.ts +++ b/src/Worker.ts @@ -13,7 +13,7 @@ * See the License for the specific language governing permissions and * limitations under the License. */ -import { EventEmitter } from 'events'; +import { EventEmitter } from './EventEmitter'; import { debugError } from './helper'; import { ExecutionContext } from './ExecutionContext'; import { JSHandle } from './JSHandle'; diff --git a/src/api.ts b/src/api.ts index fb56ef74..ca48eda9 100644 --- a/src/api.ts +++ b/src/api.ts @@ -29,6 +29,7 @@ module.exports = { Dialog: require('./Dialog').Dialog, ElementHandle: require('./JSHandle').ElementHandle, ExecutionContext: require('./ExecutionContext').ExecutionContext, + EventEmitter: require('./EventEmitter').EventEmitter, FileChooser: require('./FileChooser').FileChooser, Frame: require('./FrameManager').Frame, JSHandle: require('./JSHandle').JSHandle, diff --git a/src/helper.ts b/src/helper.ts index 11b50034..fa32cf62 100644 --- a/src/helper.ts +++ b/src/helper.ts @@ -14,11 +14,12 @@ * limitations under the License. */ import { TimeoutError } from './Errors'; -import * as debug from 'debug'; +import debug from 'debug'; import * as fs from 'fs'; import { CDPSession } from './Connection'; import { promisify } from 'util'; import Protocol from './protocol'; +import type { CommonEventEmitter } from './EventEmitter'; const openAsync = promisify(fs.open); const writeAsync = promisify(fs.write); @@ -131,13 +132,13 @@ function installAsyncStackHooks(classType: AnyClass): void { } export interface PuppeteerEventListener { - emitter: NodeJS.EventEmitter; + emitter: CommonEventEmitter; eventName: string | symbol; handler: (...args: any[]) => void; } function addEventListener( - emitter: NodeJS.EventEmitter, + emitter: CommonEventEmitter, eventName: string | symbol, handler: (...args: any[]) => void ): PuppeteerEventListener { @@ -147,7 +148,7 @@ function addEventListener( function removeEventListeners( listeners: Array<{ - emitter: NodeJS.EventEmitter; + emitter: CommonEventEmitter; eventName: string | symbol; handler: (...args: any[]) => void; }> @@ -166,7 +167,7 @@ function isNumber(obj: unknown): obj is number { } async function waitForEvent( - emitter: NodeJS.EventEmitter, + emitter: CommonEventEmitter, eventName: string | symbol, predicate: (event: T) => boolean, timeout: number, diff --git a/src/launcher/BrowserRunner.ts b/src/launcher/BrowserRunner.ts index 2d6a629b..7f81e114 100644 --- a/src/launcher/BrowserRunner.ts +++ b/src/launcher/BrowserRunner.ts @@ -14,9 +14,9 @@ * limitations under the License. */ -import * as debug from 'debug'; +import debug from 'debug'; -import * as removeFolder from 'rimraf'; +import removeFolder from 'rimraf'; import * as childProcess from 'child_process'; import { helper, assert, debugError } from '../helper'; import type { LaunchOptions } from './LaunchOptions'; diff --git a/tsconfig.json b/tsconfig.json index baf9d96a..a5fc9cfb 100644 --- a/tsconfig.json +++ b/tsconfig.json @@ -5,7 +5,8 @@ "outDir": "./lib", "target": "ESNext", "moduleResolution": "node", - "module": "CommonJS" + "module": "CommonJS", + "esModuleInterop": true }, "include": [ "src" diff --git a/utils/doclint/check_public_api/index.js b/utils/doclint/check_public_api/index.js index 4bfc90f1..22fe64ce 100644 --- a/utils/doclint/check_public_api/index.js +++ b/utils/doclint/check_public_api/index.js @@ -179,6 +179,32 @@ const expectedNonExistingMethods = new Map([ */ ['Page', new Set(['emulateMedia'])], ]); + +/* Methods that are defined in code but are not documented */ +const expectedNotFoundMethods = new Map([ + /* all the methods from our EventEmitter that we don't document for each subclass */ + [ + 'Browser', + new Set(['emit', 'listenerCount', 'off', 'on', 'once', 'removeListener']), + ], + [ + 'BrowserContext', + new Set(['emit', 'listenerCount', 'off', 'on', 'once', 'removeListener']), + ], + [ + 'CDPSession', + new Set(['emit', 'listenerCount', 'off', 'on', 'once', 'removeListener']), + ], + [ + 'Page', + new Set(['emit', 'listenerCount', 'off', 'on', 'once', 'removeListener']), + ], + [ + 'Worker', + new Set(['emit', 'listenerCount', 'off', 'on', 'once', 'removeListener']), + ], +]); + /** * @param {!Documentation} actual * @param {!Documentation} expected @@ -204,14 +230,24 @@ function compareDocumentations(actual, expected) { const methodDiff = diff(actualMethods, expectedMethods); for (const methodName of methodDiff.extra) { - const missingMethodsForClass = expectedNonExistingMethods.get(className); - if (missingMethodsForClass && missingMethodsForClass.has(methodName)) + const nonExistingMethodsForClass = expectedNonExistingMethods.get( + className + ); + if ( + nonExistingMethodsForClass && + nonExistingMethodsForClass.has(methodName) + ) continue; errors.push(`Non-existing method found: ${className}.${methodName}()`); } - for (const methodName of methodDiff.missing) + + for (const methodName of methodDiff.missing) { + const missingMethodsForClass = expectedNotFoundMethods.get(className); + if (missingMethodsForClass && missingMethodsForClass.has(methodName)) + continue; errors.push(`Method not found: ${className}.${methodName}()`); + } for (const methodName of methodDiff.equal) { const actualMethod = actualClass.methods.get(methodName); @@ -611,6 +647,48 @@ function compareDocumentations(actual, expected) { expectedName: 'VisionDeficiency', }, ], + [ + 'Method EventEmitter.emit() event', + { + actualName: 'string|symbol', + expectedName: 'Object', + }, + ], + [ + 'Method EventEmitter.listenerCount() event', + { + actualName: 'string|symbol', + expectedName: 'Object', + }, + ], + [ + 'Method EventEmitter.off() event', + { + actualName: 'string|symbol', + expectedName: 'Object', + }, + ], + [ + 'Method EventEmitter.on() event', + { + actualName: 'string|symbol', + expectedName: 'Object', + }, + ], + [ + 'Method EventEmitter.once() event', + { + actualName: 'string|symbol', + expectedName: 'Object', + }, + ], + [ + 'Method EventEmitter.removeListener() event', + { + actualName: 'string|symbol', + expectedName: 'Object', + }, + ], ]); const expectedForSource = expectedNamingMismatches.get(source);