Support retry on 200 response

[obscurecat64]

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]

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]

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]

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]

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]

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]

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]

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]

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]

@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]

@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]

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]

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]

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

[mindhells]

published as 4.3.0
Thanks a lot!