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

fix: provide statusText as error message on retryable network errors #832

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

Conversation

wattroll
Copy link

What kind of change does this PR introduce?

This PR fixes a minor, but annoying lack of error message and adds a test case for an uncovered branch.

What is the current behavior?

Network errors (502,503,504) result into AuthRetryableFetchError (correct), with "{}" as an Error#message (embarassing if displayed to the end user).

Screenshot of an error in the console

The bug is caused by erroneous usage of _getErrorMessage(error) helper in a branch where looksLikeFetchResponse(error) assertion have succeeded and error.status is 502 | 503 | 504. I believe _getErrorMessage was intended to be called on something that has been thrown or returned as an error, not on a resolved fetch response that was not ok.

The dreaded "{}" comes from the final JSON.stringify fallback of _getErrorMessage.

What is the new behavior?

I went for the simplest solution of providing Response.statusText property as an error message. Status text is the only human-readable piece of information that is reliably/readily available in this context. It is also reasonable to expect it to be clear and accurate.

Additional context

I briefly considered pulling in the Response.text() and including it in the message, but decided against it. Proxy errors rarely include any extra info there, but often include HTML formatting and styling which wouldn't be appropriate here. Also waiting for Response.text() might fail with another error.

This fix should be backwards compatible on a patch level. The Error constructor coerces the message to a String (or leaves out an empty string in case of undefined) therefore even in case of custom fetch implementations/middlewares that don't provide statusText our AuthRetryableFetchError will still have a message field of a string type.

Previously responses that indicate retryable network errors (e.g. proxy errors)
resulted into `AuthRetryableFetchError` with dreaded `"{}"` as an `Error#message`.

This commit provides `Response.statusText` as an error message in those cases.
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.

None yet

1 participant