Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork1.6k
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.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
base:main
Are you sure you want to change the base?
Uh oh!
There was an error while loading.Please reload this page.
Conversation
src/rules/no-duplicates.js Outdated
| 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; | ||
| } | ||
| } |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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?
constkinds=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 usehttps://npmjs.com/object.groupby instead, but that's still slightly more efficient)
and an even simpler approach:
varhasType=false;varhasSideEffect=false;varhasOther=false;for(vari=0;!hasOther&&i<nodes.length;i+=1){varnode=nodes[i];if(node.importKind==='type'){hasType=true;}elseif(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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
nice, done
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
bump
tests/src/rules/no-duplicates.js Outdated
| test({ | ||
| code: ` | ||
| import type { A } from 'a'; | ||
| import 'a'; | ||
| `, | ||
| options: [{ 'prefer-inline': true }], | ||
| ...parserConfig, | ||
| }), |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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';?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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 :-)
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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 supportimport type * as
https://astexplorer.net/#/gist/a676569326155f55fa15da7fd791ffda/latest
codecovbot commentedJun 25, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@## main #3194 +/- ##===========================================+ Coverage 82.25% 95.79% +13.54%=========================================== Files 94 83 -11 Lines 4283 3712 -571 Branches 1478 1345 -133 ===========================================+ Hits 3523 3556 +33+ Misses 760 156 -604 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Uh oh!
There was an error while loading.Please reload this page.
Fixes#3130,#3195.