axios / axios

Promise based HTTP client for the browser and node.js
https://axios-http.com
MIT License
105.2k stars 10.82k forks source link

Typescript definitions for AxiosInterceptorOptions included in response interceptors #5074

Open amitsainii opened 1 year ago

amitsainii commented 1 year ago

Describe the bug

With Axios 1.x we introduced an optional third argument for our interceptor use function. When I looked at the source code, I realized this third param of type AxiosInterceptorOptions is only used for request interceptor.
The issue I'm reporting is related to the typescript definition for the use method. Currently, when it comes to this third parameter, type definitions don't distinguish between request and response interceptors. Because of this currently, this third argument can be used on the response interceptor as well without typescript yelling at you. This is misguiding for our consumers as they may end up using runWhen on response interceptors which won't do anything.

To Reproduce

In a typescript project try writing the below code

Axios.interceptors.response.use((response) => response.data, undefined, {
  runWhen: (config) => config.method === 'GET',
});

Here users will expect that their response interceptor should only be fired when the request has the GET method, but in reality, this third argument would have no effect on the response interceptor.

Expected behavior

Typescript should report an error when using this third argument with a response interceptor.

Solution

This can be easily achieved by replacing the following type definition

export interface AxiosInterceptorManager<V> {
  use(onFulfilled?: (value: V) => V | Promise<V>, onRejected?: (error: any) => any, options?: AxiosInterceptorOptions): number;
  eject(id: number): void;
  clear(): void;
}

with the one mentioned below

type AxiosRequestInterceptorUse<T> = (onFulfilled?: (value: T) => T | Promise<T>, onRejected?: (error: any) => any, options?: AxiosInterceptorOptions) => number;

type AxiosResponseInterceptorUse<T> = (onFulfilled?: (value: T) => T | Promise<T>, onRejected?: (error: any) => any) => number;

export interface AxiosInterceptorManager<V> {
  use: V extends AxiosRequestConfig ? AxiosRequestInterceptorUse<V> : AxiosResponseInterceptorUse<V>;
  eject(id: number): void;
  clear(): void;
}

I can push my changes in a PR the moment you guys are on board with this.

Alternate Path

We may also consider incorporating features like runWhen for response interceptors as well. This is one of my 2 suggestions on Ability to skip request/response interceptors on per request basis. I already created a PR for another type issue with use method. But I've created this issue separately because we can go for implementing these for response interceptors as well.

Environment

amitsainii commented 1 year ago

Hi, @jasonsaayman I'd love to know the reasoning behind this getting closed. No rush though, I know you're kind of donating your free time here

jasonsaayman commented 1 year ago

Will have a look as soon as possible. I just presumed that should take care of all types however I see what you said on your PR so I will test it out

amitsainii commented 1 year ago

Sounds good @jasonsaayman. BTW https://github.com/axios/axios/issues/5070 is the issue that is tied to that PR. This is a separate issue related to typings on the response interceptor.

ForeshadowRU commented 1 year ago

Aren't there plans for adding such runWhen option to response interceptors too? I would use this feature on project to parse server response with response interceptor based on responseType === 'blob', while other response interceptor can still work with json.

amitsainii commented 1 year ago

Aren't there plans for adding such runWhen option to response interceptors too? I would use this feature on project to parse server response with response interceptor based on responseType === 'blob', while other response interceptor can still work with json.

@ForeshadowRU I did start a discussion in the same direction. If @jasonsaayman agrees, I can even PR that feature myself 👍