Fix: no-duplicate-imports allow unmergeable (fixes #12758, fixes #12760) #14238

boutahlilsoufiane · 2021-03-21T21:50:08Z

Prerequisites checklist

I have read the contributing guidelines.
The team has reached consensus on the changes proposed in this pull request. If not, I understand that the evaluation process will begin with this pull request and won't be merged until the team has reached consensus.

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.

eslint-github-bot · 2021-03-21T21:50:12Z

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.

The commit message tag must be one of the following:

The Tag is one of the following:
- Fix - for a bug fix.
- Update - either for a backwards-compatible enhancement or for a rule change that adds reported problems.
- New - implements a new feature.
- Breaking - for a backwards-incompatible enhancement or feature.
- Docs - changes to documentation only.
- Build - changes to build process only.
- Upgrade - for a dependency upgrade.
- Chore - for anything that isn't user-facing (for example, refactoring, adding tests, etc.).
You can use the labels of the issue you are working on to determine the best tag.
There should be a space following the initial tag and colon, for example 'New: Message'.

Read more about contributing to ESLint here

eslint-github-bot · 2021-03-21T21:51:49Z

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.

The length of the commit message must be less than or equal to 72

Read more about contributing to ESLint here

mdjermanovic

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

boutahlilsoufiane · 2021-03-23T20:22:40Z

Hi @mdjermanovic thanks for reviewing the PR, I appreciate your help.
Now npm run lint is running fine.

boutahlilsoufiane · 2021-04-03T18:28:27Z

@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 can you please review the PR!

lib/rules/no-duplicate-imports.js

boutahlilsoufiane · 2021-05-09T20:44:49Z

Hi @mdjermanovic, I worked on your notes, could you review the PR, please?
Thank you so much.

lib/rules/no-duplicate-imports.js

eslint#13180) & Fix: Ignore re-export all in no-duplicate-imports (fixes eslint#12760)

boutahlilsoufiane · 2021-05-23T13:51:17Z

Hi @mdjermanovic and @ljharb thanks for reviewing the PR.

ljharb · 2021-05-23T15:41:16Z

lib/rules/no-duplicate-imports.js

+ * @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"];


These two arrays can be defined at module level, so they’re not created every time this function runs.

Thank you for taking the time to review the PR. I created these two arrays at the module level in section Helpers.

ljharb · 2021-05-23T15:42:24Z

tests/lib/rules/no-duplicate-imports.js

-        "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";',


it’s hard to review these test diffs with the unrelated style changes. can they be reverted?

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)

mdjermanovic

The code looks good! I left some suggestions about the documentation, jsdoc, and tests.

docs/rules/no-duplicate-imports.md

lib/rules/no-duplicate-imports.js

tests/lib/rules/no-duplicate-imports.js

eslint#13180) & Fix: Ignore re-export all in no-duplicate-imports (fixes eslint#12760)

boutahlilsoufiane · 2021-05-30T01:20:48Z

Thank you so much @mdjermanovic for your comments, I think I've fixed everything.

mdjermanovic

LGTM, thanks!

nzakas · 2021-06-03T21:10:02Z

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

boutahlilsoufiane · 2021-06-04T12:40:06Z

Hi @nzakas, I sent a message to contact (at) eslint (dot) org.

boutahlilsoufiane · 2021-06-04T12:44:30Z

Hi @mdjermanovic, thank you so much for reviewing and accepting the PR.

mdjermanovic · 2021-06-04T15:59:08Z

Thanks for contributing!

eslint-github-bot bot added the triage An ESLint team member will look at this issue soon label Mar 21, 2021

boutahlilsoufiane changed the title ~~Issue12758&issue12760~~ Fix: ignore unmergable imports when checking no-duplicate-imports (fixes #13180) & Fix: Ignore re-export all in no-duplicate-imports (fixes #12760) Mar 21, 2021

mdjermanovic changed the title ~~Fix: ignore unmergable imports when checking no-duplicate-imports (fixes #13180) & Fix: Ignore re-export all in no-duplicate-imports (fixes #12760)~~ Fix: no-duplicate-imports allow unmergeable (fixes #13180, fixes #12760) Mar 22, 2021

mdjermanovic changed the title ~~Fix: no-duplicate-imports allow unmergeable (fixes #13180, fixes #12760)~~ Fix: no-duplicate-imports allow unmergeable (fixes #12758, fixes #12760) Mar 22, 2021

mdjermanovic linked an issue Mar 22, 2021 that may be closed by this pull request

[no-duplicate-imports] Treat namespace imports as different #12758

Closed

mdjermanovic added accepted There is consensus among the team that this change meets the criteria for inclusion bug ESLint is working incorrectly rule Relates to ESLint's core rules and removed triage An ESLint team member will look at this issue soon labels Mar 22, 2021

mdjermanovic mentioned this pull request Mar 22, 2021

Fix: ignore unmergable imports when checking no-duplicate-imports (fixes #13180) #13180

Closed

2 tasks

mdjermanovic requested changes Mar 22, 2021

View reviewed changes

boutahlilsoufiane requested a review from mdjermanovic March 31, 2021 00:08

mdjermanovic requested changes Apr 7, 2021

View reviewed changes

lib/rules/no-duplicate-imports.js Outdated Show resolved Hide resolved

lib/rules/no-duplicate-imports.js Outdated Show resolved Hide resolved

lib/rules/no-duplicate-imports.js Outdated Show resolved Hide resolved

lib/rules/no-duplicate-imports.js Outdated Show resolved Hide resolved

boutahlilsoufiane force-pushed the issue12758&issue12760 branch 4 times, most recently from 92fc460 to 4c07c89 Compare May 9, 2021 17:21

boutahlilsoufiane requested a review from mdjermanovic May 14, 2021 10:59

mdjermanovic requested changes May 19, 2021

View reviewed changes

boutahlilsoufiane added 5 commits May 23, 2021 14:44

Fix: ignore unmergable imports when checking no-duplicate-imports (fixes

e20fab5

eslint#13180) & Fix: Ignore re-export all in no-duplicate-imports (fixes eslint#12760)

Fix: ignore unmergable imports when checking no-duplicate-imports (fixes

8f33a8b

eslint#13180) & Fix: Ignore re-export all in no-duplicate-imports (fixes eslint#12760)

Fix: ignore unmergable imports when checking no-duplicate-imports (fixes

2a54321

eslint#13180) & Fix: Ignore re-export all in no-duplicate-imports (fixes eslint#12760)

Fix: ignore unmergable imports when checking no-duplicate-imports (fixes

1932d2c

eslint#13180) & Fix: Ignore re-export all in no-duplicate-imports (fixes eslint#12760)

Fix: ignore unmergable imports when checking no-duplicate-imports (fixes

7bb7a50

eslint#13180) & Fix: Ignore re-export all in no-duplicate-imports (fixes eslint#12760)

boutahlilsoufiane force-pushed the issue12758&issue12760 branch from 096c75b to 7bb7a50 Compare May 23, 2021 13:54

boutahlilsoufiane requested a review from mdjermanovic May 23, 2021 14:40

ljharb reviewed May 23, 2021

View reviewed changes

boutahlilsoufiane added 3 commits May 27, 2021 00:58

Fix: ignore unmergable imports when checking no-duplicate-imports (fixes

9d3fd4d

eslint#13180) & Fix: Ignore re-export all in no-duplicate-imports (fixes eslint#12760)

Fix: ignore unmergable imports when checking no-duplicate-imports (fixes

f97848b

eslint#13180) & Fix: Ignore re-export all in no-duplicate-imports (fixes eslint#12760)

Fix: ignore unmergable imports when checking no-duplicate-imports (fixes

d515f8d

eslint#13180) & Fix: Ignore re-export all in no-duplicate-imports (fixes eslint#12760)

mdjermanovic requested changes May 29, 2021

View reviewed changes

Fix: ignore unmergable imports when checking no-duplicate-imports (fixes

af199ef

eslint#13180) & Fix: Ignore re-export all in no-duplicate-imports (fixes eslint#12760)

mdjermanovic linked an issue May 30, 2021 that may be closed by this pull request

no-duplicate-imports should ignore export * #12760

Closed

mdjermanovic approved these changes May 30, 2021

View reviewed changes

mdjermanovic merged commit e44ce0a into eslint:master Jun 4, 2021

boutahlilsoufiane deleted the issue12758&issue12760 branch June 5, 2021 12:40

saberduck mentioned this pull request Jun 23, 2021

Update dependencies SonarSource/SonarJS#2675

Closed

This was referenced Aug 12, 2021

Update dependencies SonarSource/SonarJS#2757

Merged

Upgrade typescript-eslint, babel and eslint dependencies SonarSource/SonarJS#2753

Closed

eslint-github-bot bot locked and limited conversation to collaborators Dec 2, 2021

eslint-github-bot bot added the archived due to age This issue has been archived; please open a new issue for any further discussion label Dec 2, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix: no-duplicate-imports allow unmergeable (fixes #12758, fixes #12760) #14238

Fix: no-duplicate-imports allow unmergeable (fixes #12758, fixes #12760) #14238

boutahlilsoufiane commented Mar 21, 2021

eslint-github-bot bot commented Mar 21, 2021

eslint-github-bot bot commented Mar 21, 2021

mdjermanovic left a comment

boutahlilsoufiane commented Mar 23, 2021 •

edited

boutahlilsoufiane commented Apr 3, 2021 •

edited

boutahlilsoufiane commented May 9, 2021 •

edited

boutahlilsoufiane commented May 23, 2021

ljharb May 23, 2021

boutahlilsoufiane May 27, 2021

ljharb May 23, 2021

boutahlilsoufiane May 27, 2021 •

edited

mdjermanovic left a comment

boutahlilsoufiane commented May 30, 2021

mdjermanovic left a comment

nzakas commented Jun 3, 2021

boutahlilsoufiane commented Jun 4, 2021

boutahlilsoufiane commented Jun 4, 2021 •

edited

mdjermanovic commented Jun 4, 2021

Fix: no-duplicate-imports allow unmergeable (fixes #12758, fixes #12760) #14238

Fix: no-duplicate-imports allow unmergeable (fixes #12758, fixes #12760) #14238

Conversation

boutahlilsoufiane commented Mar 21, 2021

Prerequisites checklist

What is the purpose of this pull request? (put an "X" next to an item)

What changes did you make? (Give an overview)

eslint-github-bot bot commented Mar 21, 2021

eslint-github-bot bot commented Mar 21, 2021

mdjermanovic left a comment

Choose a reason for hiding this comment

boutahlilsoufiane commented Mar 23, 2021 • edited

boutahlilsoufiane commented Apr 3, 2021 • edited

boutahlilsoufiane commented May 9, 2021 • edited

boutahlilsoufiane commented May 23, 2021

ljharb May 23, 2021

Choose a reason for hiding this comment

boutahlilsoufiane May 27, 2021

Choose a reason for hiding this comment

ljharb May 23, 2021

Choose a reason for hiding this comment

boutahlilsoufiane May 27, 2021 • edited

Choose a reason for hiding this comment

mdjermanovic left a comment

Choose a reason for hiding this comment

boutahlilsoufiane commented May 30, 2021

mdjermanovic left a comment

Choose a reason for hiding this comment

nzakas commented Jun 3, 2021

boutahlilsoufiane commented Jun 4, 2021

boutahlilsoufiane commented Jun 4, 2021 • edited

mdjermanovic commented Jun 4, 2021

boutahlilsoufiane commented Mar 23, 2021 •

edited

boutahlilsoufiane commented Apr 3, 2021 •

edited

boutahlilsoufiane commented May 9, 2021 •

edited

boutahlilsoufiane May 27, 2021 •

edited

boutahlilsoufiane commented Jun 4, 2021 •

edited