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

Allow options.timeout to be 0 to disable timeout #887

Merged
merged 4 commits into from
Sep 30, 2017
Merged

Allow options.timeout to be 0 to disable timeout #887

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
fa81395
Allow options.timeout to be `0` to disable timeout
sukrosono Sep 26, 2017
29ac71d
update related documentation
sukrosono Sep 28, 2017
c8b3b55
drop unnecessary comment
sukrosono Sep 30, 2017
848aebb
change testing spec to grid.html instead of empty.html
sukrosono Sep 30, 2017
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
10 changes: 5 additions & 5 deletions docs/api.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -567,7 +567,7 @@ If there's no element matching `selector`, the method throws an error.

#### page.goBack(options)
- `options` <[Object]> Navigation parameters which might have the following properties:
- `timeout` <[number]> Maximum navigation time in milliseconds, defaults to 30 seconds.
- `timeout` <[number]> Maximum navigation time in milliseconds, defaults to 30 seconds, pass `0` to disable timeout.
- `waitUntil` <[string]> When to consider a navigation finished, defaults to `load`. Could be either:
- `load` - consider navigation to be finished when the `load` event is fired.
- `networkidle` - consider navigation to be finished when the network activity stays "idle" for at least `networkIdleTimeout` ms.
Expand All @@ -580,7 +580,7 @@ Navigate to the previous page in history.

#### page.goForward(options)
- `options` <[Object]> Navigation parameters which might have the following properties:
- `timeout` <[number]> Maximum navigation time in milliseconds, defaults to 30 seconds.
- `timeout` <[number]> Maximum navigation time in milliseconds, defaults to 30 seconds, pass `0` to disable timeout.
- `waitUntil` <[string]> When to consider navigation succeeded, defaults to `load`. Could be either:
- `load` - consider navigation to be finished when the `load` event is fired.
- `networkidle` - consider navigation to be finished when the network activity stays "idle" for at least `networkIdleTimeout` ms.
Expand All @@ -594,7 +594,7 @@ Navigate to the next page in history.
#### page.goto(url, options)
- `url` <[string]> URL to navigate page to. The url should include scheme, e.g. `https://`.
- `options` <[Object]> Navigation parameters which might have the following properties:
- `timeout` <[number]> Maximum navigation time in milliseconds, defaults to 30 seconds.
- `timeout` <[number]> Maximum navigation time in milliseconds, defaults to 30 seconds, pass `0` to disable timeout.
Copy link
Contributor

Choose a reason for hiding this comment

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

There's a bunch of other methods that rely on NavigationWatcher, so you should add this clarification there as well:

page.goForward()
page.boBack();
page.reload();
page.waitForNavigation()

- `waitUntil` <[string]> When to consider navigation succeeded, defaults to `load`. Could be either:
- `load` - consider navigation to be finished when the `load` event is fired.
- `networkidle` - consider navigation to be finished when the network activity stays "idle" for at least `networkIdleTimeout` ms.
Expand Down Expand Up @@ -705,7 +705,7 @@ Shortcut for [`keyboard.down`](#keyboarddownkey-options) and [`keyboard.up`](#ke

#### page.reload(options)
- `options` <[Object]> Navigation parameters which might have the following properties:
- `timeout` <[number]> Maximum navigation time in milliseconds, defaults to 30 seconds.
- `timeout` <[number]> Maximum navigation time in milliseconds, defaults to 30 seconds, pass `0` to disable timeout.
- `waitUntil` <[string]> When to consider navigation succeeded, defaults to `load`. Could be either:
- `load` - consider navigation to be finished when the `load` event is fired.
- `networkidle` - consider navigation to be finished when the network activity stays "idle" for at least `networkIdleTimeout` ms.
Expand Down Expand Up @@ -900,7 +900,7 @@ Shortcut for [page.mainFrame().waitForFunction(pageFunction[, options[, ...args]

#### page.waitForNavigation(options)
- `options` <[Object]> Navigation parameters which might have the following properties:
- `timeout` <[number]> Maximum navigation time in milliseconds, defaults to 30 seconds.
- `timeout` <[number]> Maximum navigation time in milliseconds, defaults to 30 seconds, pass `0` to disable timeout.
- `waitUntil` <[string]> When to consider navigation succeeded, defaults to `load`. Could be either:
- `load` - consider navigation to be finished when the `load` event is fired.
- `networkidle` - consider navigation to be finished when the network activity stays "idle" for at least `networkIdleTimeout` ms.
Expand Down
9 changes: 6 additions & 3 deletions lib/NavigatorWatcher.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,9 +40,12 @@ class NavigatorWatcher {

this._eventListeners = [];

const watchdog = new Promise(fulfill => this._maximumTimer = setTimeout(fulfill, this._timeout))
.then(() => 'Navigation Timeout Exceeded: ' + this._timeout + 'ms exceeded');
const navigationPromises = [watchdog];
const navigationPromises = [];
if (this._timeout) {
const watchdog = new Promise(fulfill => this._maximumTimer = setTimeout(fulfill, this._timeout))
.then(() => 'Navigation Timeout Exceeded: ' + this._timeout + 'ms exceeded');
navigationPromises.push(watchdog);
}
Copy link
Contributor Author

@sukrosono sukrosono Sep 28, 2017 •
edited

Choose a reason for hiding this comment

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

this is as the @aslushnikov suggestion, it's simple and clear code. But it think it still have problem if user pass timeout 0 as a string. I'd like add more conditional check, or we just rely on the documentation?

Copy link
Contributor

Choose a reason for hiding this comment

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

@brutalcrozt yes, we should rely on documentation in this case and expect user to pass in a number. There's also a check that timeout is always a number: https://github.com/GoogleChrome/puppeteer/blob/c46c41d89caefd1e1951055fb041521a2bcc933b/lib/NavigatorWatcher.js#L28


if (!this._ignoreHTTPSErrors) {
const certificateError = new Promise(fulfill => {
Expand Down
5 changes: 5 additions & 0 deletions test/test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -619,6 +619,11 @@ describe('Page', function() {
expect(error.message).toContain('Navigation Timeout Exceeded: 1ms');
process.removeListener('unhandledRejection', unhandledRejectionHandler);
}));
it('should disable timeout when its set to 0', SX(async function() {
let error = null;
await page.goto(PREFIX + '/grid.html', {timeout: 0}).catch(e => error = e);
expect(error).toBe(null);
}));
it('should work when navigating to valid url', SX(async function() {
const response = await page.goto(EMPTY_PAGE);
expect(response.ok).toBe(true);
Expand Down