From 425f32a35a99e8cc1c010b37307171082067cb2b Mon Sep 17 00:00:00 2001 From: Tmk Date: Sun, 2 Jan 2022 01:03:19 +0800 Subject: [PATCH 1/3] feat: add support for async waitForTarget --- docs/api.md | 4 ++-- experimental/puppeteer-firefox/lib/Browser.js | 16 +++++++++----- src/common/Browser.ts | 16 +++++++++----- test/target.spec.ts | 21 +++++++++++++++++++ 4 files changed, 45 insertions(+), 12 deletions(-) diff --git a/docs/api.md b/docs/api.md index f89e308aa9c52..bf64c81f3d111 100644 --- a/docs/api.md +++ b/docs/api.md @@ -983,7 +983,7 @@ the method will return an array with all the targets in all browser contexts. #### browser.waitForTarget(predicate[, options]) -- `predicate` <[function]\([Target]\):[boolean]> A function to be run for every target +- `predicate` <[function]\([Target]\):[boolean]|[Promise]> A function to be run for every target - `options` <[Object]> - `timeout` <[number]> Maximum wait time in milliseconds. Pass `0` to disable the timeout. Defaults to 30 seconds. - returns: <[Promise]<[Target]>> Promise which resolves to the first target found that matches the `predicate` function. @@ -1135,7 +1135,7 @@ An array of all active targets inside the browser context. #### browserContext.waitForTarget(predicate[, options]) -- `predicate` <[function]\([Target]\):[boolean]> A function to be run for every target +- `predicate` <[function]\([Target]\):[boolean]|[Promise]> A function to be run for every target - `options` <[Object]> - `timeout` <[number]> Maximum wait time in milliseconds. Pass `0` to disable the timeout. Defaults to 30 seconds. - returns: <[Promise]<[Target]>> Promise which resolves to the first target found that matches the `predicate` function. diff --git a/experimental/puppeteer-firefox/lib/Browser.js b/experimental/puppeteer-firefox/lib/Browser.js index 5d2169fe3969d..63bc56511a5a2 100644 --- a/experimental/puppeteer-firefox/lib/Browser.js +++ b/experimental/puppeteer-firefox/lib/Browser.js @@ -114,7 +114,7 @@ class Browser extends EventEmitter { } /** - * @param {function(!Target):boolean} predicate + * @param {function(!Target):boolean|Promise} predicate * @param {{timeout?: number}=} options * @return {!Promise} */ @@ -122,7 +122,13 @@ class Browser extends EventEmitter { const { timeout = 30000 } = options; - const existingTarget = this.targets().find(predicate); + let existingTarget; + for (const target of this.targets()) { + if (await predicate(target)) { + existingTarget = target; + break; + } + } if (existingTarget) return existingTarget; let resolve; @@ -141,8 +147,8 @@ class Browser extends EventEmitter { /** * @param {!Target} target */ - function check(target) { - if (predicate(target)) + async function check(target) { + if (await predicate(target)) resolve(target); } } @@ -334,7 +340,7 @@ class BrowserContext extends EventEmitter { } /** - * @param {function(Target):boolean} predicate + * @param {function(Target):boolean|Promise} predicate * @param {{timeout?: number}=} options * @return {!Promise} */ diff --git a/src/common/Browser.ts b/src/common/Browser.ts index 497615e996acd..c1dabe134f689 100644 --- a/src/common/Browser.ts +++ b/src/common/Browser.ts @@ -539,11 +539,17 @@ export class Browser extends EventEmitter { * ``` */ async waitForTarget( - predicate: (x: Target) => boolean, + predicate: (x: Target) => boolean | Promise, options: WaitForTargetOptions = {} ): Promise { const { timeout = 30000 } = options; - const existingTarget = this.targets().find(predicate); + let existingTarget: Target | undefined; + for (const target of this.targets()) { + if (await predicate(target)) { + existingTarget = target; + break; + } + } if (existingTarget) return existingTarget; let resolve: (value: Target | PromiseLike) => void; const targetPromise = new Promise((x) => (resolve = x)); @@ -561,8 +567,8 @@ export class Browser extends EventEmitter { this.removeListener(BrowserEmittedEvents.TargetChanged, check); } - function check(target: Target): void { - if (predicate(target)) resolve(target); + async function check(target: Target): Promise { + if (await predicate(target)) resolve(target); } } @@ -736,7 +742,7 @@ export class BrowserContext extends EventEmitter { * that matches the `predicate` function. */ waitForTarget( - predicate: (x: Target) => boolean, + predicate: (x: Target) => boolean | Promise, options: { timeout?: number } = {} ): Promise { return this._browser.waitForTarget( diff --git a/test/target.spec.ts b/test/target.spec.ts index 918510f1f0de4..9035e3b9daf43 100644 --- a/test/target.spec.ts +++ b/test/target.spec.ts @@ -67,6 +67,27 @@ describe('Target', function () { ).toBe('Hello world'); expect(await originalPage.$('body')).toBeTruthy(); }); + itFailsFirefox('should be able to use async waitForTarget', async () => { + const { page, server, context } = getTestState(); + + const [otherPage] = await Promise.all([ + context + .waitForTarget((target) => + target + .page() + .then( + (page) => + page.url() === server.CROSS_PROCESS_PREFIX + '/empty.html' + ) + ) + .then((target) => target.page()), + page.evaluate( + (url: string) => window.open(url), + server.CROSS_PROCESS_PREFIX + '/empty.html' + ), + ]); + expect(otherPage.url()).toContain(server.CROSS_PROCESS_PREFIX); + }); itFailsFirefox( 'should report when a new page is created and closed', async () => { From 26ffdf53209ef90cf50769e02018c9d595bb99f9 Mon Sep 17 00:00:00 2001 From: Tmk Date: Fri, 18 Feb 2022 13:02:07 +0800 Subject: [PATCH 2/3] fix: add timeout --- experimental/puppeteer-firefox/lib/Browser.js | 33 ++++++++++++------- src/common/Browser.ts | 30 ++++++++++++----- 2 files changed, 43 insertions(+), 20 deletions(-) diff --git a/experimental/puppeteer-firefox/lib/Browser.js b/experimental/puppeteer-firefox/lib/Browser.js index 63bc56511a5a2..c8c745c4f18f4 100644 --- a/experimental/puppeteer-firefox/lib/Browser.js +++ b/experimental/puppeteer-firefox/lib/Browser.js @@ -122,23 +122,34 @@ class Browser extends EventEmitter { const { timeout = 30000 } = options; - let existingTarget; - for (const target of this.targets()) { - if (await predicate(target)) { - existingTarget = target; - break; - } - } - if (existingTarget) - return existingTarget; + let remainingTimeout = timeout; + const existingTarget = await helper.waitWithTimeout( + (async () => { + const startTime = Date.now(); + let existingTarget; + for (const target of this.targets()) { + if (await predicate(target)) { + existingTarget = target; + break; + } + } + if (timeout) { + remainingTimeout = Math.max(timeout - (Date.now() - startTime), 1); + } + if (existingTarget) return existingTarget; + })(), + 'predicate', + timeout + ); + if (existingTarget) return existingTarget; let resolve; const targetPromise = new Promise(x => resolve = x); this.on(Events.Browser.TargetCreated, check); this.on('targetchanged', check); try { - if (!timeout) + if (!remainingTimeout) return await targetPromise; - return await helper.waitWithTimeout(targetPromise, 'target', timeout); + return await helper.waitWithTimeout(targetPromise, 'target', remainingTimeout); } finally { this.removeListener(Events.Browser.TargetCreated, check); this.removeListener('targetchanged', check); diff --git a/src/common/Browser.ts b/src/common/Browser.ts index c1dabe134f689..015633ae0bffc 100644 --- a/src/common/Browser.ts +++ b/src/common/Browser.ts @@ -543,24 +543,36 @@ export class Browser extends EventEmitter { options: WaitForTargetOptions = {} ): Promise { const { timeout = 30000 } = options; - let existingTarget: Target | undefined; - for (const target of this.targets()) { - if (await predicate(target)) { - existingTarget = target; - break; - } - } + let remainingTimeout = timeout; + const existingTarget = await helper.waitWithTimeout( + (async () => { + const startTime = Date.now(); + let existingTarget: Target | undefined; + for (const target of this.targets()) { + if (await predicate(target)) { + existingTarget = target; + break; + } + } + if (timeout) { + remainingTimeout = Math.max(timeout - (Date.now() - startTime), 1); + } + if (existingTarget) return existingTarget; + })(), + 'predicate', + timeout + ); if (existingTarget) return existingTarget; let resolve: (value: Target | PromiseLike) => void; const targetPromise = new Promise((x) => (resolve = x)); this.on(BrowserEmittedEvents.TargetCreated, check); this.on(BrowserEmittedEvents.TargetChanged, check); try { - if (!timeout) return await targetPromise; + if (!remainingTimeout) return await targetPromise; return await helper.waitWithTimeout( targetPromise, 'target', - timeout + remainingTimeout ); } finally { this.removeListener(BrowserEmittedEvents.TargetCreated, check); From dc1021c7a08b39bd6391fa3005adcdac3252f80a Mon Sep 17 00:00:00 2001 From: Tmk Date: Fri, 18 Feb 2022 16:49:40 +0800 Subject: [PATCH 3/3] fix: potential async bugs --- experimental/puppeteer-firefox/lib/Browser.js | 38 ++++++++----------- src/common/Browser.ts | 36 +++++++----------- test/target.spec.ts | 5 ++- 3 files changed, 33 insertions(+), 46 deletions(-) diff --git a/experimental/puppeteer-firefox/lib/Browser.js b/experimental/puppeteer-firefox/lib/Browser.js index c8c745c4f18f4..fd234500e2c9c 100644 --- a/experimental/puppeteer-firefox/lib/Browser.js +++ b/experimental/puppeteer-firefox/lib/Browser.js @@ -122,34 +122,28 @@ class Browser extends EventEmitter { const { timeout = 30000 } = options; - let remainingTimeout = timeout; - const existingTarget = await helper.waitWithTimeout( - (async () => { - const startTime = Date.now(); - let existingTarget; - for (const target of this.targets()) { - if (await predicate(target)) { - existingTarget = target; - break; - } - } - if (timeout) { - remainingTimeout = Math.max(timeout - (Date.now() - startTime), 1); - } - if (existingTarget) return existingTarget; - })(), - 'predicate', - timeout - ); - if (existingTarget) return existingTarget; let resolve; const targetPromise = new Promise(x => resolve = x); this.on(Events.Browser.TargetCreated, check); this.on('targetchanged', check); try { - if (!remainingTimeout) + if (!timeout) return await targetPromise; - return await helper.waitWithTimeout(targetPromise, 'target', remainingTimeout); + return await helper.waitWithTimeout( + Promise.race([ + targetPromise, + (async () => { + for (const target of this.targets()) { + if (await predicate(target)) { + return target; + } + } + await targetPromise; + })(), + ]), + 'target', + timeout + ); } finally { this.removeListener(Events.Browser.TargetCreated, check); this.removeListener('targetchanged', check); diff --git a/src/common/Browser.ts b/src/common/Browser.ts index 015633ae0bffc..36dde2bf79dad 100644 --- a/src/common/Browser.ts +++ b/src/common/Browser.ts @@ -543,36 +543,26 @@ export class Browser extends EventEmitter { options: WaitForTargetOptions = {} ): Promise { const { timeout = 30000 } = options; - let remainingTimeout = timeout; - const existingTarget = await helper.waitWithTimeout( - (async () => { - const startTime = Date.now(); - let existingTarget: Target | undefined; - for (const target of this.targets()) { - if (await predicate(target)) { - existingTarget = target; - break; - } - } - if (timeout) { - remainingTimeout = Math.max(timeout - (Date.now() - startTime), 1); - } - if (existingTarget) return existingTarget; - })(), - 'predicate', - timeout - ); - if (existingTarget) return existingTarget; let resolve: (value: Target | PromiseLike) => void; const targetPromise = new Promise((x) => (resolve = x)); this.on(BrowserEmittedEvents.TargetCreated, check); this.on(BrowserEmittedEvents.TargetChanged, check); try { - if (!remainingTimeout) return await targetPromise; + if (!timeout) return await targetPromise; return await helper.waitWithTimeout( - targetPromise, + Promise.race([ + targetPromise, + (async () => { + for (const target of this.targets()) { + if (await predicate(target)) { + return target; + } + } + await targetPromise; + })(), + ]), 'target', - remainingTimeout + timeout ); } finally { this.removeListener(BrowserEmittedEvents.TargetCreated, check); diff --git a/test/target.spec.ts b/test/target.spec.ts index 9035e3b9daf43..d1081d65f838b 100644 --- a/test/target.spec.ts +++ b/test/target.spec.ts @@ -86,7 +86,10 @@ describe('Target', function () { server.CROSS_PROCESS_PREFIX + '/empty.html' ), ]); - expect(otherPage.url()).toContain(server.CROSS_PROCESS_PREFIX); + expect(otherPage.url()).toEqual( + server.CROSS_PROCESS_PREFIX + '/empty.html' + ); + expect(page).not.toEqual(otherPage); }); itFailsFirefox( 'should report when a new page is created and closed',