lib: new error implementation #18857

BridgeAR · 2018-02-18T20:26:39Z

This implements a function based error system. Instead of passing in the
error code as first argument, the error code itself is a error class.
It already contains the correct error type, so while adding a new
error no one has to think about the error type anymore. In case a
single error code has more than one error type, the error class has
properties for the non default error types. Those can be used as
fallback.

This prevents typos, makes the implementation easier and it is less
verbose when writing the code for a new error.

The implementation itself does not interfere with the old
implementation. So the old and the new system can co-exist and it is
possible to slowly migrate the old ones to the new system.

By doing this I also realized that there are multiple error codes that
are currently not used at all. I also found other mistakes where a
wrong error type was in place.

So far this is only WIP and I wanted to get some feedback.

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
documentation is changed or added
commit message follows commit guidelines

Affected core subsystem(s)

lib

addaleax · 2018-02-18T21:21:50Z

lib/internal/errors.js

+
+    get name() {
+      return `${super.name} [${key}]`;
+    }


In this case we might also want to create a static get name() so that the class itself can be disambiguated from other classes of this type?

Or even better, NodeError.name = ....

I thought about this a few times now and I decided not to do that out of the following reasons:

We only use the class internal and do not expose it directly.

The main class has sub classes as properties and those should make it easy to identify the correct one.

If we add the name, the original NodeError name will not be there and I think it should be.

devsnek · 2018-02-18T22:04:33Z

lib/internal/errors.js

+      return key;
+    }
+
+    set code(value) {


is there any use case for this?

My guess is that userland modules (not aware of the standardized node core error objects) may have already been setting this property to something else.

Yes, this was specifically asked for by userland module authors who need the ability to keep specifying their own codes.

devsnek

lgtm with addaleax's comment addressed

jasnell · 2018-02-18T23:41:38Z

lib/console.js

@@ -35,12 +35,12 @@ function Console(stdout, stderr, ignoreErrors = true) {
    return new Console(stdout, stderr, ignoreErrors);
  }
  if (!stdout || typeof stdout.write !== 'function') {
-    throw new errors.TypeError('ERR_CONSOLE_WRITABLE_STREAM', 'stdout');
+    throw new ERR_CONSOLE_WRITABLE_STREAM('stdout');


The only challenge I really have with this is that it makes it far less obvious what kind of error is being thrown here.

That is also the charming side: you do not have to care about that as it was already taken care off in an earlier step. And we can verify much better that it actually has the correct type. Now you can see the types in the overview in the errors.js file. And I spotted multiple errors due to this.

This implements a function based system. Instead of passing in the error code as first argument, the error code itself is a error class. It already contains the correct error type, so while adding a new error no one has to think about the error type anymore. In case a single error code has more than one error type, the error class has properties for the non default error types. Those can be used as fallback. This prevents typos, makes the implementation easier and it is less verbose when writing the code for a new error. The implementation itself does not interfere with the old implementation. So the old and the new system can co-exist and it is possible to slowly migrate the old ones to the new system.

This updates all internal errors to the new error type. While doing so it removes unused errors. A few errors currently seem to have the wrong type. To identify them later, comments were added next to the error type.

This ports the errors to the new error system.

Some error types are not properly set. This adds comments which ones are probably falty and to what they should be set instead.

BridgeAR · 2018-02-19T01:41:40Z

I updated all error codes and removed the breaking change. It can now be used for all error codes. I also added quite a few comments about wrong error types. Please have a look. I personally suggest to keep the comments for now and change those in a different PR as that is breaking.

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

joyeecheung

LGTM with a suggestion, thanks!

joyeecheung · 2018-02-19T10:53:25Z

lib/internal/errors.js

@@ -86,6 +87,54 @@ function makeNodeError(Base) {
  };
 }

+function makeNodeError2(Base, key) {


Can this be renamed to something more descriptive e.g. makeNodeErrorWithCode?

BridgeAR · 2018-02-21T00:28:35Z

@mhdawson one NAPI code got removed that was not used (ERR_NAPI_CONS_PROTOTYPE_OBJECT). I checked every code that I removed and there are no hits anywhere for that code besides in the docs and errors.js. You can spot all removed error codes easily by looking at the documentation (I removed all of those from the documentation as well).

BridgeAR · 2018-02-22T11:21:22Z

I changed the name. Running a Light-CI just to verify the minor changes since the full CI:

https://ci.nodejs.org/job/node-test-commit-light/307/console

This implements a function based system. Instead of passing in the error code as first argument, the error code itself is a error class. It already contains the correct error type, so while adding a new error no one has to think about the error type anymore. In case a single error code has more than one error type, the error class has properties for the non default error types. Those can be used as fallback. This prevents typos, makes the implementation easier and it is less verbose when writing the code for a new error. The implementation itself does not interfere with the old implementation. So the old and the new system can co-exist and it is possible to slowly migrate the old ones to the new system. PR-URL: nodejs#18857 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Gus Caplan <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Joyee Cheung <[email protected]>

This updates all internal errors to the new error type. While doing so it removes unused errors. A few errors currently seem to have the wrong type. To identify them later, comments were added next to the error type. PR-URL: nodejs#18857 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Gus Caplan <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Joyee Cheung <[email protected]>

This ports the errors to the new error system. PR-URL: nodejs#18857 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Gus Caplan <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Joyee Cheung <[email protected]>

Some error types are not properly set. This adds comments which ones are probably falty and to what they should be set instead. PR-URL: nodejs#18857 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Gus Caplan <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Joyee Cheung <[email protected]>

PR-URL: nodejs#18857 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Gus Caplan <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Joyee Cheung <[email protected]>

This restores a broken and erroneously removed error, which was accidentially renamed to ERR_MISSING_DYNAMIC_INTSTANTIATE_HOOK (notice the "INTST" vs "INST") in 921fb84 (PR #16874) and then had documentation and implementation removed under the old name in 6e1c25c (PR #18857), as it appeared unused. This error code never worked or was documented under the mistyped name ERR_MISSING_DYNAMIC_INTSTANTIATE_HOOK, so renaming it back to ERR_MISSING_DYNAMIC_INSTANTIATE_HOOK is a semver-patch fix. Refs: #21440 Refs: #21470 Refs: #16874 Refs: #18857

This restores a broken and erroneously removed error, which was accidentially renamed to ERR_MISSING_DYNAMIC_INTSTANTIATE_HOOK (notice the "INTST" vs "INST") in 921fb84 (PR nodejs#16874) and then had documentation and implementation removed under the old name in 6e1c25c (PR nodejs#18857), as it appeared unused. This error code never worked or was documented under the mistyped name ERR_MISSING_DYNAMIC_INTSTANTIATE_HOOK, so renaming it back to ERR_MISSING_DYNAMIC_INSTANTIATE_HOOK is a semver-patch fix. Refs: nodejs#21440 Refs: nodejs#21470 Refs: nodejs#16874 Refs: nodejs#18857

This restores a broken and erroneously removed error, which was accidentially renamed to ERR_MISSING_DYNAMIC_INTSTANTIATE_HOOK (notice the "INTST" vs "INST") in 921fb84 (PR #16874) and then had documentation and implementation removed under the old name in 6e1c25c (PR #18857), as it appeared unused. This error code never worked or was documented under the mistyped name ERR_MISSING_DYNAMIC_INTSTANTIATE_HOOK, so renaming it back to ERR_MISSING_DYNAMIC_INSTANTIATE_HOOK is a semver-patch fix. Refs: #21440 Refs: #21470 Refs: #16874 Refs: #18857 PR-URL: #21493 Reviewed-By: Gus Caplan <[email protected]> Reviewed-By: Trivikram Kamat <[email protected]> Reviewed-By: Vse Mozhet Byt <[email protected]> Reviewed-By: Tiancheng "Timothy" Gu <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Ron Korving <[email protected]> Reviewed-By: Luigi Pinca <[email protected]>

MylesBorins · 2018-07-31T18:41:35Z

Backport requested for 8.x in #19244

BridgeAR requested review from jasnell and joyeecheung February 18, 2018 20:26

nodejs-github-bot added console Issues and PRs related to the console subsystem. errors Issues and PRs related to JavaScript errors originated in Node.js core. module Issues and PRs related to the module subsystem. labels Feb 18, 2018

BridgeAR added the wip Issues and PRs that are still a work in progress. label Feb 18, 2018

addaleax approved these changes Feb 18, 2018

View reviewed changes

devsnek reviewed Feb 18, 2018

View reviewed changes

devsnek approved these changes Feb 18, 2018

View reviewed changes

jasnell reviewed Feb 18, 2018

View reviewed changes

jasnell approved these changes Feb 18, 2018

View reviewed changes

BridgeAR added 5 commits February 19, 2018 01:49

errors: update all internal errors

cc0edb9

This updates all internal errors to the new error type. While doing so it removes unused errors. A few errors currently seem to have the wrong type. To identify them later, comments were added next to the error type.

console: port errors to new system

a51e50d

This ports the errors to the new error system.

errors: add comments about falsy error types

ae87200

Some error types are not properly set. This adds comments which ones are probably falty and to what they should be set instead.

errors: simplify sysError

474c498

BridgeAR removed the wip Issues and PRs that are still a work in progress. label Feb 19, 2018

BridgeAR force-pushed the error-suggestion branch from df999dc to 474c498 Compare February 19, 2018 01:36

BridgeAR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Feb 19, 2018

fixup: nicer extra class implementation

5ee0ffb

joyeecheung approved these changes Feb 19, 2018

View reviewed changes

fixup: rename

e0df496

This was referenced Jun 23, 2018

errors: fix ERR_MISSING_DYNAMIC_INSTANTIATE_HOOK #21492

Closed

errors: fix ERR_MISSING_DYNAMIC_INSTANTIATE_HOOK #21493

Closed

ChALkeR mentioned this pull request Jun 25, 2018

doc: remove 2 unused error codes from errors.md #21491

Merged

4 tasks

MylesBorins added the backport-requested-v8.x label Jul 31, 2018

BridgeAR deleted the error-suggestion branch April 1, 2019 23:38

snyk-bot mentioned this pull request Aug 15, 2021

[Snyk] Fix for 1 vulnerabilities ron-a20/node#39

Open

molllyn1 mentioned this pull request Mar 22, 2024

[Snyk] Fix for 2 vulnerabilities molllyn1/node#106

Open

137717unity mentioned this pull request Mar 22, 2024

[Snyk] Fix for 2 vulnerabilities 137717unity/node#122

Open

kissofire mentioned this pull request Mar 22, 2024

[Snyk] Fix for 2 vulnerabilities kissofire/node#124

Open

aliceUnhinged613 mentioned this pull request Mar 22, 2024

[Snyk] Fix for 2 vulnerabilities aliceUnhinged613/node#123

Open

ron-a20 mentioned this pull request Mar 22, 2024

[Snyk] Fix for 2 vulnerabilities ron-a20/node#121

Open

wedataintelligence mentioned this pull request Mar 23, 2024

[Snyk] Fix for 2 vulnerabilities wedataintelligence/node#105

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

lib: new error implementation #18857

lib: new error implementation #18857

BridgeAR commented Feb 18, 2018 •

edited

Loading

addaleax Feb 18, 2018

TimothyGu Feb 19, 2018

BridgeAR Feb 19, 2018

devsnek Feb 18, 2018

mscdex Feb 18, 2018

jasnell Feb 18, 2018

devsnek left a comment

jasnell Feb 18, 2018

BridgeAR Feb 19, 2018 •

edited

Loading

BridgeAR commented Feb 19, 2018

joyeecheung left a comment

joyeecheung Feb 19, 2018

BridgeAR commented Feb 21, 2018

BridgeAR commented Feb 22, 2018

MylesBorins commented Jul 31, 2018

lib: new error implementation #18857

lib: new error implementation #18857

Conversation

BridgeAR commented Feb 18, 2018 • edited Loading

Checklist

Affected core subsystem(s)

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

devsnek left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BridgeAR Feb 19, 2018 • edited Loading

Choose a reason for hiding this comment

BridgeAR commented Feb 19, 2018

joyeecheung left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BridgeAR commented Feb 21, 2018

BridgeAR commented Feb 22, 2018

MylesBorins commented Jul 31, 2018

BridgeAR commented Feb 18, 2018 •

edited

Loading

BridgeAR Feb 19, 2018 •

edited

Loading