Skip to content

fix(no-duplicate): fix no-duplicate false positive on side-effect + type imports #3194

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.

Sign up for GitHub

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

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
+86 −15
Open

fix(no-duplicate): fix no-duplicate false positive on side-effect + type imports #3194

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
4045eed
fix
todor-a Jun 25, 2025
7615747
apply suggested change
todor-a Jun 29, 2025
f7cd8bd
add has default
todor-a Jul 1, 2025
44bd80a
fix
todor-a Jul 1, 2025
0f812e4
remove duplicate test case
todor-a Jul 2, 2025
0afe310
extract to function
todor-a Jul 2, 2025
6ab02d0
fix
todor-a Jul 2, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 12 additions & 0 deletions src/rules/no-duplicates.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -261,8 +261,20 @@ function getFix(first, rest, sourceCode, context) {

/** @type {(imported: Map<string, import('estree').ImportDeclaration[]>, context: import('eslint').Rule.RuleContext) => void} */
function checkImports(imported, context) {
const preferInline = context.options[0] && context.options[0]['prefer-inline'];

for (const [module, nodes] of imported.entries()) {
if (nodes.length > 1) {
if (preferInline) {
const typeImports = nodes.filter((node) => node.importKind === 'type');
const sideEffectImports = nodes.filter((node) => node.specifiers.length === 0);
const valueImports = nodes.filter((node) => !typeImports.includes(node) && !sideEffectImports.includes(node));

if (typeImports.length > 0 && sideEffectImports.length > 0 && valueImports.length === 0) {
continue;
}
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the way this is written, it is literally always true, because nodes.length is > 1, and everything in it is in one of the three groups (since the "value" condition is an "else"). is that intentional?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(since the "value" condition is an "else")

not sure i got this. my thinking was - if there are duplicates and one of them is a type import, while the other is a side effect, do not err

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ahhhhh you're right, this is === 0. ok, so it seems like maybe this would work as well?

const kinds = Object.groupBy(nodes, (node) => node.importKind === 'type' ? 'type' : node.specifiers.length === 0 ? 'sideEffect' : 'other');
if (kinds.type.length > 0 && kinds.sideEffect.length > 0 && kinds.other.length === 0) {
  continue;
}

(ofc Object.groupBy would need to use https://npmjs.com/object.groupby instead, but that's still slightly more efficient)

and an even simpler approach:

var hasType = false;
var hasSideEffect = false;
var hasOther = false;
for (var i = 0; !hasOther && i < nodes.length; i += 1) {
  var node = nodes[i];
  if (node.importKind === 'type') {
    hasType = true;
  } else if (node.specifiers.length === 0) {
    hasSideEffect = true;
  } else {
    hasOther = true;
  }
}
if (!hasOther && hasType && hasSideEffect) {
  continue;
}

that way uses no deps, breaks as soon as it finds an invalidating condition, and avoids creating three arrays.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice, done

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

bump


const message = `'${module}' imported multiple times.`;
const [first, ...rest] = nodes;
const sourceCode = getSourceCode(context);
Expand Down
8 changes: 8 additions & 0 deletions tests/src/rules/no-duplicates.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -552,6 +552,14 @@ context('TypeScript', function () {
code: "import { type x } from './foo'; import type y from 'foo'",
...parserConfig,
}),
test({
code: `
import type { A } from 'a';
import 'a';
`,
options: [{ 'prefer-inline': true }],
...parserConfig,
}),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

let's also add a similar invalid test, with output?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do you mean this:

import { A } from 'a';
import 'a';

?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i think more like

import type { A } from 'a';
import B from 'a';

with prefer-inline set to true?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah, got it, will add it

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This fails but I think for the wrong reasons and is a different issue. It currently suggests:

import B, type { A } from 'a';

which is syntactically wrong

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree, I think that's part of #3195 perhaps? It'd be cool to fix that too here :-)

Copy link
Author

@todor-a todor-a Jul 1, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(sorry this is moving so slow)

i pushed a fix that allows for this condition, let me know what you think. also pushed a fix for point 2 from the linked issue.

taking a look at the failing test now

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

pushed a fix for both issues in #3195

had to disable one of the tests if the parser is babel because apparently it does not support import type * as

https://astexplorer.net/#/gist/a676569326155f55fa15da7fd791ffda/latest

]);

const invalid = [
Expand Down
Loading