Consider replacing error factories with real constructors

[pluma]

Currently the "constructors" are actually just factory functions and we have a total of three such factories in the code that do pretty much the same thing (ClientError, ServerError and the exported function).

The only reason ServerError and ClientError have any inheritance at all is that the __proto__ property of the new error is manipulated directly. This is incompatible with some possible environments (the double underscores are there for a reason).

It would be nice if the two constructors were actual constructors (i.e. return this, not a retrofitted error). For an example of how this would work see [email protected].

The obvious drawback is that Object.prototype.toString returns [object Object] instead of [object Error] and util.isError returns false (though instanceof checks pass as one would expect). The benefit is that this approach can be made to work in environments that don't support overriding __proto__.

[dougwilson]

The obvious drawback is that Object.prototype.toString returns [object Object] instead of [object Error] and util.isError returns false (though instanceof checks pass as one would expect).

Breaking this is not acceptable, so we'd only consider solutions that do not break that functionality, but I'm all-ears on solutions as long as Object.prototype.toString(err) === '[object Error]'.

[pluma]

I have no idea how Object.prototype.toString decides this, actually, and can't find anything on MDN either. If you have any idea, shoot :)

[dougwilson]

Object.prototype.toString gets the name from the internal [[Class]] property of an object (specified in ECMA-262. There is no way to alter it, which is why you can't actually inherit correctly from Error objects in JS.

[pluma]

So it's basically a question of being compatible with util.isError/Object.prototype.toString vs being compatible with environments that don't support __proto__ assignment. Not great, but such is life.

Unless someone has a brilliant idea, feel free to close this issue then.

[dougwilson]

You can support both with (this is just a simplified example):

function NotFoundError(message) {
  var err = new Error(message)
  err.name = 'NotFoundError'
  return err
}

var error = new NotFoundError('oops!')

[pluma]

Nope, that wouldn't pass instanceof checks. You need to manipulate the [[Prototype]] for that and if you don't use a real constructor you can only do that via __proto__.

Also, new is redundant in that case. It just creates a new instance of NotFoundError and immediately discards it because the "constructor" returns a value.

[dougwilson]

Gotcha. I mean, Error.captureStackTrace is just as unofficial as __proto__, so getting rid of one under that premise means getting rid of both. If Object.setPrototypeOf was adopted faster, then that would be a __proto__ alternative, but alas it's not.

[dougwilson]

P.S. I hoped on IRC since you seem to be active :)

[pluma]

I'm on #jshttp. We could use gitter if you want to keep public logs, btw. It also supports Markdown unlike IRC ;)

[dougwilson]

Wow, this has been open for a long time :) So as you may have seen, I just closed it with a commit. The commit doesn't exactly correlate with the title of the issue, but I feel like it correlates a bit with the ideas floating around to the underlying issue, which was using __proto__ in order to get a proper [[Class]] on the object.

The commit replaces using __proto__ with the setprototypeof module, which simply uses the standardized Object.setProtototypeOf function, falls back to using __proto__ if possible, otherwise will just copy the properties as a last resort. I think this should cover the compatibility bases.

[pluma]

Thanks @dougwilson. I appreciate the effort you're putting into these projects. I think the change is in spirit with this issue.

[dougwilson]

No problem :) I'm trying to match through here and pick up all these things that just kind of fell off, haha. Hopefully everything will start to get closed now. If you do have any new or additional comments, always feel free to open an issue!

[cyberwombat]

I noticed the docs don't mention using the named error constructors directly (i.e. new NotFound()). Is there a reason why? also since they are factories it seems that throw NotFound would be the call but ESLint balks at the capitalization. Is the future going to using these as legit constructors or heading towards a camelCase factory var?

[dougwilson]

The docs for using the constructor is here: https://github.com/jshttp/http-errors/blob/master/README.md#new-createerrorcode--namemsg

[dougwilson]

Please open a new issue if you're having issues or need help instead of a 2 year closed issue.