Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

updated interceptor use type definition to remove interceptor options parameter from use method for response interceptors #5237

Open
wants to merge 9 commits into
base: v1.x
Choose a base branch
from

Conversation

amitsainii
Copy link
Contributor

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.

This PR fixes this problem by using conditional type for use method wherein AxiosResponseInterceptorUse does not include an options parameter in the type definition.

Fixes: #5074

… parameter from use method for response interceptors
Copy link
Member

@jasonsaayman jasonsaayman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the contribution and it looks good, however can you please pull the changes to the index.d.cts too?

@amitsainii
Copy link
Contributor Author

@jasonsaayman I can't seem to understand what you mean. There isn't any index.d.cts file and I already made the type definition changes to index.d.ts. Can you please help me understand what this is about 🥺

@jasonsaayman
Copy link
Member

@amitsainii the file is over here we use two sets of types one for CommonJS and one for ESM 😄

@amitsainii
Copy link
Contributor Author

Thanks, @jasonsaayman I added the type definitions to index.d.cts 👍. The reason I wasn't able to locate it before was that I last pulled 1.x 15 days back and index.d.cts is added 13 days back 😄

@amitsainii
Copy link
Contributor Author

@DigitalBrainJS Is this good to go in our next release? In case there are any blockers please let me know, thanks.

@amitsainii
Copy link
Contributor Author

@DigitalBrainJS Can we unblock this, please? I don't think any further changes are needed here. Please let me know if you need more info related to the changes

@amitsainii
Copy link
Contributor Author

@jasonsaayman @DigitalBrainJS This has approval from both of you. Please let me know if there's still something that's stopping this from getting merged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Typescript definitions for AxiosInterceptorOptions included in response interceptors
3 participants