Assimilate status code from non-error objects

[jasonkarns]

The createError factory is already fairly versatile. It accepts Error objects and assimilates their name/status/stack. It accepts numbers and strings (taking the appropriate status). And it also accepts objects, assimilating their properties.

However, when given an object that contains status or statusCode, it does not assimilate that value.

My particular use case is for errors that are received from various api libraries, particularly REST APIs and Swagger clients. These objects do not inherit from Error, but the quite frequently have a status property.

If this change is accepted, then the following errorware would be quite versatile indeed.

app.use((err, req, res, next) => next(createError(err)));

It would properly support the following middleware scenarios:

• next(404) -> would become http 404
• next("Some Error") -> would become an http 500
• next(new Error("foo")) -> would become http 500, with useful stack trace
• next({status: 403, foo: "some error object thrown by an api client or library"}) -> would become http 403, assimilating other props

The first three use-cases are already supported by http-errors. The last scenario would make createErrors consistent across all types of input.

I currently accomplish it via: createError(err.status || err.statusCode, err) which handles all the use cases describe above. (Though it relies on the now deprecated usage of number as second argument.)

I can add tests if this change would be considered welcome.

[jasonkarns]

It could be a breaking change, depending...

createError(400, {status: 500}) is currently expected to result in a 400. Specifically, the status property is explicitly ignored when copying properties from the input (line 103). So either:

• the status param should take precedence over the status property from props param or
• introduce a breaking change, wherein status from props param is respected

I've gone ahead and changed the code to adhere to the former option, because that makes more sense as desired behavior to me. However, this differs from current behavior for Error objects. (When an Error instance is provided to the factory function, its status takes precedence over any status param. This seems like a bug to me, and could be trivially fixed in this PR if so desired.)

The latter option would be a breaking change but would be consistent with current behavior for Error objects. (Though, again, I almost consider it a bug.)

_err = new Error()
_err.status = 404
err = createError(500, _err) // err.status == 404

[dougwilson]

It isn't a bug; you can see from the tests it is the current intended behavior.

[jasonkarns]

None of the existing tests ensure the behavior as described. (I "fixed" the behavior without touching the tests, and they're still green.)

[dougwilson]

Ah, I see. I looked at the test I was thinking and you're right.

[jasonkarns]

Opened #38 to determine if the Error behavior is intended or a bug.

[dougwilson]

I'm fine either way, but need to get the Koa folks engaged here, as this module is one of thier top level APIs.

[jasonkarns]

need to get the Koa folks engaged here

for sure. And I'll add some new tests soon.

All the existing tests are green except the node 0.6 job fails when attempting to install/compile node via nvm.

[dougwilson]

Need to set dist to precise as of a month ago. I can push that fix to master and you can rebase your branch. To get it 😄

[jasonkarns]

rebased. tests green! (with added tests)

[dougwilson]

Hi @jasonkarns I'm honestly I just let this drop off the face of GitHub. I'm looking to release a new version of the module and so just trying to clean up issues / PR when I noticed it.

I went ahead and squash + rebased your branch so it was up-to-date.

Reading back through the comments trying to refresh my memory, I guess it looks like part of the reason this has been sitting is because it has a semver-major change included. That's actually fine because I am looking to bump the major version of this module very soon to drop Node.js 0.6 support anyway, so easy enough to go ahead and include this PR with that :+1:

So I guess the only questions / comments I have are

1. With the rebase I noticed all the tests are failing. Looks like it's just a simple logic issue with createError(null) throwing out in this new code since it assumes that object type can have properties (null is an object, but throws on property access). Easy enough I can fix that for you 👍
2. Since this is landing in a semver-major anyway as soon as I hear back, just wanted to circle around on this is the end implementation logic you want, is that right?

[jasonkarns]

Yep still looks right to me.

• status from an Error instance takes precedence over all other inputs
• status param takes next precedence
• otherwise, status or statusCode from props object (assuming you cover the props == null case)

Thanks!

[thewilkybarkid]

It'd be useful to document the supported cases, and update the types to match. (Refs https://github.com/libero/article-store/pull/194.)