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

fix problem with named export axios #5104

Closed
wants to merge 2 commits into from
Closed

fix problem with named export axios #5104

wants to merge 2 commits into from

Conversation

akphi
Copy link

@akphi akphi commented Oct 11, 2022

Instructions

Fixes #5101

I also chucked in a small fix for the default export to work with NodeNext, a bit of context here

microsoft/TypeScript#49298
microsoft/TypeScript#50690

Basically, I will make it so people can import axios as a named import rather than having to do something like:

import { default as axios } from 'axios';

UPDATE: I saw you have already taken care of #5101 by updating exports in package.json, but the problem with named exports remains. Please see my repro here akphi/issue-repo#12

I don't think import { axios } from 'axios'; work at all.

@akphi
Copy link
Author

akphi commented Oct 14, 2022

@jasonsaayman is there anything else you need me to do/explain?

Copy link
Member

@jasonsaayman jasonsaayman left a comment

Choose a reason for hiding this comment

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

Looks good 👍

@@ -28,5 +28,6 @@ export {
Cancel,
isAxiosError,
spread,
toFormData
toFormData,
axios
Copy link
Member

Choose a reason for hiding this comment

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

Actually, this should not be necessary you can already do this:

import { axios } from 'axios';

Copy link
Author

Choose a reason for hiding this comment

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

Please see my updated PR description. You can check out my repro at akphi/issue-repo#12
There are 2 problems:

  1. For ESM import, I don't think import { axios } from 'axios'; work
  2. For TS, we need to update the typings to have the named export - I just updated the PR with this change

@akphi akphi changed the title fix problem with Jest fix problem with named export axios Oct 16, 2022
@akphi
Copy link
Author

akphi commented Oct 26, 2022

@jasonsaayman hi, could you please take a look at this again when you have time? Thanks!

@kr99
Copy link

kr99 commented Oct 26, 2022

My team is also interested in this fix.

@jasonsaayman
Copy link
Member

Sure going to close this one in favour of #5162

@akphi
Copy link
Author

akphi commented Oct 31, 2022

@jasonsaayman I don't believe #5162 fixed the issue, as in akphi/issue-repo#12 I used axios@1.1.3. This is still the reasonable fix I believe

@akphi
Copy link
Author

akphi commented Nov 3, 2022

@jasonsaayman Hi, could you please take a look at this again? Thanks!

@akphi
Copy link
Author

akphi commented Nov 14, 2022

@jasonsaayman Could you let me know what else I can do for this PR? Do you want me to file a bug instead to be more formal and then resubmit my solution as a new PR?

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.

Jest tests failed after upgrading axios to v1.1.2
3 participants