From 49ce65943c9fafd6988df94e3cf513c95d2b1802 Mon Sep 17 00:00:00 2001 From: Jack Franklin Date: Thu, 7 May 2020 14:49:42 +0100 Subject: [PATCH] chore: remove src/TaskQueue (#5826) * chore: Remove src/TaskQueue The only place it's used is in `src/Page.ts` to have a chain of screenshot promises. Rather than initialize a task queue in `Browser` and pass it through a chain of constructors we instead move the class into `src/Page` and define it inline. In the future we might want to create a helpers folder to contain small utilities like that (`src/Page.ts` is already far too large) but I'm leaving that for a future PR. `TaskQueue` isn't documented in `api.md` so I don't think this is a breaking change. I updated the type of `screenshot()` to return `Promise` because if a promise rejects it's silently swallowed. I'd like to change this behaviour but one step at a time. This type only had to change as now we type the screenshot task queue correctly rather than using `any` which then exposed the incorrect `screenshot()` types. --- src/Browser.ts | 6 +----- src/JSHandle.ts | 2 +- src/Page.ts | 46 ++++++++++++++++++++++++++-------------------- src/Target.ts | 9 ++------- src/TaskQueue.ts | 34 ---------------------------------- 5 files changed, 30 insertions(+), 67 deletions(-) delete mode 100644 src/TaskQueue.ts diff --git a/src/Browser.ts b/src/Browser.ts index 2dcd11651eb..cdc06039016 100644 --- a/src/Browser.ts +++ b/src/Browser.ts @@ -17,7 +17,6 @@ import { helper, assert } from './helper'; import { Target } from './Target'; import * as EventEmitter from 'events'; -import { TaskQueue } from './TaskQueue'; import { Events } from './Events'; import { Connection } from './Connection'; import { Page } from './Page'; @@ -49,7 +48,6 @@ export class Browser extends EventEmitter { _ignoreHTTPSErrors: boolean; _defaultViewport?: Viewport; _process?: ChildProcess; - _screenshotTaskQueue = new TaskQueue(); _connection: Connection; _closeCallback: BrowserCloseCallback; _defaultContext: BrowserContext; @@ -69,7 +67,6 @@ export class Browser extends EventEmitter { this._ignoreHTTPSErrors = ignoreHTTPSErrors; this._defaultViewport = defaultViewport; this._process = process; - this._screenshotTaskQueue = new TaskQueue(); this._connection = connection; this._closeCallback = closeCallback || function (): void {}; @@ -146,8 +143,7 @@ export class Browser extends EventEmitter { context, () => this._connection.createSession(targetInfo), this._ignoreHTTPSErrors, - this._defaultViewport, - this._screenshotTaskQueue + this._defaultViewport ); assert( !this._targets.has(event.targetInfo.targetId), diff --git a/src/JSHandle.ts b/src/JSHandle.ts index 71e15c8fcde..be78fa7da22 100644 --- a/src/JSHandle.ts +++ b/src/JSHandle.ts @@ -454,7 +454,7 @@ export class ElementHandle extends JSHandle { }; } - async screenshot(options = {}): Promise { + async screenshot(options = {}): Promise { let needsViewportReset = false; let boundingBox = await this.boundingBox(); diff --git a/src/Page.ts b/src/Page.ts index 7e370ade006..c7ed0e70bd1 100644 --- a/src/Page.ts +++ b/src/Page.ts @@ -39,7 +39,6 @@ import { import { Accessibility } from './Accessibility'; import { TimeoutSettings } from './TimeoutSettings'; import { PuppeteerLifeCycleEvent } from './LifecycleWatcher'; -import { TaskQueue } from './TaskQueue'; const writeFileAsync = helper.promisify(fs.writeFile); @@ -128,20 +127,30 @@ const paperFormats: Record = { a6: { width: 4.13, height: 5.83 }, } as const; +class ScreenshotTaskQueue { + _chain: Promise; + + constructor() { + this._chain = Promise.resolve(undefined); + } + + public postTask( + task: () => Promise + ): Promise { + const result = this._chain.then(task); + this._chain = result.catch(() => {}); + return result; + } +} + export class Page extends EventEmitter { static async create( client: CDPSession, target: Target, ignoreHTTPSErrors: boolean, - defaultViewport: Viewport | null, - screenshotTaskQueue: TaskQueue + defaultViewport: Viewport | null ): Promise { - const page = new Page( - client, - target, - ignoreHTTPSErrors, - screenshotTaskQueue - ); + const page = new Page(client, target, ignoreHTTPSErrors); await page._initialize(); if (defaultViewport) await page.setViewport(defaultViewport); return page; @@ -162,19 +171,14 @@ export class Page extends EventEmitter { _coverage: Coverage; _javascriptEnabled = true; _viewport: Viewport | null; - _screenshotTaskQueue: TaskQueue; + _screenshotTaskQueue: ScreenshotTaskQueue; _workers = new Map(); // TODO: improve this typedef - it's a function that takes a file chooser or something? _fileChooserInterceptors = new Set(); _disconnectPromise?: Promise; - constructor( - client: CDPSession, - target: Target, - ignoreHTTPSErrors: boolean, - screenshotTaskQueue: TaskQueue - ) { + constructor(client: CDPSession, target: Target, ignoreHTTPSErrors: boolean) { super(); this._client = client; this._target = target; @@ -191,7 +195,7 @@ export class Page extends EventEmitter { this._emulationManager = new EmulationManager(client); this._tracing = new Tracing(client); this._coverage = new Coverage(client); - this._screenshotTaskQueue = screenshotTaskQueue; + this._screenshotTaskQueue = new ScreenshotTaskQueue(); this._viewport = null; client.on('Target.attachedToTarget', (event) => { @@ -948,7 +952,9 @@ export class Page extends EventEmitter { await this._frameManager.networkManager().setCacheEnabled(enabled); } - async screenshot(options: ScreenshotOptions = {}): Promise { + async screenshot( + options: ScreenshotOptions = {} + ): Promise { let screenshotType = null; // options.type takes precedence over inferring the type from options.path // because it may be a 0-length file with no extension created beforehand (i.e. as a temp file). @@ -1023,8 +1029,8 @@ export class Page extends EventEmitter { 'Expected options.clip.height not to be 0.' ); } - return this._screenshotTaskQueue.postTask( - this._screenshotTask.bind(this, screenshotType, options) + return this._screenshotTaskQueue.postTask(() => + this._screenshotTask(screenshotType, options) ); } diff --git a/src/Target.ts b/src/Target.ts index 894d3fa8f13..7c00fe868c4 100644 --- a/src/Target.ts +++ b/src/Target.ts @@ -18,7 +18,6 @@ import { Events } from './Events'; import { Page } from './Page'; import { Worker as PuppeteerWorker } from './Worker'; import { CDPSession } from './Connection'; -import { TaskQueue } from './TaskQueue'; import { Browser, BrowserContext } from './Browser'; import type { Viewport } from './PuppeteerViewport'; @@ -29,7 +28,6 @@ export class Target { _sessionFactory: () => Promise; _ignoreHTTPSErrors: boolean; _defaultViewport?: Viewport; - _screenshotTaskQueue: TaskQueue; _pagePromise?: Promise; _workerPromise?: Promise; _initializedPromise: Promise; @@ -43,8 +41,7 @@ export class Target { browserContext: BrowserContext, sessionFactory: () => Promise, ignoreHTTPSErrors: boolean, - defaultViewport: Viewport | null, - screenshotTaskQueue: TaskQueue + defaultViewport: Viewport | null ) { this._targetInfo = targetInfo; this._browserContext = browserContext; @@ -52,7 +49,6 @@ export class Target { this._sessionFactory = sessionFactory; this._ignoreHTTPSErrors = ignoreHTTPSErrors; this._defaultViewport = defaultViewport; - this._screenshotTaskQueue = screenshotTaskQueue; /** @type {?Promise} */ this._pagePromise = null; /** @type {?Promise} */ @@ -93,8 +89,7 @@ export class Target { client, this, this._ignoreHTTPSErrors, - this._defaultViewport, - this._screenshotTaskQueue + this._defaultViewport ) ); } diff --git a/src/TaskQueue.ts b/src/TaskQueue.ts deleted file mode 100644 index e3b8f1549ca..00000000000 --- a/src/TaskQueue.ts +++ /dev/null @@ -1,34 +0,0 @@ -/** - * Copyright 2020 Google Inc. All rights reserved. - * - * Licensed under the Apache License, Version 2.0 (the "License"); - * you may not use this file except in compliance with the License. - * You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, software - * distributed under the License is distributed on an "AS IS" BASIS, - * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. - * See the License for the specific language governing permissions and - * limitations under the License. - */ - -/* TODO(jacktfranklin@): once we are calling this from TS files we can - * avoid the horrible void | any type and instead make use of generics - * to make this into TaskQueue and let the caller tell us what types - * the promise in the queue should return. - */ -export class TaskQueue { - _chain: Promise; - - constructor() { - this._chain = Promise.resolve(); - } - - public postTask(task: () => any): Promise { - const result = this._chain.then(task); - this._chain = result.catch(() => {}); - return result; - } -}