chore: switch to throw error instead of this.error#216
Conversation
| }); | ||
| } catch (error) { | ||
| process.exit(1); | ||
| if (error instanceof Error) { |
There was a problem hiding this comment.
question: in what cases is this condition not met? I think all or most thrown errors use Error or extend it (just curious)
There was a problem hiding this comment.
In reality I don't know how anything besides an Error could reach this catch block. You're right that our application code always throws Errors.
There was a problem hiding this comment.
Its always an instance of error. You could add an assert
| case 'EACCES': | ||
| this.error('Permission denied: Cannot write to output file'); | ||
| break; | ||
| throw new Error('Permission denied: Cannot write to output file'); |
There was a problem hiding this comment.
question: do we need to pass the cause to all of these cases as well?
There was a problem hiding this comment.
it would probably be useful context! I'll add the cause here as well.
|
If there are multiple try catches the root cause will create a chain of causes which make the stack super long. Can you share what a stacktrace looks like? |
3 participants
Previously, we had two separate mechanisms for handling errors:
this.errorfrom components;Invoking
this.erroris conventional in oclif, but this kind of error handling masks the underlying behavior and throws off our linter.This PR consolidates our error handling to use the basic
Errortype and take advantage of the 'cause' Node api.[1][1]https://nodejs.org/api/errors.html#errorcause