From f8c8a9df1b49cb7f487eb0dba3e7f46ac7f7e23b Mon Sep 17 00:00:00 2001 From: Josh Black Date: Wed, 9 Oct 2024 14:52:07 -0500 Subject: [PATCH] fix(no-wildcard-imports): mix-and-match import types (#249) * fix(no-wildcard-imports): mix-and-match import types * chore: add changeset * refactor: update logic to reuse imports * fix: add support for default * fix: reuse existing type import declarations if they exist * chore: fix eslint warnings * fix: lookup for specifiers * fix: update overlap when removing and replacing node --- .changeset/grumpy-masks-lie.md | 5 + .../__tests__/no-wildcard-imports.test.js | 49 ++++-- src/rules/no-wildcard-imports.js | 156 +++++++++++++----- 3 files changed, 152 insertions(+), 58 deletions(-) create mode 100644 .changeset/grumpy-masks-lie.md diff --git a/.changeset/grumpy-masks-lie.md b/.changeset/grumpy-masks-lie.md new file mode 100644 index 00000000..0d181e40 --- /dev/null +++ b/.changeset/grumpy-masks-lie.md @@ -0,0 +1,5 @@ +--- +'eslint-plugin-primer-react': patch +--- + +Update no-wildcard-imports rule to not create separate imports for type only imports. This prevents an issue downstream with autofixers diff --git a/src/rules/__tests__/no-wildcard-imports.test.js b/src/rules/__tests__/no-wildcard-imports.test.js index db11f077..a6de13a3 100644 --- a/src/rules/__tests__/no-wildcard-imports.test.js +++ b/src/rules/__tests__/no-wildcard-imports.test.js @@ -30,7 +30,7 @@ ruleTester.run('no-wildcard-imports', rule, { // Test type import { code: `import type {SxProp} from '@primer/react/lib-esm/sx'`, - output: `import type {SxProp} from '@primer/react'`, + output: `import {type SxProp} from '@primer/react'`, errors: [ { messageId: 'wildcardMigration', @@ -44,7 +44,7 @@ ruleTester.run('no-wildcard-imports', rule, { // Test multiple type imports { code: `import type {BetterSystemStyleObject, SxProp, BetterCssProperties} from '@primer/react/lib-esm/sx'`, - output: `import type {BetterSystemStyleObject, SxProp, BetterCssProperties} from '@primer/react'`, + output: `import {type BetterSystemStyleObject, type SxProp, type BetterCssProperties} from '@primer/react'`, errors: [ { messageId: 'wildcardMigration', @@ -58,7 +58,7 @@ ruleTester.run('no-wildcard-imports', rule, { // Test import alias { code: `import type {SxProp as RenamedSxProp} from '@primer/react/lib-esm/sx'`, - output: `import type {SxProp as RenamedSxProp} from '@primer/react'`, + output: `import {type SxProp as RenamedSxProp} from '@primer/react'`, errors: [ { messageId: 'wildcardMigration', @@ -108,7 +108,7 @@ ruleTester.run('no-wildcard-imports', rule, { // Test renamed wildcard imports { code: `import type {ItemProps} from '@primer/react/lib-esm/deprecated/ActionList/Item'`, - output: `import type {ActionListItemProps as ItemProps} from '@primer/react/deprecated'`, + output: `import {type ActionListItemProps as ItemProps} from '@primer/react/deprecated'`, errors: [ { messageId: 'wildcardMigration', @@ -122,8 +122,7 @@ ruleTester.run('no-wildcard-imports', rule, { // Test mixed imports { code: `import {Dialog, type DialogProps} from '@primer/react/lib-esm/Dialog/Dialog'`, - output: `import {Dialog} from '@primer/react/experimental' -import type {DialogProps} from '@primer/react/experimental'`, + output: `import {Dialog, type DialogProps} from '@primer/react/experimental'`, errors: [ { messageId: 'wildcardMigration', @@ -134,6 +133,22 @@ import type {DialogProps} from '@primer/react/experimental'`, ], }, + // Use existing imports + { + code: `import {Box, type BoxProps} from '@primer/react' +import type {BetterSystemStyleObject} from '@primer/react/lib-esm/sx'`, + output: `import {Box, type BoxProps, type BetterSystemStyleObject} from '@primer/react' +`, + errors: [ + { + messageId: 'wildcardMigration', + data: { + wildcardEntrypoint: '@primer/react/lib-esm/sx', + }, + }, + ], + }, + // Test migrations // Test helpers ------------------------------------------------------------ @@ -155,7 +170,7 @@ import type {DialogProps} from '@primer/react/experimental'`, code: `import {ButtonBase} from '@primer/react/lib-esm/Button/ButtonBase'; import type {ButtonBaseProps} from '@primer/react/lib-esm/Button/ButtonBase'`, output: `import {ButtonBase} from '@primer/react' -import type {ButtonBaseProps} from '@primer/react'`, +import {type ButtonBaseProps} from '@primer/react'`, errors: [ { messageId: 'wildcardMigration', @@ -173,7 +188,7 @@ import type {ButtonBaseProps} from '@primer/react'`, }, { code: `import type {ButtonBaseProps} from '@primer/react/lib-esm/Button/types'`, - output: `import type {ButtonBaseProps} from '@primer/react'`, + output: `import {type ButtonBaseProps} from '@primer/react'`, errors: [ { messageId: 'wildcardMigration', @@ -209,7 +224,7 @@ import type {ButtonBaseProps} from '@primer/react'`, }, { code: `import type {SelectPanelProps} from '@primer/react/lib-esm/SelectPanel/SelectPanel'`, - output: `import type {SelectPanelProps} from '@primer/react'`, + output: `import {type SelectPanelProps} from '@primer/react'`, errors: [ { messageId: 'wildcardMigration', @@ -221,7 +236,7 @@ import type {ButtonBaseProps} from '@primer/react'`, }, { code: `import type {LabelColorOptions} from '@primer/react/lib-esm/Label/Label'`, - output: `import type {LabelColorOptions} from '@primer/react'`, + output: `import {type LabelColorOptions} from '@primer/react'`, errors: [ { messageId: 'wildcardMigration', @@ -245,7 +260,7 @@ import type {ButtonBaseProps} from '@primer/react'`, }, { code: `import type {IssueLabelTokenProps} from '@primer/react/lib-esm/Token/IssueLabelToken'`, - output: `import type {IssueLabelTokenProps} from '@primer/react'`, + output: `import {type IssueLabelTokenProps} from '@primer/react'`, errors: [ { messageId: 'wildcardMigration', @@ -257,7 +272,7 @@ import type {ButtonBaseProps} from '@primer/react'`, }, { code: `import type {TokenSizeKeys} from '@primer/react/lib-esm/Token/TokenBase'`, - output: `import type {TokenSizeKeys} from '@primer/react'`, + output: `import {type TokenSizeKeys} from '@primer/react'`, errors: [ { messageId: 'wildcardMigration', @@ -269,7 +284,7 @@ import type {ButtonBaseProps} from '@primer/react'`, }, { code: `import type {ItemProps} from '@primer/react/lib-esm/deprecated/ActionList'`, - output: `import type {ActionListItemProps as ItemProps} from '@primer/react/deprecated'`, + output: `import {type ActionListItemProps as ItemProps} from '@primer/react/deprecated'`, errors: [ { messageId: 'wildcardMigration', @@ -281,7 +296,7 @@ import type {ButtonBaseProps} from '@primer/react'`, }, { code: `import type {GroupedListProps} from '@primer/react/lib-esm/deprecated/ActionList/List'`, - output: `import type {ActionListGroupedListProps as GroupedListProps} from '@primer/react/deprecated'`, + output: `import {type ActionListGroupedListProps as GroupedListProps} from '@primer/react/deprecated'`, errors: [ { messageId: 'wildcardMigration', @@ -305,7 +320,7 @@ import type {ButtonBaseProps} from '@primer/react'`, }, { code: `import type {ItemProps} from '@primer/react/lib-esm/deprecated/ActionList/Item'`, - output: `import type {ActionListItemProps as ItemProps} from '@primer/react/deprecated'`, + output: `import {type ActionListItemProps as ItemProps} from '@primer/react/deprecated'`, errors: [ { messageId: 'wildcardMigration', @@ -375,7 +390,7 @@ import type {ButtonBaseProps} from '@primer/react'`, }, { code: `import type {ResponsiveValue} from '@primer/react/lib-esm/hooks/useResponsiveValue'`, - output: `import type {ResponsiveValue} from '@primer/react'`, + output: `import {type ResponsiveValue} from '@primer/react'`, errors: [ { messageId: 'wildcardMigration', @@ -391,7 +406,7 @@ import type {ButtonBaseProps} from '@primer/react'`, // @primer/react/lib-esm/sx { code: `import type {BetterSystemStyleObject, SxProp, BetterCssProperties} from '@primer/react/lib-esm/sx'`, - output: `import type {BetterSystemStyleObject, SxProp, BetterCssProperties} from '@primer/react'`, + output: `import {type BetterSystemStyleObject, type SxProp, type BetterCssProperties} from '@primer/react'`, errors: [ { messageId: 'wildcardMigration', diff --git a/src/rules/no-wildcard-imports.js b/src/rules/no-wildcard-imports.js index 82e3c164..36425594 100644 --- a/src/rules/no-wildcard-imports.js +++ b/src/rules/no-wildcard-imports.js @@ -265,6 +265,20 @@ module.exports = { create(context) { return { ImportDeclaration(node) { + if (node.source.value === '@primer/react/lib-esm/utils/test-helpers') { + context.report({ + node, + messageId: 'wildcardMigration', + data: { + wildcardEntrypoint: node.source.value, + }, + fix(fixer) { + return fixer.replaceText(node.source, `'@primer/react/test-helpers'`) + }, + }) + return + } + if (!node.source.value.startsWith('@primer/react/lib-esm')) { return } @@ -340,64 +354,124 @@ module.exports = { }, *fix(fixer) { for (const [entrypoint, importSpecifiers] of changes) { - const typeSpecifiers = importSpecifiers.filter(([, , type]) => { - return type === 'type' + const importDeclaration = node.parent.body.find(node => { + return ( + node.type === 'ImportDeclaration' && node.source.value === entrypoint && node.importKind !== 'type' + ) + }) + const typeImportDeclaration = node.parent.body.find(node => { + return ( + node.type === 'ImportDeclaration' && node.source.value === entrypoint && node.importKind === 'type' + ) + }) + let originalImportReplaced = false + const namedSpecifiers = importSpecifiers.filter(([imported, , type]) => { + return imported !== 'default' && type !== 'type' + }) + const namedTypeSpecifiers = importSpecifiers.filter(([imported, , type]) => { + return imported !== 'default' && type === 'type' + }) + let defaultSpecifier = importSpecifiers.find(([imported, , type]) => { + return imported === 'default' && type !== 'type' + }) + if (defaultSpecifier) { + defaultSpecifier = defaultSpecifier[1] + } + let defaultTypeSpecifier = importSpecifiers.find(([imported, , type]) => { + return imported === 'default' && type === 'type' }) + if (defaultTypeSpecifier) { + defaultTypeSpecifier = `type ${defaultTypeSpecifier[1]}` + } - // If all imports are type imports, emit emit as `import type {specifier} from '...'` - if (typeSpecifiers.length === importSpecifiers.length) { - const namedSpecifiers = typeSpecifiers.filter(([imported]) => { - return imported !== 'default' - }) - const defaultSpecifier = typeSpecifiers.find(([imported]) => { - return imported === 'default' - }) + // Reuse a type import if it exists + if (typeImportDeclaration) { + const firstSpecifier = typeImportDeclaration.specifiers[0] + const lastSpecifier = typeImportDeclaration.specifiers[typeImportDeclaration.specifiers.length - 1] - if (namedSpecifiers.length > 0 && !defaultSpecifier) { - const specifiers = namedSpecifiers.map(([imported, local]) => { + if (defaultTypeSpecifier) { + const postfix = + namedTypeSpecifiers.length > 0 || typeImportDeclaration.specifiers.length > 0 ? ', ' : ' ' + yield fixer.insertTextBeforeRange( + [firstSpecifier.range[0] - 2, firstSpecifier.range[1]], + `${defaultTypeSpecifier}${postfix}`, + ) + } + + if (namedTypeSpecifiers.length > 0) { + const specifiers = namedTypeSpecifiers.map(([imported, local]) => { if (imported !== local) { return `${imported} as ${local}` } return imported }) - yield fixer.replaceText(node, `import type {${specifiers.join(', ')}} from '${entrypoint}'`) - } else if (namedSpecifiers.length > 0 && defaultSpecifier) { - yield fixer.replaceText( - node, - `import type ${defaultSpecifier[1]}, ${specifiers.join(', ')} from '${entrypoint}'`, - ) - } else if (defaultSpecifier && namedSpecifiers.length === 0) { - yield fixer.replaceText(node, `import type ${defaultSpecifier[1]} from '${entrypoint}'`) + yield fixer.insertTextAfter(lastSpecifier, `, ${specifiers.join(', ')}`) } - - return } - // Otherwise, we have a mix of type and value imports to emit - const valueSpecifiers = importSpecifiers.filter(([, , type]) => { - return type !== 'type' - }) - - if (valueSpecifiers.length === 0) { - return - } + // Reuse an import declaration if one exists + if (importDeclaration) { + const firstSpecifier = importDeclaration.specifiers[0] + const lastSpecifier = importDeclaration.specifiers[importDeclaration.specifiers.length - 1] - const specifiers = valueSpecifiers.map(([imported, local]) => { - if (imported !== local) { - return `${imported} as ${local}` + if (defaultSpecifier) { + const postfix = namedSpecifiers.length > 0 || importDeclaration.specifiers.length > 0 ? ', ' : ' ' + yield fixer.insertTextBeforeRange( + [firstSpecifier.range[0] - 2, firstSpecifier.range[1]], + `${defaultSpecifier}${postfix}`, + ) } - return imported - }) - yield fixer.replaceText(node, `import {${specifiers.join(', ')}} from '${entrypoint}'`) - if (typeSpecifiers.length > 0) { - const specifiers = typeSpecifiers.map(([imported, local]) => { + if (namedSpecifiers.length > 0 || (!typeImportDeclaration && namedTypeSpecifiers.length > 0)) { + let specifiers = [...namedSpecifiers] + if (!typeImportDeclaration) { + specifiers.push(...namedTypeSpecifiers) + } + specifiers = specifiers.map(([imported, local, type]) => { + const prefix = type === 'type' ? 'type ' : '' + if (imported !== local) { + return `${prefix}${imported} as ${local}` + } + return `${prefix}${imported}` + }) + yield fixer.insertTextAfter(lastSpecifier, `, ${specifiers.join(', ')}`) + } + } else { + let specifiers = [...namedSpecifiers] + if (!typeImportDeclaration) { + specifiers.push(...namedTypeSpecifiers) + } + specifiers = specifiers.map(([imported, local, type]) => { + const prefix = type === 'type' ? 'type ' : '' if (imported !== local) { - return `${imported} as ${local}` + return `${prefix}${imported} as ${local}` } - return imported + return `${prefix}${imported}` }) - yield fixer.insertTextAfter(node, `\nimport type {${specifiers.join(', ')}} from '${entrypoint}'`) + let declaration = 'import ' + + if (defaultSpecifier) { + declaration += defaultSpecifier + } + + if (defaultTypeSpecifier && !typeImportDeclaration) { + declaration += defaultTypeSpecifier + } + + if (specifiers.length > 0) { + if (defaultSpecifier || defaultTypeSpecifier) { + declaration += ', ' + } + declaration += `{${specifiers.join(', ')}}` + } + + declaration += ` from '${entrypoint}'` + yield fixer.replaceText(node, declaration) + originalImportReplaced = true + } + + if (!originalImportReplaced) { + yield fixer.remove(node) } } },