feat(eslint-plugin): add consistent-type-imports rule
#2367
Conversation
|
Thanks for the PR, @ota-meshi! typescript-eslint is a 100% community driven project, and we are incredibly grateful that you are contributing to that community. The core maintainers work on this in their personal time, so please understand that it may not be possible for them to review your work immediately. Thanks again! 🙏 Please, if you or your company is finding typescript-eslint valuable, help us sustain the project by sponsoring it transparently on https://opencollective.com/typescript-eslint. As a thank you, your profile/company logo will be added to our main README which receives thousands of unique visitors per day. |
2eaf38b to
182bf9a
Compare
Codecov Report
@@ Coverage Diff @@
## v4 #2367 +/- ##
==========================================
+ Coverage 92.86% 92.93% +0.07%
==========================================
Files 286 287 +1
Lines 9064 9226 +162
Branches 2517 2564 +47
==========================================
+ Hits 8417 8574 +157
- Misses 319 320 +1
- Partials 328 332 +4
Flags with carried forward coverage won't be shown. Click here to find out more.
|
|
|
||
| ## When Not To Use It | ||
|
|
||
| If you specifically want to use both import kinds for stylistic reasons, you can disable this rule. |
There was a problem hiding this comment.
| If you specifically want to use both import kinds for stylistic reasons, you can disable this rule. | |
| - If you are not using TypeScript 3.8 (or greater), then you will not be able to use this rule, as type-only imports are not allowed. | |
| - If you specifically want to use both import kinds for stylistic reasons, you can disable this rule. |
| ```ts | ||
| import type { Foo } from './foo'; | ||
| let foo: Foo; | ||
| ``` | ||
|
|
||
| ```ts | ||
| import { Foo } from './foo'; | ||
| let foo: Foo; | ||
| ``` | ||
|
|
||
| ```ts | ||
| let foo: import('foo').Foo; | ||
| ``` |
There was a problem hiding this comment.
I think remove these examples here, and we can provide all the examples under the specific options
| } | ||
| : { | ||
| // prefer no type imports | ||
| 'ImportDeclaration[importKind=type]'( |
There was a problem hiding this comment.
styling nitpick
| 'ImportDeclaration[importKind=type]'( | |
| 'ImportDeclaration[importKind = "type"]'( |
| const importToken = sourceCode.getFirstToken(node)!; | ||
| return fixer.removeRange([ | ||
| importToken.range[1], | ||
| sourceCode.getTokenAfter(importToken)!.range[1], |
There was a problem hiding this comment.
You can use the second argument to getFirstToken to ensure you fetch a type keyword.
You can also use our nullThrows util to throw with a nice error message if it's not found.
| const importToken = sourceCode.getFirstToken(node)!; | |
| return fixer.removeRange([ | |
| importToken.range[1], | |
| sourceCode.getTokenAfter(importToken)!.range[1], | |
| const importToken = util.nullThrows( | |
| sourceCode.getFirstToken( | |
| node, | |
| token => | |
| token.type === AST_TOKEN_TYPES.Keyword && token.value === 'type', | |
| ), | |
| util.NullThrowsReasons.MissingToken('type', node.type), | |
| ); | |
| return fixer.removeRange([ | |
| importToken.range[1], | |
| sourceCode.getTokenAfter(importToken)!.range[1], |
| let bar: B; | ||
| `, | ||
| output: ` | ||
| import type A, { B } from 'foo'; |
There was a problem hiding this comment.
this is invalid syntax - a type-only import can specify a default OR named import, not both
There was a problem hiding this comment.
I didn't know that. Thank you for teaching me!
| ? { | ||
| // prefer type imports | ||
| 'ImportDeclaration[importKind=value]'( | ||
| node: TSESTree.ImportDeclaration, | ||
| ): void { | ||
| let used = false; | ||
| for (const specifier of node.specifiers) { | ||
| const id = specifier.local; | ||
| const variable = context | ||
| .getScope() | ||
| .variables.find(v => v.name === id.name)!; | ||
| for (const ref of variable.references) { | ||
| if (ref.identifier !== id) { | ||
| referenceIdToDeclMap.set(ref.identifier, node); | ||
| used = true; | ||
| } | ||
| } | ||
| } | ||
| if (used) { | ||
| allValueImports.push(node); | ||
| } | ||
| }, | ||
| 'TSTypeReference Identifier'(node: TSESTree.Identifier): void { | ||
| // Remove type reference ids | ||
| referenceIdToDeclMap.delete(node); | ||
| }, | ||
| 'Program:exit'(): void { | ||
| const usedAsValueImports = new Set(referenceIdToDeclMap.values()); | ||
| for (const valueImport of allValueImports) { | ||
| if (usedAsValueImports.has(valueImport)) { | ||
| continue; | ||
| } | ||
| context.report({ | ||
| node: valueImport, | ||
| messageId: 'typeOverValue', | ||
| fix(fixer) { | ||
| // import type Foo from 'foo' | ||
| // ^^^^^ insert | ||
| const importToken = sourceCode.getFirstToken(valueImport)!; | ||
| return fixer.insertTextAfter(importToken, ' type'); | ||
| }, | ||
| }); | ||
| } | ||
| }, | ||
| } |
There was a problem hiding this comment.
This is a great start! But there are a few problems I can see:
- this code will incorrectly convert
import A, { B } from 'foo'toimport type A, { B } from 'foo'- type-only imports must only have a default OR named imports, not both.
- this code explicitly ignores the case where you have a mixture of type and value imports
- the
TSTypeReference Identifierselector is inaccurate, as it does not handle shadowing:import Type from 'foo'; type T<Type> = Type; // this "Type" shadows the imported "Type", and will false positive
You should be able to lean much harder on the scope analyser here - as I built the scope analyser with some helpers to make it easier to inspect references.
I had a quick play, below is some code to help you. The difficult bit now will writing a resilient fixer:
{
'ImportDeclaration[importKind = "value"]'(
node: TSESTree.ImportDeclaration,
): void {
const variables = context.getDeclaredVariables(node);
const typeVariables: TSESLint.Scope.Variable[] = [];
const valueVariables: TSESLint.Scope.Variable[] = [];
for (const variable of variables) {
const onlyHasTypeReferences = variable.references.every(ref => {
if (ref.isTypeReference) {
return true;
}
if (ref.isValueReference) {
// `type T = typeof foo` will create a value reference because "foo" must be a value type
// however this value reference is safe to use with type-only imports
let parent = ref.identifier.parent;
while (parent) {
if (parent.type === AST_NODE_TYPES.TSTypeQuery) {
return true;
}
// TSTypeQuery must have a TSESTree.EntityName as its child, so we can filter here and break early
if (parent.type !== AST_NODE_TYPES.TSQualifiedName) {
break;
}
parent = parent.parent;
}
}
return false;
});
if (onlyHasTypeReferences) {
typeVariables.push(variable);
} else {
valueVariables.push(variable);
}
}
if (valueVariables.length === 0) {
// import is all type-only, convert the entire import to `import type`
// EXAMPLES:
// import { Type1, Type2 } from 'foo';
// should be fixed to:
// import type { Type1, Type2 } from 'foo';
// import Type from 'foo';
// should be fixed to:
// import type Type from 'foo';
// import * as Type from 'foo';
// should be fixed to:
// import type * as Type from 'foo';
// !!! NOTE: must take special care in this case:
// import Default, { Named } from 'foo';
// should be fixed to:
// import type Default from 'foo';
// import type { Named } from 'foo';
// context.report({ message: 'All imports in the declaration are only used as types (or some error like that)' })
} else {
// we have a mixed type/value import, so we need to split them out into multiple exports
// EXAMPLES:
// import { Value, Type } from 'foo';
// should be fixed to:
// import type { Type } from 'foo';
// import { Value } from 'foo';
// import Type, { Value } from 'foo';
// should be fixed to:
// import type Type from 'foo';
// import { Value } from 'foo';
// import Value, { Type } from 'foo';
// should be fixed to:
// import type { Type } from 'foo';
// import Value from 'foo';
// !!! NOTE: must take special care in this case:
// import Type1, { Type2, Value } from 'foo';
// should be fixed to:
// import type Type1 from 'foo';
// import type { Type2 } from 'foo';
// import { Value } from 'foo';
// context.report({ message: 'Imports "A", "B" and "C" are only used as types (or some error like that)' })
}
},
}| }, | ||
| }); | ||
|
|
||
| ruleTester.run('consistent-type-imports', rule, { |
There was a problem hiding this comment.
some extra test cases that you'll want to test (the `typeof` query)
import Type from 'foo';
type T = typeof Type;
type T = typeof Type.foo;import type Type from 'foo';
type T = typeof Type;
type T = typeof Type.foo;import { Type } from 'foo';
type T = typeof Type;
type T = typeof Type.foo;import type { Type } from 'foo';
type T = typeof Type;
type T = typeof Type.foo;import * as Type from 'foo';
type T = typeof Type;
type T = typeof Type.foo;import type * as Type from 'foo';
type T = typeof Type;
type T = typeof Type.foo;some extra test cases that you'll want to test (exports)
import Type from 'foo';
export { Type }; // is a value export
export default Type; // is a value export
export type { Type }; // is a type-only exportimport type Type from 'foo';
export { Type }; // is a type-only export
export default Type; // is a type-only export
export type { Type }; // is a type-only exportimport { Type } from 'foo';
export { Type }; // is a value export
export default Type; // is a value export
export type { Type }; // is a type-only exportimport type { Type } from 'foo';
export { Type }; // is a type-only export
export default Type; // is a type-only export
export type { Type }; // is a type-only exportimport * as Type from 'foo';
export { Type }; // is a value export
export default Type; // is a value export
export type { Type }; // is a type-only exportimport type * as Type from 'foo';
export { Type }; // is a type-only export
export default Type; // is a type-only export
export type { Type }; // is a type-only exportPlus the examples I listed in the comments in the suggestion here
c0e572a to
d4215cb
Compare
|
@bradzacher Thank you for your review! I have made your requested changes to this PR. So please check again it. This change made the auto-fix complicated. I think it should be simplified if other rules can support it. Let me know if you know a good way. |
|
@bradzacher I misunderstood. Forget what I said earlier. |
d4215cb to
bb913f0
Compare
|
Has adding this to the recommended config been discussed? |
|
This rule requires a certain (recent) version of TS, so it can't and won't be added to recommended until our minimum version range matches. |
|
Thank you. That makes sense. |
This PR adds the
consistent-type-importsrule.Fixes #2200