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

Distinguish request and response data types #4116

Merged
merged 2 commits into from Oct 6, 2021
Merged

Distinguish request and response data types #4116

merged 2 commits into from Oct 6, 2021

Conversation

caugner
Copy link
Contributor

@caugner caugner commented Oct 1, 2021

Fixes #4109 by introducing a separate type parameter for the Request data, in contrast to the Response type.

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>;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That makese sense.

In this case I can see two options here:

  1. 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>;
  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>;

What makes more sense in your opinion?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And based on your input, I have now moved the D parameter to the end, to avoid a breaking change.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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 caugner marked this pull request as draft October 1, 2021 14:48
@caugner caugner changed the title Remove misleading type parameter Distinguish request and response data types Oct 1, 2021
@caugner caugner marked this pull request as ready for review October 1, 2021 14:55
@caugner caugner marked this pull request as draft October 1, 2021 15:09
@caugner caugner marked this pull request as ready for review October 1, 2021 15:17
This was referenced Oct 5, 2021
@ardyfeb
Copy link

ardyfeb commented Oct 8, 2021

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 R = AxiosResponse should placed on last

@caugner
Copy link
Contributor Author

caugner commented Oct 8, 2021

It's does not make sense here:

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.

@ardyfeb
Copy link

ardyfeb commented Oct 8, 2021

We are on same boat, i open pr for this. maybe will pushed on major release, but this change i use internally

@joeframbach
Copy link

joeframbach commented Mar 16, 2022

unfortunately that's a breaking change for folks who already use the second type parameter

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:

declare module 'axios' {
  interface AxiosStatic {
    post<Response, Request>(
      url: string,
      data?: Request,
      config?: AxiosRequestConfig<Request>,
    ): Promise<AxiosResponse<Response, Request>>;
  }
}

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.

breaking interface change
6 participants