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

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

Closed
G-Rath opened this issue Jan 7, 2020 · 32 comments · Fixed by #14238
Closed

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

G-Rath opened this issue Jan 7, 2020 · 32 comments · Fixed by #14238
Assignees
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 good first issue Good for people who haven't worked on ESLint before help wanted The team would welcome a contribution from the community for this issue rule Relates to ESLint's core rules
Projects

Comments

@G-Rath
Copy link
Contributor

G-Rath commented Jan 7, 2020

What rule do you want to change?
no-duplicate-imports

Does this change cause the rule to produce more or fewer warnings?
Reduces warnings.

How will the change be implemented? (New option, new default behavior, etc.)?
Behavioural change, which could be either option controlled or new default behaviour.
Prior art: TSLint.

Please provide some example code that this change will affect:

import * as parsers from '@src/parsers';
import { SpecificParser } from '@src/parsers';

What does the rule currently do for this code?
Marks the imports as duplicates

What will the rule do after it's changed?
Not mark the imports as duplicates

Are you willing to submit a pull request to implement this change?
Yes


The primary use I have for the above is with mocking in jest, such as here where I want to both use the parsers & also spy on them to test that specific ones are called.

While I can use destructuring, or reference off parsers, the former won't be understood by auto-importers (i.e TS & IDE), and the latter would be inconsistent to how I use the functions in the rest of the codebase.

@G-Rath G-Rath added enhancement This change enhances an existing feature of ESLint rule Relates to ESLint's core rules triage An ESLint team member will look at this issue soon labels Jan 7, 2020
@mdjermanovic mdjermanovic added evaluating The team will evaluate this issue to decide whether it meets the criteria for inclusion and removed triage An ESLint team member will look at this issue soon labels Jan 8, 2020
@mdjermanovic
Copy link
Member

mdjermanovic commented Jan 8, 2020

I think this is reasonable since these two cannot be merged into one, so the rule at the moment indirectly disallows named imports if the namespace is imported.

This could be indeed user's preference, but I'm not sure that such restriction was the intention of this particular rule.

Maybe this can be treated as a bug and become a new default behavior instead of an option?

@G-Rath
Copy link
Contributor Author

G-Rath commented Jan 8, 2020

Maybe this can be treated as a bug and become a new default behavior instead of an option?

I've got no major preference so your call; If released as a bug fix / default behaviour change it shouldn't have any negative impact since it's fixing what is currently an error.

Worst case we'll have someone in 6 months asking if no-duplicate-imports can support treating namespace & named imports as the same, and it should be fairly easy to add an option for that if desired.

Either way, the actual fix implementation is mostly uneffected by if it's controlled by an option or not (it just changes the amount of code that's in the rule). If people are happy, I'll get started on a PR later this week :)

@mdjermanovic
Copy link
Member

mdjermanovic commented Jan 8, 2020

If people are happy, I'll get started on a PR later this week :)

Let's wait for other opinions from the team, and for consensus about the new option if this isn't a bug.

While at the subject, what should be the behavior in cases such as these:

import * as parsers1 from 'parsers';
import * as parsers2 from 'parsers';
// these two can be merged
import * as parsers from 'parsers';
import DefaultParser from 'parsers';
import DefaultParser, * as parsers from 'parsers';
import { SpecificParser } from 'parsers';

@G-Rath
Copy link
Contributor Author

G-Rath commented Jan 8, 2020

Let's wait for other opinions from the team, and for consensus about the new option if this isn't a bug.

👍

While at the subject, what should be the behavior in cases such as these:

The first I class as an actual duplicate that can be fixed by replacing the second with const parsers2 = parsers1, since there shouldn't be any difference in what's imported. (similar to no-useless-rename`)
The second is mergeable, and so a duplicate.
The third not mergeable, and so not a duplicate.


I think in general the behaviour of no-duplicate-imports should be "an import that can be merged with another is a duplicate of that other".

@kaicataldo
Copy link
Member

kaicataldo commented Jan 9, 2020

I think in general the behaviour of no-duplicate-imports should be "an import that can be merged with another is a duplicate of that other".

This makes sense as a heuristic to me. 👍

@mdjermanovic
Copy link
Member

mdjermanovic commented Jan 10, 2020

The first I class as an actual duplicate that can be fixed by replacing the second with const parsers2 = parsers1, since there shouldn't be any difference in what's imported. (similar to no-useless-rename`)

That's reasonable, but following the same logic the original example can be replaced with const { SpecificParser } = parsers ? Also, this behavior wouldn't match "an import that can be merged with another is a duplicate of that other".

Not saying that it shouldn't be reported as a duplicate, just trying to find out what makes the most sense as the responsibility of this rule.

@G-Rath
Copy link
Contributor Author

G-Rath commented Jan 10, 2020

That's reasonable, but following the same logic the original example can be replaced with const { SpecificParser } = parsers ?

Probably (I've not tried, but I don't think the transpiling would change it enough to cause that not to work), but it would mean I'd have to rename if I was already importing that specific parser.

Also, this behavior wouldn't match "an import that can be merged with another is a duplicate of that other".

Actually I think you could argue it does, because those imports can be merged by renaming them to share the same name.

I think what you're after here is a heuristic for what "can be merged" means, which I'd say is:

An import is considered mergable with another if the act of merging does not require the creation of extra nodes.

Then, with that defined, this rule takes on the heuristic for its responsibility to be:

An import that can be merged with another import is a duplicate of that other.

So, in applying this we get:

// mergable, as parsers1 & parsers2 hold the same reference, and so their identifiers can be replaced.
import * as parsers1 from 'parsers';
import * as parsers2 from 'parsers';

// mergable, as they can be combined without changing behaviour & remaining syntactically valid.
import defaultValue from 'parsers';
import { anotherValue } from 'parsers';

// mergable as they will both trigger any side effects the module has.
import * as parsers2 from 'parsers';
import from 'parsers';

// not mergable, as they would require new nodes to be created.
import { anotherValue } from 'parsers';
import * as parsers1 from 'parsers';

Personally, I also think that it's a good thing to encourage not duplicating such imports, as (based on my understanding on the specification for modules) there is completely no reason to do so - it's effectively the namespace form of no-useless-rename.

Overall, I totally agree that this is an interesting question, but I don't think it's big enough to be worth the bikeshed given it's very easy to implement an option for later 🙂

I do think that the fixer for merging the last case should be a suggestion, since it will cause other to be invalid.

@ljharb
Copy link
Sponsor Contributor

ljharb commented Jan 25, 2020

imo this isn't an enhancement, it's a bug, and should be marked as such.

@nzakas nzakas added bug ESLint is working incorrectly and removed enhancement This change enhances an existing feature of ESLint labels Feb 13, 2020
@nzakas
Copy link
Member

nzakas commented Feb 13, 2020

I agree, this seems like a bug.

@kaicataldo kaicataldo added accepted There is consensus among the team that this change meets the criteria for inclusion and removed evaluating The team will evaluate this issue to decide whether it meets the criteria for inclusion labels Feb 16, 2020
@kaicataldo
Copy link
Member

kaicataldo commented Feb 16, 2020

Marking as accepted since three team members agree that this is a bug.

@kaicataldo kaicataldo added help wanted The team would welcome a contribution from the community for this issue good first issue Good for people who haven't worked on ESLint before labels Feb 16, 2020
@JoaoDsv
Copy link

JoaoDsv commented Feb 18, 2020

Working on this issue.

@JoaoDsv
Copy link

JoaoDsv commented Feb 18, 2020

Solved in this PR. If someone can have a look 🙏

@nzakas nzakas linked a pull request Feb 18, 2020 that will close this issue
2 tasks
@eslint-deprecated eslint-deprecated bot added the auto closed The bot closed this issue label Apr 8, 2020
@eslint-deprecated
Copy link

eslint-deprecated bot commented Apr 8, 2020

Unfortunately, it looks like there wasn't enough interest from the team
or community to implement this change. While we wish we'd be able to
accommodate everyone's requests, we do need to prioritize. We've found
that accepted issues failing to be implemented after 90 days tend to
never be implemented, and as such, we close those issues.
This doesn't mean the idea isn't interesting or useful, just that it's
not something the team can commit to.

Thanks for contributing to ESLint and we appreciate your understanding.

@eslint-deprecated eslint-deprecated bot added the auto closed The bot closed this issue label May 16, 2020
@eslint-deprecated
Copy link

eslint-deprecated bot commented May 16, 2020

Unfortunately, it looks like there wasn't enough interest from the team
or community to implement this change. While we wish we'd be able to
accommodate everyone's requests, we do need to prioritize. We've found
that accepted issues failing to be implemented after 90 days tend to
never be implemented, and as such, we close those issues.
This doesn't mean the idea isn't interesting or useful, just that it's
not something the team can commit to.

Thanks for contributing to ESLint and we appreciate your understanding.

@ljharb
Copy link
Sponsor Contributor

ljharb commented May 16, 2020

This remains a bug with an open PR; please reopen. cc @mdjermanovic

@mdjermanovic mdjermanovic removed the auto closed The bot closed this issue label May 16, 2020
@mdjermanovic mdjermanovic reopened this May 16, 2020
@nzakas
Copy link
Member

nzakas commented Jun 19, 2020

Per #12767 (comment), we should also address #12760 in this issue.

@boutahlilsoufiane
Copy link
Contributor

boutahlilsoufiane commented Jan 17, 2021

I'm going to do this, give me two weeks.

@nzakas
Copy link
Member

nzakas commented Jan 19, 2021

@boutahlilsoufiane great, thanks!

@misfitonie
Copy link

misfitonie commented Jan 19, 2021

Hi, if this ticket is still open, i'll be happy to work on it :)

@nzakas
Copy link
Member

nzakas commented Jan 20, 2021

@misfitonie we do have a volunteer (@boutahlilsoufiane) so let’s wait for now unless you two want to decide between yourselves who wants to work on this.

@misfitonie
Copy link

misfitonie commented Jan 25, 2021

@boutahlilsoufiane do you need some help ? Or you just need time ?

@boutahlilsoufiane
Copy link
Contributor

boutahlilsoufiane commented Jan 25, 2021

@misfitonie thank you for proposing your help, I need just time.

@boutahlilsoufiane
Copy link
Contributor

boutahlilsoufiane commented Feb 20, 2021

After spending too many hours trying to figure out how to run eslint project to test my changes in lib\rules\no-duplicate-imports.js, I didn't find anything. I tried this command node ./bin/eslint.js test.js.

test.js:

import * as parsers1 from 'parsers';
import * as parsers2 from 'parsers';

The eslint report these errors:

  1:1   error  Import and export declarations are not supported yet      node/no-unsupported-features/es-syntax
  1:10  error  'StringCursor' is defined but never used              no-unused-vars
  1:24  error  'parseTFAttribute' is defined but never used          no-unused-vars
  1:48  error  Strings must use doublequote                          quotes
  1:48  error  "@src/parser" is not found                            node/no-missing-import
  2:1   error  Import and export declarations are not supported yet  node/no-unsupported-features/es-syntax
  2:10  error  'TFNodeType' is defined but never used                no-unused-vars
  2:28  error  Strings must use doublequote                          quotes
  2:28  error  "@src/types" is not found                             node/no-missing-import
  2:41  error  Newline required at end of file but not found         eol-last

I need a way to test my code, I'm stuck!!, I will appreciate any help

@mdjermanovic
Copy link
Member

mdjermanovic commented Feb 23, 2021

Hi @boutahlilsoufiane!

The tests for this change should be added to tests/lib/rules/no-duplicate-imports.js

Command to run the tests: npm run test:cli tests/lib/rules/no-duplicate-imports.js

You can find more details about the tests here:

@boutahlilsoufiane
Copy link
Contributor

boutahlilsoufiane commented Mar 7, 2021

Hi mdjermanovic!
Thank you, I can now test my code.

@SimonAlling
Copy link

SimonAlling commented Mar 18, 2021

Could be worth mentioning that TSLint has/had an option for this; maybe we can reuse their implementation or at least use it as inspiration?

Config

“allow-namespace-imports” allows you to import namespaces on separate lines.

Config examples

"no-duplicate-imports": [true, {"allow-namespace-imports": true}]

Schema

{
  "type": "object",
  "properties": {
    "allow-namespace-imports": {
      "type": "boolean"
    }
  }
}

@nzakas nzakas added this to Needs Triage in Triage via automation Mar 25, 2021
@nzakas nzakas moved this from Needs Triage to Pull Request Opened in Triage Mar 25, 2021
@boutahlilsoufiane
Copy link
Contributor

boutahlilsoufiane commented May 19, 2021

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

@boutahlilsoufiane
Copy link
Contributor

boutahlilsoufiane commented May 26, 2021

Hi @mdjermanovic, could you review the PR, please?

Triage automation moved this from Pull Request Opened to Complete Jun 4, 2021
mdjermanovic pushed a commit that referenced this issue Jun 4, 2021
…) (#14238)

* Fix: ignore unmergable imports when checking no-duplicate-imports (fixes #13180) & Fix: Ignore re-export all in no-duplicate-imports (fixes #12760)

* Fix: ignore unmergable imports when checking no-duplicate-imports (fixes #13180) & Fix: Ignore re-export all in no-duplicate-imports (fixes #12760)

* Fix: ignore unmergable imports when checking no-duplicate-imports (fixes #13180) & Fix: Ignore re-export all in no-duplicate-imports (fixes #12760)

* Fix: ignore unmergable imports when checking no-duplicate-imports (fixes #13180) & Fix: Ignore re-export all in no-duplicate-imports (fixes #12760)

* Fix: ignore unmergable imports when checking no-duplicate-imports (fixes #13180) & Fix: Ignore re-export all in no-duplicate-imports (fixes #12760)

* Fix: ignore unmergable imports when checking no-duplicate-imports (fixes
 #13180) & Fix: Ignore re-export all in no-duplicate-imports (fixes #12760)

* Fix: ignore unmergable imports when checking no-duplicate-imports (fixes
 #13180) & Fix: Ignore re-export all in no-duplicate-imports (fixes #12760)

* Fix: ignore unmergable imports when checking no-duplicate-imports (fixes
 #13180) & Fix: Ignore re-export all in no-duplicate-imports (fixes #12760)

* Fix: ignore unmergable imports when checking no-duplicate-imports (fixes #13180) & Fix: Ignore re-export all in no-duplicate-imports (fixes #12760)
@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 good first issue Good for people who haven't worked on ESLint before help wanted The team would welcome a contribution from the community for this issue rule Relates to ESLint's core rules
Projects
Triage
Complete
10 participants