From 1a57ba22a86c9bb686021e4113f62662e39dde3a Mon Sep 17 00:00:00 2001 From: Jack Franklin Date: Fri, 17 Apr 2020 10:32:56 +0100 Subject: [PATCH] chore: Migrate TaskQueue to TypeScript (#5658) This is a simple module but took a bit of work because: * It wraps a Promise that can return basically anything. In a pure TS codebase we'd solve these with generics, so you could do `new TaskQueue` where `T` will be what's returned from the queue, but because we're calling that from JS we can't yet. I've left a TODO and once we migrate the call sites to TS we can do a much better job than the `void | any` type I've gone with for now. * It was used in typedefs via `Puppeteer.TaskQueue`. I've removed that entry from `externs.d.ts` in favour of importing it and using the type directly. This does mean that we have imports that ESLint doesn't realiase are actually used but I think this is better than maintaining `externs.d.ts`. --- .eslintrc.js | 3 ++- src/Page.js | 9 +++++++-- src/Target.js | 6 +++++- src/TaskQueue.js | 17 ----------------- src/TaskQueue.ts | 36 ++++++++++++++++++++++++++++++++++++ src/externs.d.ts | 2 -- 6 files changed, 50 insertions(+), 23 deletions(-) delete mode 100644 src/TaskQueue.js create mode 100644 src/TaskQueue.ts diff --git a/.eslintrc.js b/.eslintrc.js index 85305e5027cc1..6677501ea9f2f 100644 --- a/.eslintrc.js +++ b/.eslintrc.js @@ -111,7 +111,8 @@ module.exports = { "no-unused-vars": 0, "@typescript-eslint/no-unused-vars": 2, "semi": 0, - "@typescript-eslint/semi": 2 + "@typescript-eslint/semi": 2, + "@typescript-eslint/no-empty-function": 0 } } ] diff --git a/src/Page.js b/src/Page.js index cb20013755251..e06da26885ced 100644 --- a/src/Page.js +++ b/src/Page.js @@ -30,6 +30,11 @@ const {Worker: PuppeteerWorker} = require('./Worker'); const {createJSHandle} = require('./JSHandle'); const {Accessibility} = require('./Accessibility'); const {TimeoutSettings} = require('./TimeoutSettings'); + +// This import is used as a TypeDef, but ESLint's rule doesn't +// understand that unfortunately. +// eslint-disable-next-line no-unused-vars +const {TaskQueue} = require('./TaskQueue'); const writeFileAsync = helper.promisify(fs.writeFile); class Page extends EventEmitter { @@ -38,7 +43,7 @@ class Page extends EventEmitter { * @param {!Puppeteer.Target} target * @param {boolean} ignoreHTTPSErrors * @param {?Puppeteer.Viewport} defaultViewport - * @param {!Puppeteer.TaskQueue} screenshotTaskQueue + * @param {!TaskQueue} screenshotTaskQueue * @return {!Promise} */ static async create(client, target, ignoreHTTPSErrors, defaultViewport, screenshotTaskQueue) { @@ -53,7 +58,7 @@ class Page extends EventEmitter { * @param {!Puppeteer.CDPSession} client * @param {!Puppeteer.Target} target * @param {boolean} ignoreHTTPSErrors - * @param {!Puppeteer.TaskQueue} screenshotTaskQueue + * @param {!TaskQueue} screenshotTaskQueue */ constructor(client, target, ignoreHTTPSErrors, screenshotTaskQueue) { super(); diff --git a/src/Target.js b/src/Target.js index 3b3ae42a06799..caf2627b44721 100644 --- a/src/Target.js +++ b/src/Target.js @@ -17,6 +17,10 @@ const {Events} = require('./Events'); const {Page} = require('./Page'); const {Worker: PuppeteerWorker} = require('./Worker'); +// This import is used as a TypeDef, but ESLint's rule doesn't +// understand that unfortunately. +// eslint-disable-next-line no-unused-vars +const {TaskQueue} = require('./TaskQueue'); class Target { /** @@ -25,7 +29,7 @@ class Target { * @param {!function():!Promise} sessionFactory * @param {boolean} ignoreHTTPSErrors * @param {?Puppeteer.Viewport} defaultViewport - * @param {!Puppeteer.TaskQueue} screenshotTaskQueue + * @param {!TaskQueue} screenshotTaskQueue */ constructor(targetInfo, browserContext, sessionFactory, ignoreHTTPSErrors, defaultViewport, screenshotTaskQueue) { this._targetInfo = targetInfo; diff --git a/src/TaskQueue.js b/src/TaskQueue.js deleted file mode 100644 index 8cfb7a726e1eb..0000000000000 --- a/src/TaskQueue.js +++ /dev/null @@ -1,17 +0,0 @@ -class TaskQueue { - constructor() { - this._chain = Promise.resolve(); - } - - /** - * @param {Function} task - * @return {!Promise} - */ - postTask(task) { - const result = this._chain.then(task); - this._chain = result.catch(() => {}); - return result; - } -} - -module.exports = {TaskQueue}; \ No newline at end of file diff --git a/src/TaskQueue.ts b/src/TaskQueue.ts new file mode 100644 index 0000000000000..e7ac6ba1fade7 --- /dev/null +++ b/src/TaskQueue.ts @@ -0,0 +1,36 @@ +/** + * 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. + */ +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; + } +} + +export = { TaskQueue }; diff --git a/src/externs.d.ts b/src/externs.d.ts index 8cfb97ad55b46..7c0a2c564ab12 100644 --- a/src/externs.d.ts +++ b/src/externs.d.ts @@ -2,7 +2,6 @@ import { Connection as RealConnection, CDPSession as RealCDPSession } from './Co import { Browser as RealBrowser, BrowserContext as RealBrowserContext} from './Browser.js'; import {Target as RealTarget} from './Target.js'; import {Page as RealPage} from './Page.js'; -import {TaskQueue as RealTaskQueue} from './TaskQueue.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 {JSHandle as RealJSHandle, ElementHandle as RealElementHandle} from './JSHandle.js'; @@ -21,7 +20,6 @@ declare global { export class Mouse extends RealMouse {} export class Keyboard extends RealKeyboard {} export class Touchscreen extends RealTouchscreen {} - export class TaskQueue extends RealTaskQueue {} export class Browser extends RealBrowser {} export class BrowserContext extends RealBrowserContext {} export class Target extends RealTarget {}