fix(types): fixed AxiosRequestConfig header interface by refactoring it to RawAxiosRequestConfig;

[DigitalBrainJS]

• Refactored AxiosRequestConfig to RawAxiosRequestConfig interface;
• Re-added AxiosRequestConfig as a subinterface of RawAxiosRequestConfig;

export interface AxiosRequestConfig<D = any> extends RawAxiosRequestConfig {
  headers: AxiosRequestHeaders;
}

axios.interceptors.request.use((req: AxiosRequestConfig) => {
  req.headers.set('foo', 'bar');
  req.headers['Content-Type'] = 123;
  return req;
});

Fixes #5415

[github-advanced-security]

CodeQL found more than 10 potential problems in the proposed changes. Check the Files changed tab for more details.

[tada5hi]

@DigitalBrainJS would be great to merge this fix soon ✌️

[karlhorky]

Maybe the description should be edited here to also add:

Fixes #5416

[xgenem]

This would be nice since you no longer have to typecast it like what I did.

[jonesmac]

@DigitalBrainJS thanks for hoping on this so quick. I've spent some time looking over this issue and it looks like it stems back from trying to support AxiosHeaders class as a valid type of header property on the AxiosRequestConfig type. I wonder if there's really any value to supporting this use case?

7a2f901#diff-243f86a2e64131743ca04681df5d21438e79fcabe06f18029cb8dcef6a70121fR351

I've seen other request libraries express headers as an Object/Map so is there really value to supporting a custom class like AxiosHeaders in the config? In my case, as a library that wants to allow consumers to pass an axios config but add my custom headers (i.e. CONTENT-TYPE / AUTHORIZATION), the library will need to detect which type of header collection you are passing (Record or AxiosHeaders) and update accordingly.

If you think you need to support both, I'd suggest making the AxiosHeaders constructor accept the other AxiosRequestConfig['headers'] type of RawAxiosRequestHeaders. Right now it only takes type RawAxiosHeaders = Record<string, AxiosHeaderValue> which doesn't sufficiently overlap type RawAxiosRequestHeaders = Partial<RawAxiosHeaders & MethodsHeaders & CommonHeaders>;

That way, if a client passes a config of either headers type, the AxiosHeaders class can handle converting it keeping both header types somewhat interoperable.

[baraeb92]

This might need your attention @jasonsaayman

[DigitalBrainJS]

@jonesmac Yes, the need to support AxiosHeaders as config headers was due to the previous bug #5226 - some plugins (axios-retry) and users re-use config inside interceptors by passing internal config directly to a new Axios request. This is more of a backward compatibility fix than a new feature, although users can now use AxiosHeaders as a helper for case-insensitive header preprocessing (merge, delete).

[DavidRigglemanININ]

I was disappointed to find the patch release (1.2.3) contain breaking changes and traced it back to this particular change. I was able to update multiple spots in my code base with the latest changes and can build fine now, but was surprised to see a non-backwards compatible change in a patch release and without really any note of it in the release notes. Hopefully, this can be considered in future releases. Thanks so much your work in creating and updating this great library!

[arminrosu]

I was disappointed to find the patch release (1.2.3) contain breaking changes and traced it back to this particular change.

Ditto. In my case, it broke multiple js clients generated by openapi-generator, as it defaults method arguments to options: AxiosRequestConfig = {}. Unfortunately, patches to openapi-generator are slow to be integrated, so I downgraded axios@1.2.2 for now.

[ecyrbe]

I think this should be fixed because this is a breaking change.
It breaks clients using AxiosRequestConfig. Zodios that is using axios got broken by this.

At least i was hoping that axios would not make breaking changes on stable public interfaces like that and tag them as patch.

edit: to clarify, with this change, i can't make zodios compatible with axios < 1.2.3 while making it compatible with axios 1.2.3 and i guess any lib using axios out there will have the same trouble. I was happy that i have until now been able to maintain zodios compatible with even 0.x versions. Is there any way to do something to not break compatibility ?

[soulchild]

Clients generated by orval are broken as well:

Argument of type '{ headers?: AxiosRequestHeaders | undefined; url?: string | undefined; method?: string | undefined; baseURL?: string | undefined; transformRequest?: AxiosRequestTransformer | AxiosRequestTransformer[] | undefined; ... 31 more ...; formSerializer?: FormSerializerOptions | undefined; }' is not assignable to parameter of type 'AxiosRequestConfig<any>'.

Please revert this change and re-release it as a major release instead ASAP.

[arminrosu]

Further $0.02 - I think the root issue is that the same Typings are used for input arguments and output types, whereas e.g. headers can be defaulted to and output as a required property.

Looking at the code, this is a non-trivial task to fix quickly overall, but this PR should be reverted.

@DigitalBrainJS are you aware of the discussion here? Should we have raised a new issue?

[DavidRigglemanININ]

If you end up reverting this PR, just don't try to do something like republish 1.2.3 again with the fix. That would just make the problem even worse for those who did take the time to make the necessary updates.

[ecyrbe]

I found a way to make my code compatible with all versions, by extracting the types from function signature instead of importing them :
https://github.com/ecyrbe/zodios/blob/feature/v11/packages/axios/src/axios-provider.ts#L23

Hope this helps

[DigitalBrainJS]

@arminrosu I hope #5486 should fix this, as it renames the inner config interface instead of the outer one (and keeps RawAxiosRequestConfig as an alias of the external config interface, AxiosRequestConfig=RawAxiosRequestConfig). This should minimize the impact on the Axios ecosystem caused by the attempt to fix the #5415 issue. Otherwise, we would have had to revert several releases as the problems with types start earlier than in the last one.

[arminrosu]

@DigitalBrainJS ah, sweet, didn't see the fix PR. Thanks for the quick turn-around! Keep up the good work!