Distinguish request and response data types#4116
Distinguish request and response data types#4116jasonsaayman merged 2 commits intoaxios:masterfrom caugner:patch-2
Conversation
index.d.ts
Outdated
| options<T = never, R = AxiosResponse<T>>(url: string, config?: AxiosRequestConfig): Promise<R>; | ||
| post<T = never, R = AxiosResponse<T>>(url: string, data?: T, config?: AxiosRequestConfig): Promise<R>; | ||
| put<T = never, R = AxiosResponse<T>>(url: string, data?: T, config?: AxiosRequestConfig): Promise<R>; | ||
| patch<T = never, R = AxiosResponse<T>>(url: string, data?: T, config?: AxiosRequestConfig): Promise<R>; |
There was a problem hiding this comment.
The idea of allowing AxiosRequestConfig to specify the type of data was a good one. The real issue is that T is used as the type of both the request body and the response body.
There was a problem hiding this comment.
That makese sense.
In this case I can see two options here:
- to omit the type parameter 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>;
- to introduce a third type parameter (e.g.
Dfor 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>;
What makes more sense in your opinion?
There was a problem hiding this comment.
I have updated this PR in accordance to the second option.
Having said this, it could make sense to rename the type parameter to Data to make it more obvious what it stands for.
There was a problem hiding this comment.
And based on your input, I have now moved the D parameter to the end, to avoid a breaking change.
There was a problem hiding this comment.
I'd like to voice my support of using a verbose name for the generics in this case, so they're a little more expressive! Hopefully that'll help avoid any issues like #4109 in the future.
There was a problem hiding this comment.
@remcohaszing opted to rename the generic type parameters separately, and as follows:
T → ResponseData
R → ReturnValue
D → RequestData
If @jasonsaayman prefers this as well, I could make the necessary changes right here in this PR, but since that's a distinct improvement, whereas the current changes are mainly fixing a regression, I would recommend to separate that renaming for better traceabilty.
caugner
changed the title
|
It's does not make sense here: + post<T = never, R = AxiosResponse<T>, D = any>(url: string, data?: D, config?: AxiosRequestConfig<D>): Promise<R>;user who need to type its request data need to fill second generic: axios.post<ResponseData, AxiosResponse<ResponseData>, RequestData>(...)it should be axios.post<ResponseData, RequestData>(...)or if they change response object axios.post<ResponseData, RequestData, CustomResponse>(...)people rarely changing response object (from interceptor), so |
I suggested the same as you, but unfortunately that's a breaking change for folks who already use the second type parameter. The order might be fixed in the future though. |
|
We are on same boat, i open pr for this. maybe will pushed on major release, but this change i use internally |
Co-authored-by: Jay <[email protected]>
What do you mean breaking change, it's already broken. This is a bugfix. The second type parameter is unusable, there can't be folks using it. Here is my monkeypatch solution which also incorporates meaningful type argument names: |
6 participants
Fixes #4109 by introducing a separate type parameter for the Request data, in contrast to the Response type.