From c8feb3e472e8e5d2aef70a1dd5d057de8da9050d Mon Sep 17 00:00:00 2001 From: Nikolay Vitkov <34244704+Lightning00Blade@users.noreply.github.com> Date: Wed, 11 Oct 2023 10:11:22 +0200 Subject: [PATCH] refactor: use RxJS for waitForTarget (#11029) --- .../puppeteer.browsercontext.waitfortarget.md | 6 +-- packages/puppeteer-core/src/api/Browser.ts | 47 ++++++++----------- .../puppeteer-core/src/api/BrowserContext.ts | 4 +- .../puppeteer-core/src/bidi/BrowserContext.ts | 3 +- .../puppeteer-core/src/bidi/Connection.ts | 5 +- packages/puppeteer-core/src/cdp/Browser.ts | 3 +- test/src/navigation.spec.ts | 22 ++++++--- 7 files changed, 46 insertions(+), 44 deletions(-) diff --git a/docs/api/puppeteer.browsercontext.waitfortarget.md b/docs/api/puppeteer.browsercontext.waitfortarget.md index 690874e8..c8307135 100644 --- a/docs/api/puppeteer.browsercontext.waitfortarget.md +++ b/docs/api/puppeteer.browsercontext.waitfortarget.md @@ -14,9 +14,7 @@ This will look all open [browser contexts](./puppeteer.browsercontext.md). class BrowserContext { abstract waitForTarget( predicate: (x: Target) => boolean | Promise, - options?: { - timeout?: number; - } + options?: WaitForTargetOptions ): Promise; } ``` @@ -26,7 +24,7 @@ class BrowserContext { | Parameter | Type | Description | | --------- | ---------------------------------------------------------------------------- | ------------ | | predicate | (x: [Target](./puppeteer.target.md)) => boolean \| Promise<boolean> | | -| options | { timeout?: number; } | _(Optional)_ | +| options | [WaitForTargetOptions](./puppeteer.waitfortargetoptions.md) | _(Optional)_ | **Returns:** diff --git a/packages/puppeteer-core/src/api/Browser.ts b/packages/puppeteer-core/src/api/Browser.ts index 1dc46ba4..22e6ad58 100644 --- a/packages/puppeteer-core/src/api/Browser.ts +++ b/packages/puppeteer-core/src/api/Browser.ts @@ -18,15 +18,23 @@ import type {ChildProcess} from 'child_process'; import type {Protocol} from 'devtools-protocol'; +import { + firstValueFrom, + from, + merge, + raceWith, + filterAsync, + fromEvent, + type Observable, +} from '../../third_party/rxjs/rxjs.js'; import {EventEmitter, type EventType} from '../common/EventEmitter.js'; -import {debugError, waitWithTimeout} from '../common/util.js'; -import {Deferred} from '../util/Deferred.js'; +import {debugError} from '../common/util.js'; +import {timeout} from '../common/util.js'; import {asyncDisposeSymbol, disposeSymbol} from '../util/disposable.js'; import type {BrowserContext} from './BrowserContext.js'; import type {Page} from './Page.js'; import type {Target} from './Target.js'; - /** * @public */ @@ -387,31 +395,14 @@ export abstract class Browser extends EventEmitter { predicate: (x: Target) => boolean | Promise, options: WaitForTargetOptions = {} ): Promise { - const {timeout = 30000} = options; - const targetDeferred = Deferred.create>(); - - this.on(BrowserEvent.TargetCreated, check); - this.on(BrowserEvent.TargetChanged, check); - try { - this.targets().forEach(check); - if (!timeout) { - return await targetDeferred.valueOrThrow(); - } - return await waitWithTimeout( - targetDeferred.valueOrThrow(), - 'target', - timeout - ); - } finally { - this.off(BrowserEvent.TargetCreated, check); - this.off(BrowserEvent.TargetChanged, check); - } - - async function check(target: Target): Promise { - if ((await predicate(target)) && !targetDeferred.resolved()) { - targetDeferred.resolve(target); - } - } + const {timeout: ms = 30000} = options; + return await firstValueFrom( + merge( + fromEvent(this, BrowserEvent.TargetCreated) as Observable, + fromEvent(this, BrowserEvent.TargetChanged) as Observable, + from(this.targets()) + ).pipe(filterAsync(predicate), raceWith(timeout(ms))) + ); } /** diff --git a/packages/puppeteer-core/src/api/BrowserContext.ts b/packages/puppeteer-core/src/api/BrowserContext.ts index d183000c..05e36701 100644 --- a/packages/puppeteer-core/src/api/BrowserContext.ts +++ b/packages/puppeteer-core/src/api/BrowserContext.ts @@ -18,7 +18,7 @@ import {EventEmitter, type EventType} from '../common/EventEmitter.js'; import {debugError} from '../common/util.js'; import {asyncDisposeSymbol, disposeSymbol} from '../util/disposable.js'; -import type {Browser, Permission} from './Browser.js'; +import type {Browser, Permission, WaitForTargetOptions} from './Browser.js'; import type {Page} from './Page.js'; import type {Target} from './Target.js'; @@ -128,7 +128,7 @@ export abstract class BrowserContext extends EventEmitter */ abstract waitForTarget( predicate: (x: Target) => boolean | Promise, - options?: {timeout?: number} + options?: WaitForTargetOptions ): Promise; /** diff --git a/packages/puppeteer-core/src/bidi/BrowserContext.ts b/packages/puppeteer-core/src/bidi/BrowserContext.ts index 7a1fe2c0..dbab820c 100644 --- a/packages/puppeteer-core/src/bidi/BrowserContext.ts +++ b/packages/puppeteer-core/src/bidi/BrowserContext.ts @@ -16,6 +16,7 @@ import * as Bidi from 'chromium-bidi/lib/cjs/protocol/protocol.js'; +import type {WaitForTargetOptions} from '../api/Browser.js'; import {BrowserContext} from '../api/BrowserContext.js'; import type {Page} from '../api/Page.js'; import type {Target} from '../api/Target.js'; @@ -58,7 +59,7 @@ export class BidiBrowserContext extends BrowserContext { override waitForTarget( predicate: (x: Target) => boolean | Promise, - options: {timeout?: number} = {} + options: WaitForTargetOptions = {} ): Promise { return this.#browser.waitForTarget(target => { return target.browserContext() === this && predicate(target); diff --git a/packages/puppeteer-core/src/bidi/Connection.ts b/packages/puppeteer-core/src/bidi/Connection.ts index d86a08fc..40058bb1 100644 --- a/packages/puppeteer-core/src/bidi/Connection.ts +++ b/packages/puppeteer-core/src/bidi/Connection.ts @@ -288,8 +288,9 @@ export class BidiConnection extends EventEmitter { return; } this.#closed = true; - this.#transport.onmessage = undefined; - this.#transport.onclose = undefined; + // Both may still be invoked and produce errors + this.#transport.onmessage = () => {}; + this.#transport.onclose = () => {}; this.#callbacks.clear(); } diff --git a/packages/puppeteer-core/src/cdp/Browser.ts b/packages/puppeteer-core/src/cdp/Browser.ts index 6074d2b1..3240c6eb 100644 --- a/packages/puppeteer-core/src/cdp/Browser.ts +++ b/packages/puppeteer-core/src/cdp/Browser.ts @@ -27,6 +27,7 @@ import { type IsPageTargetCallback, type Permission, type TargetFilterCallback, + type WaitForTargetOptions, } from '../api/Browser.js'; import {BrowserContext, BrowserContextEvent} from '../api/BrowserContext.js'; import {CDPSessionEvent, type CDPSession} from '../api/CDPSession.js'; @@ -453,7 +454,7 @@ export class CdpBrowserContext extends BrowserContext { override waitForTarget( predicate: (x: Target) => boolean | Promise, - options: {timeout?: number} = {} + options: WaitForTargetOptions = {} ): Promise { return this.#browser.waitForTarget(target => { return target.browserContext() === this && predicate(target); diff --git a/test/src/navigation.spec.ts b/test/src/navigation.spec.ts index a18a8ae1..bbabbb5c 100644 --- a/test/src/navigation.spec.ts +++ b/test/src/navigation.spec.ts @@ -19,6 +19,7 @@ import type {ServerResponse} from 'http'; import expect from 'expect'; import {type Frame, TimeoutError} from 'puppeteer'; import type {HTTPRequest} from 'puppeteer-core/internal/api/HTTPRequest.js'; +import type {HTTPResponse} from 'puppeteer-core/internal/api/HTTPResponse.js'; import {Deferred} from 'puppeteer-core/internal/util/Deferred.js'; import {getTestState, setupTestBrowserHooks} from './mocha-utils.js'; @@ -837,18 +838,27 @@ describe('navigation', function () { server.setRoute('/one-style.html', (_req, res) => { return serverResponses.push(res); }); - const navigations = []; + const navigations: Array> = []; for (let i = 0; i < 3; ++i) { navigations.push(frames[i]!.goto(server.PREFIX + '/one-style.html')); await server.waitForRequest('/one-style.html'); } // Respond from server out-of-order. const serverResponseTexts = ['AAA', 'BBB', 'CCC']; - for (const i of [1, 2, 0]) { - serverResponses[i]!.end(serverResponseTexts[i]); - const response = (await navigations[i])!; - expect(response.frame()).toBe(frames[i]); - expect(await response.text()).toBe(serverResponseTexts[i]); + try { + for (const i of [1, 2, 0]) { + const response = await getResponse(i); + expect(response.frame()).toBe(frames[i]); + expect(await response.text()).toBe(serverResponseTexts[i]); + } + } catch (error) { + await Promise.all([getResponse(0), getResponse(1), getResponse(2)]); + throw error; + } + + async function getResponse(index: number) { + serverResponses[index]!.end(serverResponseTexts[index]); + return (await navigations[index])!; } }); });