Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Consider un-deprecating non-error status codes #50

Closed
jaydenseric opened this issue Jun 6, 2018 · 5 comments
Closed

Consider un-deprecating non-error status codes #50

jaydenseric opened this issue Jun 6, 2018 · 5 comments

Comments

@jaydenseric
Copy link

Currently throwing errors with non-error status codes works, but causes a deprecation notice to log:

screen shot 2018-06-07 at 12 27 48 am

There are instances where it is perfectly valid to throw an error yet respond with a 200 status. GraphQL servers are required to respond with a 200 status when errors were encountered within resolvers (see graphql/express-graphql#118 (comment)).

You can see this use case in action here: https://github.com/jaydenseric/graphql-api-koa/blob/v0.2.0/src/index.mjs#L182.

@dougwilson
Copy link
Contributor

We talked about it for 2 years and it's already decided to remove the feature, which is why it is deprecated.

@jaydenseric
Copy link
Author

it's already decided to remove the feature, which is why it is deprecated.

What were the reasons for the decision?

I tried to find discussion / actual reasons for the original deprecation, but there doesn't appear to be any: https://github.com/jshttp/http-errors/search?q=deprecate&type=Issues

Your commit to master deprecating it had no reference to any issues or discussion: 858d082

Unlike deprecating, un-deprecating should not be disruptive. The "deprecation" was only advisory anyway.

@dougwilson
Copy link
Contributor

The discussions were on the express and koa gitter channels, I apologize.

The root of the issue is that (1) 200s are not errors and you are misusing the library and (2) it caused endless confusion for koa users because of this. You use case sounds valid for graphql, but thus module is for http semantics not graphql ones. I would suggest use a different lib for better graphql support.

@jaydenseric
Copy link
Author

A user would have to consciously set a 200 status on the error, and so must know what they are doing. I'm not sure why anyone would get confused.

thus module is for http semantics not graphql ones

A GraphQL server sends HTTP responses like any other server. createError is used in 18 places in graphql-api-koa, only one requires a 200 status. It's not appealing to have to migrate to a different lib for this one use, especially when users already have http-errors in their node_modules.

Since the GraphQL use case is solid, and likely to become more common, why not support it? Pretty much the only change necessary is to remove the advisory deprecation notice here.

@dougwilson
Copy link
Contributor

Not going to happen, sorry.

@jshttp jshttp locked as too heated and limited conversation to collaborators Jun 6, 2018
# for free to subscribe to this conversation on GitHub. Already have an account? #.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants