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

axios 1.2.3 causes an AxiosHeader error #5476

Closed
laterdayi opened this issue Jan 18, 2023 · 32 comments · Fixed by #5482
Closed

axios 1.2.3 causes an AxiosHeader error #5476

laterdayi opened this issue Jan 18, 2023 · 32 comments · Fixed by #5482

Comments

@laterdayi
Copy link

laterdayi commented Jan 18, 2023

Describe the bug

axios 1.2.3 causes an AxiosHeader error

The type {Token: string; Refresh: string; } "to type" AxiosRequestHeaders ".

Type "{Token: string; Refresh: string; } "Missing the following properties for type" AxiosHeaders ": set, get, has, delete and 19 others

Type "{params: {LayoutId: (string | number | null) []; check: string; }; } "cannot be assigned to an argument of type" AxiosRequestConfig ".

Type {params: {" LayoutId: (string | number | null) []; check: string; }; The attribute "headers" is missing from "}", but is required for type "AxiosRequestConfig"

To Reproduce

No response

Code snippet

No response

Expected behavior

No response

Axios Version

No response

Adapter Version

No response

Browser

No response

Browser Version

No response

Node.js Version

No response

OS

No response

Additional Library Versions

No response

Additional context/Screenshots

No response

@laterdayi
Copy link
Author

in 1.2.2 ,it work

@laterdayi
Copy link
Author

herader is not required to use it

@alecgibson
Copy link

I think this was a type-breaking change introduced in #5420

You now need to replace usage of AxiosRequestConfig with RawAxiosRequestConfig.

@tada5hi
Copy link

tada5hi commented Jan 18, 2023

TS2741: Property 'headers' is missing in type '{ proxy: false; }' but required in type 'ClientRequestConfig<any>'.

@tada5hi
Copy link

tada5hi commented Jan 18, 2023

follow up issue of #5416

@laterdayi
Copy link
Author

such as

 import type { AxiosRequestConfig } from 'axios';

 function execute(queryParams: ExportParamsType | object, config?: AxiosRequestConfig): Promise<void>;

This means that all types that use AxiosRequestConfig need to be replaced All with RawAxiosRequestConfig ?

@tada5hi @alecgibson

@tada5hi
Copy link

tada5hi commented Jan 18, 2023

@laterdayi yep, because the headers property is mandatory in the AxiosRequestConfig type. Another option would be to create an AxiosHeader instance instead switching the type.

But I'm not sure if the behaviour changes again in an upcoming release :person_shrugging:

@laterdayi
Copy link
Author

@tada5hi The current solution is to change everything you use with AxiosRequestConfig to RawAxiosRequestConfig, right?

@tada5hi
Copy link

tada5hi commented Jan 18, 2023

@laterdayi Taking into account the current version, yes.

This should also avoid the following generic bug: #5478

@laterdayi
Copy link
Author

@tada5hi Expect the next release to fix this

@arthurfiorette
Copy link
Contributor

Another problem here is that you cannot create, with valid typescript, a instance of AxiosRequestHeaders.

// Type '{}' is missing the following properties from type
// 'AxiosHeaders': set, get, has, delete, and 19 more.
const headers: AxiosRequestHeaders = {};

// Type 'AxiosHeaders' is not assignable to type 
// 'Partial<RawAxiosHeaders & MethodsHeaders & CommonHeaders>'
const headers: AxiosRequestHeaders = new AxiosHeaders();

@bombillazo
Copy link

Im getting this error when setting headers inline:
image

@VGalata
Copy link

VGalata commented Jan 19, 2023

AxiosResponse is also affected by the introduced breaking change as it has config: AxiosRequestConfig<D> (see here in code).

@FrankEssenberger
Copy link

The PR #5420 ontroduced a breaking API change in a patch versin release 1.2.3. This also broke our pipeline. As mentioned replacing AxiosRequestConfig with RawAxiosRequestConfig solves the issue. However, it would be great if you could be more careful in the future. If you look at the changes adding a mandatory field headers is obviously not type compatible in structural typing. So you should use assertions/checks in version 1.X.Y and make it mandatory in version 2.X.Y.

@bombillazo
Copy link

bombillazo commented Jan 19, 2023

You now need to replace usage of AxiosRequestConfig with RawAxiosRequestConfig.

As mentioned replacing AxiosRequestConfig with RawAxiosRequestConfig solves the issue.

Can confirm this works for us! 🙌

@laterdayi
Copy link
Author

laterdayi commented Jan 19, 2023

I wonder if it can be fixed in the next version, or has Axios decided to change from AxiosRequestConfig to RawAxiosRequestConfig?

@arthurfiorette
Copy link
Contributor

That's another thing we should think about. Why make everyone change from AxiosRequestConfig to RawAxiosRequestConfig, axios should've thought to creating another type like FullAxiosRequestConfig to have mandatory headers and kept AxiosRequestConfig for everyone.

@DigitalBrainJS
Copy link
Collaborator

type like FullAxiosRequestConfig

@arthurfiorette This was the second solution considered, however, it doesn't really fit in with the current naming convention where the Raw prefix is added to indicate untouched user input. Having something like InternalAxiosConfig is better for compatibility, but will likely lead to type-naming confusion. In any case, we need to somehow separate the raw config type provided by the user from the internal config representation, because in the future it will become even more difficult to do. Probably there is no absolutely perfect solution here, so we are open to suggestions.

@laterdayi
Copy link
Author

@DigitalBrainJS May I ask when the confirmed scheme will be available? Shall we change it to RawAxiosRequestConfig or fix this problem in the next version

@jameyg42
Copy link

regardless of what the best way to address the internal/external config going forward, surely there's realization that the harm done by 1.2.3 far outweighs the benefit and 1.2.3 should be "pulled". Patch releases simply should not break builds.

@rcoundon
Copy link

Switching to RawAxiosRequestConfig doesn't appear to work for the request method

const options: RawAxiosRequestConfig = {
  method: 'GET',
  url: '/someurl',
};
const { data } = await apiClient.request<SomeClientSpecificResponseType>(options);

Causes error:

ESLint: Unsafe argument of type `RawAxiosRequestConfig` assigned to a parameter of type `AxiosRequestConfig<any>`.(@typescript-eslint/no-unsafe-argument)

@DigitalBrainJS
Copy link
Collaborator

@jameyg42 Definitely should, however, it is not always possible to make a fix for one thing without any impact on another. It seems that no matter what solution we choose here, the consequences cannot be completely avoided. The only question is where they will appear and how significantly they will affect.

@bombillazo
Copy link

bombillazo commented Jan 19, 2023

What about the prefix Custom?

To me, Raw sounds like some type that is used internally by Axios and has not been sanitized/transformed to make it usable by others.

@DigitalBrainJS
Copy link
Collaborator

So it seems there are three options:

  1. leave the update as is:
    External (user) config - RawAxiosConfig
    Internal - AxiosConfig
  2. partial rollback with an alias (probably until the next major release):
    External (user) config - AxiosConfig = RawAxiosConfig (alias)
    Internal - InternalAxiosConfig
  3. partial rollback forever
    External (user) config - AxiosConfig
    Internal - InternalAxiosConfig

Any suggestions are welcome.

@arthurfiorette
Copy link
Contributor

Can we use 3 until next big release?

@rcoundon
Copy link

So it seems there are three options:

  1. leave the update as is:
    External (user) config - RawAxiosConfig
    Internal - AxiosConfig
  2. partial rollback with an alias (probably until the next major release):
    External (user) config - AxiosConfig = RawAxiosConfig (alias)
    Internal - InternalAxiosConfig
  3. partial rollback forever
    External (user) config - AxiosConfig
    Internal - InternalAxiosConfig

Any suggestions are welcome.

I don't think 1 is quite right - see #5476 (comment)

@DigitalBrainJS
Copy link
Collaborator

@rcoundon that was another issue due to a missing generic parameter in the interface declaration. This has been fixed in the codebase with #5478.

@DigitalBrainJS
Copy link
Collaborator

@arthurfiorette If we are still going to rename the interface in the next major version, then the 2nd option could be preferable since users who have already changed the interface's name will not be forced to rename it back. I don't think they'll be thrilled about it :) Of course, it depends on whether users use the updated AxiosConfig interface somewhere as an internal config or not. We need to think carefully about this step.

@rcoundon
Copy link

@rcoundon that was another issue due to a missing generic parameter in the interface declaration. This has been fixed in the codebase with #5478.

Ok - thanks for clarifying

@JoseLion
Copy link

JoseLion commented Jan 21, 2023

I believe option 2 is the best approach at this point. We'll keep the fix introduced by #5420 to solve issue #5415 while leaving the interface name as before. Still a (relatively) small breaking change because of the required prop, but users won't have to change AxiosConfig to RawAxiosConfig everywhere. It seems like a good compromise between all circumstances to me 🙂

@JoseLion
Copy link

Another problem here is that you cannot create, with valid typescript, a instance of AxiosRequestHeaders.

// Type '{}' is missing the following properties from type
// 'AxiosHeaders': set, get, has, delete, and 19 more.
const headers: AxiosRequestHeaders = {};

// Type 'AxiosHeaders' is not assignable to type 
// 'Partial<RawAxiosHeaders & MethodsHeaders & CommonHeaders>'
const headers: AxiosRequestHeaders = new AxiosHeaders();

This is another big piece to consider, though 🤔

dhess added a commit to hackworthltd/primer-app that referenced this issue Jan 22, 2023
Note: we do not upgrade `axios` here, because the 1.2.2 -> 1.2.3
inexplicably breaks our `primer-api.ts` bindings. See:

axios/axios#5476

I'm shocked that they made this change in a minor point release!
dhess added a commit to hackworthltd/primer-app that referenced this issue Jan 23, 2023
Note: we do not upgrade `axios` here, because the 1.2.2 -> 1.2.3
inexplicably breaks our `primer-api.ts` bindings. See:

axios/axios#5476

I'm shocked that they made this change in a minor point release!
@nfantone
Copy link

1.2.6 is also affected. Last healthy version seems to be 1.2.2 as of now.

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 a pull request may close this issue.