-
Notifications
You must be signed in to change notification settings - Fork 115
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Remove inherits and setprototypeof dependenciess, use real classes #111
base: master
Are you sure you want to change the base?
Conversation
Hey, this is for sure the direction we had planned on going. I think there is also #105 which was started by @jonchurch. I will wait to see what he says, but want to add that until we get a new major out of this (which I am not sure when we plan to), the package supports back to 0.8 and so cannot accept this as is. Edit: the way I phrased that might have not completely expressed my meaning, sorry. I should add here that we are absolutely looking for folks to help, and if you are willing to help us with more than just this to get it to the next major release we would happily have the help. |
@wesleytodd sounds good, was just sending this out to make sure classes could be used with all tests passing. With a major version, I think a lot more things could be simplified too. |
I think what we will need to do for this is start a 3.x branch and re-target to that branch and then also in there start updating the CI (drop everything before current LTS) and what not to prepare for cleaning this stuff up. @jonchurch I know you added yourself as a repo captian, and if you are interested in driving this release let me know. I only added myself originally because I wasn't sure anyone else wanted to take this one over, and will happily let you run this one if that is what you want. Otherwise I can do the above steps, I will await your answer before taking action. |
@wesleytodd and @jonchurch is there any good discussion about the type and extent of changes that would be considered? I got interested here because I considered using this library in a server project of mine, and what I ended up using is something of my own that I think is a lot simpler - essentially just a HttpError extension of Error with the most of fields from this package, then I might do codegen for some error-code specific subclasses. That would be ~63 subclass declarations in source rather than dynamically generating them, with the possibility of keeping |
I am not overly opinionated here, the main thing I care about is the UX improvements to be made by using sub-classable (is that a real word? 😆) internals. Honestly I see this lib as a low change velocity package so once the initial work is done it shouldn't change that much, hence why I am alright not trying overly hard to improve the maintainer experience here. That said, I am not opposed to it either. So if you want to do the work then I would say I am open to the changes you propose. |
@justinfagnani hey there 👋 Good job on the PR! Do you have your code that you mentioned pushed somewhere? I'm interested in something simpler, and I'm tired of reinventing the wheel in each project 😊 Thanks! |
Maybe is worth doing new major of http-errors, since express.js v5 will be soon released ? |
Express 5 has already been released, and we cut out anything that was not a breaking change for express. I am pretty sure this change would be a major here but not in how we use it in express. For example here: https://github.com/expressjs/express/blob/b31910c542c7079d8a763aff346400b6f4c0eaee/lib/response.js#L17 Thus, we can land it any time and do a major here if we like. The team has just be VERY focused on security and the v5 release lately, so sorry for the delay here. This one is on my radar though, and as we get time to start reviewing and preparing release again we will get to it. |
Fixes #110 and #98
This converts the
HttpError
constructor function to a real class and the constructor factories to use subclasses. This removes the need forinherits
.setprototypeof
wasn't needed anymore with that change.Changing to real classes fixes #98 as HttpError is now subclassable. The ban against direct instantiation of
HttpError
is re-implemented with anew.target
check.Also,
Error.captureStackTrace
should no longer be necessary as the Error constructor captures the stack up to the Error subclass's constructor.