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

[New] import/default: support default export in TSExportAssignment #1689

Merged
merged 1 commit into from
Jun 6, 2020

Conversation

Maxim-Mazurok
Copy link
Contributor

Possible duplicate of #1528 (but uses tsconfig.json)
Aims to fix #1527

For example, I want to import React as import React from "react". I can do this by using esModuleInterop: true in tsconfig.json, but eslint doesn't know about that.

This is a draft PR as I'm not really familiar with the codebase. I want to share my approach and receive feedback.

@coveralls
Copy link

coveralls commented Mar 19, 2020

Coverage Status

Coverage decreased (-0.03%) to 97.708% when pulling 0b585a1 on Maxim-Mazurok:master into 0547c7e on benmosher:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.08%) to 97.715% when pulling 80f1a66 on Maxim-Mazurok:master into 1a3a128 on benmosher:master.

5 similar comments
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.08%) to 97.715% when pulling 80f1a66 on Maxim-Mazurok:master into 1a3a128 on benmosher:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.08%) to 97.715% when pulling 80f1a66 on Maxim-Mazurok:master into 1a3a128 on benmosher:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.08%) to 97.715% when pulling 80f1a66 on Maxim-Mazurok:master into 1a3a128 on benmosher:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.08%) to 97.715% when pulling 80f1a66 on Maxim-Mazurok:master into 1a3a128 on benmosher:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.08%) to 97.715% when pulling 80f1a66 on Maxim-Mazurok:master into 1a3a128 on benmosher:master.

@ljharb
Copy link
Member

ljharb commented Jun 2, 2020

@Maxim-Mazurok this seems fine, but is hard to review without tests. Could you add some?

@Maxim-Mazurok
Copy link
Contributor Author

Sure, @ljharb, I will first test if #1527 was actually resolved and then will add some tests.
Also, as I've mentioned here that we should consider the case when there are multiple tsconfig.json files. Anyway, supporting one root config would be a good starting point, I think.

@Maxim-Mazurok
Copy link
Contributor Author

Maxim-Mazurok commented Jun 2, 2020

@ljharb, I've created reproduction repo: https://github.com/Maxim-Mazurok/eslint-plugin-import-1689-reproduction

The issue is still present in 2.20.2 and on master branch.

@ljharb
Copy link
Member

ljharb commented Jun 2, 2020

Thanks for confirming. I'd love to review and land this PR; could you modify your repro case to tests in this repo?

@Maxim-Mazurok
Copy link
Contributor Author

Sure, creating a repro was the first step :) Now I'm going to add tests.

@Maxim-Mazurok Maxim-Mazurok marked this pull request as ready for review June 4, 2020 09:56
@Maxim-Mazurok
Copy link
Contributor Author

@ljharb I fixed my code and added tests, please, take a look.
I also had to fix import ReactDOM from 'react-dom'; case in my last commit to make it work on real-world app, where you import both React and ReactDOM.

Copy link
Member

@ljharb ljharb left a comment

Choose a reason for hiding this comment

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

We have array-includes for includes already; and i added a try/catch around the JSON.parse just in case, and i rebased it.

See comments, tho.

src/ExportMap.js Outdated Show resolved Hide resolved
@ljharb ljharb changed the title feature(import/default): support default export in TSExportAssignment. [New] import/default: support default export in TSExportAssignment Jun 5, 2020
@Maxim-Mazurok
Copy link
Contributor Author

@ljharb thanks for your comments and fixes. I added fix for JSON.parse. Please, don't force-push your commits that overwrite my commits, because I have no visibility as to what was changed by you. I understand that I did more commits than necessary, but you can "squash and merge" at the end to fix this problem. Thanks :)

@Maxim-Mazurok Maxim-Mazurok requested a review from ljharb June 6, 2020 05:20
@ljharb
Copy link
Member

ljharb commented Jun 6, 2020

@Maxim-Mazurok "squash and merge" is not viable - it leaves the PR ref cluttered with the old commits, and orphaned from the master branch.

I'm going to force push this one again before landing it; while you can uncheck the "allow edits" box to prevent me from doing that, that has the unfortunate consequence of meaning I'll never merge the PR on any repo I manage, so I hope you won't do that :-)

@ljharb ljharb merged commit 0b585a1 into import-js:master Jun 6, 2020
@Maxim-Mazurok
Copy link
Contributor Author

Oh, right, I totally forgot that, now it makes sense :)

Thanks!

@ljharb
Copy link
Member

ljharb commented Jun 7, 2020

Thank you for the PR!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

Handle TSExportAssignment exports other than namespaces
3 participants