Ky: beforeRetry hook should expose failed response

[lkostrowski]

Hi

I was using beforeRetry to handle token refreshing - which I eventually moved to afterResponse.

However I think that hooks, being a kind of "setup" api (lets say I set them once to be used in the background, so I dont have to handle them when I do requests) should expose raw data instead of parsed one.

So, this interface

export type BeforeRetryHook = (
    input: Input,
    options: NormalizedOptions,
    error: Error,
    retryCount: number,
) => void | Promise<void>;

I would change to

export type BeforeRetryHook = (
    input: Input,
    options: NormalizedOptions,
    response: Response,
    retryCount: number,
) => void | Promise<void>;

or to avoid breaking changes

export type BeforeRetryHook = (
    input: Input,
    options: NormalizedOptions,
    error: Error,
    retryCount: number,
    response: Response
) => void | Promise<void>;

This allows me to control if and on what conditions request should be called again, for example my (real life) scenario.

I want to refresh tokens in retry hook. I want them to refresh only if I have status 403 and backend error code (in body) is token_expired. Otherwise I dont want tokens to be refreshed.

So having access to response to failed request will be very helpful. I guess the philosophy is the same (I have errors) but they should not be parsed.

What do you think guys?

[sholladay]

We can consider _adding_ the response to the beforeRetry signature, but we cannot use it to _replace_ error, because we may be retrying a request that failed at the network layer and never received a response. We only know for sure that there was an error.

[lkostrowski]

Sure. But adding response as an extra param will help for sure

[sholladay]

What are your thoughts on my comment here? https://github.com/sindresorhus/ky/pull/180#issuecomment-537754102

If we added response to beforeRetry without changing anything else, it would then have five arguments, which is too many, in my opinion. At that point, I think we should either combine certain arguments or restructure them to be attached as properties of a single object argument.

[lkostrowski]

I would add it as 5th argument (which sucks) and prepare single object param for next breaking change release.

[sholladay]

Our next release is going to be breaking anyway. So no difference, assuming a PR for this is able to land somewhat soon.

[sindresorhus]

I agree a single object parameter makes sense.

[lkostrowski]

I will do this this weekend.