ping @jasnell @jdalton @evanlucas

Is this semver-major?

Also I couldn't locate user-facing documentation explaining that code was immutable, but maybe I didn't look in the right place. Did I miss it? @jasnell

Here's my take on it:

function createClass(Super) {
  return class NodeError extends Super {
    constructor(key, ...args) {
      super(getMessage(key, args))
      this[codeSym] = key
    }

    get code() {
      return this[codeSym]
    }

    set code(value) {
      setProperty(this, "code", { value })
    }

    get name() {
      return super.name + " [" + this[codeSym] + "]"
    }

    set name(value) {
      setProperty(this, "name", { value })
    }
  }
}

The setProperty helper codifies to:

Object.defineProperty(this, "code", {
  configurable: true,
  enumerable: true,
  value,
  writable: true
})

This way they're still getters until they are overwritten.

This way they're still getters until they are overwritten.

Naïve question perhaps, but what's the benefit of keeping it as a getter?

Naïve question perhaps, but what's the benefit of keeping it as a getter?

Keeps the existing dynamic behavior. I don't have the background on why the getter was used in the first place though.

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

Shouldn't name be made mutable too?

Shouldn't name be made mutable too?

I suppose so. And probably add a test to confirm that when name and message have been modified, the results of e.toString() and console.log(e) are analogous to modifying those properties on an Error object and calling e.toString() and console.log(e) in the browser (assuming the console.log() behavior is reasonably consistent across browsers). Specifically, on Chrome at least, I see that toString() uses the new over-ridden values, but console.log() uses the original values.

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

it looks like we can get rid of the symbol now

@jdalton are you suggesting to make both .name and .code overridable? and in that case it will be possible to completely hide the preassigned code.

makeNodeErr((e) => {
  e.name = 'g';
  e.code = 'f';
  e.message = 'h';
  e.toString() === 'g: h';
});

How about keeping e[kCode] in some way part of the toString() output?
Or add a .nodeCode() accessor?

it looks like we can get rid of the symbol now

As mentioned in #15694 (comment), pristine NodeErrors should have .name and .code as prototype getters so that instances of the class will only have .stack as an own property. In that case assigning the code to a symbol field still makes sense.

@refack

are you suggesting to make both .name and .code overridable? and in that case it will be possible to completely hide the preassigned code.

Yep. Error objects are created with certain info then thrown to user-land where folks can do what they want with them.

How about keeping e[kCode] in some way part of the toString() output?
Or add a .nodeCode() accessor?

I'm not a fan of obstructing user-land here. My take is that Node creates an error that walks like an error and talks like an error (no own-enumerable properties, standard toString behavior, etc.) and then throws it out. After that it's no longer Node's concern. Folks can catch it, augment it, decorate it, or shallow it. The tweaks to makeNodeError are minimal and the kCode symbol will still exist on the error object so folks can restore the original getters/behavior of a modified error if desired.

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

@jdalton I have to make one significant change in addition to a few unimportant linting changes.

The significant change is to define the name property with enumerable: false to be consistent with standard ES Error objects. The test fails otherwise (which is why CI is red red red). I'm assuming that I should do that, but just in case that's totally wrong in your view or anyone else's, I'm mentioning it here...

I'm leaving code as enumerable.

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

@Trott

The significant change is to define the name property with enumerable: false to be consistent with standard ES Error objects.

That's handled by the getter being defined in the class definition. It's non-enumerable by default.
When you set an own property however it will become enumerable:

var e = new Error("x")
console.log(e.name) // "Error"
console.log(Object.keys(e)) // []
e.name = "foo"
console.log(Object.keys(e)) // ["name"]

+1 to make the name property enumerable after the setter is used.

@Trott As mentioned in my previous comment, making the name property non-enumerable when assigned, myError.name = "foo", is the wrong behavior (doesn't match built-in). If you need to restore the original getter for unit tests you can simple delete the own property:

console.log(myError.name); // "foo"
delete myError.name
console.log(myError.name) // "Error [ERR_TLS_HANDSHAKE_TIMEOUT]"

That removes the own property and goes back to the prototypal property.
For reference I've done this in my own tests.

I'm not a fan of obstructing user-land here. My take is that Node creates an error that walks like an error and talks like an error (no own-enumerable properties, standard toString behavior, etc.) and then throws it

My only concern is 3rd party unintentional clobbering. I'd rather we make it a tiny bit hard for libraries to completely clobber the new behaviour. So can we think of a way to get to the code even if .code and .name are overridden?

It kinda looks like you're in a bad testing state somehow @Trott. Could the test be running with showHidden turned on by accident? That would explain why you're seeing braces for untouched errors which have no enumerable properties but have the kCode as a non-enumerable property.

I'm not seeing that with this PR.

@jdalton I'm not seeing it anymore either even after I roll back the last commit I just added. I must have gotten confused about what binary corresponded to what code changes and been running a binary against an earlier version of this PR. Sorry about the unproductive detour.

@jdalton I'll split out the console.log()/util.inspect() changes into a separate PR so this one can land without an extended debate as to whether such things are breaking changes or not.

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

CI looks good. (OS X failure is infra issues and unrelated.)

Landed in 6e172be and 42a2a9b . Thanks, @jdalton! 🎉

This is not landing cleanly on 8.x, could you please send a backport.

Added the dont-land labels for v6.x and v4.x because the new error system does not exist in those release lines. Will backport for v8.x.

@MylesBorins Backport in #16078