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

Fixed type definition of use method on AxiosInterceptorManager to match the the README #5071

Merged

Conversation

amitsainii
Copy link
Contributor

On the Interceptor section on README.md, where we are explaining the usage of the newly introduced synchronous and runWhen flags on AxiosInterceptorOptions, we are passing null for the optional parameters for example:

axios.interceptors.request.use(function (config) {
  config.headers.test = 'I am only a header!';
  return config;
}, null, { synchronous: true });

When using this in a typescript project you'll get an error saying:

Argument of type 'null' is not assignable to parameter of type '((error: any) => any) | undefined'

The reason is we didn't specify null as a type for our interceptor handlers.

As per the typescript docs

When strictNullChecks is truenull and undefined have their own distinct types and you’ll get a type error if you try to use them where a concrete value is expected.

This PR includes null as a type for the interceptor methods

Fixes: #5070

@amitsainii
Copy link
Contributor Author

I could've updated the README to have null changed to undefined instead, but some people(including me) prefer passing null as an argument for an optional parameter.

I had another idea to make use accept a single param with the following typescript interface

interface AxiosInterceptorUseParams<V> {
  onFulfilled?(value: V): V;
  onRejected?: (error: any) => any;
  options?: AxiosInterceptorOptions
}

But that would've been a major breaking change, so I just incorporated the null type for now

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.

This seems pretty reasonable, thanks for the contribution.

@jasonsaayman
Copy link
Member

Actually, I am incorrect, any in this instance cover null too? Let me know if I have it wrong but I am closing for now.

@amitsainii
Copy link
Contributor Author

Hi @jasonsaayman
Thanks for the response. You may want to try this out with strictNullChecks or strict set to true in tsconfig in your IDE and you'll surely get the type error I mentioned in the PR description. Type definition currently clearly specifies that these can either be a function or undefined(because them being optional).
You can also refer to https://www.typescriptlang.org/tsconfig#strictNullChecks
According to the docs

When strictNullChecks is false, null and undefined are effectively ignored by the language. This can lead to unexpected errors at runtime.
When strictNullChecks is true, null and undefined have their own distinct types and you’ll get a type error if you try to use them where a concrete value is expected.

If you still have any concerns, please reach out, I'd be quick to respond 👍

@jasonsaayman
Copy link
Member

I will do, adding myself to this for now

@jasonsaayman jasonsaayman self-assigned this Oct 14, 2022
@jasonsaayman jasonsaayman reopened this Oct 15, 2022
@jasonsaayman jasonsaayman merged commit 448e1fc into axios:v1.x Oct 30, 2022
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.

Incorrect type definition for use method on AxiosInterceptorManager
2 participants