Skip to content
This repository has been archived by the owner on Mar 25, 2021. It is now read-only.

New rule: no-duplicate-imports #2869

Closed
adidahiya opened this issue Jun 2, 2017 · 10 comments · Fixed by #3075
Closed

New rule: no-duplicate-imports #2869

adidahiya opened this issue Jun 2, 2017 · 10 comments · Fixed by #3075

Comments

@adidahiya
Copy link
Contributor

Just like http://eslint.org/docs/rules/no-duplicate-imports

bad:

import { merge } from 'module';
import something from 'another-module';
import { find } from 'module';

good:

import { merge, find } from 'module';
import something from 'another-module';
@chances
Copy link

chances commented Aug 9, 2017

In some cases you cannot simply merge imports.

Why should this cause a linting error?

If they're impossible to merge then that's not an error.

@IllusionMH
Copy link
Contributor

@chances can you provide examples when they cannot be merged?
If it is common case - might be added as rule option or may not be included into recommended config (not it is only in all and latest).

In most cases it may be causes by copy/paste or bugs in auto-imports.
Rule helps to avoid such error cases. exceptional cases can be handled with inline disable comments or rule can be disabled in your configuration.

@ajafff
Copy link
Contributor

ajafff commented Aug 9, 2017

That's a quote from my PR. Here the example:

import foo from 'foo';
import foo2 from 'foo'; // cannot be combined with preceding import

import * as bar from 'bar';
import {baz} from 'bar'; // cannot be combined with preceding import

If you find something like that in your code, it should be refactored anyway.
"cannot simply be merged" means that the autofixer would need to rename all references to the merged import in the file.

@buu700
Copy link
Contributor

buu700 commented Aug 11, 2017

@ajafff, how is it recommended to refactor those cases? Right now I have a case where I need both bar and baz, and the only workaround I can see is to either disable this rule or use bar.baz everywhere (which would work, but just be a little verbose in this case). Specifically, a library I'm using exports bar as a class and baz as an interface.

It seems like it'd make more sense for non-fixable instances to be treated by the rule as valid.

@IllusionMH
Copy link
Contributor

Cases mentioned above looks like lazy copy/paste. Both cases can be normalized with hands and I don't see reason to leave them as separate imports.

@buu700 if bar.baz is too verbose, that what reason for * as barin first place? Only to export it?
Otherwise it is better to use several named imports ({anything, baz}) from this module or stick to bar.anything for consistency.

@buu700
Copy link
Contributor

buu700 commented Aug 11, 2017

As mentioned, I need to use both baz and bar (as a class), which is because the imported module is a JS library that does things that would be silly/unexpected in idiomatic TypeScript.

bar.baz is basically okay, and is the solution I ended up going with, but there are two reasons I don't really like it:

  1. Other classes reference baz directly, so the fact that this class is using bar.baz all over the place is a little inconsistent.

  2. The slight extra verbosity has turned some lines that would've each been one line into groups of three lines due to our configured max line length.

Not the end of the world, but in this case I'm not sure it's obvious that the rule helped more than harmed.

@ajafff
Copy link
Contributor

ajafff commented Aug 12, 2017

You could use a local alias:

import * as bar from 'bar';
import baz = bar.baz;

@buu700
Copy link
Contributor

buu700 commented Aug 14, 2017

Yeah, I considered that as well, but I think the version of this that triggers the error still isn't obviously worse than any of the alternatives. Usually when a lint rule fails there's a direct or recommended fix that's a clear improvement, but in this case it seems like all the possible workarounds are just to avoid triggering it on a technicality.

Maybe this could be allowed via an option similar to quotemark's avoid-escape option?

HyphnKnight pushed a commit to HyphnKnight/tslint that referenced this issue Apr 9, 2018
@GabeAtWork
Copy link

Are there any plans to implement autofix for this rule?

@JoshuaKGoldberg
Copy link
Contributor

@GabeAtWork no, but only because nobody has filed a issue asking for it. Good idea! We should discuss in a separate issue in case their are off edge cases to hammer out.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants