core(fetcher): ensure fetch doesn't cause unhandled promise #11036
Conversation
lighthouse-core/gather/fetcher.js
Outdated
|
|
||
| this.driver.evaluateAsync(`${injectIframe}(${JSON.stringify(url)})`, { | ||
| useIsolation: true, | ||
| }).catch(err => console.error(err)); |
There was a problem hiding this comment.
should we bother logging this?
There was a problem hiding this comment.
why not just put it into a Promise.all([racePromise, injectPromise]) to get into the normal control flow?
There was a problem hiding this comment.
I don't see how. Send PR?
|
#11033 doesn't effect canary chrome or LR*. We should follow up with a 6.1.1 release just for npm. @patrickhulce and I don't know why Node 12 crashes on this error :o * still need to confirm. @mikedijkstra can you provide some more URLs where this crash occurs? |
There was a problem hiding this comment.
also need to add reject on errors on the two .sendCommand that are dangling atm
a test would be nice for this, though I'm not entirely sure how to add one 🤔
maybe our super ancient sentry client is doing the exiting instead of node? we can probably just disable that and/or upgrade it
lighthouse-core/gather/fetcher.js
Outdated
|
|
||
| this.driver.evaluateAsync(`${injectIframe}(${JSON.stringify(url)})`, { | ||
| useIsolation: true, | ||
| }).catch(err => console.error(err)); |
There was a problem hiding this comment.
why not just put it into a Promise.all([racePromise, injectPromise]) to get into the normal control flow?
|
yeah I don't know why it's exiting #11033 hasn't landed yet... |
connorjclark
changed the title
connorjclark
changed the title
Co-authored-by: Patrick Hulce <[email protected]>
connorjclark
changed the title
Co-authored-by: Patrick Hulce <[email protected]>
Co-authored-by: Patrick Hulce <[email protected]>
Fixes #11033