Allow proper prototype inheritance and subclassing

[jonchurch]

closes #98

The Issue

Currently subclasses of error classes like NotFound do not correctly set up the prototype chain, leading to unexpected behavior with instanceof checks. Also, extending HttpError is not possible due to a check that prevents instantiating the abstract class. Additionally,

Extending error classes like NotFound do not wire up the prototype properly to be able to check insanceof on it.

const createErrors = require('http-errors')

class ValidationError extends createErrors.BadRequest {}

assert(err instanceof ValidationError) // false

Extending HttpError is not possible currently because of the throw to prevent instantiating the abstract class.

const createErrors = require('http-errors')

class NewBase extends createErrors.HttpError // extending HttpError will throw a type error here
// TypeError: cannot construct abstract class

The Change

This PR uses Reflect.construct when creating errors in the factory functions to properly set prototype to the target when used with new, or to the factory function itself when called without new. These changes ensure that subclasses of the named HTTP error constructors, such as NotFound, correctly inherit properties and behaviors while maintaining the expected prototype chain.

Key Changes:

• Reflect.construct within ClientError and ServerError constructors to dynamically set the prototype based on new.target, supporting correct inheritance for subclasses.
• Implemented a check in HttpError to prevent direct instantiation but allow subclassing, maintaining its intended use as an abstract class and throw behavior when instantiated.

Developers can now extend built-in HTTP error constuctors more intuitively, ensuring instances of custom errors are recognized as instances of their specific class as well as their HTTP error ancestors:

const createErrors = require('http-errors')

class CustomError extends createErrors.NotFound {}

const err = new CustomError()

console.log(err instanceof CustomError) // true
console.log(err instanceof createErrors.NotFound) // true
console.log(err instanceof createErrors.HttpError) // true

HttpError instantiation change:

// allows subclassing abstract HttpError
// extending this exported constructor would previously throw w/ TypeError: cannot construct abstract class
class CustomBaseHttpError extends createErrors.HttpError {}

const custom = new CustomBaseHttpError()

// direct instantion w/ new is still prevented and will throw
new createErrors.HttpError() // throws

// as is direct instantion via the factor functions
createErrors.HttpError()

Resources

Some context I found about the intent of the named methods #29 (comment)

[jonchurch]

this will fail below Node 6 bc of use of Refelct and new.target

I think new.target went stable in Node 6.5

[jonchurch]

needs some kind of docs nod to the new capabilities? It changes undocumented features, but can at least update the docs to show an example of subclassing.

Needs an update to History as well

[wesleytodd]

I think there is a bunch of good stuff here, but maybe a few many changes at once to get the job done?

Also, I am not sure, but other times I have seen Reflect in use it has been called out as being really slow. This is hot path code, so I think we should be really careful about benchmarking if we introduce an api known to cause perf issues. Can you maybe comment a bit about what why that was required to enable sub classing? I need to brush up on this package (which I dont have time for this morning), but nothing about subclassing inherently requires usage of Reflect so I am a bit suspicious of this change.

[jonchurch]

There's no rush to merge this. It can't right now anyways bc it would be a semver major Node.js version change in it's current form. I am actively experimeting with a variety of approaches which don't require Reflect, and so are back-compat w/o node version bump.

In the original version of the code, we set the prototype of the instance explicitly to ClientError.prototype using setPrototypeOf. This manual setting of the prototype was done for every instance created by the constructor, regardless of whether it was a direct instantiation of ClientError or an instance of a subclass.

The issue is repro'd by:

const createError = require('http-errors')

class CustomError extends createError.NotFound {}
const err = new CustomError()

console.log(err instanceof CustomError) // false
logProtoChain(err) // NotFound.HttpError.Error

We have Factory Functions, not classes, so the proto story is different from what folks expect when using classes.

I'll endeavor to explain why Reflect is needed here, this is not complicated JS but it is high cognitive load bc the issue this aims to fix inherently crosses that pre-es6 divide and you have to hold old an new patterns in your head. Currently we dynamically create Factory functions, so its a little unique even for factory functions.

[wesleytodd]

As this is a major, did you explore leaning in and just using sub classing instead of factory functions? Not sure I immediately have enough context to know if this is a good idea, but seems worth considering to me instead of working hard to fix.

Another thing to look at instead of Reflect would be to see if we wanted this in the current major to do it with older style patterns. The setprototypeof package is just a wrapper to support __proto and Object.setPrototypeOf and either way I think a major should drop it along with the node versions it supports, but we might consider still just conditionally doing the prototype manipulation to make it a minor instead of a major.

[jonchurch]

Converted to a draft as it should've been one to begin with, I don't intend to land this to master currently.

It requires a major for Node version requirements of the current PR, and not knowing what the next major would look like it's hard to tell if this would even be needed. Keeping it open though because I think it is worth living as a draft right now to help with understanding of the current package's internals w/r/t object creation

@wesleytodd yes I explored that but it is an API breaking change to drop the createErrors.NotFound() and I first wanted to understand exactly why subclassing wasn't working. maybe a live chat is best to discuss this, or a write up from me. tldr is the package's impementation combines factory function and constructor function concepts in such a way that neither behave how a JS dev would expect, this PR addresses that by allowing you to subclass the quasi-factory-constructor functions like NotFound while maintaing the existing API of the package

[wesleytodd]

Cool, lets try and get this on the list for things to address with the next major. I absolutely feel like this is a key feature to land. Do we think this should land before v5 of express since this is a direct dep? Feels like it to me but I don't want to add too much without it being important.

Also, nudge nudge, you should consider signing up to be a captain for this project.

[jonchurch]

prior discussion about the way object creation is handled #11