diff --git a/.changeset/short-pumas-kneel.md b/.changeset/short-pumas-kneel.md new file mode 100644 index 0000000..80378d1 --- /dev/null +++ b/.changeset/short-pumas-kneel.md @@ -0,0 +1,5 @@ +--- +"eslint-plugin-primer-react": minor +--- + +use-styled-react-import: Add ThemeProvider, BaseStyles and useTheme. Allow theme components to be imported from styled-react without sx diff --git a/src/rules/__tests__/use-styled-react-import.test.js b/src/rules/__tests__/use-styled-react-import.test.js index db4489a..aa5cd22 100644 --- a/src/rules/__tests__/use-styled-react-import.test.js +++ b/src/rules/__tests__/use-styled-react-import.test.js @@ -3,6 +3,7 @@ const {RuleTester} = require('eslint') const ruleTester = new RuleTester({ languageOptions: { + parser: require(require.resolve('@typescript-eslint/parser', {paths: [require.resolve('eslint-plugin-github')]})), ecmaVersion: 'latest', sourceType: 'module', parserOptions: { @@ -25,6 +26,8 @@ ruleTester.run('use-styled-react-import', rule, { // Valid: Utilities imported from styled-react `import { sx } from '@primer/styled-react'`, + `import { useTheme } from '@primer/styled-react'`, + `import { sx, useTheme } from '@primer/styled-react'`, // Valid: Component not in the styled list `import { Avatar } from '@primer/react' @@ -40,6 +43,14 @@ ruleTester.run('use-styled-react-import', rule, { // Valid: Component without sx prop imported from styled-react (when not used) `import { Button } from '@primer/styled-react'`, + + // Valid: allowedComponents without sx prop imported from styled-react + `import { ThemeProvider, BaseStyles } from '@primer/styled-react' + const Component = ({children}) => {children}`, + + // Valid: Component with sx prop AND allowedComponents + `import { ThemeProvider, Button } from '@primer/styled-react' + const Component = () => `, ], invalid: [ // Invalid: Box with sx prop imported from @primer/react @@ -205,6 +216,43 @@ import { Box } from '@primer/styled-react' }, ], }, + { + code: `import { useTheme } from '@primer/react'`, + output: `import { useTheme } from '@primer/styled-react'`, + errors: [ + { + messageId: 'moveToStyledReact', + data: {importName: 'useTheme'}, + }, + ], + }, + { + code: `import { sx, useTheme } from '@primer/react'`, + output: `import { sx, useTheme } from '@primer/styled-react'`, + errors: [ + { + messageId: 'moveToStyledReact', + data: {importName: 'sx'}, + }, + { + messageId: 'moveToStyledReact', + data: {importName: 'useTheme'}, + }, + ], + }, + + // Invalid: Utility import from @primer/react that should be from styled-react, mixed with other imports + { + code: `import { sx, useAnchoredPosition } from '@primer/react'`, + output: `import { useAnchoredPosition } from '@primer/react' +import { sx } from '@primer/styled-react'`, + errors: [ + { + messageId: 'moveToStyledReact', + data: {importName: 'sx'}, + }, + ], + }, // Invalid: Button and Link, only Button uses sx { @@ -335,6 +383,61 @@ import { Button } from '@primer/react' }, ], }, + + // Invalid: ThemeProvider and BaseStyles - should move to styled-react + { + code: ` + import { ThemeProvider, BaseStyles } from '@primer/react' + `, + output: ` + import { ThemeProvider, BaseStyles } from '@primer/styled-react' + `, + errors: [ + { + messageId: 'moveToStyledReact', + data: {importName: 'ThemeProvider'}, + }, + { + messageId: 'moveToStyledReact', + data: {importName: 'BaseStyles'}, + }, + ], + }, + + // Invalid: ThemeProvider, Button without sx prop - only ThemeProvider should be from styled-react + { + code: ` + import { ThemeProvider, Button } from '@primer/react' + const Component = () => + `, + output: ` + import { Button } from '@primer/react' +import { ThemeProvider } from '@primer/styled-react' + const Component = () => + `, + errors: [ + { + messageId: 'moveToStyledReact', + data: {importName: 'ThemeProvider'}, + }, + ], + }, + + // Invalid: Utility and type imports from @primer/react that should be from styled-react + { + code: `import { sx, type SxProp } from '@primer/react'`, + output: `import { sx, type SxProp } from '@primer/styled-react'`, + errors: [ + { + messageId: 'moveToStyledReact', + data: {importName: 'sx'}, + }, + { + messageId: 'moveToStyledReact', + data: {importName: 'SxProp'}, + }, + ], + }, ], }) diff --git a/src/rules/use-styled-react-import.js b/src/rules/use-styled-react-import.js index 9b1d8ff..d6ee943 100644 --- a/src/rules/use-styled-react-import.js +++ b/src/rules/use-styled-react-import.js @@ -23,13 +23,17 @@ const defaultStyledComponents = [ 'Truncate', 'Octicon', 'Dialog', + 'ThemeProvider', + 'BaseStyles', ] +const componentsToAlwaysImportFromStyledReact = new Set(['ThemeProvider', 'BaseStyles']) + // Default types that should be imported from @primer/styled-react const defaultStyledTypes = ['BoxProps', 'SxProp', 'BetterSystemStyleObject'] // Default utilities that should be imported from @primer/styled-react -const defaultStyledUtilities = ['sx'] +const defaultStyledUtilities = ['sx', 'useTheme'] /** * @type {import('eslint').Rule.RuleModule} @@ -246,7 +250,11 @@ module.exports = { // Report errors for components used WITHOUT sx prop that are imported from @primer/styled-react for (const componentName of allUsedComponents) { // If component is used but NOT with sx prop, and it's imported from styled-react - if (!componentsWithSx.has(componentName) && styledReactImports.has(componentName)) { + if ( + !componentsWithSx.has(componentName) && + styledReactImports.has(componentName) && + !componentsToAlwaysImportFromStyledReact.has(componentName) + ) { const importInfo = styledReactImports.get(componentName) context.report({ node: importInfo.specifier, @@ -337,53 +345,67 @@ module.exports = { } } - // Also report for types and utilities that should always be from styled-react + // Also report for types, utilities and components that should always be from styled-react for (const [importName, importInfo] of primerReactImports) { - if ((styledTypes.has(importName) || styledUtilities.has(importName)) && !styledReactImports.has(importName)) { + if ( + (styledTypes.has(importName) || + styledUtilities.has(importName) || + componentsToAlwaysImportFromStyledReact.has(importName)) && + !styledReactImports.has(importName) + ) { context.report({ node: importInfo.specifier, messageId: 'moveToStyledReact', data: {importName}, fix(fixer) { const {node: importNode, specifier, importSource} = importInfo - const otherSpecifiers = importNode.specifiers.filter(s => s !== specifier) - // Convert @primer/react path to @primer/styled-react path - const styledReactPath = importSource.replace('@primer/react', '@primer/styled-react') + const fixes = [] - // If this is the only import, replace the whole import - if (otherSpecifiers.length === 0) { - const prefix = styledTypes.has(importName) ? 'type ' : '' - return fixer.replaceText(importNode, `import { ${prefix}${importName} } from '${styledReactPath}'`) - } + // we consolidate all the fixes for the import in the first specifier + const isFirst = importNode.specifiers[0] === specifier + if (!isFirst) return null - // Otherwise, remove from current import and add new import - const fixes = [] + const specifiersToMove = importNode.specifiers.filter(specifier => { + const name = specifier.imported.name + return ( + styledUtilities.has(name) || + styledTypes.has(name) || + componentsToAlwaysImportFromStyledReact.has(name) + ) + }) + + const remainingSpecifiers = importNode.specifiers.filter(specifier => { + return !specifiersToMove.includes(specifier) + }) + + // Convert @primer/react path to @primer/styled-react path + const styledReactPath = importSource.replace('@primer/react', '@primer/styled-react') - // Remove the specifier from current import - if (importNode.specifiers.length === 1) { + if (remainingSpecifiers.length === 0) { + // if there are no remaining specifiers, we can remove the whole import fixes.push(fixer.remove(importNode)) } else { - const isFirst = importNode.specifiers[0] === specifier - const isLast = importNode.specifiers[importNode.specifiers.length - 1] === specifier - - if (isFirst) { - const nextSpecifier = importNode.specifiers[1] - fixes.push(fixer.removeRange([specifier.range[0], nextSpecifier.range[0]])) - } else if (isLast) { - const prevSpecifier = importNode.specifiers[importNode.specifiers.length - 2] - fixes.push(fixer.removeRange([prevSpecifier.range[1], specifier.range[1]])) - } else { - const nextSpecifier = importNode.specifiers[importNode.specifiers.indexOf(specifier) + 1] - fixes.push(fixer.removeRange([specifier.range[0], nextSpecifier.range[0]])) - } + const remainingNames = remainingSpecifiers.map(spec => + spec.importKind === 'type' ? `type ${spec.imported.name}` : spec.imported.name, + ) + fixes.push( + fixer.replaceText(importNode, `import { ${remainingNames.join(', ')} } from '${importSource}'`), + ) } - // Add new import - const prefix = styledTypes.has(importName) ? 'type ' : '' - fixes.push( - fixer.insertTextAfter(importNode, `\nimport { ${prefix}${importName} } from '${styledReactPath}'`), - ) + if (specifiersToMove.length > 0) { + const movedComponents = specifiersToMove.map(spec => + spec.importKind === 'type' ? `type ${spec.imported.name}` : spec.imported.name, + ) + const onNewLine = remainingSpecifiers.length > 0 + fixes.push( + fixer.insertTextAfter( + importNode, + `${onNewLine ? '\n' : ''}import { ${movedComponents.join(', ')} } from '${styledReactPath}'`, + ), + ) + } return fixes },