softonic / axios-retry

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

feat: add disableOtherResponseInterceptors option #260

Open yutak23 opened 11 months ago

yutak23 commented 11 months ago

This ought to fix https://github.com/softonic/axios-retry/issues/246.

As commented in the following issue, axios-retry calls a new axios in the response interceptor, so if it succeeds after a retry, for example, the following console.log will be executed many times. https://github.com/softonic/axios-retry/issues/246#issuecomment-1785032766

axios.interceptors.response.use(
    (response) => {
        console.log('Response Interceptor');
        return response;
    },
    null
);

The proposal is to set the disableOtherResponseInterceptors option to true so that the only response interceptor for axios during the retry process is the one set in axios-retry, so if the retry is successful, the response interceptor set in axios for the first request is executed only once.

yutak23 commented 11 months ago

@mindhells, I would appreciate your confirmation.

mindhells commented 11 months ago

I'm sorry @yutak23 I've been very busy lately. I'm not really sure this should be part of the responsibility of this library 🤔 Can you guarantee other interceptors are not executed at that point, regardless of the order they have been registered?

yutak23 commented 11 months ago

I'm sorry @yutak23 I've been very busy lately. I'm not really sure this should be part of the responsibility of this library 🤔 Can you guarantee other interceptors are not executed at that point, regardless of the order they have been registered?

@mindhells , I apologize for rushing you. Yes, i implemented it so that the intended movement is independent of the order in which the interceptors are registered. I have enhanced the test cases in the latest commit.

lavagri commented 9 months ago

@mindhells I apologize for creating any confusion, maybe my previous explanations in the issue were too abstract. I still think that axios-retry should handle this. Let me try to convince you with a more concrete example.

TikTok Marketing API returns 200 mainly for all responses, wrapping everything in this structure

{ 
    code: 40002, // everything that not "0" = "error"; in this example it's kind of 400, but statusCode still 200 
    ...,
    data: {...} // present in all "success" (code: "0") and sometimes in "error" responses
}

I want to achieve 2 things and still use axios-retry library:

  1. (optional, maybe out of this PR, but still 🙂) attach my interceptors before axios-retry and map code to specific error, throw it, catch it in retryCondition(), and retry if it's needed.
  2. attach my interceptor after axios-retry: return axiosResponse.data.data (if code = "0" = success). But the current implementation will call my interceptors N times, which eventually leads to "Cannot read properties of undefined" errors: you can't read axiosResponse.data.data.data (* N times accessing .data, where N = retry times)

I expect that If axios-retry lib strategy is to add +1 interceptor to the original axios client, it will execute only that interceptor on retries w/o touching my pre/post interceptors.

mindhells commented 9 months ago

@mindhells I apologize for creating any confusion, maybe my previous explanations in the issue were too abstract. I still think that axios-retry should handle this. Let me try to convince you with a more concrete example.

TikTok Marketing API returns 200 mainly for all responses, wrapping everything in this structure

{ 
    code: 40002, // everything that not "0" = "error"; in this example it's kind of 400, but statusCode still 200 
    ...,
    data: {...} // present in all "success" (code: "0") and sometimes in "error" responses
}

I want to achieve 2 things and still use axios-retry library:

  1. (optional, maybe out of this PR, but still 🙂) attach my interceptors before axios-retry and map code to specific error, throw it, catch it in retryCondition(), and retry if it's needed.
  2. attach my interceptor after axios-retry: return axiosResponse.data.data (if code = "0" = success). But the current implementation will call my interceptors N times, which eventually leads to "Cannot read properties of undefined" errors: you can't read axiosResponse.data.data.data (* N times accessing .data, where N = retry times)

I expect that If axios-retry lib strategy is to add +1 interceptor to the original axios client, it will execute only that interceptor on retries w/o touching my pre/post interceptors.

Hi @lavagri, thanks a lot for trying to give more detail on the motivations for this PR. I quite not get your 2nd point 😅, what it's what makes the response to have a nested structure (.data.data.data)?

On the other hand, my concerns with having this implementation are:

But, given the feedback I'm seeing in #246, let's do the following: if https://github.com/axios/axios/pull/6138 gets merged and @yutak23 confirms the use cases you are mentioning are covered, we'll merge this.