[WIP] errors: add internal/errors module by jasnell · Pull Request #6573 · nodejs/node · GitHub

Checklist

tests and code linting passes
a test and/or benchmark is included
documentation is changed or added
the commit message follows commit guidelines

Affected core subsystem(s)

errors (new subsystem)

Description of change

Note: This is a _work in progress. I am opening the PR to begin the discussion about the overall design. I will be iterating based on the feedback. This is far from being ready to land in it's current state._

This is step one to making it so that changes to error messages are not forced to be semver-major changes any more. The internal errors.js module exports three custom error class instances that use an assigned static error code to look up the error message. The message itself can change but the error code would remain the same. Changes to error code would still be considered semver-major, but changes to the messages themselves would not be.

This also updates the Error objects created in lib/internal to use the new mechanism, changing and streamlining several of the error messages in the process. Obviously, this is a semver-major change on it's own.

Several test cases were also modified and a new common.throws method added to test/common.js that understands the new code mechanism. This is added common.js so that the existing assert module api is unchanged.

Update: This PR is purposefully being held back from landing until we are closer to actually releasing v7. Until then, it will need to be periodically updated to pick up any other changes in error handling that land. PRs are welcome. Please label any other PRs that affect error handling with the error-handling label so that those can be easily tracked.

We should be safe to remove the E prefix on the error codes... anything at Error[Symbol.for('code')] will be an error.

Thoughts on exporting the error codes?

... with all of the ENO codes its hard for me not to think of Brian Eno ... well done :)

Error[Symbol.for('code')]

Yeah, considered that possibility also. Definitely makes sense to do it that way.

Somewhat related to #4311

@nodejs/ctc ... this now covers all Error/TypeError/RangeError throws we generate from lib/*.js . It does not yet include the errors thrown from src/*. It's a sizable change. To make it easier to review, I'd start here and here. The former being the new lib/internal/errors.js file and the latter being an example of the impact of the change.

Essentially this PR currently consolidates, and makes consistent, all error messages thrown in lib/*.js and associates a stable code with each.

Please review.

does this need to be ...args? whoops, this goes to message()

Nope. https://github.com/nodejs/node/pull/6573/files#diff-5a3344c263a73c663dd1cfcb91880fd7R75

Should we have NodeSyntaxError for invalid arguments? (Since we kinda treat them as that?)

no need for module?

We currently use TypeError for the most part for invalid arguments. There are a few cases where Error is used. If we wanted to switch to using SyntaxError then I would recommend doing that in a separate semver-major PR.

We currently use TypeError for the most part for invalid arguments.

oooh true. that is a better option.

Attaching to existing globals makes me a bit hesitant, but I understand it does kinda make sense here.

I understand the hesitancy. Use of Symbol should make this less of a problem. TBH, however, we're not documenting that we're hanging these off Error so I'm fine changing this part.

I'd like to wait to hear others chime in.

Could we possibly prefix the symbol or some such? e.g. Symbol.for(node.${sym}); to prevent collisions.

sorry. i'm missing why this needs to be hung off the global Error?

Overall seems good. Definitely think this is a good approach.

Updated.

shouldn't requires be at the top of the file?

When there are only one or two cases in a file, I did it like this so the
require is only called in error conditions
On May 9, 2016 5:27 PM, "Rod Vagg" notifications@github.com wrote:

In lib/_debugger.js
#6573 (comment):
@@ -126,7 +126,8 @@ Protocol.prototype.execute = function(d) {
break;
 default:
 throw new Error('Unknown state');
 const errors = require('internal/errors');
shouldn't requires be at the top of the file?

—
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
https://github.com/nodejs/node/pull/6573/files/56d34d36a841cee62adb657548b90bd27ebce870#r62595987

Lazy loading is pretty common in core code. Though I'm not sure how much it actually saves us now.

Is it required that this also include C++ errors or could we do that separately but agree on one syntax?

Do we know what syntax constrains we may have from natives errors than we would from JS, if any?

No, the C++ errors can be done separately, I just want to make sure it gets done. The important thing is that there needs to be agreement on syntax.

The only constraint that I can see is that the C++ error messages tend to be far more terse.

@jasnell just curious, when we upgrade v8 within a major do we ever verify that none of the error messages changed?

Not that I'm aware of.

nit: space in ... args.

does the linter not catch this?

I haven't run the linter on this in a while so not sure. I'll be doing an update on this next week so I'll see for sure what lint issues come up.

The linter will not catch this because we do not have the rest-spread-spacing rule enabled. Sounds like we'd like to enforce no space between the spread operator and the argument, so I'll get a PR for that in Real Soon Now...

Other than the fact that this would generate stacks one line shorter than Error.stackTraceLimit I'm not sure what the problem would be with doing something like:

function fixErrStack(stack) {
  const newstring = [];
  const idx0 = stack.search(/[\r\n]+/);
  newstring.push(stack.substr(0, idx0));
  const stack2 = stack.substr(idx0).trim();
  const idx1 = stack2.search(/[\r\n]+/);
  newstring.push(stack2.substr(idx1).trim());
  return newstring.join('\n');  // detect whether to insert \r\n?
}

const e = new Error('hi');
e.stack = fixErrStack(e.stack);

instead of creating two error objects.

@trevnorris ... sorry, not following

@jasnell

Not that I'm aware of.

Logically I'd say that changes to error messages in v8 would also be a major if we treat them as major in node. Hasn't been a problem to date, but just throwing it out there.

... sorry, not following

How you are using Error.captureStackTrace() in an extended Error class (I believe to remove the top most function call). In order to prevent needing to create two stack traces instead of just one.

Re: treating v8 error changes as majors, I don't disagree, just not sure what all we can do about it.
Re: the captureStackTrace()... yeah, that part is irritating. I'll make a note to explore that further.

node version 7 officially deprecates `fs.read` and `fs.readSync` This is done using `internal/util`. The unfortunate side affect being that `graceful-fs v3.x` explodes when running the code in `vm` as `internal/util` is not accessible from userland. This commit uses a regular expression to replaces the require of the specific internal util function with the source of that util function. As such `graceful-fs v3.x` will no longer explode in node v7. One advantage to this approach is that any future deprecation will not break graceful-fs.

I think this returns false positives for e.g. Uint8Array – maybe the message should just read something like must be of type ${expected}?

Pulling this off the v7 milestone. It's not going to get done in time for that.

Going to close this PR in favor of breaking these changes up into multiple separate PRs that can be done incrementally.

Why use var ? I think better to use let.

jasnell added wip Issues and PRs that are still a work in progress. semver-major PRs that contain breaking changes and should be released in the next major version. discuss Issues opened for discussions and feedbacks. error-handling labels May 4, 2016

geek reviewed May 4, 2016
View reviewed changes

jasnell force-pushed the no-major-errors branch 3 times, most recently from 2efc255 to 76bc6e6 Compare May 6, 2016 19:27

Fishrock123 reviewed May 9, 2016
View reviewed changes

jasnell force-pushed the no-major-errors branch from 76bc6e6 to 56d34d3 Compare May 9, 2016 21:08

rvagg reviewed May 10, 2016
View reviewed changes

trevnorris reviewed Aug 11, 2016
View reviewed changes

Trott mentioned this pull request Aug 16, 2016

Node.js Foundation Core Technical Committee (CTC) Meeting 2016-08-17 #8125

Closed

Trott removed the ctc-agenda label Aug 16, 2016

This was referenced Aug 17, 2016

RFE: Articulate benefits of moving code to internal/... #8149

Closed

lib,fs: use process.emitWarning instead of internal print dep warning #8166

Closed

addaleax reviewed Aug 23, 2016
View reviewed changes

jasnell removed this from the 7.0.0 milestone Aug 26, 2016

jasnell removed the blocked PRs that are blocked by other issues or PRs. label Aug 29, 2016

jasnell mentioned this pull request Aug 30, 2016

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

Closed

Trott force-pushed the master branch from b0df363 to c5ce7f4 Compare September 21, 2016 00:09

jasnell changed the title errors: add internal/errors module [WIP] errors: add internal/errors module Sep 27, 2016

rvagg force-pushed the master branch 2 times, most recently from c133999 to 83c7a88 Compare October 18, 2016 17:01

jasnell closed this Oct 24, 2016

jasnell mentioned this pull request Oct 24, 2016

errors: add internal/errors.js #9265

Closed

3 tasks

tarunama reviewed Apr 30, 2018

View reviewed changes

+                  const ch = expected.codePointAt(0) | 0x20;
+                  const prefix = (ch === 0x61 || ch === 0x65 ||
+                                  ch === 0x69 || ch === 0x6f ||
+                                  ch === 0x75) ? 'an' : 'a';

+              // portable with the readable-streams module.
+              function typeError(code, args) {
+                const assert = require('assert');
+                var msg;

+              }
+              class NodeRangeError extends RangeError {
+                constructor(key, ... args) {

[WIP] errors: add internal/errors module by jasnell · Pull Request #6573 · nodejs/node · GitHub

Uh oh!

Uh oh!

[WIP] errors: add internal/errors module - #6573

[WIP] errors: add internal/errors module#6573jasnell wants to merge 1 commit intonodejs:masternodejs/node:masterfrom jasnell:no-major-errorsjasnell/node:no-major-errorsCopy head branch name to clipboard

Conversation

jasnell commented May 4, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Affected core subsystem(s)

Description of change

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Fishrock123 commented May 5, 2016

Uh oh!

jasnell commented May 9, 2016

Uh oh!

Fishrock123 May 9, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Fishrock123 commented May 9, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jasnell commented May 9, 2016

Uh oh!

Fishrock123 commented May 9, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Fishrock123 commented May 9, 2016

Uh oh!

jasnell commented May 9, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Fishrock123 commented Aug 10, 2016

Uh oh!

jasnell commented Aug 10, 2016

Uh oh!

trevnorris commented Aug 11, 2016

Uh oh!

jasnell commented Aug 11, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

trevnorris commented Aug 11, 2016

Uh oh!

jasnell commented Aug 11, 2016

Uh oh!

trevnorris commented Aug 11, 2016

Uh oh!

jasnell commented Aug 11, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

[WIP] errors: add internal/errors module#6573
jasnell wants to merge 1 commit into
nodejs:masterfrom
jasnell:no-major-errors

jasnell commented May 4, 2016 •
edited
Loading

Fishrock123 May 9, 2016 •
edited
Loading