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.

Sign up for GitHub

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

Jump to bottom

Updating axios in types to be lower case #2797

Merged
merged 2 commits into from
Oct 27, 2020
Merged

Updating axios in types to be lower case #2797

merged 2 commits into from
Oct 27, 2020

Conversation

remcohaszing
Copy link
Contributor

@remcohaszing remcohaszing commented Mar 4, 2020 •
edited
Loading

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
Contributor

TimWolla commented Apr 7, 2020

This is part of #2872

@TimWolla TimWolla mentioned this pull request Jun 14, 2020
Closed
@chinesedfan chinesedfan linked an issue Jun 15, 2020 that may be closed by this pull request
Align auto-import with README examples #3017
Closed
@donaldpipowitch
Copy link

Should this be closed then? @chinesedfan

@remcohaszing
Copy link
Contributor Author

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

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
Contributor

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
Member

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

Thank you all for the responses. ❤️

@TimWolla
Copy link
Contributor

@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?

@remcohaszing
Updating axios in types to be lower case
b862130 
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 lower-case-axios-type branch from b6f5e62 to b862130 Compare August 30, 2020 13:58
@remcohaszing
Copy link
Contributor Author

@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 minimized.

Sign in to view
@remcohaszing
Copy link
Contributor Author

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
Contributor

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
Contributor

0.21.0 appears to be released now. Any update?

@jasonsaayman
Copy link
Member

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

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
Contributor

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
Member

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
This was referenced Mar 18, 2021
Open
Merged
Closed
Open
Merged
Merged
Closed
Merged
Open
Merged
Merged
Open
Closed
Merged
Open
Merged
Merged
Open
Merged
Merged
Merged
Open
@sync-by-unito sync-by-unito bot mentioned this pull request Jun 28, 2021
Closed
mbargiel pushed a commit to mbargiel/axios that referenced this pull request Jan 27, 2022
@remcohaszing @chinesedfan
Updating axios in types to be lower case (axios#2797)
b30a1db 
Co-authored-by: Xianming Zhong <chinesedfan@qq.com>
mbargiel pushed a commit to mbargiel/axios that referenced this pull request Jan 27, 2022
@timemachine3030 @remcohaszing
@chinesedfan @jasonsaayman
Use different socket for Win32 test (axios#3375)
accdb83 
* 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 (axios#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>
@snyk-bot snyk-bot mentioned this pull request Feb 22, 2022
Closed
@remcohaszing remcohaszing deleted the lower-case-axios-type branch July 26, 2022 10:03
@Omrisnyk Omrisnyk mentioned this pull request Dec 20, 2023
Open
@Omrisnyk Omrisnyk mentioned this pull request Jan 2, 2024
Open
@Omrisnyk Omrisnyk mentioned this pull request Mar 16, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jasonsaayman jasonsaayman jasonsaayman approved these changes

@chinesedfan chinesedfan chinesedfan approved these changes

Assignees

@jasonsaayman jasonsaayman

Labels
None yet
Projects
None yet
Milestone
v0.21.1
Development

Successfully merging this pull request may close these issues.

Align auto-import with README examples
5 participants
@remcohaszing @TimWolla @donaldpipowitch @jasonsaayman @chinesedfan