Allow proper prototype inheritance and subclassing #105

jonchurch · 2024-03-09T04:11:55Z

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 · 2024-03-09T04:18:38Z

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

I think new.target went stable in Node 6.5

this change allows us to throw for createErrors.HttpError() and new createErrors.HttpError(), but not on class Foo extends createErrors.HttpError {}

jonchurch · 2024-03-09T04:32:43Z

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 · 2024-03-11T15:17:17Z

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 · 2024-03-11T17:04:46Z

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 · 2024-03-11T17:44:31Z

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 · 2024-03-15T06:11:58Z

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 · 2024-03-16T14:11:17Z

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 · 2024-03-17T07:24:28Z

prior discussion about the way object creation is handled #11

jonchurch added 2 commits March 8, 2024 22:20

adjust prototype chain of base classes, check new.target of HttpError

accaf39

add tests for subclasses and inherittance of custom errors

565e1a0

jonchurch changed the title ~~Jonchurch/prototype~~ Allow proper prototype inheritance and subclassing Mar 9, 2024

jonchurch added 2 commits March 8, 2024 23:20

make HttpError throw on direct instantiaton

007cafe

this change allows us to throw for createErrors.HttpError() and new createErrors.HttpError(), but not on class Foo extends createErrors.HttpError {}

add tests for direct instantiation

7ebb33f

jonchurch added the enhancement label Mar 9, 2024

jonchurch added 5 commits March 8, 2024 23:33

change comment wording

ee8fc92

add inheritance test suite

d9a716b

remove conditional proto set

76977e9

remove duped test

b45f34e

removed unused dep setprototypeof

5a4efd9

jonchurch force-pushed the jonchurch/prototype branch from e01a3eb to 5a4efd9 Compare March 10, 2024 01:38

jonchurch marked this pull request as draft March 15, 2024 05:22

wesleytodd mentioned this pull request Jul 22, 2024

Remove inherits and setprototypeof dependenciess, use real classes #111

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow proper prototype inheritance and subclassing #105

Allow proper prototype inheritance and subclassing #105

Allow proper prototype inheritance and subclassing #105

Are you sure you want to change the base?

Allow proper prototype inheritance and subclassing #105

Conversation

The Issue

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

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

The Change

Key Changes:

Resources

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

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