errors: add internal/errors.js#11220

jasnell · 2017-02-07T17:51:57Z

Add the internal/errors.js core mechanism.

This was extracted from #9265 with some updates. It adds the basic mechanism for moving to the internal errors module without modifying any of the actual error instances within core. Transitioning to the use of this module should be done incrementally in small chunks over time.

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)

errors

joyeecheung · 2017-02-07T18:02:30Z

doc/guides/using-internal-errors.md

s/adding/added/

joyeecheung · 2017-02-07T18:10:14Z

doc/guides/using-internal-errors.md

If I understand correctly, the name of this function being so short is to echo the existing convention for error names like ENOTFOUND? But this convention is no longer followed in this new error code system?

This is short to make it as brief as possible. It is intended to be used only within this file. It is exported only to facilitate testing.

Yes, but this is a guide so I think this would be read by contributors, especially those who are new? And they might need to change that file and call E to register new errors.

joyeecheung · 2017-02-07T18:22:12Z

doc/guides/using-internal-errors.md

Are the arguments left undocumented because they are non-standard? Probably an explanation would help readers to understand what they are(e.g. a link to MDN)

The arguments are arbitrary and dependent on the specific error message (essentially the same as util.format(). Specifically, the error message for a key may be something like 'Foo %s %d', so that errors.Error('KEY', 's', '1') would produce the error message 'Foo s 1'

Thanks for the explanation, but does this built-in errors.Error() method have a formatting function defined? It seems to just pass the arguments to the Error base class, so we already know what the format would be(at least for V8)?

Or, do we allow users to customize the format of these builtin errors?

Look at the message() function. When a key is registered using the E() function, it's associated value is either a string with optional util.format tokens or a function that returns a string. Inside message(), the key lookup returns this value. If the value is a string, it is passed through util.format(), otherwise the arguments are passed to the function, allowing more customized formatting for the error.
Again, this is not a user facing API. This is for use only within core and would be used anywhere we throw an internal error.

Oh, yes, but "users" I meant "consumers of this API, i.e. contributors". I think my questions have been answered, thanks.
On a side note, I think I was having questions because the formatting behavior is explained near the bottom of this document, so by the time I read this, I was not sure how the args are going to be consumed. A reference to the formatting process might help, or maybe the formatting process can be explained first, but maybe that's just me :).

joyeecheung · 2017-02-07T18:24:34Z

lib/internal/errors.js

Maybe we can just use the rest operator? Probably faster in newer versions of V8.

rest parameters are not yet fully optimized. There is a benchmark already to test those. Once they have been optimized we can switch.

Do you refer to benchmark/misc/console.js? I just run it with the latest master and it seems the usage with arguments is already optimized?
./node benchmark/run.js --filter console misc misc/console.js misc/console.js n=1000000 concat=1 method="restAndSpread": 499,544.4494311414 misc/console.js n=1000000 concat=0 method="restAndSpread": 513,992.8268692781 misc/console.js n=1000000 concat=1 method="argumentsAndApply": 619,756.4629049919 misc/console.js n=1000000 concat=0 method="argumentsAndApply": 624,507.957596242 misc/console.js n=1000000 concat=1 method="restAndApply": 688,473.7438723565 misc/console.js n=1000000 concat=0 method="restAndApply": 681,323.2961384911 misc/console.js n=1000000 concat=1 method="restAndConcat": 895,355.2343246405

It's possible that things have improved with the most recent v8 drops. I'm running the benchmark/es/restparams-bench.js right now to compare

Oh, yes, those are the right benchmarks.
Also I discovered something interesting. If I leave the forced optimization there, the results are being consistent with:
es/restparams-bench.js es/restparams-bench.js millions=100 method="copy": 30.148206272683915 es/restparams-bench.js millions=100 method="rest": 22.696521524914942 es/restparams-bench.js millions=100 method="arguments": 55.12597317932212
But if I comment out the forced optimization:
es/restparams-bench.js es/restparams-bench.js millions=100 method="copy": 30.754392030574046 es/restparams-bench.js millions=100 method="rest": 33.315344080305636 es/restparams-bench.js millions=100 method="arguments": 57.77862383401849

Oh, very nice, these results are MUCH better than they were just a couple of months ago. Switching to rest params!!
improvementconfidencep.valuees/restparams-bench.jsmillions=100method="arguments"-0.17%0.9399783es/restparams-bench.jsmillions=100method="copy"0.02%0.9923623es/restparams-bench.jsmillions=100method="rest"4.06%0.1729710

joyeecheung · 2017-02-07T18:30:29Z

lib/internal/errors.js

A more informative message would be helpful here.

This assert is not intended to be user facing. This is simply to make sure proper use of the internal API and is here only to make sure that node itself is passing in the right kind of value.

joyeecheung · 2017-02-07T18:32:26Z

lib/internal/errors.js

If I am reading this right, the second time a key is passed into this, it will be in the map and this assertion won't fail, even though the error is not properly registered with E?

ah, right, good point.

joyeecheung · 2017-02-07T18:36:03Z

test/parallel/test-internal-errors.js

What happens if this block is executed twice?(TEST_FOO_KEY gets passed twice)

addaleax · 2017-02-07T18:57:08Z

doc/guides/using-internal-errors.md

Since we now have rest parameters, I’d be in favour of using ...args in these places if that’s possible?

addaleax · 2017-02-07T18:58:08Z

doc/guides/using-internal-errors.md

There’s a missing backtick on all copies of this line. :D

addaleax · 2017-02-07T19:01:18Z

lib/internal/errors.js

Maybe this can be a mixin to avoid code duplication?

Possibly. I did it this way in order to preserve instanceof checks (e.g. (new error.TypeError()) instanceof TypeError) but may be able to find a way around that.

Not sure, would that really be much of a problem? What I had in mind was something like this:
constmakeNodeError=(Base)=>classNodeErrorextendsBase{[…currentcode,exceptonlyonce…]staticgetname(){return`Node${super.name}`;}/* optional */};constNodeError=makeNodeError(Error);constNodeRangeError=makeNodeError(RangeError);constNodeTypeError=makeNodeError(TypeError);

Ah right :-) .. that works too

jasnell · 2017-02-07T19:56:21Z

Updated! PTAL

jasnell · 2017-02-07T19:59:13Z

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

addaleax

Looking good, thanks for (re-)starting this!

And by the way, I don’t think this really counts semver-minor if it doesn’t touch the public API. (Probably makes no difference, though. 😄)

addaleax · 2017-02-07T20:01:34Z

doc/guides/using-internal-errors.md

Missing quote ;)

grr.. lol ...

Hmmm, the linter doesn't lint code inside doc/guides?

addaleax · 2017-02-07T20:05:40Z

lib/internal/errors.js

fmt(...args);? Not sure whether that was part of your conversation with @joyeecheung or not

That conversation is about rest operator and this is spread operator...not sure about the performance of spread though? From the results of benchmark/misc/console.js with method="restAndSpread" compared method="restAndApply" I think the spread operator is not ready yet.

It doesn't look like we have a benchmark for the spread operator so I'll open a PR to add one

addaleax · 2017-02-07T20:08:39Z

lib/internal/errors.js

I am not sure I understand why this file uses symbol properties of exports… wouldn’t const messages = new Map(); as a key → message map be a bit easier?

yeah doesn't really make much sense now. This was originally done to make it easier to discover/enumerate these but that originated in a much older version

jasnell · 2017-02-07T20:45:22Z

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

jasnell · 2017-02-07T21:18:31Z

Failure in CI is unrelated.

evanlucas · 2017-02-08T06:00:35Z

So, one concern I have is how will we backport these? Will we set the error codes on master and then just backport? If so, what happens if a new error is added to just a release line, but not to master?

jasnell · 2017-02-08T06:06:59Z

This would likely need to land in Node.js 8.0.0 and not be backported. The PRs to port existing errors to this mechanism would be semver-major initially so would no backport to older majors. Only changes after porting would be semver-minor or patch.

As much as possible we should try to avoid adding new types of errors (new error codes). On the off chance that we are forced to land something in a release line and not master, those would land in that release line and should be added to the documentation in master but not necessarily be used in master. Hopefully that made sense.

evanlucas · 2017-02-08T06:08:13Z

Thanks @jasnell! That makes sense to me. Just wanted to make sure it had been thought through. :]

Trott · 2017-02-09T00:22:17Z

doc/guides/using-internal-errors.md

Nit: semver-major label is kind of our internal jargon and people aren't going to understand (without an explanation) why we consider error message changes as semver major anyway.
Maybe just embrace completeness as this is a guide and go with something more like this?:
...parameterized message. [END OF PARAGRAPH]
The intent of the module is to allow errors provided by Node.js to be assigned a
permanent identifier. Without a permanent identifier, userland code may need to inspect
error messages to distinguish one error from another. An unfortunate result
of that practice is that changes to error messages result in broken code in the ecosystem.
For that reason, Node.js has considered error message changes to be breaking changes.
By providing a permanent identifier for a specific error, we reduce the need for userland
code to inspect error messages.

Trott · 2017-02-09T00:22:52Z

@jasnell This feature is long overdue. Thanks for doing all the work to get us to the point where we can do this!

Add the internal/errors.js core mechanism.

jasnell · 2017-02-09T16:29:40Z

@Trott, updated with your suggestion in the guide

jasnell · 2017-02-09T16:30:55Z

New CI before landing: https://ci.nodejs.org/job/node-test-pull-request/6315/

Add the internal/errors.js core mechanism. PR-URL: #11220 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Joyee Cheung <[email protected]>

jasnell · 2017-02-09T21:48:53Z

Landed in 159749d

Add the internal/errors.js core mechanism. PR-URL: #11220 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Joyee Cheung <[email protected]>

Trott · 2017-02-14T17:29:20Z

test/common.js

 };
+
+// Useful for testing expected internal/error objects
+exports.expectsError=functionexpectsError(code,type,message){


This function was added to common.js but not documented in the test/README.md.

evanlucas · 2017-02-14T18:54:40Z

and documented or not, err.code is used all over the ecosystem...

jasnell · 2017-02-14T18:55:05Z

Why? None of those existing errors or codes are affected by this in any way. The code property was selected precisely because it is already used.

jasnell · 2017-02-14T18:58:06Z

To be certain, this mechanism does not change or override any of the existing core uses of .code, does not interfere with any known public uses of .code, and would be used only to add .code to errors generated by core that do not already have a .code property.

joyeecheung · 2017-02-14T19:00:13Z

Hmm, so for now we just leave the err.code untouched for those modules that already set this, and update all the other modules to set err.code with a new convention? That sounds good to me, though we should document about this inconsistency, otherwise it just causes more confusions to the users IMO.

jasnell · 2017-02-14T19:01:35Z

For now, yes. Eventually we should work on making all of the errors consistent, but for those errors that already have a .code, there's no immediate need to change those.

joyeecheung · 2017-02-14T19:06:22Z

lib/internal/errors.js

+}
+
+getcode(){
+returnthis[kCode];


Hmm, so if a PR updates a module to throw the new errors, since code is read only here, any code setting .code later would not have effect, then this could break existing behavior unless we check carefully enough before landing those PRs? Can we add a setter that do some kinds of assertion here?

For example:
setcode(options){if(typeofoptions==='object'&&options.override===true){this[kCode]=options.code;return;}assert.fail('This error can not be migrated for now');}

EDIT: forgot return ^

I'd rather just leave it read only. If someone really wanted to override the value of .code, they can easily use Object.defineProperty() to do so.

then just raise the assertion then? This is just to prevent we accidently migrate an error that already have its .code set by existing code (but it could be hard to catch if that code happens much later than its creation, like in a callback), and by throwing this new kind of error, that code can't actually set .code.

There are no cases that I'm aware of where we set the code later. And in the case a user attempts to set the code, I'd rather they not see an assertion error. We can best catch these cases in code review

Yeah, that can not be let loose in the user land. Anyway thanks for bearing with me :P

evanlucas · 2017-02-14T21:35:21Z

I think @Trott brings up a good point...

I'm not sure how I feel about error messages changing from TypeError to TypeError[ERR_INVALID_ARG_TYPE]. I didn't realize this changed the output of the error message :[

Trott · 2017-02-14T22:23:07Z

I'm not sure how I feel about error messages changing from TypeError to TypeError[ERR_INVALID_ARG_TYPE]. I didn't realize this changed the output of the error message

I'm still not sure where I stand on this. I'm trying to work out the cost/benefit on this.

Like, if I thought that changing the error message to that would mostly-eliminate full-message-text-sniffing and therefore allow us to treat message changes as semver-patch, I'd probably be all for it.

But I'm not sure I can convince myself that will happen.

And there's a potential of a lot of adverse effects on the ecosystem from this.

jasnell · 2017-02-14T22:38:55Z

And there's a potential of a lot of adverse effects on the ecosystem from this

Serious question: Like what?

The code is included in the .name property (and thus printed when the Error is output to the console) in order to give users come static identifier they can copy and google for as easily as possible, even if the message itself changes (or, at some point down the road, is translated).

The .code property allows this to be easily acquired programatically for the same effect.

The actual .message property is not touched, except when going through and normalizing the error messages that Node.js is producing. The error code is omitted from the message property specifically to discourage users from parsing the message to get that information. Any regexp checks on the message will only be impacted by the actual changes to the error messages and not by the inclusion of the code in the name.

For each of these errors, err instanceof Error, err instanceof TypeError and err instanceof RangeError still works as expected. util.isError(err) returns true. Error.prototype.toString(err) returns Error.

Trott · 2017-02-14T23:29:55Z

This is a total nit, but the lack of space before the opening square bracket suggests it's an index or property operator and not for human consumption. I wonder if it would be better to use parentheses and to use a space. Maybe put it on the other side of the colon while we're at it:

TypeError: (ERR_INVALID_ARG_TYPE) The "warning" argument must be one of type Error, or string

jasnell · 2017-02-14T23:35:28Z

Putting it on the other side of the colon implies that we're either customizing the toString() output or prepending it to the message, neither of which is something that I'd like to do as it becomes a much more invasive change. We could wrap within parens and put a space in front.

Trott · 2017-02-14T23:41:21Z

Serious question: Like what?

Anything that uses assert.throws() with a regular expression could be susceptible. Anything that matches on /TypeError: foo/ now needs to match on /TypeError[ERR_CODE]: foo/. (This problem is actually how I first noticed this change. A previously-passing assert.throws() was failing in a PR because the regexp did not have the [ERR_CODE] part.)

You raise some good points I didn't realize that mitigate things. I guess CITGM runs on the various PRs can help provide some data about how widespread issues might be.

jasnell · 2017-02-14T23:47:26Z

Yep, uses of assert.throws() can be impacted just like any error message change, which is why moving to this new mechanism requires a semver-major label. It would have no more significant impact than making any change to an error message (add a period to your /TypeError: foo\./ and you also run into issues). This is not unique to this change and is why the change is opt in, and why common.expectsError() is added for our own tests.

Trott · 2017-02-15T00:00:18Z

Yep, uses of assert.throws() can be impacted just like any error message change, which is why moving to this new mechanism requires a semver-major label. It would have no more significant impact than making any change to an error message (add a period to your /TypeError: foo./ and you also run into issues). This is not unique to this change and is why the change is opt in, and why common.expectsError() is added for our own tests.

I think these two things are very different:

"we changed a handful of error messages in this next major version and it's going to break a few people's tests here and there"
"we've changed every/nearly-every/most/half/whatever error message emitted by Node.js core"

jasnell · 2017-02-15T00:06:02Z

Yep, which is why changes need to happen incrementally. The initial version of this pr included changes for everything, all at once. I backed off that so that individual changes can be looked at case by case

Trott · 2017-02-15T00:07:02Z

BTW, I'm not arguing against this. I'm still trying to assess it.

The fact that this will change pretty much all (or at least a very large number of) core error messages was a surprise to me. Judging from his comments, it surprised @evanlucas as well. Small data set, yes. It's entirely possible this is totally the way to go.
¯\(ツ)/¯

Add the internal/errors.js core mechanism. PR-URL: nodejs#11220 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Joyee Cheung <[email protected]>

jasnell added the semver-minor PRs that contain new features and should be released in the next minor version. label Feb 7, 2017

nodejs-github-bot added build Issues and PRs related to build files or the CI. errors Issues and PRs related to JavaScript errors originated in Node.js core. labels Feb 7, 2017

joyeecheung reviewed Feb 7, 2017
View reviewed changes

addaleax reviewed Feb 7, 2017
View reviewed changes

jasnell force-pushed the internal-errors branch from 871565c to aa81f06Compare February 7, 2017 19:56

addaleax approved these changes Feb 7, 2017
View reviewed changes

jasnell force-pushed the internal-errors branch from aa81f06 to 6f2fe59Compare February 7, 2017 20:31

joyeecheung approved these changes Feb 7, 2017
View reviewed changes

Trott reviewed Feb 9, 2017
View reviewed changes

errors: add internal/errors.js
08f6372
Add the internal/errors.js core mechanism.

jasnell force-pushed the internal-errors branch from 6f2fe59 to 08f6372Compare February 9, 2017 16:29

jasnell added a commit that referenced this pull request Feb 9, 2017
errors: add internal/errors.js
159749d
Add the internal/errors.js core mechanism. PR-URL: #11220 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Joyee Cheung <[email protected]>

jasnell closed this Feb 9, 2017

jasnell mentioned this pull request Feb 9, 2017
Tracking Issue: Migrate errors to internal/errors.js #11273
Closed
80 tasks

joyeecheung mentioned this pull request Feb 10, 2017
New error system in WHATWG URL implementation #11299
Closed

italoacasas pushed a commit that referenced this pull request Feb 13, 2017
errors: add internal/errors.js
0d5e30b
Add the internal/errors.js core mechanism. PR-URL: #11220 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Joyee Cheung <[email protected]>

italoacasas mentioned this pull request Feb 13, 2017
Proposal - 7.6.0 (Current) #11185
Merged

Trott reviewed Feb 14, 2017
View reviewed changes

joyeecheung reviewed Feb 14, 2017
View reviewed changes

Trott mentioned this pull request Feb 14, 2017
Node.js 8.0.0 Tracking Issue #10117
Closed

italoacasas pushed a commit to italoacasas/node that referenced this pull request Feb 25, 2017
errors: add internal/errors.js
214a392
Add the internal/errors.js core mechanism. PR-URL: nodejs#11220 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Joyee Cheung <[email protected]>

italoacasas mentioned this pull request Feb 25, 2017
7.7.0 Proposal #11553
Merged

krydos pushed a commit to krydos/node that referenced this pull request Feb 25, 2017
errors: add internal/errors.js
a1b3bc2
Add the internal/errors.js core mechanism. PR-URL: nodejs#11220 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Joyee Cheung <[email protected]>

srl295 mentioned this pull request Mar 27, 2017
Localization of Node's own output/error messages nodejs/Intl#12
Closed

jasnell mentioned this pull request Mar 28, 2017
errors: add space between error name and code #12099
Closed
3 tasks

gibfahn mentioned this pull request Apr 24, 2017
Potential Semver Minor Backports nodejs/Release#188
Closed
31 tasks

MylesBorins added dont-land-on-v4.x labels Apr 24, 2017

refack mentioned this pull request Jun 26, 2017
meta: semver impact of Error messages #13937
Closed

targos mentioned this pull request Sep 28, 2017
Readonly error.code has ecosystem impact. #15658
Closed

srl295 mentioned this pull request Apr 3, 2018
Key based i18n vs default language i18n nodejs/i18n#50
Closed

joaolucasl mentioned this pull request Aug 23, 2020
Expose the errors module from the internal directory #34348
Closed

Uh oh!

errors: add internal/errors.js#11220

errors: add internal/errors.js #11220

Conversation

jasnell commented Feb 7, 2017

Checklist

Affected core subsystem(s)

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!

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!

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!

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!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jasnell commented Feb 7, 2017

Uh oh!

jasnell commented Feb 7, 2017

Uh oh!

addaleax left a comment

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment