search

ethanent / phin

Node HTTP client
MIT License
576 stars 33 forks source link

Strict mode - Throw error if response isn't 2xx #75

Open XuluWarrior opened 3 years ago

ethanent commented 3 years ago

EDIT: I looked more closely at how you handled the strict option. This looks like a reasonable solution to me. It should satisfy the complaints about attempting to parse JSON for non-200 responses while still providing expected behavior for those who don't have the same requirement.

jdforsythe commented 3 years ago

@ethanent @XuluWarrior I agree this would be a good solution. The only comment I'd have is that the user may want to be able to inspect the response, which would get lost with this solution. Adding a class that extends Error and attaches the response to it would make the response available if the user wants to inspect it.

It might even be helpful to have separate ClientError for 4xx and ServerError for 5xx. I'm not sure if you'd want to lump 1xx and 3xx into a single "other" error or separate InformationalResponseError and RedirectResponseError. This would allow instanceof checks for easier error handling - for instance the user may want to retry on a ServerError but a ClientError indicates retrying would be futile.

jumoog commented 3 years ago

I like the idea. And agree with @jdforsythe on separate ClientError for 4xx and ServerError for 5xx.

XuluWarrior commented 2 years ago

I kind of put this PR down after my initial attempt worked well enough for me but I think it is still worth finishing.

I agree that extending Error with a NetworkError/ConnectionError? type would be useful. Although, I'm not so sure about the benefit of having separate ClientError and ServerError.

There are client errors where simply retrying later has a chance of success (e.g. 425 "To Early" and 429 "To Many Requests"). And there are server errors which require client action (e.g. 505 "HTTP Version Not Supported" and 511 "Network Authentication Required") As a developer, I think I would be more interested in the exact code than the category of response.

XuluWarrior commented 10 months ago

I'd forgotten that I'd created this PR but I still believe that "strict mode" really helps streamline code that uses phin. (I'm happily using my fork for this reason)

Any thoughts as whether this can be merged in some form? I'm happy to put the additional effort to address review comments.