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

Updating axios in types to be lower case #2797

Merged
merged 2 commits into from Oct 27, 2020

Conversation

@remcohaszing
Copy link
Contributor

@remcohaszing remcohaszing commented Mar 4, 2020

All examples use the lower case variable axios. However, when auto importing
in a TypeScript based editor, the import was named Axios, because this is how
it was defined in the typings.

Closes #3017

@TimWolla
Copy link

@TimWolla TimWolla commented Apr 7, 2020

This is part of #2872

@donaldpipowitch
Copy link

@donaldpipowitch donaldpipowitch commented Jun 15, 2020

Should this be closed then? @chinesedfan

@remcohaszing
Copy link
Contributor Author

@remcohaszing remcohaszing commented Jun 15, 2020

I’d rather not have this closed. #2872 does more than this PR. I don’t know the details of Axios required to merge #2872, but I do know this PR is fine and should be merged, even if #2872 gets rejected for other reasons.

@donaldpipowitch
Copy link

@donaldpipowitch donaldpipowitch commented Jun 15, 2020

Totally agree. I'd love to see this merged immediately. I basically made the same MR (#3018) and just would like to know which MR I need to watch now and if there is anything which blocks this MR from being merged as it is three months old 😞

@TimWolla
Copy link

@TimWolla TimWolla commented Jun 15, 2020

As the author of #2872 I agree with you that this PR is good and I believe it should be merged, because it's obviously correct.

@jasonsaayman
Copy link
Collaborator

@jasonsaayman jasonsaayman commented Jun 17, 2020

Hi,

I will be working on merge's again very soon, this should get merged I just think it will go into something like v0.20.1 as we wanted the first 0.20.x release to fix some issues and regressions and not add many new features. Trust me this will be seen to soon.

Thanks

@donaldpipowitch
Copy link

@donaldpipowitch donaldpipowitch commented Jun 17, 2020

Thank you all for the responses. ❤️

@TimWolla
Copy link

@TimWolla TimWolla commented Jul 24, 2020

@jasonsaayman I am seeing that v0.20.0 has been released by now. Are you able to share a status update regarding this PR / merges in general, yet? Is there anything we can help with moving this forward?

All examples use the lower case variable `axios`. However, when auto importing
in a TypeScript based editor, the import was named `Axios`, because this is how
it was defined in the typings.
@remcohaszing remcohaszing force-pushed the remcohaszing:lower-case-axios-type branch from b6f5e62 to b862130 Aug 30, 2020
@remcohaszing
Copy link
Contributor Author

@remcohaszing remcohaszing commented Sep 22, 2020

@jasonsaayman @chinesedfan I noticed you have been tagging pull requests for v0.20.1. Can this be included? It’s non breaking.

@TimWolla

This comment has been hidden.

@remcohaszing
Copy link
Contributor Author

@remcohaszing remcohaszing commented Sep 22, 2020

It’s non breaking.

Is it actually? For the default export I agree, but I believe the renaming of the exported const technically is a small breakage. But that one is easily fixed and should only affect error reporting during type checking without any functional change.

Yes, first the value is declared, which is an internal detail. Next it’s exported as default, for which the name doesn’t matter.

All this affects is how TypeScript editors will autocomplete the default import. Type validity is untouched.

@TimWolla
Copy link

@TimWolla TimWolla commented Sep 22, 2020

Yes, first the value is declared, which is an internal detail.

Ah, indeed. Somehow my brain added an export in front of that const.

@TimWolla
Copy link

@TimWolla TimWolla commented Oct 26, 2020

0.21.0 appears to be released now. Any update?

@jasonsaayman
Copy link
Collaborator

@jasonsaayman jasonsaayman commented Oct 27, 2020

Hi @TimWolla,

I will be looking to merge this ASAP. I am just going to get clarity on how we are going to handle breaking changes from here on out. I will have feedback before the end of the week.

Thanks

@jasonsaayman jasonsaayman self-assigned this Oct 27, 2020
@remcohaszing
Copy link
Contributor Author

@remcohaszing remcohaszing commented Oct 27, 2020

I will be looking to merge this ASAP. I am just going to get clarity on how we are going to handle breaking changes from here on out.

This one is non-breaking though.

@TimWolla
Copy link

@TimWolla TimWolla commented Oct 27, 2020

I can confirm that it is non-breaking. I apologize for any confusion I might have caused with my previous question on that topic.

@jasonsaayman
Copy link
Collaborator

@jasonsaayman jasonsaayman commented Oct 27, 2020

Ok cool TS is not my strongest point but just to make 100% certain it works the same as common js in that importing the module imports the default export right?

@jasonsaayman jasonsaayman added this to the v0.21.1 milestone Oct 27, 2020
@TimWolla
Copy link

@TimWolla TimWolla commented Oct 27, 2020

Ok cool TS is not my strongest point but just to make 100% certain it works the same as common js in that importing the module imports the default export right?

TypeScript modules implement the ES 6 module syntax (https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/import) and then compile it it down to whatever you like, including CommonJS.

The default import boils down to const foo = require('axios') with foo being an arbitrary name. IDEs just suggest the name used for the default export that is used in the types. The casing of the default export within the .d.ts thus is just an implementation detail that has no effect whatsoever.

@TimWolla
Copy link

@TimWolla TimWolla commented Oct 27, 2020

See this example on the TypeScript playground: https://www.typescriptlang.org/play?module=1#code/JYWwDg9gTgLgBAMwhRUIjgIgIYA9gQDOmA3AFBlIQAUA5ABYwxgBcA9GwKa7bgA2nAHQBjdLQCUJIA

Select the proper module in the TS Config if necessary (the select on the right):

image

@jasonsaayman
Copy link
Collaborator

@jasonsaayman jasonsaayman commented Oct 27, 2020

@TimWolla thanks that's great!

Copy link
Collaborator

@jasonsaayman jasonsaayman left a comment

Can confirm that this is not a breaking issue and am happy to implement it 👍

@chinesedfan chinesedfan merged commit 820fe6e into axios:master Oct 27, 2020
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
timemachine3030 pushed a commit to timemachine3030/axios that referenced this pull request Oct 31, 2020
Co-authored-by: Xianming Zhong <chinesedfan@qq.com>
@jasonsaayman jasonsaayman added this to Done in v0.21.1 Nov 19, 2020
jasonsaayman added a commit that referenced this pull request Dec 9, 2020
* Remove the skipping of the `socket` http test

* Use different socket path for Win32

 - See: https://github.com/nodejs/node-v0.x-archive/blob/master/test/simple/test-pipe-stream.js#L73
 - Also: https://github.com/nodejs/node-v0.x-archive/blob/master/test/common.js#L39

* Updating axios in types to be lower case (#2797)

Co-authored-by: Xianming Zhong <chinesedfan@qq.com>

Co-authored-by: Pilot <timemachine@ctrl-c.club>
Co-authored-by: Remco Haszing <remcohaszing@gmail.com>
Co-authored-by: Xianming Zhong <chinesedfan@qq.com>
Co-authored-by: Jay <jasonsaayman@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Linked issues

Successfully merging this pull request may close these issues.

5 participants