util: add util/errors - #38575

I'm +1

I think we should also expose the mechanism to create errors-with-codes. I did https://github.com/fastify/fastify-error to map the behavior of core.

@mcollina

I'm +1

Which are you +1 about? That we should export all of the ERR_ constructors?

It's important to keep in mind that this is not 100% of the error codes we use. There are several raised from C++ that are not covered by the internal/errors exports.

I'm +1 to exporting the JS ones, e.g. internal/errors.

@mcollina @ronag ....

Ok, I've added a utility method to this that allows creating custom error types with codes...

const {
  makeErrorWithCode,
} = require('util/errors');

const messages = new Map([
  ['FOO', 'hello %s'],
  ['BAR', (name) => `hello ${name}`],
]);

const FOO = makeErrorWithCode('FOO', { Base: TypeError, messages });

throw new FOO('world');  // message: hello world

I've added basic documentation. What I'm not sure about is how we should document the constructors for the various ERR_ errors.

CI: https://ci.nodejs.org/job/node-test-pull-request/37973/

Should we export all of the ERR_* constructors like this?

What's the upside of exporting the constructors? The constructors take custom arguments and without documentations of the signatures they would be rather difficult to use, even though the sigantures can be quite unstable, since we change the message formatters all the time, adding more details. Doing a instanceof check with the constructors also seems less reliable than just checking the error.code property, which was what these errors were designed for.

Should we export all of the ERR_* constructors like this?

I could change my mind but I'm currently -1 on this, unless there's a very good reason to do it.
If such a reason exists, I think we should at least make sure that changing ERR_* constructors is never semver-major.

Ok, I've updated this to remove the exposed ERR_ constructors for the expressed by both @targos and @joyeecheung ... I just don't think we're ready to make those constructors public and commit to an API contract for those yet. Everything else remains the same except that the documentation moves to util.md

CI: https://ci.nodejs.org/job/node-test-pull-request/38008/

CI: https://ci.nodejs.org/job/node-test-pull-request/38044/ 💚

@nodejs/tsc ... this needs another TSC review and sign off

Note to folks: While this has the sign off from the two TSC members necessary to land, @targos has requested more time to review it. Please do not land this PR until they've had a few days to complete their review.

@james I'm wondering about the motivation/use case. Could you give us a 1 paragraph level description of the problem being solved, use case for the exposed APIs?

@mhdawson ... See https://github.com/fastify/fastify-error for an example use case motivating the api for creating the error classes.

Regarding the use case for exposing AbortError and DOMException, it comes down to consistency. We want users to make use of things like AbortSignal. It makes sense to make it easy for them to do so in a way that is consistent with core's use.

@jasnell thanks, that helps :)

@targos ... any further thoughts on this?

Sorry, I didn't find time to look at this during the long weekend. I'll do it first time tomorrow morning!

There's also at least one difference to our API: Our internals allow to define multiple error classes for the same error code. That is not possible with the new API.

That was intentional to simplify the API a bit. There's nothing to stop the user code from defining multiple custom errors with separate types and the same code.

If we really want to expose this, we should also document, that input is going to be validated while creating a
new instance during runtime. The error being thrown at this point is going to point to Node.js though, that it's an implementation mistake on our side. This would from now on definitely be wrong and we would have to update that to throw something else instead.

That's fine. We can change the error that is thrown when this API is used.

And if I am not mistaken, it's possible to create errors with identical codes that are already in use by e.g., Node.js. This is not ideal. I would also rather differentiate codes that are used by Node.js and ones that are created by libraries as we might add duplicate codes that way later on.

I'm not convinced that's really a problem. What exactly could we do differently here that would make you +1?

Removed the author ready label while we get the objection resolved.

@DerekNonGeneric @BridgeAR ... Ok, but I'm going to need a bit more detail in terms of what would be acceptable to you both here. It's clear that you don't want what the API currently does but it's not clear what you would want.

I would like to focus on the need for exposing an API to create errors with code, and leave out the current implementation to a later discussion.

Long time ago, we underwent a massive refactoring to add a code property to all Node.js errors. We did this change because developers relied on the .message property of errors, which lead to high unstable code. I believe this was an extremely useful addition to Node.js. I think we can all agree to the benefits of this, and if not please voice your concerns clearly.

As we have clarified that creating errors with a .code property is useful, we are limiting our users by not exposing a utility to do so themselves. They will need to implement their own system, if at all. I think we can promote a better pattern for the whole ecosystem by recommending them to add .code to their errors by exposing such an utility.
As an example, check out https://github.com/fastify/fastify-error.

The point of debate is if we agree something like this is worthwhile doing first and then figure out the best implementation and what to expose.

I see value in helping to drive a common way to create/use the error code on Exceptions.

@mcollina, @jasnell what would the trade offs be between building into Node.js core versus a module a repo within the Node.js org? I expect there would be things like less code re-use, not being an official part of Node.js AP, etc in terms of promoting adoption but it would be good to get your takes on that.

I can coordinate moving https://github.com/fastify/fastify-error if we want to make it official.

My assumption is that we would link the module from the Node.js docs?

If we moved it over I think referencing in the docs would make sense to me.

Going to go ahead and close this PR given that it likely won't be moving forward as is.

@jasnell In #38575 (comment) I was just asking about different options but if moving into org satisfies the need from your perspective then seems like a reasonable way to move forward.