s/adding/added/

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.

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

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

                                                        improvement confidence   p.value
 es/restparams-bench.js millions=100 method="arguments"     -0.17 %            0.9399783
 es/restparams-bench.js millions=100 method="copy"           0.02 %            0.9923623
 es/restparams-bench.js millions=100 method="rest"           4.06 %            0.1729710

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.

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.

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

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

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

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:

const makeNodeError = (Base) => class NodeError extends Base {
  [… current code, except only once …]
  static get name() { return `Node${super.name}`; } /* optional */
};

const NodeError = makeNodeError(Error);
const NodeRangeError = makeNodeError(RangeError);
const NodeTypeError = makeNodeError(TypeError);

Ah right :-) .. that works too

Missing quote ;)

grr.. lol ...

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

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

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

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.

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

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:

set code(options) {
  if (typeof options === '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