Skip to content

Conversation

@yanivefraim
Copy link
Contributor

@yanivefraim yanivefraim commented Mar 30, 2018

Closes #2037
(no answer from @felixfbecker regarding #2084. Created a new PR. Sorry for that...)

const waitForHidden = !!options.hidden;
const polling = waitForVisible || waitForHidden ? 'raf' : 'mutation';
return this.waitForFunction(predicate, {timeout: options.timeout, polling}, selectorOrXPath, isXPath, waitForVisible, waitForHidden);
const timeout = helper.isNumber(options.timeout) ? options.timeout : 30000;
Copy link
Contributor

Choose a reason for hiding this comment

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

is this a drive-by? Looks like an unrelated bugfix.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually it is related to this comment:

#2084 (comment)

Copy link
Contributor

Choose a reason for hiding this comment

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

Your comment link didn't work for me.

But if I understand correctly, the page.waitForSelector/page.waitForXPath don't currently have default timeout 30 seconds (despite documentation claiming so), and this change brings it back.

Copy link
Contributor Author

@yanivefraim yanivefraim Mar 30, 2018

Choose a reason for hiding this comment

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

sorry, created it from mobile (it is working on mobile... strange)

The comment was: #2084 (comment)

Copy link
Contributor

Choose a reason for hiding this comment

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

ah I see now. Thank you for the explanation.

Copy link
Contributor

@aslushnikov aslushnikov left a comment

Choose a reason for hiding this comment

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

Nice patch

@aslushnikov aslushnikov merged commit dde45fa into puppeteer:master Mar 30, 2018
@felixfbecker
Copy link

Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants