util: add util/errors #38575

jasnell · 2021-05-06T20:51:56Z

Exports a subset of internal/errors as require('util/errors').

The reason for util/errors is because there is already a errors
module on npm that has a non-trivial number of downloads per week.
Unfortunately that makes it semver-major

Signed-off-by: James M Snell jasnell@gmail.com
Fixes: #38361
Fixes: #14554

mcollina · 2021-05-06T22:12:26Z

I'm +1

mcollina · 2021-05-06T22:14:19Z

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.

jasnell · 2021-05-07T14:42:14Z

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

mcollina · 2021-05-07T14:58:49Z

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

jasnell · 2021-05-07T16:15:59Z

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

nodejs-github-bot · 2021-05-07T22:35:05Z

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

joyeecheung · 2021-05-08T16:35:24Z

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.

mcollina

Good work!

doc/api/errors.md

targos · 2021-05-10T13:52:36Z

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.

jasnell · 2021-05-10T22:25:45Z

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

nodejs-github-bot · 2021-05-10T22:26:41Z

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

lib/util/errors.js

nodejs-github-bot · 2021-05-12T19:09:07Z

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

Exports a subset of `internal/errors` as `require('util/errors')`. The reason for `util/errors` is because there is already a `errors` module on npm that has a non-trivial number of downloads per week. Signed-off-by: James M Snell <jasnell@gmail.com>

mcollina

lgtm

jasnell · 2021-05-13T03:41:34Z

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

targos · 2021-05-13T08:22:20Z

lib/util/errors.js

+  }
+  if (typeof message !== 'function' &&
+      typeof message !== 'string') {
+    throw new ERR_INVALID_ERROR_MESSAGE(code);


Why not ERR_INVALID_ARG_TYPE ? It supports multiple types and is used almost everywhere else for this kind of error.

I realize message is not an arg here. Why is it only validated now rather than before adding it to the messages map?

Please let me think a bit about the proposed API. There's something that doesn't feel right.

The API was designed with minimal changes to the internals in mind.

ronag

LGTM

jasnell · 2021-05-13T20:40:43Z

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.

mhdawson · 2021-05-14T13:18:15Z

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

jasnell · 2021-05-14T14:06:17Z

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

mhdawson · 2021-05-14T17:45:22Z

@jasnell thanks, that helps :)

jasnell · 2021-05-17T19:00:49Z

@targos ... any further thoughts on this?

targos · 2021-05-17T20:32:41Z

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

BridgeAR

I am not certain that we should really expose our makeErrorWithCode, even with a layer above abstracting it.

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.

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. 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.

For these reasons I am currently against adding this to core.

jasnell · 2021-05-19T16:26:40Z

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?

jasnell · 2021-05-20T16:43:28Z

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

DerekNonGeneric

Agreed that the code property is going to be subject to debate here.

Thanks for putting a hold on this PR for the time being. AFAICT, it does still seem to be half-baked.

Changing it up so that the error codes give us a way to know if the error came from Node.js internals or not seems smart.

jasnell · 2021-05-24T14:32:30Z

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

mcollina · 2021-05-24T14:49:01Z

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.

mhdawson · 2021-05-26T21:12:09Z

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.

mcollina · 2021-05-27T10:55:34Z

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?

mhdawson · 2021-05-31T15:24:12Z

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

jasnell · 2021-05-31T15:28:52Z

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

mhdawson · 2021-05-31T15:32:03Z

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

jasnell added the semver-major PRs that contain breaking changes and should be released in the next major version. label May 6, 2021

github-actions bot added build Issues and PRs related to build files or the CI. needs-ci PRs that need a full CI run. labels May 6, 2021

jasnell marked this pull request as ready for review May 7, 2021 16:13

jasnell force-pushed the add-util-errors branch from c70a8e2 to 457afdd Compare May 7, 2021 22:34

mcollina reviewed May 9, 2021

View reviewed changes

doc/api/errors.md Outdated Show resolved Hide resolved

jasnell force-pushed the add-util-errors branch 2 times, most recently from 1b97759 to 0ce6341 Compare May 10, 2021 22:23

jasnell removed the needs-ci PRs that need a full CI run. label May 10, 2021

aduh95 reviewed May 11, 2021

View reviewed changes

lib/util/errors.js Outdated Show resolved Hide resolved

lib/util/errors.js Outdated Show resolved Hide resolved

lib/util/errors.js Outdated Show resolved Hide resolved

aduh95 reviewed May 11, 2021

View reviewed changes

lib/util/errors.js Show resolved Hide resolved

jasnell force-pushed the add-util-errors branch from 0ce6341 to 82a8fd0 Compare May 12, 2021 19:07

util: add util/errors

6148519

Exports a subset of `internal/errors` as `require('util/errors')`. The reason for `util/errors` is because there is already a `errors` module on npm that has a non-trivial number of downloads per week. Signed-off-by: James M Snell <jasnell@gmail.com>

jasnell force-pushed the add-util-errors branch from 82a8fd0 to 6148519 Compare May 12, 2021 19:55

mcollina approved these changes May 12, 2021

View reviewed changes

jasnell added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. review wanted PRs that need reviews. labels May 13, 2021

targos reviewed May 13, 2021

View reviewed changes

ronag approved these changes May 13, 2021

View reviewed changes

jasnell removed the review wanted PRs that need reviews. label May 13, 2021

targos self-assigned this May 17, 2021

targos approved these changes May 18, 2021

View reviewed changes

BridgeAR requested changes May 18, 2021

View reviewed changes

jasnell removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label May 20, 2021

DerekNonGeneric suggested changes May 22, 2021

View reviewed changes

jasnell closed this May 31, 2021

kanongil mentioned this pull request Oct 21, 2021

Make AbortError public #38361

Closed

targos removed their assignment Oct 31, 2021

util: add util/errors #38575

util: add util/errors #38575

Conversation

jasnell commented May 6, 2021 • edited Loading

mcollina commented May 6, 2021

mcollina commented May 6, 2021

jasnell commented May 7, 2021 • edited Loading

mcollina commented May 7, 2021

jasnell commented May 7, 2021

nodejs-github-bot commented May 7, 2021

joyeecheung commented May 8, 2021

mcollina left a comment

Choose a reason for hiding this comment

targos commented May 10, 2021

jasnell commented May 10, 2021

nodejs-github-bot commented May 10, 2021

nodejs-github-bot commented May 12, 2021 • edited by jasnell Loading

mcollina left a comment

Choose a reason for hiding this comment

jasnell commented May 13, 2021

targos May 13, 2021

Choose a reason for hiding this comment

targos May 13, 2021

Choose a reason for hiding this comment

targos May 13, 2021

Choose a reason for hiding this comment

jasnell May 13, 2021

Choose a reason for hiding this comment

ronag left a comment

Choose a reason for hiding this comment

jasnell commented May 13, 2021

mhdawson commented May 14, 2021

jasnell commented May 14, 2021

mhdawson commented May 14, 2021

jasnell commented May 17, 2021

targos commented May 17, 2021

BridgeAR left a comment • edited Loading

Choose a reason for hiding this comment

jasnell commented May 19, 2021 • edited Loading

jasnell commented May 20, 2021

DerekNonGeneric left a comment

Choose a reason for hiding this comment

jasnell commented May 24, 2021

mcollina commented May 24, 2021

mhdawson commented May 26, 2021

mcollina commented May 27, 2021

mhdawson commented May 31, 2021

jasnell commented May 31, 2021

mhdawson commented May 31, 2021

jasnell commented May 6, 2021 •

edited

Loading

jasnell commented May 7, 2021 •

edited

Loading

nodejs-github-bot commented May 12, 2021 •

edited by jasnell

Loading

BridgeAR left a comment •

edited

Loading

jasnell commented May 19, 2021 •

edited

Loading