Should Node.js standard library throw custom Error subclasses?

[Ginden]

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]

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

[Qard]

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]

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]

@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]

-1 to defining new error types.

[benjamingr]

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]

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.

[Trott]

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.

[Almenon]

@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]

@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?

[cjihrig]

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]

Yes, still -1

[Trott]

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]

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

[addaleax]

@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]

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]

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]

@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]

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

[Almenon]

@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]

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

[Almenon]

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]

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]

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]

Potentially OT, but I'd like a way to create synthetic system errors, especially common FS errors like ENOENT, from Node's API. All I really need is the constructor part, not the subclassing part. (Please point me to the right issue if this isn't the correct place to request this.)

…

On Tue, Feb 19, 2019 at 15:37 Jordan Harband ***@***.***> wrote: 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. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#8342 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AERrBOfCyt71DQdcqIFj4FdwW_3Q0fzSks5vPGCagaJpZM4JwyAA> .

[Almenon]

@ljharb what about typeof?

[ljharb]

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

[BridgeAR]

@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]

@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]

@ljharb util.types.isNativeError(value).

[ljharb]

@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]

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]

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.