Errors thrown by hooks should be treated as fatal

[gutenye]

The API returns { status: 'error', message: 'foo' } for errors, and I want to handle it in afterResponse hook, here's the code.

const api = ky.create({
  prefixUrl: 'http://localhost',
  hooks: {
    afterResponse: [
      async res => {
        const body = await res.json()
        if (body.status === 'error') {
          throw new Error(body.message)
        }
      },
    ]
  }
})
const users = await api.get('users').json()

The problem is it sends two requests to the server.

GET http://localhost/users
GET http://localhost/users

Expect it only send one request to the server. (Because of retry)

[whitecrownclown]

@gutenye That's because ky has a default retry value.

codesandbox

You could do

const users = api.get('users', {retry: 0}).json();

[sholladay]

Yep, this is behaving as expected. By throwing an error, you have told Ky that the request failed, so it tries again. I concur with @whitecrownclown's suggestion to disable retries, if that is what you desire.

[gutenye]

Retry failed requests made with one of the below methods that result in a network error or one of the below status codes.

Methods: GET PUT HEAD DELETE OPTIONS TRACE
Status codes: 408 413 429 500 502 503 504

The doc said it retries in a network error (aka ky.HTTPError), not custom error. And I want a central place to handle the API error (not network error) while keeping the retry feature, is there a way to do it?

So now I have to do this on every Api call:

const body = await api.get(path, { retry: 3 }).json()
if (body.status === 'error') {
  if (body.error_code === 'invalid') {
    throw new InvalidError(body.error_message)
  } else if (body.error_code === 'limit_exceed') {
    throw new LimitExceedError(body.error_message)
  } else {
    throw new ApiError(body.error_message)
  }
}

Or I can use a wrapper but make things complicated. If possible, I'd like to put them in hooks.

[sholladay]

Well, I would argue that there are different categories of errors going on here, and thus they need to be handled differently. There are fatal errors, like invalid syntax, and then there are operational errors, like a rate limit being reached. The latter should be retried. It would be significantly easier if the API just responded with the appropriate HTTP status codes, so that Ky could handle these errors automatically. However, you can simulate that behavior by doing return new Response() in the hook and mapping the API's fatal errors to the appropriate status code. That will tell Ky not to retry, as instructed by the HTTP specification.

Or you could simply throw operational errors in the hook and leave the rest to handle after the ky.get() call.

[gutenye]

For anyone comes into this problem, I use throw new ky.HTTPError({ statusText: body.error_message }) in the hooks with retry enabled. Thanks for the helps.

[sindresorhus]

I don't think throwing a normal error should retry the request. That's not the expected behavior for me either.

We should clearly document the behavior either way though.

[sholladay]

I may start work on this issue later this week. Seems like the way forward is to only retry ky.HTTPErrors.

The main problem I have with this is that Ky will no longer retry network errors like when the Wi-Fi is struggling, which I specifically want it to do. I haven't spent much time looking at the errors that fetch can throw, but my guess is that retrying network errors while ignoring other kinds of errors is going to require some ugly error.messsge detection.

If anyone has ideas, please chime in.

[sindresorhus]

@sholladay You should definitely comment in whatwg/fetch#526.

[spruce-bruce]

@sholladay I filed #185 which is related to this issue somewhat.

I've done a lot of work figuring out how fetch fails and posted about it here https://medium.com/to-err-is-aaron/detect-network-failures-when-using-fetch-40a53d56e36

[spruce-bruce]

Also check out what redux-offline is doing for this: https://github.com/redux-offline/redux-offline/blob/develop/docs/api/config.md#discard

They provide a hook (similar to your suggestion here #185 (comment)) that lets users take control of all of the retry logic if they need to. This works a little differently for them than it would for you because redux-offline also provides a hook for executing the network request in the first place, so I as a user get to decide what errors are thrown by my fetch logic. I'm not sure what this means for ky exactly, but at the very least you must admit that there are instances where a user will want to retry a request (even with a status code of 200) that ky couldn't possibly know about and perform automatically. I think a perfectly reasonable answer to this problem is "sophisticated retries are outside of the scope of this package"

edit:
And to answer a question specifically Network request failed is the only error i've ever seen fetch throw. It throws this when the request fails due to an unreliable network, but also for other reasons that shouldn't be retried (like an impossible domain name in the path).

[mararrdeveloper]

Retry failed requests made with one of the below methods that result in a network error or one of the below status codes.

Methods: GET PUT HEAD DELETE OPTIONS TRACE
Status codes: 408 413 429 500 502 503 504

The doc said it retries in a network error (aka ky.HTTPError), not custom error. And I want a central place to handle the API error (not network error) while keeping the retry feature, is there a way to do it?

So now I have to do this on every Api call:

const body = await api.get(path, { retry: 3 }).json()
if (body.status === 'error') {
  if (body.error_code === 'invalid') {
    throw new InvalidError(body.error_message)
  } else if (body.error_code === 'limit_exceed') {
    throw new LimitExceedError(body.error_message)
  } else {
    throw new ApiError(body.error_message)
  }
}

Or I can use a wrapper but make things complicated. If possible, I'd like to put them in hooks.

Wouldn't be easier having an onError hook that gets fired once all the retries have already accomplished? A hook though to transform errors in something else.

[hedaukartik]

const api = ky.create({
prefixUrl: 'http://localhost',
hooks: {
afterResponse: [
async res => {
const body = await res.json()
if (body.status === 'error') {
throw new Error(body.message)
}
},
]
}
})

res.json gives me an error "unexpected end of input"

Help needed

[ryne2010]

I have my api instance set up as

  throwHttpErrors: false,
  hooks: {
    afterResponse: [
      async (_input, _options, response) => {
        const body = await response.json();
        if (body.error) {
          console.error(body.error.message);
          throw new Error(body.error.message);
        }
      },
    ],
  },
});

This is passing my server error message as the argument to the new Error instance. I am then using that custom message in the UI.

[sholladay]

After mulling this over, I've come to agree that errors thrown by hooks should be treated as fatal.

I think the way to solve this is not by trying to distinguish between different types of errors after they bubble up to the retry logic, but rather to better define Ky's request lifecycle and whether hooks are even in scope for the retry (and timeout) logic in the first place.

Right now, beforeRequest and afterResponse hooks are tightly coupled to the fetch request in terms of error handling.

In pseudo-code, we're basically doing:

try {
    beforeRequest();
    fetch();
    afterResponse();
}
catch (error) {
    retry([beforeRequest, fetch, afterResponse]);
}

Whereas, we should probably be doing:

beforeRequest();
try {
    fetch();
}
catch (error) {
    retry([fetch]);
}
afterResponse();

It's definitely a breaking change. And we may want to provide a mechanism for hooks to trigger a retry. But this should make a lot of scenarios more predictable.