Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork2.8k
feat(eslint-plugin): addconsistent-type-imports rule#2367
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
feat(eslint-plugin): addconsistent-type-imports rule#2367
Uh oh!
There was an error while loading.Please reload this page.
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 onhttps://opencollective.com/typescript-eslint. As a thank you, your profile/company logo will be added to our main README which receives thousands of unique visitorsper day. |
2eaf38b to182bf9aComparecodecovbot commentedAug 7, 2020 • 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 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.
|
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.
Thanks for working on this!
This is a great start. A few comments on the docs and the implementation.
| ## 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
| If youspecifically want to use both import kinds for stylistic reasons, you can disablethis rule. | |
| -If youare not using TypeScript 3.8 (or greater), then you will not be able to usethis 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I think remove these examples here, and we can provide all the examples under the specific options
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
| } | ||
| : { | ||
| // prefer no type imports | ||
| 'ImportDeclaration[importKind=type]'( |
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
You can use the second argument togetFirstToken to ensure you fetch atype keyword.
You can also use ournullThrows util to throw with a nice error message if it's not found.
| constimportToken=sourceCode.getFirstToken(node)!; | |
| returnfixer.removeRange([ | |
| importToken.range[1], | |
| sourceCode.getTokenAfter(importToken)!.range[1], | |
| constimportToken=util.nullThrows( | |
| sourceCode.getFirstToken( | |
| node, | |
| token=> | |
| token.type===AST_TOKEN_TYPES.Keyword&&token.value==='type', | |
| ), | |
| util.NullThrowsReasons.MissingToken('type',node.type), | |
| ); | |
| returnfixer.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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
this is invalid syntax - a type-only import can specify a defaultOR named import, not both
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 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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 mustonly have a defaultOR 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:importTypefrom'foo';typeT<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{constvariables=context.getDeclaredVariables(node);consttypeVariables:TSESLint.Scope.Variable[]=[];constvalueVariables:TSESLint.Scope.Variable[]=[];for(constvariableofvariables){constonlyHasTypeReferences=variable.references.every(ref=>{if(ref.isTypeReference){returntrue;}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 importsletparent=ref.identifier.parent;while(parent){if(parent.type===AST_NODE_TYPES.TSTypeQuery){returntrue;}// TSTypeQuery must have a TSESTree.EntityName as its child, so we can filter here and break earlyif(parent.type!==AST_NODE_TYPES.TSQualifiedName){break;}parent=parent.parent;}}returnfalse;});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, { |
bradzacherAug 9, 2020 • 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.
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.
some extra test cases that you'll want to test (the `typeof` query)
importTypefrom'foo';typeT=typeofType;typeT=typeofType.foo;
importtypeTypefrom'foo';typeT=typeofType;typeT=typeofType.foo;
import{Type}from'foo';typeT=typeofType;typeT=typeofType.foo;
importtype{Type}from'foo';typeT=typeofType;typeT=typeofType.foo;
import*asTypefrom'foo';typeT=typeofType;typeT=typeofType.foo;
importtype*asTypefrom'foo';typeT=typeofType;typeT=typeofType.foo;
some extra test cases that you'll want to test (exports)
importTypefrom'foo';export{Type};// is a value exportexportdefaultType;// is a value exportexporttype{Type};// is a type-only export
importtypeTypefrom'foo';export{Type};// is a type-only exportexportdefaultType;// is a type-only exportexporttype{Type};// is a type-only export
import{Type}from'foo';export{Type};// is a value exportexportdefaultType;// is a value exportexporttype{Type};// is a type-only export
importtype{Type}from'foo';export{Type};// is a type-only exportexportdefaultType;// is a type-only exportexporttype{Type};// is a type-only export
import*asTypefrom'foo';export{Type};// is a value exportexportdefaultType;// is a value exportexporttype{Type};// is a type-only export
importtype*asTypefrom'foo';export{Type};// is a type-only exportexportdefaultType;// is a type-only exportexporttype{Type};// is a type-only export
Plus the examples I listed in the comments in the suggestionhere
c0e572a tod4215cbCompare@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. |
ota-meshi commentedAug 11, 2020 • 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.
@bradzacher I misunderstood. Forget what I said earlier. |
d4215cb tobb913f0CompareThere 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 LGTM!
Thanks for your work here - the fixer is a complex piece of work so you've done a great job 😄
tilgovi commentedAug 17, 2020
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. |
tilgovi commentedAug 17, 2020
Thank you. That makes sense. |
This PR adds the
consistent-type-importsrule.Fixes#2200