Skip to content
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

status 201 should not throw error by default #128

Open
hst-m opened this issue Nov 29, 2020 · 8 comments
Open

status 201 should not throw error by default #128

hst-m opened this issue Nov 29, 2020 · 8 comments

Comments

@hst-m
Copy link

hst-m commented Nov 29, 2020

201 status code:

The request has succeeded and a new resource has been created as a result. This is typically the response sent after POST requests, or some PUT requests.

This is a successful status response and is commonly used for post request responses, it should not throw error by default

@bencrox
Copy link

bencrox commented Dec 10, 2020

I face the same issue. Though this is easy to handle, just add 201 to statusCodes.

But I rather expect to see 201 or other codes to be returned in the response/resultant object.
Predefining acceptable codes would lead to obscured error handling.

@hst-m
Copy link
Author

hst-m commented Dec 10, 2020

I expect everyone will have this issue, 201 is a successful response and endpoints may return 201 or 200 and it could change at any time by whoever runs the endpoint, better to default to 201 allowed because everyone will need to add it in

@bencrox
Copy link

bencrox commented Dec 12, 2020

I just not sure letting other results to be caught is a good idea. It works, and there exist work arounds. But is it elegant?

202 : some IOT , rest DELETE may return this.
301, 304, 307, 308 : also common to see, even among API.

These are not errors. I am used to think 4XX / 5XX as error.

I can understand 3XX usually lead to extra operations. But I think 202, or in general 2XX, shall be accepted by default

@ShawTim
Copy link

ShawTim commented Dec 12, 2020

It should honor the definition of HTTP status code.
Which is, 2XX are success, 3XX are redirect, 4XX are client error, 5XX are server error.
It should throw error for 4XX/5XX only if it needs to.

@aledpardo
Copy link

This is how the lib was designed. You should pass to bent all the statuses the server might respond:

const client = bent(200,201,202,203,204,301,302...);

This not an issue, but rather a design decision.

@AlexSpelt
Copy link

Adding a better understandable error is also better for the dev experience. Yesterday I was stuck on.

StatusError: Created
    at ClientRequest.<anonymous> (/home/alex/Documents/DHLParcelModule/node_modules/bent/src/nodejs.js:133:23)
    at Object.onceWrapper (events.js:422:26)
    at ClientRequest.emit (events.js:315:20)
    at ClientRequest.EventEmitter.emit (domain.js:482:12)
    at HTTPParser.parserOnIncomingClient [as onIncoming] (_http_client.js:596:27)
    at HTTPParser.parserOnHeadersComplete (_http_common.js:119:17)
    at TLSSocket.socketOnData (_http_client.js:469:22)
    at TLSSocket.emit (events.js:315:20)
    at TLSSocket.EventEmitter.emit (domain.js:482:12)
    at addChunk (_stream_readable.js:295:12) 
{
  statusCode: 201,
  json: [AsyncFunction],
  text: [Function],
  arrayBuffer: [Function],
  headers: {
    date: 'Tue, 20 Jul 2021 13:55:42 GMT',
    'content-type': 'application/json',
    'content-length': '251',
    connection: 'close',
    vary: 'Origin',
    'x-content-type-options': 'nosniff',
    'cf-cache-status': 'DYNAMIC',
    'expect-ct': 'max-age=604800, report-uri="https://report-uri.cloudflare.com/cdn-cgi/beacon/expect-ct"',
    'strict-transport-security': 'max-age=15552000; includeSubDomains; preload',
    server: 'cloudflare',
    'cf-ray': '671cb04eea992056-AMS'
  }
}

You have an error!
Here is the successful request!

Really confusing in my opinion (at least for me). I'm not against throwing an error. But an error like below should help devs understand why they're getting an error. Maybe just only output this error if there is a 2XX HTTP code.

The request returned status code 201. Status code 201 is not allowed for this request.

@aledpardo
Copy link

@AlexSpelt that's a good point.

I liked the first part of your proposed error message. What do you think of rephrasing the 2nd part a bit, like:

... 201 status code was not set as an expected response status.

@AlexSpelt
Copy link

That is even better indeed. Except for the "201 status code" part instead of "status code 201".

But that might be the language barrier for me. English is not my native language.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants