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: no-duplicate-imports allow unmergeable (fixes #12758, fixes #12760) #14238

Merged
merged 9 commits into from Jun 4, 2021

Conversation

boutahlilsoufiane
Copy link
Contributor

@boutahlilsoufiane boutahlilsoufiane commented Mar 21, 2021

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 eslint-github-bot bot added the triage An ESLint team member will look at this issue soon label Mar 21, 2021
@eslint-github-bot
Copy link

eslint-github-bot bot commented Mar 21, 2021

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

@boutahlilsoufiane 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
@eslint-github-bot
Copy link

eslint-github-bot bot commented Mar 21, 2021

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 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 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 mdjermanovic linked an issue Mar 22, 2021 that may be closed by this pull request
@mdjermanovic 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
Copy link
Member

@mdjermanovic mdjermanovic left a comment

@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
Copy link
Contributor Author

boutahlilsoufiane commented Mar 23, 2021

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

@boutahlilsoufiane
Copy link
Contributor Author

boutahlilsoufiane commented Apr 3, 2021

@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 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 boutahlilsoufiane force-pushed the issue12758&issue12760 branch 4 times, most recently from 92fc460 to 4c07c89 Compare May 9, 2021
@boutahlilsoufiane
Copy link
Contributor Author

boutahlilsoufiane commented May 9, 2021

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

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
lib/rules/no-duplicate-imports.js Outdated Show resolved Hide resolved
lib/rules/no-duplicate-imports.js Outdated Show resolved Hide resolved
@boutahlilsoufiane
Copy link
Contributor Author

boutahlilsoufiane commented May 23, 2021

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

* @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"];
Copy link
Sponsor Contributor

@ljharb ljharb May 23, 2021

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.

Copy link
Contributor Author

@boutahlilsoufiane boutahlilsoufiane May 27, 2021

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";',
Copy link
Sponsor Contributor

@ljharb ljharb May 23, 2021

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?

Copy link
Contributor Author

@boutahlilsoufiane boutahlilsoufiane May 27, 2021

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.

Copy link
Member

@mdjermanovic mdjermanovic left a comment

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

docs/rules/no-duplicate-imports.md Outdated Show resolved Hide resolved
docs/rules/no-duplicate-imports.md Outdated Show resolved Hide resolved
docs/rules/no-duplicate-imports.md Outdated Show resolved Hide resolved
docs/rules/no-duplicate-imports.md Outdated Show resolved Hide resolved
docs/rules/no-duplicate-imports.md 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
tests/lib/rules/no-duplicate-imports.js Show resolved Hide resolved
tests/lib/rules/no-duplicate-imports.js Show resolved Hide resolved
tests/lib/rules/no-duplicate-imports.js Show resolved Hide resolved
@boutahlilsoufiane
Copy link
Contributor Author

boutahlilsoufiane commented May 30, 2021

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

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

@mdjermanovic mdjermanovic left a comment

LGTM, thanks!

@nzakas
Copy link
Member

nzakas commented Jun 3, 2021

@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
Copy link
Contributor Author

boutahlilsoufiane commented Jun 4, 2021

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

@boutahlilsoufiane
Copy link
Contributor Author

boutahlilsoufiane commented Jun 4, 2021

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

@mdjermanovic mdjermanovic merged commit e44ce0a into eslint:master Jun 4, 2021
14 checks passed
@mdjermanovic
Copy link
Member

mdjermanovic commented Jun 4, 2021

Thanks for contributing!

@boutahlilsoufiane boutahlilsoufiane deleted the issue12758&issue12760 branch Jun 5, 2021
@eslint-github-bot eslint-github-bot bot locked and limited conversation to collaborators Dec 2, 2021
@eslint-github-bot 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
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
accepted There is consensus among the team that this change meets the criteria for inclusion archived due to age This issue has been archived; please open a new issue for any further discussion bug ESLint is working incorrectly rule Relates to ESLint's core rules
Projects
None yet
Development

Successfully merging this pull request may close these issues.

no-duplicate-imports should ignore export * [no-duplicate-imports] Treat namespace imports as different
4 participants