Skip to content

support "err instanceof http-errors" for all individual errors - #22

Closed
krmannix wants to merge 1 commit into
jshttp:masterfrom
VerbalCare:master
Closed

support "err instanceof http-errors" for all individual errors#22
krmannix wants to merge 1 commit into
jshttp:masterfrom
VerbalCare:master

Conversation

@krmannix

Copy link
Copy Markdown
Contributor

Allows for a broader instanceof check. Rather than needing to check if an error is a specific status code type, one can check for a generic HttpError as well.

var HttpError = require('http-errors');
var notFoundError = HttpError(404);
if (notFoundError instanceof HttpError) {
// true
}
if (notFoundError instanceof HttpError['404']) {
// true
}
if (notFoundError.name === 'NotFoundError') {
// true
}

@krmannix

Copy link
Copy Markdown
ContributorAuthor

As mentioned in #21, builds fail for Node.js 0.6 and 0.8 because of dependencies, not due to the PR

@krmannix

Copy link
Copy Markdown
ContributorAuthor

tests now pass for this pr, and rebased with the most current master branch

@dougwilson

Copy link
Copy Markdown
Contributor

Thanks a lot! I'm currently mulling over this right now. I am 100% for adding another object into the prototype such that they all inherit from it, i.e. StatusError -> HttpError -> Error. The only thing I'm debating with myself right now is if the actual factory function should be that HttpError class or not, particularly because it will have different semantics than using the other current constructors.

Then, after that, I'm thinking, if the HttpError class is not the factory function, then what should new HttpError() (or new HttpError(message)) actually result in? Should it's signature be new HttpError(code, message)? Perhaps we should say that HttpError is more like an abstract class, and thus just not allow new HttpError(...) at all.

My current thoughts are leaning towards the following:

  1. Add a new export, HttpError that all error classes here inherit from. This would thus look like require('http-errors').HttpError.
  2. Throw a TypeError when trying to construct that directly, to make it behave like an abstract class.

What are your thoughts? If this sounds good or any other thoughts, let me know. I can easily alter your own pull request to reflect this if you are not around to change it, or just don't fell like it because this sounds like a good enough change.

@krmannix

Copy link
Copy Markdown
ContributorAuthor

My thought would be that the HttpError (or whatever the overall parent object is) eventually should be the factory function, but I think the best change would be what you suggested - it's the least disruptive and fits with the current style of using this module while still allowing for the functionality suggested in this PR.

Please feel free to change it, and thanks!

@dougwilson

Copy link
Copy Markdown
Contributor

Nice! And, so here's something that may interest you: so, because this module is depending on too large of a version range of statuses, and uses that module to define it's interface, I plan to release a new version version of this module to fix that situation. This means that now is a good time to bring up PRs/discussions around breaking changes :)!

@krmannix

Copy link
Copy Markdown
ContributorAuthor

👍 awesome! I'd be happy to put some thought in and see if I can help out. where will you be listing your proposed changes?

@dougwilson

Copy link
Copy Markdown
Contributor

where will you be listing your proposed changes?

Feel free to make any issues/PRs you like :) When I get rolling, it'll be in a release PR, like what I've done with Express and other modules (example: expressjs/body-parser#66). This would track the merged things and provide an overview of the progress and goals at the top.

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

@krmannix@dougwilson