Catch resource errors #44
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
neocotic
reviewed
Nov 23, 2018
| process.on('unhandledRejection', (e) => { | ||
| cli.error(`convert-svg-to-png failed: ${e.stack}`); | ||
| process.exit(1); | ||
| }) |
There was a problem hiding this comment.
I don't really like the idea of letting errors bubble up this high. If it's not catchable, I'd rather not through anything.
neocotic
reviewed
Nov 23, 2018
| this[_page].on('requestfailed', (msg) => { | ||
| const resource = msg.url(); | ||
| throw new Error(`Unable to load resource: ${resource}`); | ||
| }); |
There was a problem hiding this comment.
These errors are being thrown within event handlers, so may not be caught by the calling code in the expect async manner. Not sure how easy/clean this is going to be to implement though.
We also need tests added to cover these scenarios.
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.
(Duplicate of closed PR #43, opened against the correct branch)
As I mentioned in #42, here's "enough" code to cause rendering to stop on a resource load failure. The first section at L281 catches server errors and not founds, while the second at 289 should catch things such as the network connection dropping during load, timeouts, etc... (I should point out that I have not yet tested that specific section.)
I don't at all expect this to be merged as is, as it still has a number of things to be addressed. Likely highest on the list is that throwing an error in those event handlers causes an unhandled promise rejection that I spent a pretty long time trying to catch without luck. I'm not sure if it's causing something inside Puppeteer to reject or if it's just something I managed to miss in the Converter code, but as you can see in the convert-svg-to-png change, the only way I was able to get around it for the time being was to add a global unhandledRejection handler. Additionally, when used as a library, I had to add a similar handler to my application to prevent the promise rejection issue as well.
Secondly, I need to write some tests for this, though I was hoping to get the unhandled rejection issue fixed first. If you have any suggestions on that, I'd be happy to dive in a bit more. I'll also rebase/squash this and amend the commit message to match your style once we've settled on some code you're ready to merge.