Add try/catch when writing yarn-error.log #2095
Conversation
If yarn is lacking write access, the `fs.writeFileSync()` method throws an uncaught exception that blocks further execution. Simply catching it and allowing further exection provides desired behaviour by giving the user feedback and having yarn terminate. Fixes #2094
src/cli/index.js
Outdated
| fs.writeFileSync(errorLoc, log.join('\n\n') + '\n'); | ||
| try { | ||
| fs.writeFileSync(errorLoc, log.join('\n\n') + '\n'); | ||
| } catch (err) {} |
There was a problem hiding this comment.
This should probably at least console.error or something rather than silently ignoring it.
There was a problem hiding this comment.
Maybe something like reporter.error('Could not write yarn-error.log: ' + err) (with proper localized string)
There was a problem hiding this comment.
The thing is, #346 already handles it marvelously. It just needs to get there. I was thinking to rather piggyback on that one, rather than handling it explicitly. But I can of course change into doing so.
| reporter.info(reporter.lang('bugReport', errorLoc)); | ||
| } catch (err) { | ||
| if (err.code === 'EACCES') { | ||
| reporter.error(reporter.lang('noFilePermission', err.path)); |
There was a problem hiding this comment.
We don't use console.error anywhere, better to keep to reporter.error anyway.
How about you wrap only fs.writeFileSync into try/catch?
bestander
left a comment
bestander
left a comment
There was a problem hiding this comment.
- no need for console.error
- try/catch too broad
|
Seems to have been solved by #1592 and this can be closed. |
3 participants
Summary
If yarn is lacking write access, the
fs.writeFileSync()method throws anuncaught exception that blocks further execution.
Simply catching it and allowing further exection provides desired behaviour by
giving the user feedback and having yarn terminate.
Test plan
now results in
Previously it just kept hanging forever.
Fixes #2094