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.

Sign up for GitHub

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

Jump to bottom

Add await method about issue#7407 #7414

Closed
wants to merge 3 commits into from
Closed

Add await method about issue#7407 #7414

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
e389820
Add await
zorori777 Jul 8, 2021
4d25c81
Merge branch 'main' into fix-issue-7407
zorori777 Jul 17, 2021
0719933
Merge branch 'main' into fix-issue-7407
jschfflr Sep 10, 2021
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
2 changes: 1 addition & 1 deletion src/common/Browser.ts
View file
Open in desktop
Expand Up @@ -433,7 +433,7 @@ export class Browser extends EventEmitter {
url: 'about:blank',
browserContextId: contextId || undefined,
});
const target = this._targets.get(targetId);
const target = await this._targets.get(targetId);
Copy link

Choose a reason for hiding this comment

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

Not sure whether you've looked into the source code but this._targets is a map of Target instances. The assignment happens here:

puppeteer/src/common/Browser.ts

Lines 350 to 361 in 44b22bb

const target = new Target(
targetInfo,
context,
() => this._connection.createSession(targetInfo),
this._ignoreHTTPSErrors,
this._defaultViewport
);
assert(
!this._targets.has(event.targetInfo.targetId),
'Target should not exist before targetCreated'
);
this._targets.set(event.targetInfo.targetId, target);

The target's constructor might be doing some async work, but you can't mark a constructor as async and await the class instantiation (i. e. await new Target(...) is not possible). The instance will be immediately returned and the constructor's async work happens afterwards in the background.

Did you actually try building puppeteer with this change to see whether this fixes the issue for you?

Edit: the only thing I could imagine is that adding the await would defer this line to be executed later (i. e. pushes it to the end of the event loop). I just did a search and it seems like that's how it works according to this SO answer ("if you await things you will always defer the continuation to a microtask"). TIL 🤓

@mathiasbynens referring to #7351 (comment), is it possible that the await deferring did actually have an effect?

assert(
await target._initializedPromise,
'Failed to create target for page'
Expand Down