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

Update readme.md to clarify HTTPError #638

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

gnowland
Copy link

Clarifies that some errors (especially network errors) will not be an instance of HTTPError and not contain a response. Language copied from hooks.beforeRetry.

I believe it is important to reiterate this at the HTTPError level, as it is not immediately intuitive that all errors wouldn't be an instance of HTTPError.

Whether all errors should be an instance of HTTPError is up to debate (and I gather is actively under consideration in a number of issues/discussions), and is outside the scope of this PR.

Clarifies that some errors (especially network errors) will not be an instance of HTTPError and not contain a response. Language copied from `hooks.beforeRetry`.

I believe it is important to reiterate this at the HTTPError level, as it is not immediately intuitive that all errors wouldn't be an instance of HTTPError.

Whether all errors *should* be an instance of HTTPError is up to debate (and I gather is actively under consideration in a number of issues/discussions), and is outside the scope of this PR.
@sholladay
Copy link
Collaborator

sholladay commented Sep 25, 2024

Language copied from hooks.beforeRetry.

It would be better to tailor the language to the context of this section and keep it focused on what HTTPError is, as opposed to what it is not. Something like "An HTTPError is thrown when a response with a non-2XX status code is received."

Whether all errors should be an instance of HTTPError is up for debate

We are going to provide a NetworkError class in the future but it has no bearing on HTTPError. Neither does TimeoutError or AbortError or TypeError. You can handle all of them the same way or distinctly as you see fit, but they have basically nothing in common except for being associated with a request.

@gnowland
Copy link
Author

gnowland commented Sep 30, 2024

Thanks so much for your review and comments @sholladay !

It would be better to tailor the language to the context of this section and keep it focused on what HTTPError is, as opposed to what it is not. Something like "An HTTPError is thrown when a response with a non-2XX status code is received."

IMHO it's actually more clear to as it stands. I'm a seasoned old hat so I understand what you mean, but I don't expect many developers utilizing this library would inherently know that, say, an offline server whose "response" is an ERR_CONNECTION_REFUSED wouldn't be accompanied with "a non-2xx status code" (like a 503) off the top of their heads.

@sholladay
Copy link
Collaborator

sholladay commented Oct 1, 2024

You're welcome! The same potential confusion applies to fetch, don't you think? Network errors like that don't have headers or a status code and don't result in a Response object being returned. They reject the fetch promise instead of resolving it successfully. It's such a different scenario and if someone doesn't know that, then I think overloading them with information is not useful. We can have a more general section about errors to explain some of this, but I don't think the HTTPError section should dive too deep into other error types. I'm okay with something simple, though, like "does not include network errors".

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 this pull request may close these issues.

2 participants