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 handling of network errors for beforeRetry hook #276

Merged
merged 4 commits into from
Sep 28, 2020

Conversation

sholladay
Copy link
Collaborator

@sholladay sholladay commented Jul 24, 2020

Fixes #251
Fixes #275

The PR fixes some problems with hooks caused by how we handle responses:

  • Most importantly, it ensures that beforeRetry hooks get called as expected in case of a network error where no response was received. Previously, if a network error occurred, Ky would attempt to clone a non-existent response, causing an exception immediately before the beforeRetry hook, meaning it wouldn't be called. The simplest fix, which I've implemented here, is to just remove the response parameter (which was merely a shortcut) and make people use error.response instead, which IMO is actually a bit clearer anyway, in that it is obvious the response may be undefined. There are some other possible ways to fix this... we could make it a getter, or use error.response && error.response.clone(). Of those two alternatives, I'd prefer a getter, in order to maintain a single source of truth for what the response is.
  • Additionally, it ensures that the response received by afterResponse hooks and the error.response from HTTP errors has the custom parseJson function applied to it. I've implemented a _decorateResponse() method to achieve this, which can be used to apply the custom parser anywhere a response is cloned.

@sholladay sholladay changed the title Single source of truth for response in beforeRetry hooks Fix handling of network errors for beforeRetry hook Jul 28, 2020
@sindresorhus
Copy link
Owner

sindresorhus commented Aug 21, 2020

Can you update https://github.com/sindresorhus/ky/blob/master/index.d.ts too? Docs-wise.

@sindresorhus sindresorhus changed the title Fix handling of network errors for beforeRetry hook Fix handling of network errors for beforeRetry hook Aug 21, 2020
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.

parseJson() is not called in AfterResponseHook TypeError o.response is undefined
2 participants