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<string | Buffer
| void>` 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

@@ -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),

src/JSHandle.ts

@@ -454,7 +454,7 @@ export class ElementHandle extends JSHandle {
     };
   }
 
-  async screenshot(options = {}): Promise<string | Buffer> {
+  async screenshot(options = {}): Promise<string | Buffer | void> {
     let needsViewportReset = false;
 
     let boundingBox = await this.boundingBox();

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<string, PaperFormat> = {
   a6: { width: 4.13, height: 5.83 },
 } as const;
 
+class ScreenshotTaskQueue {
+  _chain: Promise<Buffer | string | void>;
+
+  constructor() {
+    this._chain = Promise.resolve<Buffer | string | void>(undefined);
+  }
+
+  public postTask(
+    task: () => Promise<Buffer | string>
+  ): Promise<Buffer | string | void> {
+    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<Page> {
-    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<string, PuppeteerWorker>();
   // TODO: improve this typedef - it's a function that takes a file chooser or something?
   _fileChooserInterceptors = new Set<Function>();
 
   _disconnectPromise?: Promise<Error>;
 
-  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<Buffer | string> {
+  async screenshot(
+    options: ScreenshotOptions = {}
+  ): Promise<Buffer | string | void> {
     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)
     );
   }
 

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<CDPSession>;
   _ignoreHTTPSErrors: boolean;
   _defaultViewport?: Viewport;
-  _screenshotTaskQueue: TaskQueue;
   _pagePromise?: Promise<Page>;
   _workerPromise?: Promise<PuppeteerWorker>;
   _initializedPromise: Promise<boolean>;
@@ -43,8 +41,7 @@ export class Target {
     browserContext: BrowserContext,
     sessionFactory: () => Promise<CDPSession>,
     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<!Puppeteer.Page>} */
     this._pagePromise = null;
     /** @type {?Promise<!PuppeteerWorker>} */
@@ -93,8 +89,7 @@ export class Target {
           client,
           this,
           this._ignoreHTTPSErrors,
-          this._defaultViewport,
-          this._screenshotTaskQueue
+          this._defaultViewport
         )
       );
     }

src/TaskQueue.ts

@@ -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<T> and let the caller tell us what types
- * the promise in the queue should return.
- */
-export class TaskQueue {
-  _chain: Promise<void | any>;
-
-  constructor() {
-    this._chain = Promise.resolve();
-  }
-
-  public postTask(task: () => any): Promise<void | any> {
-    const result = this._chain.then(task);
-    this._chain = result.catch(() => {});
-    return result;
-  }
-}