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

Should Node.js standard library throw custom Error subclasses? #8342

Closed
Ginden opened this issue Aug 30, 2016 · 33 comments
Closed

Should Node.js standard library throw custom Error subclasses? #8342

Ginden opened this issue Aug 30, 2016 · 33 comments
Labels
errors Issues and PRs related to JavaScript errors originated in Node.js core. feature request Issues that request new features to be added to Node.js. stale

Comments

@Ginden
Copy link

Ginden commented Aug 30, 2016

As we know, ECMAScript define only few Error subclasses. W3C specifications adds to this list DOMException and URIError. Right now, Node.js implements seven Errors:

  • Error
  • EvalError
  • RangeError
  • ReferenceError
  • SyntaxError
  • TypeError
  • URIError

On other side we have Java standard library that defines 74 classes extending java.lang.Exception and numerous other extending these subclasses. Most of them doesn't make sense in Node.js context, as our standard library is much smaller, but I'm convinced that 7 errors defined in ECMAScript are too general to provide meaningful information on types of error that can happen.

Therefore, I propose:

  • make all exceptions coming from syscalls inherit from new class SystemError (class SystemError extends Error)
    • create classes like FileSystemError and NetworkError to inherit from SystemError
  • add class DeprecationError inheriting from Errror for deprecated features
  • discourage creating direct instances of Error in standard library

Disadvantages:

  • code directly checking err.constructor === TypeError can break

Advantages:

  • better typing for TypeScript
    • many IDEs can use TypeScript definition files and use them as ad-hoc documentation, even if you write in JavaScript
  • Bluebird .catch(klass, handler) easier to use

Loose ideas:

  • export classes like DatabaseError to be subclassed by database modules

I'm working on pull request to replace new Error with new TypeError or new RangeError wherever it makes sense.

@mscdex
Copy link
Contributor

mscdex commented Aug 30, 2016

A couple of immediate thoughts:

  • Node.js does not implement those *Error objects, V8 does
  • -1 to exporting random non-core-related Error objects like DatabaseError, IMHO that is out of scope for node

@mscdex mscdex added feature request Issues that request new features to be added to Node.js. error-handling labels Aug 30, 2016
@Qard
Copy link
Member

Qard commented Aug 30, 2016

DatabaseError probably does not makes sense. Most database errors would be NetworkError. There may be query-related errors, but those should probably be more specific to the particular database module, so I'm also -1 on that.

The other stuff sounds good to me though. Being able to type-check errors would be fantastic.

@jasnell
Copy link
Member

jasnell commented Aug 30, 2016

Related to this is my PR #6573, which begins attaching a stable error code to every error. I need to get through and get that PR rebased and updated but it should give an idea on the direction.

@Qard
Copy link
Member

Qard commented Aug 30, 2016

@jasnell I like the error code idea too. I'd actually like to see both though. Error types let us group errors by category to handle them similarly. For example, there's multiple ways in which network access can error, so you may want the detail to see if retrying is a valid approach. However, in a higher level API, you may receive network errors alongside other non-network errors, so branching logic on that error type would be quite helpful. 😸

@cjihrig
Copy link
Contributor

cjihrig commented Aug 30, 2016

-1 to defining new error types.

@benjamingr
Copy link
Member

I just want to point out you can use stuff like .catch in bluebird without error subclasses - by using something like https://github.com/petkaantonov/core-error-predicates .

I'm also -1 on this at this point since it seems like a lot of effort to replace a system that already works (the error codes).

@dead-claudia
Copy link

dead-claudia commented Oct 3, 2016

I would prefer a non-native Error subtype for Node's own errors, if anything for easier differentiation at a glance and a better ability to use them in testing (no need to reimplement Node's magic for a mock FS error).

I am 👎 for a complex error hierarchy (that's just not very idiomatic, and it's unnecessarily complex), but something like just FSError, NetworkError, CryptoError, etc. (just the high level) would suffice.

@mscdex mscdex added errors Issues and PRs related to JavaScript errors originated in Node.js core. and removed error-handling labels Oct 28, 2016
@Trott
Copy link
Member

Trott commented Jul 15, 2017

This issue has been inactive for sufficiently long that it seems like perhaps it should be closed. Feel free to re-open (or leave a comment requesting that it be re-opened) if you disagree. I'm just tidying up and not acting on a super-strong opinion or anything like that.

@Trott Trott closed this as completed Jul 15, 2017
@Almenon
Copy link

Almenon commented Feb 18, 2019

@Trott Can this be re-opened please? At the very least node should have a SystemError class. With typescript becoming more and more popular the lack of types is becoming more troublesome.

@Trott
Copy link
Member

Trott commented Feb 18, 2019

@Trott Can this be re-opened please? At the very least node should have a SystemError class. With typescript becoming more and more popular the lack of types is becoming more troublesome.

I'm going to re-open, but I suspect this will get closed pretty quickly. I could certainly be wrong about that, though. I think there will be a lot of resistance among Node.js core devs to add this stuff and support it. But let's see....

@mscdex @benjamingr @cjihrig You're all still -1 on adding something like SystemError?

@Trott Trott reopened this Feb 18, 2019
@cjihrig
Copy link
Contributor

cjihrig commented Feb 18, 2019

I'm still -1, but it seems like that ship sailed long ago. Also, https://nodejs.org/api/errors.html#errors_system_errors seems to be what you're looking for, isn't it?

@mscdex
Copy link
Contributor

mscdex commented Feb 18, 2019

Yes, still -1

@Trott
Copy link
Member

Trott commented Feb 18, 2019

I'm still -1, but it seems like that ship sailed long ago. Also, https://nodejs.org/api/errors.html#errors_system_errors seems to be what you're looking for, isn't it?

For better or for worse, SystemError is not used nearly as often as I would expect it to be used based on the docs.

For example, a system error will occur if an application attempts to read a file that does not exist.

Alas, this shows Error rather than SystemError:

try { fs.openSync('fhqwhgads'); } catch (e) { console.log(e.constructor.name); }

(Insert sad trombone sound.)

Changing it to readSync() gives NodeError so there's probably a lot of consistency work to go in.

@devsnek
Copy link
Member

devsnek commented Feb 18, 2019

all our errors have well known codes now. I'm not sure why we would need more beyond that

@addaleax
Copy link
Member

@devsnek I think @BridgeAR makes some good points in #20803 (comment).

I’m not saying there should be subclasses or anything, but I do agree with him in that it would be nice to classify errors in some way (even just programming errors vs. runtime errors would be worth a lot, imo).

@devsnek
Copy link
Member

devsnek commented Feb 18, 2019

if js had a way of matching the catch like java I would absolutely agree but since it doesn't I don't really think it's worth it, especially since the solution wouldn't be standard. (compare to dom exception enum or whatever)

@addaleax
Copy link
Member

all our errors have well known codes now. I'm not sure why we would need more beyond that

Btw, lots of errors from our C++ code still don’t have codes. :)

@Almenon
Copy link

Almenon commented Feb 19, 2019

@cjihrig @mscdex mind explaining why the -1 for just SystemError?

@cjihrig https://nodejs.org/api/errors.html#errors_system_errors is documentation, not a type. I shouldn't have to refer to online documentation when modern IDE's offer intellisense. It took me a while to find that link to see attributes that would have been just a "." away with intellisense. (thanks for the link though)

all our errors have well known codes now. I'm not sure why we would need more beyond that

@devsnek Better intellisense = faster and less error prone node development experience for users. I can see all the attributes and the documentation and type for each attribute if a object is properly typed. And if I do something stupid like adding 1 to SystemError.code (a string) a type system like typescript would warn me so I can fix my mistake.

@devsnek
Copy link
Member

devsnek commented Feb 19, 2019

@Almenon can you explain how a NodeError class with a code property breaks intellisense?

@Almenon
Copy link

Almenon commented Feb 19, 2019

@devsnek it doesn't break intellisense, I'm talking about the lack of intellisense for systemerror. For example, take the following member of childprocess:

on(event: "error", listener: (err: Error) => void): this;

It is typed as Error, but it should be typed as SystemError. SystemError has several extra properties not present in Error.

@devsnek
Copy link
Member

devsnek commented Feb 19, 2019

isn't that a problem with your intellisese types, not node?

@Almenon
Copy link

Almenon commented Feb 19, 2019

I don't own node's intellisense types - node's types are defined by the node team over at https://github.com/DefinitelyTyped/DefinitelyTyped/tree/master/types/node

I'm not sure if the types are automatically generated or manually defined, maybe the change I want just needs to happen over in DefinitelyTyped. I'll open a issue there.

@Qard
Copy link
Member

Qard commented Feb 19, 2019

Having a proper error type hierarchy does have the advantage of identifying errors by category with instanceof rather than having to check a giant list of codes when you want to catch many things.

Just because everything can be one type, doesn't mean it should be. 🙄

@ljharb
Copy link
Member

ljharb commented Feb 19, 2019

That’s a disadvantage since instanceof is both unreliable and doesn’t work cross-realm.

Any types node adds should offer a robust cross-realm brand checking mechanism.

@dead-claudia
Copy link

dead-claudia commented Feb 19, 2019 via email

@Almenon
Copy link

Almenon commented Feb 20, 2019

@ljharb what about typeof?

@ljharb
Copy link
Member

ljharb commented Feb 20, 2019

@Almenon that’s robust, but can’t be used to differentiate between non-function objects.

@BridgeAR
Copy link
Member

@ljharb

That’s a disadvantage since instanceof is both unreliable and doesn’t work cross-realm.

Do you think there would be any chance to add a new language feature that could replace instanceof in favor of one which works cross-realm?

Any types node adds should offer a robust cross-realm brand checking mechanism.

I agree about that and to do so we could use another property similar to the code property which groups different Node.js errors together.

That is not ideal either but it's likely the best we can do at the moment.

@ljharb
Copy link
Member

ljharb commented Feb 20, 2019

@BridgeAR I would dearly love one, but https://github.com/jasnell/proposal-istypes was the last attempt.

A .code property isn't sufficient; it'd need to be like Array.isArray - a static brand-checking method.

@Ginden
Copy link
Author

Ginden commented Feb 20, 2019

@ljharb
Copy link
Member

ljharb commented Feb 20, 2019

@Ginden that particular one isn't helpful for me since it won't work in non-node environments; but yes, a util.types method for node-specific errors would certainly suffice.

@github-actions
Copy link
Contributor

There has been no activity on this feature request for 5 months and it is unlikely to be implemented. It will be closed 6 months after the last non-automated comment.

For more information on how the project manages feature requests, please consult the feature request management document.

@github-actions github-actions bot added the stale label Feb 28, 2022
@github-actions
Copy link
Contributor

github-actions bot commented Apr 4, 2022

There has been no activity on this feature request and it is being closed. If you feel closing this issue is not the right thing to do, please leave a comment.

For more information on how the project manages feature requests, please consult the feature request management document.

@github-actions github-actions bot closed this as completed Apr 4, 2022
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. feature request Issues that request new features to be added to Node.js. stale
Projects
None yet
Development

No branches or pull requests