From 1971a3f8027416cd1fb33b1d50faa035599917de Mon Sep 17 00:00:00 2001 From: David Zearing Date: Fri, 22 Oct 2021 18:43:50 -0700 Subject: [PATCH] feat(eslint-plugin): adding `consistent-type-exports` rule (#3936) --- packages/eslint-plugin/README.md | 1 + .../docs/rules/consistent-type-exports.md | 34 +++ packages/eslint-plugin/src/configs/all.ts | 1 + .../src/rules/consistent-type-exports.ts | 287 ++++++++++++++++++ .../src/rules/consistent-type-imports.ts | 7 +- packages/eslint-plugin/src/rules/index.ts | 2 + packages/eslint-plugin/src/util/misc.ts | 19 ++ .../rules/consistent-type-exports.test.ts | 263 ++++++++++++++++ .../eslint-plugin/tests/util/misc.test.ts | 25 ++ 9 files changed, 634 insertions(+), 5 deletions(-) create mode 100644 packages/eslint-plugin/docs/rules/consistent-type-exports.md create mode 100644 packages/eslint-plugin/src/rules/consistent-type-exports.ts create mode 100644 packages/eslint-plugin/tests/rules/consistent-type-exports.test.ts create mode 100644 packages/eslint-plugin/tests/util/misc.test.ts diff --git a/packages/eslint-plugin/README.md b/packages/eslint-plugin/README.md index 6b7c2701ffa..ad338c2dd4a 100644 --- a/packages/eslint-plugin/README.md +++ b/packages/eslint-plugin/README.md @@ -107,6 +107,7 @@ Pro Tip: For larger codebases you may want to consider splitting our linting int | [`@typescript-eslint/consistent-indexed-object-style`](./docs/rules/consistent-indexed-object-style.md) | Enforce or disallow the use of the record type | | :wrench: | | | [`@typescript-eslint/consistent-type-assertions`](./docs/rules/consistent-type-assertions.md) | Enforces consistent usage of type assertions | | | | | [`@typescript-eslint/consistent-type-definitions`](./docs/rules/consistent-type-definitions.md) | Consistent with type definition either `interface` or `type` | | :wrench: | | +| [`@typescript-eslint/consistent-type-exports`](./docs/rules/consistent-type-exports.md) | Enforces consistent usage of type exports | | :wrench: | :thought_balloon: | | [`@typescript-eslint/consistent-type-imports`](./docs/rules/consistent-type-imports.md) | Enforces consistent usage of type imports | | :wrench: | | | [`@typescript-eslint/explicit-function-return-type`](./docs/rules/explicit-function-return-type.md) | Require explicit return types on functions and class methods | | | | | [`@typescript-eslint/explicit-member-accessibility`](./docs/rules/explicit-member-accessibility.md) | Require explicit accessibility modifiers on class properties and methods | | :wrench: | | diff --git a/packages/eslint-plugin/docs/rules/consistent-type-exports.md b/packages/eslint-plugin/docs/rules/consistent-type-exports.md new file mode 100644 index 00000000000..cb3b01f51df --- /dev/null +++ b/packages/eslint-plugin/docs/rules/consistent-type-exports.md @@ -0,0 +1,34 @@ +# Enforces consistent usage of type exports (`consistent-type-exports`) + +TypeScript 3.8 added support for type-only exports. + +Type-only exports allow you to specify that 1 or more named exports are exported as type-only. This allows +transpilers to drop exports without knowing the types of the dependencies. + +## Rule Details + +This rule aims to standardize the use of type exports style across a codebase. + +Given a class `Button`, and an interface `ButtonProps`, examples of **correct** code: + +```ts +export { Button } from 'some-library'; +export type { ButtonProps } from 'some-library'; +``` + +Examples of **incorrect** code: + +```ts +export { Button, ButtonProps } from 'some-library'; +``` + +## When Not To Use It + +- If you are using a TypeScript version less than 3.8, then you will not be able to use this rule as type exports are not supported. +- If you specifically want to use both export kinds for stylistic reasons, you can disable this rule. + +## Attributes + +- [ ] ✅ Recommended +- [x] 🔧 Fixable +- [x] 💭 Requires type information diff --git a/packages/eslint-plugin/src/configs/all.ts b/packages/eslint-plugin/src/configs/all.ts index 17c06645e61..538be53ce58 100644 --- a/packages/eslint-plugin/src/configs/all.ts +++ b/packages/eslint-plugin/src/configs/all.ts @@ -22,6 +22,7 @@ export = { '@typescript-eslint/consistent-type-assertions': 'error', '@typescript-eslint/consistent-type-definitions': 'error', '@typescript-eslint/consistent-type-imports': 'error', + '@typescript-eslint/consistent-type-exports': 'error', 'default-param-last': 'off', '@typescript-eslint/default-param-last': 'error', 'dot-notation': 'off', diff --git a/packages/eslint-plugin/src/rules/consistent-type-exports.ts b/packages/eslint-plugin/src/rules/consistent-type-exports.ts new file mode 100644 index 00000000000..98e6bd355e4 --- /dev/null +++ b/packages/eslint-plugin/src/rules/consistent-type-exports.ts @@ -0,0 +1,287 @@ +import { + TSESTree, + ParserServices, + AST_NODE_TYPES, + TSESLint, +} from '@typescript-eslint/experimental-utils'; +import { SymbolFlags } from 'typescript'; +import * as util from '../util'; + +type Options = []; + +interface SourceExports { + source: string; + reportValueExports: ReportValueExport[]; + typeOnlyNamedExport: TSESTree.ExportNamedDeclaration | null; + valueOnlyNamedExport: TSESTree.ExportNamedDeclaration | null; +} + +interface ReportValueExport { + node: TSESTree.ExportNamedDeclaration; + typeSpecifiers: TSESTree.ExportSpecifier[]; + valueSpecifiers: TSESTree.ExportSpecifier[]; +} + +type MessageIds = + | 'typeOverValue' + | 'singleExportIsType' + | 'multipleExportsAreTypes'; + +export default util.createRule({ + name: 'consistent-type-exports', + defaultOptions: [], + meta: { + type: 'suggestion', + docs: { + description: 'Enforces consistent usage of type exports', + recommended: false, + requiresTypeChecking: true, + }, + messages: { + typeOverValue: + 'All exports in the declaration are only used as types. Use `export type`.', + + singleExportIsType: + 'Type export {{exportNames}} is not a value and should be exported using `export type`.', + multipleExportsAreTypes: + 'Type exports {{exportNames}} are not values and should be exported using `export type`.', + }, + schema: [], + fixable: 'code', + }, + + create(context) { + const sourceCode = context.getSourceCode(); + const sourceExportsMap: { [key: string]: SourceExports } = {}; + const parserServices = util.getParserServices(context); + + return { + ExportNamedDeclaration(node: TSESTree.ExportNamedDeclaration): void { + // Coerce the source into a string for use as a lookup entry. + const source = getSourceFromExport(node) ?? 'undefined'; + const sourceExports = (sourceExportsMap[source] ||= { + source, + reportValueExports: [], + typeOnlyNamedExport: null, + valueOnlyNamedExport: null, + }); + + // Cache the first encountered exports for the package. We will need to come + // back to these later when fixing the problems. + if (node.exportKind === 'type') { + if (!sourceExports.typeOnlyNamedExport) { + // The export is a type export + sourceExports.typeOnlyNamedExport = node; + } + } else if (!sourceExports.valueOnlyNamedExport) { + // The export is a value export + sourceExports.valueOnlyNamedExport = node; + } + + // Next for the current export, we will separate type/value specifiers. + const typeSpecifiers: TSESTree.ExportSpecifier[] = []; + const valueSpecifiers: TSESTree.ExportSpecifier[] = []; + + // Note: it is valid to export values as types. We will avoid reporting errors + // when this is encountered. + if (node.exportKind !== 'type') { + for (const specifier of node.specifiers) { + const isTypeBased = isSpecifierTypeBased(parserServices, specifier); + + if (isTypeBased === true) { + typeSpecifiers.push(specifier); + } else if (isTypeBased === false) { + // When isTypeBased is undefined, we should avoid reporting them. + valueSpecifiers.push(specifier); + } + } + } + + if ( + (node.exportKind === 'value' && typeSpecifiers.length) || + (node.exportKind === 'type' && valueSpecifiers.length) + ) { + sourceExports.reportValueExports.push({ + node, + typeSpecifiers, + valueSpecifiers, + }); + } + }, + + 'Program:exit'(): void { + for (const sourceExports of Object.values(sourceExportsMap)) { + // If this export has no issues, move on. + if (sourceExports.reportValueExports.length === 0) { + continue; + } + + for (const report of sourceExports.reportValueExports) { + if (!report.valueSpecifiers.length) { + // Export is all type-only; convert the entire export to `export type`. + context.report({ + node: report.node, + messageId: 'typeOverValue', + *fix(fixer) { + yield* fixExportInsertType(fixer, sourceCode, report.node); + }, + }); + } else { + // We have both type and value violations. + const allExportNames = report.typeSpecifiers.map( + specifier => `${specifier.local.name}`, + ); + + if (allExportNames.length === 1) { + const exportNames = allExportNames[0]; + + context.report({ + node: report.node, + messageId: 'singleExportIsType', + data: { exportNames }, + *fix(fixer) { + yield* fixSeparateNamedExports(fixer, sourceCode, report); + }, + }); + } else { + const exportNames = util.formatWordList(allExportNames); + + context.report({ + node: report.node, + messageId: 'multipleExportsAreTypes', + data: { exportNames }, + *fix(fixer) { + yield* fixSeparateNamedExports(fixer, sourceCode, report); + }, + }); + } + } + } + } + }, + }; + }, +}); + +/** + * Helper for identifying if an export specifier resolves to a + * JavaScript value or a TypeScript type. + * + * @returns True/false if is a type or not, or undefined if the specifier + * can't be resolved. + */ +function isSpecifierTypeBased( + parserServices: ParserServices, + specifier: TSESTree.ExportSpecifier, +): boolean | undefined { + const checker = parserServices.program.getTypeChecker(); + const node = parserServices.esTreeNodeToTSNodeMap.get(specifier.exported); + const symbol = checker.getSymbolAtLocation(node); + const aliasedSymbol = checker.getAliasedSymbol(symbol!); + + if (!aliasedSymbol || aliasedSymbol.escapedName === 'unknown') { + return undefined; + } + + return !(aliasedSymbol.flags & SymbolFlags.Value); +} + +/** + * Inserts "type" into an export. + * + * Example: + * + * export type { Foo } from 'foo'; + * ^^^^ + */ +function* fixExportInsertType( + fixer: TSESLint.RuleFixer, + sourceCode: Readonly, + node: TSESTree.ExportNamedDeclaration, +): IterableIterator { + const exportToken = util.nullThrows( + sourceCode.getFirstToken(node), + util.NullThrowsReasons.MissingToken('export', node.type), + ); + + yield fixer.insertTextAfter(exportToken, ' type'); +} + +/** + * Separates the exports which mismatch the kind of export the given + * node represents. For example, a type export's named specifiers which + * represent values will be inserted in a separate `export` statement. + */ +function* fixSeparateNamedExports( + fixer: TSESLint.RuleFixer, + sourceCode: Readonly, + report: ReportValueExport, +): IterableIterator { + const { node, typeSpecifiers, valueSpecifiers } = report; + const source = getSourceFromExport(node); + const separateTypes = node.exportKind !== 'type'; + const specifiersToSeparate = separateTypes ? typeSpecifiers : valueSpecifiers; + const specifierNames = specifiersToSeparate.map(getSpecifierText).join(', '); + + const exportToken = util.nullThrows( + sourceCode.getFirstToken(node), + util.NullThrowsReasons.MissingToken('export', node.type), + ); + + // Filter the bad exports from the current line. + const filteredSpecifierNames = ( + separateTypes ? valueSpecifiers : typeSpecifiers + ) + .map(getSpecifierText) + .join(', '); + const openToken = util.nullThrows( + sourceCode.getFirstToken(node, util.isOpeningBraceToken), + util.NullThrowsReasons.MissingToken('{', node.type), + ); + const closeToken = util.nullThrows( + sourceCode.getLastToken(node, util.isClosingBraceToken), + util.NullThrowsReasons.MissingToken('}', node.type), + ); + + // Remove exports from the current line which we're going to re-insert. + yield fixer.replaceTextRange( + [openToken.range[1], closeToken.range[0]], + ` ${filteredSpecifierNames} `, + ); + + // Insert the bad exports into a new export line above. + yield fixer.insertTextBefore( + exportToken, + `export ${separateTypes ? 'type ' : ''}{ ${specifierNames} }${ + source ? ` from '${source}'` : '' + };\n`, + ); +} + +/** + * Returns the source of the export, or undefined if the named export has no source. + */ +function getSourceFromExport( + node: TSESTree.ExportNamedDeclaration, +): string | undefined { + if ( + node.source?.type === AST_NODE_TYPES.Literal && + typeof node.source.value === 'string' + ) { + return node.source.value; + } + + return undefined; +} + +/** + * Returns the specifier text for the export. If it is aliased, we take care to return + * the proper formatting. + */ +function getSpecifierText(specifier: TSESTree.ExportSpecifier): string { + return `${specifier.local.name}${ + specifier.exported.name !== specifier.local.name + ? ` as ${specifier.exported.name}` + : '' + }`; +} diff --git a/packages/eslint-plugin/src/rules/consistent-type-imports.ts b/packages/eslint-plugin/src/rules/consistent-type-imports.ts index abefc9a4a8a..d549ab8f728 100644 --- a/packages/eslint-plugin/src/rules/consistent-type-imports.ts +++ b/packages/eslint-plugin/src/rules/consistent-type-imports.ts @@ -264,8 +264,9 @@ export default util.createRule({ messageId: MessageIds; data: Record; } => { + const typeImports = util.formatWordList(importNames); + if (importNames.length === 1) { - const typeImports = importNames[0]; if (isTypeImport) { return { messageId: 'aImportInDecoMeta', @@ -278,10 +279,6 @@ export default util.createRule({ }; } } else { - const typeImports = [ - importNames.slice(0, -1).join(', '), - importNames.slice(-1)[0], - ].join(' and '); if (isTypeImport) { return { messageId: 'someImportsInDecoMeta', diff --git a/packages/eslint-plugin/src/rules/index.ts b/packages/eslint-plugin/src/rules/index.ts index 2d9730dd8c8..83e76f0a23d 100644 --- a/packages/eslint-plugin/src/rules/index.ts +++ b/packages/eslint-plugin/src/rules/index.ts @@ -11,6 +11,7 @@ import commaSpacing from './comma-spacing'; import consistentIndexedObjectStyle from './consistent-indexed-object-style'; import consistentTypeAssertions from './consistent-type-assertions'; import consistentTypeDefinitions from './consistent-type-definitions'; +import consistentTypeExports from './consistent-type-exports'; import consistentTypeImports from './consistent-type-imports'; import defaultParamLast from './default-param-last'; import dotNotation from './dot-notation'; @@ -133,6 +134,7 @@ export default { 'consistent-indexed-object-style': consistentIndexedObjectStyle, 'consistent-type-assertions': consistentTypeAssertions, 'consistent-type-definitions': consistentTypeDefinitions, + 'consistent-type-exports': consistentTypeExports, 'consistent-type-imports': consistentTypeImports, 'default-param-last': defaultParamLast, 'dot-notation': dotNotation, diff --git a/packages/eslint-plugin/src/util/misc.ts b/packages/eslint-plugin/src/util/misc.ts index 95fa4a359c6..052e7bfd06c 100644 --- a/packages/eslint-plugin/src/util/misc.ts +++ b/packages/eslint-plugin/src/util/misc.ts @@ -133,11 +133,30 @@ function getEnumNames(myEnum: Record): T[] { return Object.keys(myEnum).filter(x => isNaN(parseInt(x))) as T[]; } +/** + * Given an array of words, returns an English-friendly concatenation, separated with commas, with + * the `and` clause inserted before the last item. + * + * Example: ['foo', 'bar', 'baz' ] returns the string "foo, bar, and baz". + */ +function formatWordList(words: string[]): string { + if (!words?.length) { + return ''; + } + + if (words.length === 1) { + return words[0]; + } + + return [words.slice(0, -1).join(', '), words.slice(-1)[0]].join(' and '); +} + export { arraysAreEqual, Equal, ExcludeKeys, findFirstResult, + formatWordList, getEnumNames, getNameFromIndexSignature, getNameFromMember, diff --git a/packages/eslint-plugin/tests/rules/consistent-type-exports.test.ts b/packages/eslint-plugin/tests/rules/consistent-type-exports.test.ts new file mode 100644 index 00000000000..301d70e54dc --- /dev/null +++ b/packages/eslint-plugin/tests/rules/consistent-type-exports.test.ts @@ -0,0 +1,263 @@ +import rule from '../../src/rules/consistent-type-exports'; +import { RuleTester, getFixturesRootDir } from '../RuleTester'; + +const rootDir = getFixturesRootDir(); + +const ruleTester = new RuleTester({ + parser: '@typescript-eslint/parser', + parserOptions: { + ecmaVersion: 2020, + sourceType: 'module', + tsconfigRootDir: rootDir, + project: './tsconfig.json', + }, +}); + +ruleTester.run('consistent-type-exports', rule, { + valid: [ + "export { Foo } from 'foo';", + "export type { AnalyzeOptions } from '@typescript-eslint/scope-manager';", + "export { BlockScope } from '@typescript-eslint/experimental-utils';", + "export type { BlockScope } from '@typescript-eslint/experimental-utils';", + ` +const variable = 1; +class Class {} +enum Enum {} +function Func() {} +namespace ValueNS { + export const x = 1; +} + +export { variable, Class, Enum, Func, ValueNS }; + `, + ` +type Alias = 1; +interface IFace {} +namespace TypeNS { + export type x = 1; +} + +export type { Alias, IFace, TypeNS }; + `, + ` +const foo = 1; +export type { foo }; + `, + ` +namespace NonTypeNS { + export const x = 1; +} + +export { NonTypeNS }; + `, + ], + invalid: [ + { + code: "export { AnalyzeOptions } from '@typescript-eslint/scope-manager';", + output: + "export type { AnalyzeOptions } from '@typescript-eslint/scope-manager';", + errors: [ + { + messageId: 'typeOverValue', + line: 1, + column: 1, + }, + ], + }, + { + code: "export { AnalyzeOptions, BlockScope } from '@typescript-eslint/scope-manager';", + output: + `export type { AnalyzeOptions } from '@typescript-eslint/scope-manager';\n` + + `export { BlockScope } from '@typescript-eslint/scope-manager';`, + errors: [ + { + messageId: 'singleExportIsType', + line: 1, + column: 1, + }, + ], + }, + { + code: ` +export { + AnalyzeOptions, + BlockScope, + CatchScope, +} from '@typescript-eslint/scope-manager'; + `, + output: ` +export type { AnalyzeOptions } from '@typescript-eslint/scope-manager'; +export { BlockScope, CatchScope } from '@typescript-eslint/scope-manager'; + `, + errors: [ + { + messageId: 'singleExportIsType', + line: 2, + column: 1, + }, + ], + }, + { + code: ` +export { + AnalyzeOptions, + BlockScope, + Definition, + CatchScope, +} from '@typescript-eslint/scope-manager'; + `, + // eslint-disable-next-line @typescript-eslint/internal/plugin-test-formatting + output: ` +export type { AnalyzeOptions, Definition } from '@typescript-eslint/scope-manager'; +export { BlockScope, CatchScope } from '@typescript-eslint/scope-manager'; + `, + errors: [ + { + messageId: 'multipleExportsAreTypes', + line: 2, + column: 1, + }, + ], + }, + { + code: "export { Definition as Foo } from '@typescript-eslint/scope-manager';", + output: + "export type { Definition as Foo } from '@typescript-eslint/scope-manager';", + errors: [ + { + messageId: 'typeOverValue', + line: 1, + column: 1, + }, + ], + }, + { + code: ` +export { + Definition as Foo, + BlockScope, +} from '@typescript-eslint/scope-manager'; + `, + output: ` +export type { Definition as Foo } from '@typescript-eslint/scope-manager'; +export { BlockScope } from '@typescript-eslint/scope-manager'; + `, + errors: [ + { + messageId: 'singleExportIsType', + line: 2, + column: 1, + }, + ], + }, + { + code: ` +export { + Definition as Foo, + BlockScope as BScope, + CatchScope as CScope, +} from '@typescript-eslint/scope-manager'; + `, + // eslint-disable-next-line @typescript-eslint/internal/plugin-test-formatting + output: ` +export type { Definition as Foo } from '@typescript-eslint/scope-manager'; +export { BlockScope as BScope, CatchScope as CScope } from '@typescript-eslint/scope-manager'; + `, + errors: [ + { + messageId: 'singleExportIsType', + line: 2, + column: 1, + }, + ], + }, + { + code: ` +import { Definition } from '@typescript-eslint/scope-manager'; +export { Definition }; + `, + output: ` +import { Definition } from '@typescript-eslint/scope-manager'; +export type { Definition }; + `, + errors: [ + { + messageId: 'typeOverValue', + line: 3, + column: 1, + }, + ], + }, + { + code: ` +import { CatchScope, Definition } from '@typescript-eslint/scope-manager'; +export { CatchScope, Definition }; + `, + output: ` +import { CatchScope, Definition } from '@typescript-eslint/scope-manager'; +export type { Definition }; +export { CatchScope }; + `, + errors: [ + { + messageId: 'singleExportIsType', + line: 3, + column: 1, + }, + ], + }, + { + code: ` +type Alias = 1; +interface IFace {} +namespace TypeNS { + export type x = 1; + export const f = 1; +} + +export { Alias, IFace, TypeNS }; + `, + output: ` +type Alias = 1; +interface IFace {} +namespace TypeNS { + export type x = 1; + export const f = 1; +} + +export type { Alias, IFace }; +export { TypeNS }; + `, + errors: [ + { + messageId: 'multipleExportsAreTypes', + line: 9, + column: 1, + }, + ], + }, + { + code: ` +namespace TypeNS { + export interface Foo {} +} + +export { TypeNS }; + `, + output: ` +namespace TypeNS { + export interface Foo {} +} + +export type { TypeNS }; + `, + errors: [ + { + messageId: 'typeOverValue', + line: 6, + column: 1, + }, + ], + }, + ], +}); diff --git a/packages/eslint-plugin/tests/util/misc.test.ts b/packages/eslint-plugin/tests/util/misc.test.ts new file mode 100644 index 00000000000..c4291f00d9f --- /dev/null +++ b/packages/eslint-plugin/tests/util/misc.test.ts @@ -0,0 +1,25 @@ +import * as misc from '../../src/util/misc'; + +describe('formatWordList', () => { + it('can format with no words', () => { + expect(misc.formatWordList([])).toBe(''); + }); + + it('can format with 1 word', () => { + expect(misc.formatWordList(['foo'])).toBe('foo'); + }); + + it('can format with 2 words', () => { + expect(misc.formatWordList(['foo', 'bar'])).toBe('foo and bar'); + }); + + it('can format with 3 words', () => { + expect(misc.formatWordList(['foo', 'bar', 'baz'])).toBe('foo, bar and baz'); + }); + + it('can format with 4 words', () => { + expect(misc.formatWordList(['foo', 'bar', 'baz', 'boz'])).toBe( + 'foo, bar, baz and boz', + ); + }); +});