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

[todor-a]

Fixes #3130.

[ljharb]

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?

[todor-a]

(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

[ljharb]

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.

[todor-a]

nice, done

[ljharb]

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

[todor-a]

do you mean this:

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

?

[ljharb]

i think more like

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

with prefer-inline set to true?

[todor-a]

ah, got it, will add it

[todor-a]

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

[ljharb]

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

[todor-a]

(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

[codecov]

Codecov Report

Attention: Patch coverage is 10.52632% with 17 lines in your changes missing coverage. Please review.

Project coverage is 91.74%. Comparing base (01c9eb0) to head (44bd80a).

Files with missing lines	Patch %	Lines
src/rules/no-duplicates.js	10.52%	17 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3194      +/-   ##
==========================================
+ Coverage   82.25%   91.74%   +9.48%     
==========================================
  Files          94       83      -11     
  Lines        4283     3707     -576     
  Branches     1478     1340     -138     
==========================================
- Hits         3523     3401     -122     
+ Misses        760      306     -454     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.