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

Implement setDefaultWaitTimeout or setDefaultTimeouts #3319

Closed
entrptaher opened this issue Sep 28, 2018 · 9 comments
Closed

Implement setDefaultWaitTimeout or setDefaultTimeouts #3319

entrptaher opened this issue Sep 28, 2018 · 9 comments
Labels
chromium Issues with Puppeteer-Chromium feature

Comments

@entrptaher
Copy link
Contributor

entrptaher commented Sep 28, 2018

Right now we have page.setDefaultNavigationTimeout(timeout) which is very useful. If we want to take things forward, we can implement the following,

setDefaultWaitTimeout(timeout)

This will help set a default timeout for waitFor function or similar to those. There are obviously more waitFor use cases than goto.

setDefaultTimeouts({ navigation: timeout, waitFor: timeout })

Same like above, this will let one update all timeouts at once or just the one they want. Will remove the need for setDefaultNavigationTimeout or setDefaultWaitTimeout if happens and window for more useful api

PS: I have a project where I have at least thousands of waitFor cases for some reason and hundreds of navigation calls, this feels really promising in that sense where people have lots of test cases.

@aslushnikov
Copy link
Contributor

I like the setDefaultTimeouts thing - thanks for suggesting!

@alexbepple
Copy link

@aslushnikov #2079 lists setDefaultTimeout as a breaking change. What does it break? It seems to only add.

What is your thinking on setDefaultTimeouts({ navigation: timeout, waitFor: timeout }) as suggested here vs setDefaultTimeouts(timeout) as suggested in #3158?

And finally, do you mind if I take a stab at this?

@SimonSchick
Copy link

I'd very much like to see this as well, if @alexbepple is already working on this I will wait, otherwise I might create a PR if I get around to do it.

@aslushnikov
Copy link
Contributor

@SimonSchick @alexbepple please go ahead and submit a PR!

@SimonSchick
Copy link

I'd rather discuss this a little, do we want more fine grained control over timeouts?

Current idea:

page.setTimeouts({
  element: <number>,
  navigation: <number>,
  target: <number>,
  network: <number>, // request and responses
})

where as all property should be optional so you can set individual timeouts without settings all ther others.

@alexbepple
Copy link

alexbepple commented Nov 13, 2018

@SimonSchick I am in favor of setDefaultTimeouts over setTimeouts. Not only does this fit in more nicely with the existing setDefaultNavigationTimeout, but also communicates intent more clearly.

I am wondering whether anybody has a use case for such a fine-grained timeout control? Personally, I favor simpler solutions. So I would have been happy with an all-encompassing setDefaultTimeout(int). But the already implemented setDefaultNavigationTimeout shows in a different direction. The simplest thing without making this a breaking change would be to use binary categorization: navigation and everything else ('waitFor'). @SimonSchick Do you have a particular reason to distinguish between element, target and network? Also another problem arises: in which category does waitForFunction belong?

Even with a more differentiating approach, I would like to see an all-encompassing setDefaultTimeouts(int).

@entrptaher
Copy link
Contributor Author

entrptaher commented Nov 13, 2018

@alexbepple, Yes there are fine needs for such.

I have a project where I have at least thousands of waitFor cases for some reason and hundreds of navigation calls, this feels really promising in that sense where people have lots of test cases.

Sounds really ambiguous, but, either you make another wrapper on top of puppeteer for your specific case, or implement the function directly to avoid overhead.

Ie,

  • You need 60 seconds timeout for a page to load.
  • However, some sample waitFor task should take 120 seconds by default.

I did not have any use case for target, network and such yet, but I think once we have basic version like navigation and waitFor, we can extend it in future, and have more control over the execution.

Simplest reason would be setDefaultNavigationTimeout being there but not a default waitFor timeout.

@aslushnikov
Copy link
Contributor

We've discussed this with @JoelEinbinder and really liked the following approach.

  1. For now, let's keep it simple and introduce a single page.setDefaultTimeout(number) method.
    This overrides all timeouts we use for navigations and waitFor* methods.
page.setDefaultNavigationTimeout(10000);
// Set all timeouts everywhere for 5 seconds.
// However, the navigation will still default to 10 seconds.
page.setDefaultTimeout(5000);
  1. If the need emerges, we can always be fancy and teach page.setDefaultTimeout to accept an object with a set of per-method configuration methods:
page.setDefaultTimeout({
  navigation: 10000, // equal to page.setDefaultNavigationTimeout(1000),
  waitForSelector: 12000, // set default timeout for page.waitForSelector
});

Let me know what you think.

@aslushnikov aslushnikov added the chromium Issues with Puppeteer-Chromium label Dec 6, 2018
aslushnikov added a commit to aslushnikov/puppeteer that referenced this issue Jan 28, 2019
Method `page.setDefaultTimeout` overrides default 30 seconds timeout
for all `page.waitFor*` methods, including navigation and waiting
for selectors.

Fix puppeteer#3319.
aslushnikov added a commit that referenced this issue Jan 29, 2019
Method `page.setDefaultTimeout` overrides default 30 seconds timeout
for all `page.waitFor*` methods, including navigation and waiting
for selectors.

Fix #3319.
@batpurev
Copy link

batpurev commented Jul 2, 2021

Hi Gurus,

Have this been implemented? I tried to use this setting but getting this error message.
image

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
chromium Issues with Puppeteer-Chromium feature
Projects
None yet
Development

No branches or pull requests

6 participants
@alexbepple @aslushnikov @batpurev @SimonSchick @entrptaher and others