Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Userland errors vs. Node errors #15057

Closed
ronkorving opened this issue Aug 28, 2017 · 11 comments
Closed

Userland errors vs. Node errors #15057

ronkorving opened this issue Aug 28, 2017 · 11 comments
Labels
errors Issues and PRs related to JavaScript errors originated in Node.js core. question Issues that look for answers.

Comments

@ronkorving
Copy link
Contributor

  • Version: 8.4.0
  • Platform: all
  • Subsystem: errors

When I create and manage errors, I often add a .code property on my error objects. Node of course does this as well. This is all fine, until my use case became "send my codes to the the end-user and turn them into a human readable error description". Once I catch an error and start sending error.code to the user, I have no easy way to identify if this is one of the userland errors, or a Node error.

This is of course not a bug in Node, it's just a paradoxical situation, with arguably many solutions. But I wonder if I'm really the only one out there who has been using error.code for userland errors. If so, I should change my approach. But if this is actually really common, perhaps there's a case to be made to make Node errors more identifiable as coming from Node (they could be instances of a NodeError class, or they could perhaps have an extra property on them).

Of course the easiest approach is for me to just pick a new property name for userland error codes, or create a specialized error class myself. But I have to wonder, am I alone on this? Should this be the burden of users, or should Node perhaps stop claiming the Error class and .code property for itself? Really hoping for some feedback from users and @nodejs/collaborators. Thanks.

@BridgeAR
Copy link
Member

If they have the code property they are of class NodeErrors already.

@BridgeAR BridgeAR added the question Issues that look for answers. label Aug 28, 2017
@mscdex mscdex added the errors Issues and PRs related to JavaScript errors originated in Node.js core. label Aug 28, 2017
@ronkorving
Copy link
Contributor Author

@BridgeAR That's great news, I just found

Error: makeNodeError(Error),
TypeError: makeNodeError(TypeError),
RangeError: makeNodeError(RangeError),
which I guess is what you're referring to. How can a user do any detection on this though? The class seems private.

@addaleax
Copy link
Member

Maybe we should just add something like get isNodeError() { return true; } to the NodeError class to make it detectable?

@ronkorving
Copy link
Contributor Author

@addaleax I would be all for that 👍

@ronkorving
Copy link
Contributor Author

Looking a bit further by the way, I realize that AssertionError is not a NodeError, but the assert module is used first and foremost for Node internals. This seems rather inconsistent, and adding an isNodeError property would make that situation worse, since AssertionError objects would either never be considered a NodeError, or always. Either way is wrong, because assert is used a lot by users.

The only way to truly solve that, seems to be to stop using the assert library altogether for internals. Thoughts?

@benjamingr
Copy link
Member

Once I catch an error and start sending error.code to the user, I have no easy way to identify if this is one of the userland errors, or a Node error.

What's stopping you from fixing this on your side by defining a common error subclass or property to errors you raise with .code? I have been doing that for quite some time and it seems to work fine.

@ronkorving
Copy link
Contributor Author

@benjamingr Nothing is stopping me from doing that at all.

Of course the easiest approach is for me to just pick a new property name for userland error codes, or create a specialized error class myself.

But since I'm developing a middleware, now my users will have to use my middleware's error classes instead of their own. On top of that, they can no longer use just any assertion library, since that too will throw errors of the wrong class.

Maybe there is room for Node to make this all a bit easier.

@tniessen
Copy link
Member

Maybe we should just add something like get isNodeError() { return true; } to the NodeError class to make it detectable?

I don't think this scales well. Let's say you use a package foo, which uses a package bar, which uses a node API. Okay, let's assume isNodeError allows us to check whether an error was thrown from within node. How about foo and bar? Should package authors add isFooError / isNodeError? What if v8 decides to add code properties to their errors? Should we manually re-throw those with an additional isV8Error property?

I don't have a better idea right now, I just think the benefits of having such a property are minimal.

Once I catch an error and start sending error.code to the user, I have no easy way to identify if this is one of the userland errors, or a Node error.

How did you do this before, did you rely on the existence / absence of the code property?

@ronkorving
Copy link
Contributor Author

@tniessen You are right about other modules. That is an issue.
It really bothers me that that there is no obvious elegant solution to all this.
The "use your own error classes" doesn't cut it either for the reasons I stated above.

How did you do this before, did you rely on the existence / absence of the code property?

I didn't really use .code until recently, but of course now (in arguably rare cases where Node throws me a NodeError in production) I'm conflicting in namespace.

While I don't consider this solved, I'm gonna close this issue. I don't see a single obvious solution here. Worth a blog post maybe ;)

@benjamingr
Copy link
Member

@ronkorving I see a lot of value in discussion of a longer term error strategy in Node.js - if you'd open a nodejs/CTC issue I would participate. We spent a lot of time considering g different models for bluebird and how to distinguish our own errors.

@ronkorving
Copy link
Contributor Author

Thanks 👍
Will do

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
errors Issues and PRs related to JavaScript errors originated in Node.js core. question Issues that look for answers.
Projects
None yet
Development

No branches or pull requests

6 participants