Skip to content

feat(Page): Add global timeout setting #1728

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
Merged
merged 4 commits into from
Jan 10, 2018
Merged

feat(Page): Add global timeout setting #1728

merged 4 commits into from
Jan 10, 2018

Conversation

@radu125
Copy link
Contributor

@radu125 radu125 commented Jan 5, 2018

References #1514

@googlebot
Copy link

googlebot commented Jan 5, 2018

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed, please reply here (e.g. I signed it!) and we'll verify. Thanks.

  • If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address on your commit. Check your existing CLA data and verify that your email is set on your git commits.
  • If your company signed a CLA, they designated a Point of Contact who decides which employees are authorized to participate. You may need to contact the Point of Contact for your company and ask to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the project maintainer to go/cla#troubleshoot. The email used to register you as an authorized contributor must be the email used for the Git commit.
  • In order to pass this check, please resolve this problem and have the pull request author add another comment and the bot will run again. If the bot doesn't comment, it means it doesn't think anything has changed.

@radu125
Copy link
Contributor Author

radu125 commented Jan 5, 2018

I signed the CLA.

@googlebot
Copy link

googlebot commented Jan 5, 2018

CLAs look good, thanks!

@aslushnikov
Copy link
Contributor

aslushnikov commented Jan 5, 2018

Can you please eliminate the default 30000 timeout from the NavigatorWatcher?

https://github.com/GoogleChrome/puppeteer/blob/8e9c54a789c9d51f9861053b21565ad21a5ffe6f/lib/NavigatorWatcher.js#L41-L48

I'd do this by passing timeout explicitly to the NavigatorWatcher as one of the arguments in its constructor.

@radu125
feat: pass timeout as separate argument
5830ca4 
Remove the default 30000 timeout from NavigatorWatcher and pass the
timeout to the NavigatorWatcher constructor as a separate argument
@radu125
Copy link
Contributor Author

radu125 commented Jan 7, 2018

I did it, but the AppVeyor build failed, and I can't figure out why that test failed.

aslushnikov
aslushnikov reviewed Jan 9, 2018
View reviewed changes
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.

@raduaron26 no worries about appveyour, some of our tests are flaky.

Thank you for working on this. I have a minor comment, other than that it's good to go.

test/test.js Outdated
});

describe('Page.setDefaultNavigationTimeout', function() {
it('should work', async({page, server}) => {
Copy link
Contributor

@aslushnikov aslushnikov Jan 9, 2018

Choose a reason for hiding this comment

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

We generally try hard not to reach out to private fields.
Let's drop this test – i think the other one you add is sufficient.

@radu125
feat: remove unit test
d5e9676 
Remove unit test in which the _defaultNavigationTimeout private field is
accessed
@radu125
Copy link
Contributor Author

radu125 commented Jan 9, 2018

I removed the test. I'm glad I could contribute and thank you too for your help :)

aslushnikov
aslushnikov reviewed Jan 9, 2018
View reviewed changes
docs/api.md
* [page.select(selector, ...values)](#pageselectselector-values)
* [page.setContent(html)](#pagesetcontenthtml)
* [page.setCookie(...cookies)](#pagesetcookiecookies)
* [page.setDefaultNavigationTimeout(timeout)](#pagesetdefaultnavigationtimeouttimeout)
Copy link
Contributor

@aslushnikov aslushnikov Jan 9, 2018

Choose a reason for hiding this comment

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

So this setting affects 5 methods: page.goto, page.goBack, page.goForward, page.reload and page.waitForNavigation.
Can you please:

  • refer to the methods from the doc of the method, explaining that this method will change defaults for them, and that the default value is 30 seconds
  • update documentation for the methods themselves, citing page.setDefaultNavigationTimeout.

This should make it more understandable for our clients.

aslushnikov
aslushnikov suggested changes Jan 9, 2018
View reviewed changes
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.

@raduaron26 I've just figured there's one last nit for the documentation before this should be merged. Thanks again for taking your time for this

@radu125
feat: Update documentation
d86ade1 
Update documentation regarding the page.setDefaultNavigationTimeout
functionality
@radu125
Copy link
Contributor Author

radu125 commented Jan 10, 2018

I updated the documentation, let me know if you want me to make any tweaks to what I wrote.

aslushnikov
aslushnikov approved these changes Jan 10, 2018
View reviewed changes
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.

Thank you, this looks good.

@aslushnikov aslushnikov merged commit ec8e40f into puppeteer:master Jan 10, 2018
@radu125 radu125 deleted the global-timeout branch January 10, 2018 21:55
igorshapiro pushed a commit to WiserSolutions/puppeteer that referenced this pull request Jan 16, 2018
@radu125 @igorshapiro
feat(Page): Add global navigation timeout setting (puppeteer#1728)
2f0fe9c 
This patch introduces `page.setDefaultNavigationTimeout` method to override the 
default 30 seconds navigation timeout.

Fixes puppeteer#1514
@stackflows
Copy link

stackflows commented Feb 19, 2018

@aslushnikov: This only affects navigation. waitForSelector() etc. should have a similar global "default" setting. Shall I write a feature request for this?

@Peta5000
Copy link

Peta5000 commented Jul 2, 2018

@stackflows Did you write a feature request for this? This feature would be so precious.

@spinningarrow
Copy link

spinningarrow commented Nov 9, 2018 •
edited
Loading

@Peta5000 it looks like #1514 is the issue for this but it is closed. I've added a comment there voting to reopen.

UPDATE: This is a part of #2079

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

Reviewers

1 more reviewer

@aslushnikov aslushnikov aslushnikov approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@radu125 @googlebot @aslushnikov @stackflows @Peta5000 @spinningarrow