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

Make the default type of response data never #3002

Merged
merged 2 commits into from Sep 5, 2021

Conversation

@remcohaszing
Copy link
Contributor

@remcohaszing remcohaszing commented Jun 7, 2020

This requires TypeScript users to explicitly define the type of the data they
are consuming.

Before this, data was any by default. This means TypeScript consumers didn’t
get type safety if they forgot to specify the type.

Technically this is a breaking change for TypeScript users, as this will report
errors if they forgot to specifiy the response type. The simplest workaround
would be to explicitly set the response type to any, so it’s not breaking
much.

The unknown type is probably a slightly better fit, but this requires
TypeScript ^3.

data is still any in the very specific use case mentioned in
microsoft/TypeScript#38969

This requires TypeScript users to explicitly define the type of the data they
are consuming.

Before this, data was `any` by default. This means TypeScript consumers didn’t
get type safety if they forgot to specify the type.

Technically this is a breaking change for TypeScript users, as this will report
errors if they forgot to specifiy the response type. The simplest workaround
would be to explicitly set the response type to `any`, so it’s not breaking
much.

The `unknown` type is probably a slightly better fit, but this requires
TypeScript ^3.

`data` is still `any` in the very specific use case mentioned in
microsoft/TypeScript#38969
@carloschida
Copy link
Contributor

@carloschida carloschida commented Jun 9, 2020

I do agree with you in that unknown would be the optimal choice.

Having said that If we have to choose between defaulting to never or to any, I'd got for the latter.
The reason is that users that want type safety are already overriding the default, whereas users that don't want it will be just confused as to why they should declare it.

Additionally, if one were to re-write axios in TS, I'm convinced that never wouldn't be the best choice for the functions that employ AxiosResponse.

Loading

@rijkvanzanten
Copy link
Contributor

@rijkvanzanten rijkvanzanten commented Oct 1, 2021

@jasonsaayman Mind adding this to the release notes? It's currently omitted from https://github.com/axios/axios/blob/master/CHANGELOG.md#0220-october-01-2021

Loading

@jasonsaayman
Copy link
Member

@jasonsaayman jasonsaayman commented Oct 1, 2021

@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

Loading

@rijkvanzanten
Copy link
Contributor

@rijkvanzanten rijkvanzanten commented Oct 1, 2021

@jasonsaayman Are you sure about that? Looking at the axios folder in my node_modules for 0.22.0, the "AxiosResponse" type is already set to "never" as per this PR 🤔

In 0.22.0:

https://unpkg.com/axios@0.22.0/index.d.ts

..

export interface AxiosResponse<T = never>  {

..

In 0.21.4:

https://unpkg.com/axios@0.21.4/index.d.ts

..

export interface AxiosResponse<T = any>  {

..

Loading

@TheVaan
Copy link

@TheVaan TheVaan commented Oct 1, 2021

I can confirm that this change is inside 0.22.0 - I've the same issue and was surprised about breaking changes in 0.22.0 without marking it breaking in the change log.

Loading

@ImRodry
Copy link
Contributor

@ImRodry ImRodry commented Oct 4, 2021

Please undo this PR. The never type is for things that can NEVER be present, not for something that has an unknown type. Typescript v3 is pretty old and there is no reason to support that, however, this should remain as any because any is a type that is supposed to be mutated, unlike unknown which acts as a "We don't know what this is, please don't touch this property" which is very clearly not the case. I changed to axios because node-fetch did this stupid change and now you guys are doing it too. Please don't kill your package

Loading

@jasonsaayman
Copy link
Member

@jasonsaayman jasonsaayman commented Oct 5, 2021

Ok well if this is in that release that is a pretty large issue, I will check out the fixes provided and try release asap. Someone must have not worked from the given 0.22 branch.

Loading

@remcohaszing
Copy link
Contributor Author

@remcohaszing remcohaszing commented Oct 5, 2021

Let’s change this to unknown. The lowest supported TypeScript version by DefinitelyTyped is 3.7, so I think it’s safe to match that.

I would make a PR, but it’s going to have merge conflicts with #4116, so I’ll wait.

Loading

@huineng
Copy link

@huineng huineng commented Oct 5, 2021

why not just leave it to 'any', even changing it to unknown is a 'breaking' change as we will have to do several modifications to our code.

before ...
if (response.data?.message) {
// do something
}

after 
if ((response.data as any)?.message) {
// do something
}

Loading

@remcohaszing
Copy link
Contributor Author

@remcohaszing remcohaszing commented Oct 5, 2021

This change provides better type safety, which is what most TypeScript users want.

Changing it from any to never was slightly breaking, which is why it was released as semver minor (below 1.0.0). Changing it from never to unknown isn’t.

Looking at your example, your code should look like this:

interface MyResponseData {
  message: string;
}

const response = await axios.get<MyResponseData>('…');

if (response.data.message) {
  // do something
}

But if you really want to lose type safety, you could still make it explicit:

const response = await axios.get<any>('…');

if (response.data.message) {
  // do something
}

Loading

@ImRodry
Copy link
Contributor

@ImRodry ImRodry commented Oct 5, 2021

Why are you the ones to decide what TypeScript users want? In my code I fetch an API that returns one JSON object with 2 string properties, of which I only need to use one of them, so it makes no sense to make an interface for this. Having to add an explicit any is even more nuts and some of us have rules that disallow that. The library shouldn’t have a say in what users want, and should just let them use it in whichever way they want, and this change is a big step in the wrong direction

Loading

@rijkvanzanten
Copy link
Contributor

@rijkvanzanten rijkvanzanten commented Oct 5, 2021

and this change is a big step in the wrong direction

This is very subjective. I for one welcome this change, but that doesn't mean that "The TypeScript Community" as a whole wants this.

Loading

@ImRodry
Copy link
Contributor

@ImRodry ImRodry commented Oct 5, 2021

Well but that’s why the package should allow everything and let the users decide what they wanna do, not do shit for them because you don’t always need an explicit type assertion

Loading

@rijkvanzanten
Copy link
Contributor

@rijkvanzanten rijkvanzanten commented Oct 5, 2021

Well but that’s why the package should allow everything and let the users decide what they wanna do, not do shit for them because you don’t always need an explicit type assertion

You're still allowed to do anything you'd like, this is just a matter of deciding what the better default is for the project as a whole 🤷🏻

Loading

@ImRodry
Copy link
Contributor

@ImRodry ImRodry commented Oct 5, 2021

No we’re not allowed to do what we want because it’s good practice not to allow explicit any types and most of us have that rule set. This change requires us to make an interface for every damn axios request we make

Loading

@rijkvanzanten
Copy link
Contributor

@rijkvanzanten rijkvanzanten commented Oct 5, 2021

No we’re not allowed to do what we want because it’s good practice not to allow explicit any types

Wait, so you're arguing that people should NOT use any, and therefore axios should default to any? If anything, your example shows why you'd be in favor of this change, not against it 😅

Anywho, lets move this discussion to a new issue or elsewhere on the internet, as I'm sure it'll get old pretty quick to get notifications from people bickering over this on a closed PR 👍🏻

Loading

@bratanon
Copy link

@bratanon bratanon commented Oct 5, 2021

I agree with @ImRodry on this one.

We are also facing this issue where we don't allow explicit any, which forces us to create interfaces for every request.

Wait, so you're arguing that people should NOT use any, and therefore axios should default to any?
@rijkvanzanten YES, and it makes perfect sense. Axios might not have the rule mentioned above, and that is ok, but don't force that on to all our consumers.

Loading

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

9 participants