Skip to content

Commit

Permalink
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
jackfranklin committed May 7, 2020
1 parent 4fdb1e3 commit 49ce659
Show file tree
Hide file tree
Showing 5 changed files with 30 additions and 67 deletions.
6 changes: 1 addition & 5 deletions src/Browser.ts
Expand Up @@ -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';
Expand Down Expand Up @@ -49,7 +48,6 @@ export class Browser extends EventEmitter {
_ignoreHTTPSErrors: boolean;
_defaultViewport?: Viewport;
_process?: ChildProcess;
_screenshotTaskQueue = new TaskQueue();
_connection: Connection;
_closeCallback: BrowserCloseCallback;
_defaultContext: BrowserContext;
Expand All @@ -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 {};

Expand Down Expand Up @@ -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),
Expand Down
2 changes: 1 addition & 1 deletion src/JSHandle.ts
Expand Up @@ -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();
Expand Down
46 changes: 26 additions & 20 deletions src/Page.ts
Expand Up @@ -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);

Expand Down Expand Up @@ -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;
Expand All @@ -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;
Expand All @@ -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) => {
Expand Down Expand Up @@ -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).
Expand Down Expand Up @@ -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)
);
}

Expand Down
9 changes: 2 additions & 7 deletions src/Target.ts
Expand Up @@ -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';

Expand All @@ -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>;
Expand All @@ -43,16 +41,14 @@ export class Target {
browserContext: BrowserContext,
sessionFactory: () => Promise<CDPSession>,
ignoreHTTPSErrors: boolean,
defaultViewport: Viewport | null,
screenshotTaskQueue: TaskQueue
defaultViewport: Viewport | null
) {
this._targetInfo = targetInfo;
this._browserContext = browserContext;
this._targetId = targetInfo.targetId;
this._sessionFactory = sessionFactory;
this._ignoreHTTPSErrors = ignoreHTTPSErrors;
this._defaultViewport = defaultViewport;
this._screenshotTaskQueue = screenshotTaskQueue;
/** @type {?Promise<!Puppeteer.Page>} */
this._pagePromise = null;
/** @type {?Promise<!PuppeteerWorker>} */
Expand Down Expand Up @@ -93,8 +89,7 @@ export class Target {
client,
this,
this._ignoreHTTPSErrors,
this._defaultViewport,
this._screenshotTaskQueue
this._defaultViewport
)
);
}
Expand Down
34 changes: 0 additions & 34 deletions src/TaskQueue.ts

This file was deleted.

0 comments on commit 49ce659

Please sign in to comment.