Skip to content

fix(eslint-plugin): [no-import-type-side-effects] correctly ignore zero-specifier imports #6444

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

Merged
merged 1 commit into from
Feb 9, 2023
+5 −0
Merged

fix(eslint-plugin): [no-import-type-side-effects] correctly ignore zero-specifier imports #6444

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
4 changes: 4 additions & 0 deletions packages/eslint-plugin/src/rules/no-import-type-side-effects.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,10 @@ export default util.createRule<Options, MessageIds>({
'ImportDeclaration[importKind!="type"]'(
node: TSESTree.ImportDeclaration,
): void {
if (node.specifiers.length === 0) {
return;
}
Comment on lines 29 to +34
Copy link
Member

Choose a reason for hiding this comment

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

Per our recent conversation somewhere, if you wanted to, ESQuery can reduce this code count a bit 😄

Suggested change
'ImportDeclaration[importKind!="type"]'(
node: TSESTree.ImportDeclaration,
): void {
if (node.specifiers.length === 0) {
return;
}
'ImportDeclaration[importKind!="type"][specifiers.0]'(
node: TSESTree.ImportDeclaration,
): void {

Copy link
Member Author

@bradzacher bradzacher Feb 9, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

back in the day I was all in on esquery selectors. I thought it was great to dump everything into a selector and keep the code logic about processing the node.

now a days though I tend to feel that leaning too heavily into the custom DSL makes code less accessible and harder to understand / debug.
[specifiers.0] is one of those parts of the DSL that's super unclear as to what it's doing, IMO.

I think leveraging super clear parts of the DSL (prop existence, parents) are great and easy to understand, but for anything more complex it's better to write the clearer logic.

also as an interesting aside just because...
I figured out that selectors actually hide performance cost of rules! ESLint only tracks (1) time spent in create and (2) time spent in selector functions for a rule.

so time spent compiling and evaluation selector strings is perf cost in ESLint land - not for the rule.

Which is funny for rules like those in eslint-plugin-unicorn which heavily (ab)use selectors over logic, because they'll look like they're faster than other rules!

Copy link
Member

Choose a reason for hiding this comment

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

Ooh I wonder if we should have a lint rule for selectors. And maybe put it in eslint-plugin-eslint-plugin...

Copy link
Member Author

@bradzacher bradzacher Feb 9, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Separately I've also envisioned a lint rule that enforces the type annotation on the function arg matches the selector to improve safety.

EG

// bad
'ImportDeclaration[importKind != "type"]'(node: TSESTree.Literal)
'BlockStatement > *'(node: TSESTree.Literal)

// good
'ImportDeclaration[importKind != "type"]'(node: TSESTree.ImportDeclaration)
'ImportDeclaration[importKind != "type"]'(node: TSESTree.ImportDeclaration & {importKind: 'type'})
'BlockStatement > *'(node: TSESTree.Node)

but such a rule would be unnecessary if we build template literal types... hmmmm

Copy link
Member

Choose a reason for hiding this comment

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


const specifiers: TSESTree.ImportSpecifier[] = [];
for (const specifier of node.specifiers) {
if (
Expand Down
1 change: 1 addition & 0 deletions packages/eslint-plugin/tests/rules/no-import-type-side-effects.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ ruleTester.run('no-import-type-side-effects', rule, {
"import type T, { U } from 'mod';",
"import T, { type U } from 'mod';",
"import type * as T from 'mod';",
"import 'mod';",
],
invalid: [
{
Expand Down