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: no-duplicate-imports allow unmergeable (fixes #12758, fixes #12760) #14238
Fix: no-duplicate-imports allow unmergeable (fixes #12758, fixes #12760) #14238
Conversation
Hi @boutahlilsoufiane!, thanks for the Pull Request The pull request title isn't properly formatted. We ask that you update the message to match this format, as we use it to generate changelogs and automate releases.
Read more about contributing to ESLint here |
Hi @boutahlilsoufiane!, thanks for the Pull Request The pull request title isn't properly formatted. We ask that you update the message to match this format, as we use it to generate changelogs and automate releases.
Read more about contributing to ESLint here |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@boutahlilsoufiane thanks for the PR!
Can you fix the linting errors, so we could take a look at the code?
npm run lint
or npm test
can show the errors locally.
Hi @mdjermanovic thanks for reviewing the PR, I appreciate your help. |
Hi @mdjermanovic can you please review the PR! |
92fc460
to
4c07c89
Compare
Hi @mdjermanovic, I worked on your notes, could you review the PR, please? |
eslint#13180) & Fix: Ignore re-export all in no-duplicate-imports (fixes eslint#12760)
eslint#13180) & Fix: Ignore re-export all in no-duplicate-imports (fixes eslint#12760)
eslint#13180) & Fix: Ignore re-export all in no-duplicate-imports (fixes eslint#12760)
eslint#13180) & Fix: Ignore re-export all in no-duplicate-imports (fixes eslint#12760)
eslint#13180) & Fix: Ignore re-export all in no-duplicate-imports (fixes eslint#12760)
Hi @mdjermanovic and @ljharb thanks for reviewing the PR. |
096c75b
to
7bb7a50
Compare
lib/rules/no-duplicate-imports.js
Outdated
* @returns {boolean} True if (import|export) type is belong to (ImportSpecifier|ExportSpecifier) or (ImportNamespaceSpecifier|ExportNamespaceSpecifier) and false if it doesn't. | ||
*/ | ||
function isImportExportSpecifier(importExportType, type) { | ||
const namedTypes = ["ImportSpecifier", "ExportSpecifier"]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These two arrays can be defined at module level, so they’re not created every time this function runs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for taking the time to review the PR. I created these two arrays at the module level in section Helpers.
"import os from \"os\";\nexport { something } from \"os\";", | ||
{ | ||
code: "import os from \"os\";\nexport { hello } from \"hello\";", | ||
'import os from "os";\nimport fs from "fs";', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it’s hard to review these test diffs with the unrelated style changes. can they be reverted?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I reverted the file, I think you could now see diffs between commits.
eslint#13180) & Fix: Ignore re-export all in no-duplicate-imports (fixes eslint#12760)
eslint#13180) & Fix: Ignore re-export all in no-duplicate-imports (fixes eslint#12760)
eslint#13180) & Fix: Ignore re-export all in no-duplicate-imports (fixes eslint#12760)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code looks good! I left some suggestions about the documentation, jsdoc, and tests.
eslint#13180) & Fix: Ignore re-export all in no-duplicate-imports (fixes eslint#12760)
Thank you so much @mdjermanovic for your comments, I think I've fixed everything. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks!
@boutahlilsoufiane if you can drop me your email address (either sent a message to contact (at) eslint (dot) org or ping me on Discord), we'd like to thank you for this pull request. |
Hi @nzakas, I sent a message to contact (at) eslint (dot) org. |
Hi @mdjermanovic, thank you so much for reviewing and accepting the PR. |
Thanks for contributing! |
Prerequisites checklist
What is the purpose of this pull request? (put an "X" next to an item)
[X] Documentation update
[X] Bug fix (template)
What changes did you make? (Give an overview)
Fix the issue here: #12758
This PR fixes a bug in the rule no-duplicate-imports and follows this heuristic An import that can be merged with another import is a duplicate of that other. So for example, if we have specific import and namespace import, these two can’t be merged. We should remove the mark import as duplicate.
Fix the issue here: #12760
This PR fixes also a bug in the rule no-duplicate-imports, it removes the export is duplicate mark from exportAllDeclaration because it can’t be merged with any other import/export, but there is a case when the export can be marked as duplicated, like this example (export * from "mod"; export * from "mod"), the second export is duplicated.