Skip to content

Commit

Permalink
chore: Migrate TaskQueue to TypeScript (#5658)
Browse files Browse the repository at this point in the history
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<T>` 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`.
  • Loading branch information
jackfranklin committed Apr 17, 2020
1 parent ef3befa commit 1a57ba2
Show file tree
Hide file tree
Showing 6 changed files with 50 additions and 23 deletions.
3 changes: 2 additions & 1 deletion .eslintrc.js
Expand Up @@ -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
}
}
]
Expand Down
9 changes: 7 additions & 2 deletions src/Page.js
Expand Up @@ -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 {
Expand All @@ -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<!Page>}
*/
static async create(client, target, ignoreHTTPSErrors, defaultViewport, screenshotTaskQueue) {
Expand All @@ -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();
Expand Down
6 changes: 5 additions & 1 deletion src/Target.js
Expand Up @@ -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 {
/**
Expand All @@ -25,7 +29,7 @@ class Target {
* @param {!function():!Promise<!Puppeteer.CDPSession>} 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;
Expand Down
17 changes: 0 additions & 17 deletions src/TaskQueue.js

This file was deleted.

36 changes: 36 additions & 0 deletions 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<T> and let the caller tell us what types
* the promise in the queue should return.
*/
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;
}
}

export = { TaskQueue };
2 changes: 0 additions & 2 deletions src/externs.d.ts
Expand Up @@ -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';
Expand All @@ -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 {}
Expand Down

0 comments on commit 1a57ba2

Please sign in to comment.