Add option timeoutAsWarning #248
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
stereobooster
force-pushed
the
ignore-timeouts
branch
2 times, most recently
from
f45ee80 to
4f3be79
Compare
peterbe
reviewed
Aug 8, 2018
There was a problem hiding this comment.
I get nervous about this change. It's weird that safeReject gets called but then it decides to exit out of that.
I'd need to see how this works. Do you have some URLs I can play with on the command line?
Also, where it usually happens, this bug, can we not do something like:
...
// Oh shit this is not good!
// But before we reject the whole thing,
// check that it's not because a bogus timeout
// See this URLHERE about a bug in puppeteer.
if (tracker.urls().length) {
safeReject('Crap!')
} else if (timeoutAsWarning) {
// When verbose warn about weird timeouts without active URLs
console.warn("Crap but no URLs actually timed out")
}
README.md
Outdated
| @@ -176,6 +176,7 @@ key is `urls`. Other optional options are: | |||
| of strings for headless Chrome](https://peter.sh/experiments/chromium-command-line-switches/). | |||
| * `cssoOptions` - CSSO compress function [options](https://github.com/css/csso#compressast-options) | |||
| * `timeout` - Maximum navigation time in milliseconds, defaults to 30 seconds, pass 0 to disable timeout. | |||
| * `timeoutAsWarning` - This is workaround for the bug in puppeter, when there are no open connections but puppeteer reports it as timeout anyway. If you set it to true minimalcss will report timeouts as warnings instead of rejection. Take a note real timeouts still be reported as errors. | |||
There was a problem hiding this comment.
"the bug in puppeteer" should be a link to the bug.
src/run.js
Outdated
| @@ -137,8 +136,14 @@ const processPage = ({ | |||
| )}`; | |||
| } else if (urls.length > 0) { | |||
| error.message += `\nFor ${urls[0]}`; | |||
| } else if (options.timeoutAsWarning) { | |||
| // This is workaround for the bug in puppeter, when there are | |||
There was a problem hiding this comment.
Again, a URL to the bug here would be super useful.
stereobooster
force-pushed
the
ignore-timeouts
branch
from
4f3be79 to
9887209
Compare
stereobooster
commented
Aug 8, 2018
| @@ -285,7 +285,21 @@ const processPage = ({ | |||
| // Second, goto the page and evaluate it with JavaScript. | |||
| // The 'waitUntil' option determines how long we wait for all | |||
| // possible assets to load. | |||
| response = await page.goto(pageUrl, { waitUntil: 'networkidle0' }); | |||
| try { | |||
| response = await page.goto(pageUrl, { waitUntil: 'networkidle0' }); | |||
There was a problem hiding this comment.
this will not work, we do not have response in case of error
|
Ahh it will not work, but it was nice idea |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
not sure about option name