cli: write report in runLighthouse before quitting Chrome #7339
Conversation
brendankenny
requested review from
patrickhulce and
paulirish
as code owners
| */ | ||
| function showRuntimeError(err) { | ||
| console.error('Runtime error encountered:', err.friendlyMessage || err.message); | ||
| if (err.stack) { | ||
| console.error(err.stack); | ||
| } | ||
| process.exit(_RUNTIME_ERROR_CODE); |
There was a problem hiding this comment.
what's the benefit of these return process.exit changes?
There was a problem hiding this comment.
what's the benefit of these
return process.exitchanges?
purely tsc, you have to return it to take advantage of the @return {never} chain
lighthouse-cli/run.js
Outdated
| if (!launchedChrome) return; | ||
|
|
||
| try { | ||
| await launchedChrome.kill(); |
There was a problem hiding this comment.
should we time this out?
There was a problem hiding this comment.
should we time this out?
probably, but the Chrome child process still keeps the main process going, so it just prints a Couldn't quit Chrome process. Timed out. error message but still sits there spinning. Is that better?
We could also process.exit(1) in that case, but assuming we want to always exit in that case seems like it might be excessive?
| return Promise.resolve({}); | ||
|
|
||
| await potentiallyKillChrome(launchedChrome); | ||
| process.removeListener('unhandledRejection', handleTheUnhandled); |
There was a problem hiding this comment.
should this be removed in catch too?
There was a problem hiding this comment.
should this be removed in
catchtoo?
handleError always ends with a process.exit(), so it doesn't matter in the catch case. If we moved handleError to bin.js, though, we could put both potentiallyKillChrome and removing the listener to a finally block, which would be kind of nice.
brendankenny
changed the title
while we wait for the fix for https://crbug.com/936272 and/or in chrome-launcher, this change at least writes the report to disk before waiting for Chrome to close.
Also removes the
unhandledRejectionlistener when done (fixing theMaxListenersExceededWarningfor @hoten) and convertsrunLighthouseto use async/await, as the ancient promise business in there was inscrutable. I think there's still more to improve here, both bikeshedding and file organization (e.g.handleErrorshould maybe be inbin.js, not in here); happy for more ideas or leaving all that for yet another day.