Revert "Improved type-safety for AxiosRequestConfig (#2995)"

[remcohaszing]

This reverts commit 4eeb3b1.

The reverted commit assumed the request and response body have to be of the same type. This makes axios 0.22.0 unusable with TypeScript.

Refs #2995
Closes #4109

[caugner]

The commit you're reverting here is already included in v0.21.4, which doesn't have the issue, so IMO the regression must have been introduced in a different commit.

[caugner]

The commit you're reverting here is already included in v0.21.4, which doesn't have the issue, so IMO the regression must have been introduced in a different commit.

Sorry, my bad, It only looks like that commit was part of that release, but the v0.21.4 does actually not contain those changes, see: https://unpkg.com/axios@0.21.4/index.d.ts

PS: Is this inconsistency between NPM release and git tag a known issue?

[caugner]

Having given this revert another thought, wouldn't it be sufficient to remove AxiosRequestConfig's type parameter, instead of reverting the any to never change as well?

See #4116.

[remcohaszing]

Having given this revert another thought, wouldn't it be sufficient to remove AxiosRequestConfig's type parameter, instead of reverting the any to never change as well?

No. The problem isn’t that AxiosRequestConfig accepts a generic. The problem is that the reverted commit forces the request body to the of the same type as the response body. The two shouldn’t be related.

While it is possible to add better type safety, it’s a bit more complex than that. Simply reverting at least fixes usage of axios 0.22.0 with TypeScript.

[caugner]

No. The problem isn’t that AxiosRequestConfig accepts a generic. The problem is that the reverted commit forces the request body to the of the same type as the response body. The two shouldn’t be related.

Hm, if it does indeed make sense to keep the AxiosRequestConfig generic type parameter (rather than dropping it), the two options I can see are:

1. to omit it in the request/get/delete/... method parameter types,

   -  request<T = never, R = AxiosResponse<T>> (config: AxiosRequestConfig<T>): Promise<R>;
   +  request<T = never, R = AxiosResponse<T>> (config: AxiosRequestConfig): Promise<R>;

2. to introduce a third type parameter (e.g. D for data) there:

   -  request<T = never, R = AxiosResponse<T>> (config: AxiosRequestConfig<T>): Promise<R>;
   +  request<T = never, D = any, R = AxiosResponse<T>> (config: AxiosRequestConfig<D>): Promise<R>;

Wouldn't one of these forward fixes make more sense than reverting back?

PS: My rationale includes that releasing the full revert as a patch release would require the subsequent fixed type-safety improvement to be released in another minor release (due to the any to never change), whereas a single patch release would suffice for the forward fix.

[remcohaszing]

Option 2 is a step in the right direction, but since type parameters are ordered, introducing a new type parameter before an existing one is a breaking change.

request<T = never, R = AxiosResponse<T, D>, D> (config: AxiosRequestConfig<D>): Promise<R>;

Although I have my doubts if it’s even that useful.

The real benefit isn’t that the input config is typed. In fact, it would be better to omit method, url and data from HTTP methods which accept url and data that accept them as positional parameters.

post<T = any, R = AxiosResponse<T>>(url: string, data?: any, config?: Omit<AxiosRequestConfig, 'method' | 'url' | 'data'>): Promise<R>;

Putting it all together, it becomes something like:

post<T = never, R = AxiosResponse<T, D>, D>(url: string, data?: any, config?: Omit<AxiosRequestConfig<T, D>, 'method' | 'url' | 'data'>): Promise<R>;

The only benefit this has over accepting any data, is that the types of data and response.config.data are synchronized.

[caugner]

Although I have my doubts if it’s even that useful.

I didn't see the benefit at first, but I more and more think it is, especially when generating those data typings automatically from API definitions.

[caugner]

post<T = never, R = AxiosResponse<T, D>, D>(url: string, data?: any, config?: Omit<AxiosRequestConfig<T, D>, 'method' | 'url' | 'data'>): Promise<R>;

Do we really need to pass the T parameter to AxiosRequestConfig, given that it's not used there?

[caugner]

Okay, we cannot reference the D parameter in R = AxiosResponse<T, D>, if it comes afterwards. 🤔

The following would be ideal, but unfortunately a breaking change:

-post<T = never, R = AxiosResponse<T>>(url: string, data?: T, config?: AxiosRequestConfig<T>): Promise<R>;
+post<T = never, D = any, R = AxiosResponse<T>>(url: string, data?: D, config?: Omit<AxiosRequestConfig<T, D>, 'method' | 'url' | 'data'>): Promise<R>;

[rijkvanzanten]

PS: Is this inconsistency between NPM release and git tag a known issue?

From what I can tell from @jasonsaayman 's response (#3002 (comment)):

@rijkvanzanten this change has not been released, for 0.22.0 we worked in a separate branch from master, this change will probably only make it into v1

it sounds like they're using "master" (which is the tagged branch) as the 1.0.0 code, with release branches for the 0.x line

@caugner

[gstamac]

I created another PR to fix this. Please check if that's what you're looking for.
#4133
I didn't reorder the generic types to not introduce a breaking change.

[remcohaszing]

Closing this in favor of #4133.

[caugner]

@remcohaszing Any reason to prefer #4133 over #4116?

[remcohaszing]

@remcohaszing Any reason to prefer #4133 over #4116?

Nope. I just picked the one that was getting most attention.