softonic / axios-retry

Axios plugin that intercepts failed requests and retries them whenever possible
Other
1.89k stars 167 forks source link

Support retry on 200 response #272

Closed obscurecat64 closed 5 months ago

obscurecat64 commented 5 months ago

This solves #267

Before we couldn't retry on any 200 response. This will add functionality that supports retrying for any logic defined by the user.

mindhells commented 5 months ago

Not sure this is a good idea, as axios itself is capable of doing this just providing it with a validateStatus callback (see https://axios-http.com/docs/handling_errors). Or am I missing something?

obscurecat64 commented 5 months ago

You're right, i realise this can be accomplished using validateStatus together with retryCondition 😭. This change is not required then, and i'll close it.

matikucharski commented 5 months ago

Not sure this is a good idea, as axios itself is capable of doing this just providing it with a validateStatus callback (see https://axios-http.com/docs/handling_errors). Or am I missing something?

validateStatus is using only status number to check if should retry. There may be cases when you want to retry request even if server is returning 200 but there are errors in returned data. Or in case that response returns empty array and you know there should be at lest one element

matikucharski commented 5 months ago

You're right, i realise this can be accomplished using validateStatus together with retryCondition 😭. This change is not required then, and i'll close it.

Please reopen it as your change is very welcomed :) validateStatus only can cause that axios treats every 200 request as errored. In retryCondition it canot be changed and if it returns false to retry then whole request fails. There is no option to successfully return result in that case

obscurecat64 commented 5 months ago

Oh right, retryCondition only serves to retry and cannot change whether a response should be resolved or rejected. Using validateStatus will be too general as it rejects solely based on the status code. @mindhells could you take a look again also please?

mindhells commented 5 months ago

My concern was about overlapping functionality with what axios itself provides. This should be a simple library with a very specific purpose, and also there's no big community behind it to be maintained. If the validateStatus is not enough to cover this use case and there's no other easy way around it (additional response interceptor + retryCondition for example), the I will be happy to merge this PR, but please sign your commits 😅
In either case, thanks a lot for taking the time to contribute 🙇

obscurecat64 commented 5 months ago

I understand where you're coming from; thank you for helping to maintain the project 🙏

The library right now can handle retrying requests when the response comes back with a status code that is non-2xx. However, as brought up by @matikucharski and in issue #267 , there are cases where we also want to retry when the status code is 2xx.

From what I've read from the axios documentation we can use validateStatus to define whether to resolve or reject the promise for a given HTTP response status code. Let's say if we were to use pass () => false to validateStatus, this means that 2xx responses will also be rejected and thus be treated as an axios error. While we can use retryCondition to define logic as to when to retry, the side-effect of setting validateStatus to () => false also means that now all responses are treated as an error. We cannot control when the response is treated as "with no issues" just by setting validateStatus, so the user will have to add an additional interceptor for this.

This library is about retrying requests, so for me I think it should also support retrying 2xx status code responses innately. As far as I know, there is no single option in axios that can be set to cover this use case. With this feature, the user should be able to further control when the response should be treated as an error and not an error, thus enabling retrying on 2xx responses too.

Would like to know what you think 🙏

mindhells commented 5 months ago

I understand where you're coming from; thank you for helping to maintain the project 🙏

The library right now can handle retrying requests when the response comes back with a status code that is non-2xx. However, as brought up by @matikucharski and in issue #267 , there are cases where we also want to retry when the status code is 2xx.

From what I've read from the axios documentation we can use validateStatus to define whether to resolve or reject the promise for a given HTTP response status code. Let's say if we were to use pass () => false to validateStatus, this means that non-2xx responses will be rejected and thus be treated as an axios error. While we can use retryCondition to define logic as to when to retry, the side-effect of setting validateStatus to () => false also means that now all responses are treated as an error. We cannot control when the response is treated as "with no issues" just by setting validateStatus, so the user will have to add an additional interceptor for this.

This library is about retrying requests, so for me I think it should also support retrying 2xx status code responses innately. As far as I know, there is no single option in axios that can be set to cover this use case. With this feature, the user should be able to further control when should the request be treated as an error and not an error, thus enabling retrying on 2xx responses too.

Would like to know what you think 🙏

Makes sense to me, but please sign your commits so I can merge the PR 🙏 Thanks again!

obscurecat64 commented 5 months ago

@mindhells I have signed the commits now.

One more thing that I would like your opinion on (perhaps also @matikucharski) before we merge:

should we make validateResponse only for 2xx responses? Perhaps there isn't a scenario where the user would want to resolve non-2xx responses.

If we do this, then the user doesn't need to specify:

validateResponse: (response) => {
  // the line below each time
  if (response.status < 200 || response.status >= 300) return false;
  return response.data === 'ok!';
}
mindhells commented 5 months ago

@mindhells I have signed the commits now.

One more thing that I would like your opinion on (perhaps also @matikucharski) before we merge:

should we make validateResponse only for 2xx responses? Perhaps there isn't a scenario where the user would want to resolve non-2xx responses.

If we do this, then the user doesn't need to specify:

validateResponse: (response) => {
  // the line below each time
  if (response.status < 200 || response.status >= 300) return false;
  return response.data === 'ok!';
}

At this point I'd keep your implementation and give full control to the user. But let's hear others' opinion. Maybe you can ask in the issue as well.

obscurecat64 commented 5 months ago

Done.

What you said makes sense too. It's possible to resolve non-2xx responses in axios with validateStatus, so we can allow it too.

matikucharski commented 5 months ago

At this point I'd keep your implementation and give full control to the user. But let's hear others' opinion. Maybe you can ask in the issue as well.

I agree. I wouldn't prevent users from handling other status codes when response is received.

obscurecat64 commented 5 months ago

@mindhells in that case i think we should be ready to merge?

mindhells commented 5 months ago

published as 4.3.0 Thanks a lot!