adds rethrow utility
#3925
adds rethrow utility
#3925
Conversation
dimitropoulos
force-pushed
the
feat/rethrow
branch
from
aaeddff to
bd5deea
Compare
| throw newError; | ||
| } | ||
|
|
||
| if (typeof thrownError === 'string') { |
There was a problem hiding this comment.
arguably, we should not allow this... but just to be extra friendly it's probably (unfortunately) common enough out there in the world that https://eslint.org/docs/rules/no-throw-literal can't save everyone from everyone else.
There was a problem hiding this comment.
What you had found in #3920 (review) (overwriting error.message doesn't replace the message in the stack trace) was quite valuable.
I did some research after that and it seems like a hacky solution to modify the stack with a regex and re-throw. The need to do this indicates to me that we could improve how we work with errors to begin with, and that a cleaner solution would be to throw custom errors and handle them appropriately.
For example, if the GQL API request inside the VCS class throws a generic error saying we don't have access, we throw new InvalidAccessError(err) from the networking layer itself. This allows us to handle the errors further up, and we immediately know what class of error was thrown from the business logic to know what kind of message to show in the UI.
As another example, inso does this by throwing an InsoError in areas where we caught an expected error and want to exit gracefully, versus a regular Error indicating an unhandled exception. InsoError still contains the actual error that occurred, including the stack trace, but we can now differentiate how to show a particular error to the user.
As I was researching this I also noticed that an AggregateError has been introduced in Node 15 (there is a note in https://github.com/sindresorhus/aggregate-error) and that would further complicate this implementation.
For these reasons I think we should not go ahead with this approach and transition to using custom errors. I'm loving the tests 🙌🏽
Keen for @gatzjames' view on this as well.
regarding: motivations for this solution
Could you share what you looked at? (no biggie if you don't recall and were just poking around)
That's the reason for 100% test coverage (and more, testing of various edge cases like throwing literals/non-errors). This isn't actually all that uncommon to do, for examples:
I have known for a while that react, in particular, is rewriting stack traces but I didn't bother to look it up because I thought it would be too annoying to find (as many things in the react source code are, at least for me in my experience). That was a mistake, because finding the above two examples showed me what they're really using, which is using $ npm --prefix packages/insomnia-app why error-stack-parservThe above output points to another project in the same GitHub org as Needless to say, what we're doing here is probably a lot more common than any of us realize. I knew it was a thing react and a few other libraries and webpack plugins were doing when they needed to, but it seems that it goes beyond that. It'd be an argumentum ad populum to say that because lots of other people are doing this that it means we're justified in also doing so. I'm bringing it up just to say that it serves as evidence to me that there's sufficient lack of support in the way Errors natively work that often leads projects that are dealing with and mutating stack traces to resort to measures like this. However, regarding regex, check out all the regexes here https://github.com/stacktracejs/error-stack-parser/blob/master/error-stack-parser.js. I wish I had seen these sooner because I would have probably just used this package, although the regex I came up with is pretty dang simple comparatively. regarding: AggregateError@gatzjames brought up AggregateError to me fairly early on and the reason I gave him that we don't want to use it is that nowhere else in the project is using it (so, it adds another kind of concept and we'd need to make sure it integrates well). My other initial reaction is that it's also another concept that has different semantics. Take the following example:
It has a different way of dealing with it than any other error. Now, that's not a huge deal I guess because TypeScript will help us, but it's something to at least consider. In the above notice that the message is blank unless you set it as a second argument (highlighted on the left, My goal was to avoid the complexity of doing this and just have a tool that will rethrow. The irony is, in some ways at least, it looks like I kinda rebuilt that. Compare the signature of what I made: Then there's the other thing which is that Node 15 is required to use Aggregate error and we're seemingly locked into Node 12 for at least the foreseeable future. Although, I think I'd be cool with a really good polyfill if we can find one. regarding: sindresorhus/aggregate-errorThis is cool, and like usual with this author, very forward thinking for the time it was created. However, it has a wildly different signature than what ended up going into Node 15 (years later, to the author's credit). I'd say that makes it a total non-starter on this npm - but I'm glad you mentioned it! regarding: throwing custom errors
How does this solve the problem of the stack trace being swallowed, which was (at it's core) the entire motivation for rethrow in the first place? Otherwise, we can just overwrite the error message and forget about the stack trace altogether or copy over the old stack trace and ignore that the messages don't match. Note, also, that rethrow will handle all of those errors, as is. questions
|
|
A couple of things I found while doing some research on this topic: Always reading Except for async errors there are cases where we might want to rewrite the message of a sync error: function throwAnError() {
throw Error('A system error occurred.')
}
function main() {
try {
throwAnError()
} catch (error) {
console.error(`${error}`)
}
}would need to become async for rethrow to work import rethrow from 'SOME_PATH/rethrow';
function throwAnError() {
throw Error('A system error occurred.')
}
async function asyncMain() {
try {
await rethrow("Some new message", throwAnError())
} catch (error) {
console.error(`${error}`)
}
}Another one is that although it's recommended for promise rejections to reject with an instance of Error it's not enforced and the rejection could be any kind of arbitrary value and not only error or string. I believe that creating a new error that wraps the previous error in one of its properties (e.g. error.originalError) is a more flexible solution and doesn't require from us to handle all these edge cases. e.g. export class AppError extends Error {
name = 'AppError'
message = ''
originalError = ''
constructor(originalError, message, ...otherStuff) {
super(message);
this.originalError = originalError;
}
}
function throwAnError() {
throw Error('A system error occurred.')
}
try {
throwAnError()
} catch (error) {
console.error(`${error}`)
throw new AppError(error, "Some new message")
} |
dimitropoulos
force-pushed
the
feat/rethrow
branch
from
bd5deea to
d9c5ff1
Compare
I don't see a practical use-case for this mattering. Could you provide an example of how we could measure a user-detectable (i.e. even a single frame drop) performance degradation as a result of this? Even in non-ideal conditions, it's an exceedingly rare thing that happens (that an error is thrown) and I find it hard to believe it will actually matter in any measurable way.
Yep, definitely agree that this is a bummer but the only other solution is to make
Not sure if you saw the note I left on this (#3925 (comment)). This is explicitly handled and tested in https://github.com/Kong/insomnia/pull/3925/files#diff-5dc414a34c22d89bc59f0880ebf478d8f3f088f1f9d70cf90535b1eae48c496cR137-R143 for strings, but I added https://github.com/Kong/insomnia/pull/3925/files#diff-5dc414a34c22d89bc59f0880ebf478d8f3f088f1f9d70cf90535b1eae48c496cR145-R152 for numbers as well. The only thing missing here are objects and booleans so if we want to cover those explicitly then we can always do so, although I'd say that it's best not to because
This means that all of our tooling and error handling throughout the app is going to also have to have custom logic for handling I believe I provided solutions to all the edge cases you mentioned, but please let me know if you are aware of any other. |
|
@dimitropoulos agree that we can solve it like that and wouldn't want to block this from getting merged if we really need this functionality. I did some digging and found that exception chaining in js is actually a more common problem than I thought. 😱 |
I don't follow, I'm sorry. The purpose of this abstraction is to keep with simple, regular, plain ol' Errors. AggregateError, Error.cause, and other solutions all introduce new concepts, while this does not. It allows us to keep using and handling regular errors.
Again, I don't follow. But overriding the error message is all this function does. It needs to do so, as well, in the stack, so it handles doing that. To me, this seems like the simplest possible solution because it introduces no new concepts. Can you point to something in particular that seems complex to you so that I can better understand? And just to say it out loud, we can throw away this PR for all I care - that's not my issue, but we need solution for overwriting error messages that will handle stacks, one way or the other, and although better solutions may be available to us months or years from now, this solution is available today and it's very well tested and the approach is used in our stack in multiple places already. |
In some recent PRs (#3923 (comment) and #3920) it has come up that we could use a utility that can rethrow an error whilst not swallowing the stack trace. This PR adds a rethrow utility that will do exactly that.