Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: add support for async waitForTarget #7885

Merged
merged 3 commits into from Feb 18, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
4 changes: 2 additions & 2 deletions docs/api.md
Expand Up @@ -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<boolean>]> 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.
Expand Down Expand Up @@ -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<boolean>]> 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.
Expand Down
27 changes: 19 additions & 8 deletions experimental/puppeteer-firefox/lib/Browser.js
Expand Up @@ -114,25 +114,36 @@ class Browser extends EventEmitter {
}

/**
* @param {function(!Target):boolean} predicate
* @param {function(!Target):boolean|Promise<boolean>} predicate
* @param {{timeout?: number}=} options
* @return {!Promise<!Target>}
*/
async waitForTarget(predicate, options = {}) {
const {
timeout = 30000
} = options;
const existingTarget = this.targets().find(predicate);
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)
return await targetPromise;
return await helper.waitWithTimeout(targetPromise, 'target', timeout);
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);
Expand All @@ -141,8 +152,8 @@ class Browser extends EventEmitter {
/**
* @param {!Target} target
*/
function check(target) {
if (predicate(target))
async function check(target) {
if (await predicate(target))
resolve(target);
}
}
Expand Down Expand Up @@ -334,7 +345,7 @@ class BrowserContext extends EventEmitter {
}

/**
* @param {function(Target):boolean} predicate
* @param {function(Target):boolean|Promise<boolean>} predicate
* @param {{timeout?: number}=} options
* @return {!Promise<Target>}
*/
Expand Down
22 changes: 15 additions & 7 deletions src/common/Browser.ts
Expand Up @@ -539,20 +539,28 @@ export class Browser extends EventEmitter {
* ```
*/
async waitForTarget(
predicate: (x: Target) => boolean,
predicate: (x: Target) => boolean | Promise<boolean>,
options: WaitForTargetOptions = {}
): Promise<Target> {
const { timeout = 30000 } = options;
const existingTarget = this.targets().find(predicate);
if (existingTarget) return existingTarget;
let resolve: (value: Target | PromiseLike<Target>) => void;
const targetPromise = new Promise<Target>((x) => (resolve = x));
this.on(BrowserEmittedEvents.TargetCreated, check);
this.on(BrowserEmittedEvents.TargetChanged, check);
try {
if (!timeout) return await targetPromise;
return await helper.waitWithTimeout<Target>(
targetPromise,
Promise.race([
targetPromise,
(async () => {
for (const target of this.targets()) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ugh, additional edge cases come to mind. WDYT about the following: we start waiting for a target and start checking existing targets one by one (and say there is a hundred of targets), then a matching target gets created and we resolve the waitForTarget with it. Although we resolved the inner promise continues executing and keeps checking the remaining targets one by one. I think that should be addressed by checking if the waitWithTimeout has been resolved and returning earlier.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think let's leave this for a follow-up (seems to be rather an edge case)

if (await predicate(target)) {
return target;
}
}
await targetPromise;
})(),
]),
'target',
timeout
);
Expand All @@ -561,8 +569,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<void> {
if (await predicate(target)) resolve(target);
}
}

Expand Down Expand Up @@ -736,7 +744,7 @@ export class BrowserContext extends EventEmitter {
* that matches the `predicate` function.
*/
waitForTarget(
predicate: (x: Target) => boolean,
predicate: (x: Target) => boolean | Promise<boolean>,
options: { timeout?: number } = {}
): Promise<Target> {
return this._browser.waitForTarget(
Expand Down
24 changes: 24 additions & 0 deletions test/target.spec.ts
Expand Up @@ -67,6 +67,30 @@ 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()).toEqual(
server.CROSS_PROCESS_PREFIX + '/empty.html'
);
expect(page).not.toEqual(otherPage);
});
itFailsFirefox(
'should report when a new page is created and closed',
async () => {
Expand Down