Skip to content

Commit 7f5af96

Browse files
Copilotjonrohan
andauthored
Fix use-styled-react-import fixer stripping type keyword from import specifiers (#548)
* Initial plan * Fix: preserve type keyword in import specifiers when moving imports between @primer/react and @primer/styled-react Agent-Logs-Url: https://github.com/primer/eslint-plugin-primer-react/sessions/333e9cfd-bc26-4b6b-ae2d-24fe1479cda9 Co-authored-by: jonrohan <54012+jonrohan@users.noreply.github.com> * Create breezy-geese-flash.md --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: jonrohan <54012+jonrohan@users.noreply.github.com> Co-authored-by: Jon Rohan <jonrohan@github.com>
1 parent 32fa317 commit 7f5af96

3 files changed

Lines changed: 74 additions & 18 deletions

File tree

.changeset/breezy-geese-flash.md

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
---
2+
"eslint-plugin-primer-react": patch
3+
---
4+
5+
Fix use-styled-react-import fixer stripping type keyword from import specifiers

src/rules/__tests__/use-styled-react-import.test.js

Lines changed: 35 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -364,7 +364,7 @@ import { Button } from '@primer/react'
364364
<Button sx={{ color: 'red' }}>Styled button</Button>
365365
</div>
366366
)`,
367-
output: `import { Link, Button } from '@primer/styled-react'
367+
output: `import { Button, Link } from '@primer/styled-react'
368368
const Component = () => (
369369
<div>
370370
<Link sx={{ color: 'red' }} />
@@ -438,6 +438,40 @@ import { ThemeProvider } from '@primer/styled-react'
438438
},
439439
],
440440
},
441+
442+
// Invalid: type keyword preserved when splitting imports - component with sx moved to styled-react
443+
{
444+
code: `import { Button, type ButtonProps } from '@primer/react'
445+
const Component = () => <Button sx={{ color: 'red' }}>Click me</Button>`,
446+
output: `import { type ButtonProps } from '@primer/react'
447+
import { Button } from '@primer/styled-react'
448+
const Component = () => <Button sx={{ color: 'red' }}>Click me</Button>`,
449+
errors: [
450+
{
451+
messageId: 'useStyledReactImport',
452+
data: {componentName: 'Button'},
453+
},
454+
],
455+
},
456+
457+
// Invalid: type keyword preserved on moved specifiers (type + utility)
458+
{
459+
code: `import { type SxProp, sx, Button } from '@primer/react'
460+
const Component = () => <Button>Click me</Button>`,
461+
output: `import { Button } from '@primer/react'
462+
import { type SxProp, sx } from '@primer/styled-react'
463+
const Component = () => <Button>Click me</Button>`,
464+
errors: [
465+
{
466+
messageId: 'moveToStyledReact',
467+
data: {importName: 'SxProp'},
468+
},
469+
{
470+
messageId: 'moveToStyledReact',
471+
data: {importName: 'sx'},
472+
},
473+
],
474+
},
441475
],
442476
})
443477

src/rules/use-styled-react-import.js

Lines changed: 34 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,15 @@
33
const url = require('../url')
44
const {getJSXOpeningElementName} = require('../utils/get-jsx-opening-element-name')
55

6+
/**
7+
* Format a specifier name, preserving the `type` keyword if present.
8+
* @param {import('estree').ImportSpecifier} spec
9+
* @returns {string}
10+
*/
11+
function formatSpecifier(spec) {
12+
return spec.importKind === 'type' ? `type ${spec.imported.name}` : spec.imported.name
13+
}
14+
615
// Default components that should be imported from @primer/styled-react when used with sx prop
716
const defaultStyledComponents = [
817
'ActionList',
@@ -197,19 +206,24 @@ module.exports = {
197206
// Convert @primer/react path to @primer/styled-react path
198207
const styledReactPath = importSource.replace('@primer/react', '@primer/styled-react')
199208

209+
// Find the original specifier nodes for moved components to preserve importKind
210+
const movedSpecifiers = changes.originalSpecifiers.filter(spec =>
211+
componentsToMove.has(spec.imported.name),
212+
)
213+
200214
// If no components remain, replace with new import directly
201215
if (remainingSpecifiers.length === 0) {
202-
const movedComponents = changes.toMove.join(', ')
216+
const movedComponents = movedSpecifiers.map(formatSpecifier).join(', ')
203217
fixes.push(fixer.replaceText(importNode, `import { ${movedComponents} } from '${styledReactPath}'`))
204218
} else {
205219
// Otherwise, update the import to only include remaining components
206-
const remainingNames = remainingSpecifiers.map(spec => spec.imported.name)
220+
const remainingNames = remainingSpecifiers.map(formatSpecifier)
207221
fixes.push(
208222
fixer.replaceText(importNode, `import { ${remainingNames.join(', ')} } from '${importSource}'`),
209223
)
210224

211225
// Add new styled-react import
212-
const movedComponents = changes.toMove.join(', ')
226+
const movedComponents = movedSpecifiers.map(formatSpecifier).join(', ')
213227
fixes.push(
214228
fixer.insertTextAfter(importNode, `\nimport { ${movedComponents} } from '${styledReactPath}'`),
215229
)
@@ -290,10 +304,16 @@ module.exports = {
290304
// Check if there's an existing primer-react import to merge with
291305
const existingPrimerReactImport = Array.from(primerReactImportNodes)[0]
292306

307+
// Find the original specifier nodes for moved components to preserve importKind
308+
const movedSpecifiers = changes.originalSpecifiers.filter(spec =>
309+
componentsToMove.has(spec.imported.name),
310+
)
311+
293312
if (existingPrimerReactImport && remainingSpecifiers.length === 0) {
294313
// Case: No remaining styled-react imports, merge with existing primer-react import
295-
const existingSpecifiers = existingPrimerReactImport.specifiers.map(spec => spec.imported.name)
296-
const newSpecifiers = [...existingSpecifiers, ...changes.toMove].filter(
314+
const existingNames = existingPrimerReactImport.specifiers.map(formatSpecifier)
315+
const movedNames = movedSpecifiers.map(formatSpecifier)
316+
const newSpecifiers = [...existingNames, ...movedNames].filter(
297317
(name, index, arr) => arr.indexOf(name) === index,
298318
)
299319

@@ -306,8 +326,9 @@ module.exports = {
306326
fixes.push(fixer.remove(importNode))
307327
} else if (existingPrimerReactImport && remainingSpecifiers.length > 0) {
308328
// Case: Some styled-react imports remain, merge moved components with existing primer-react
309-
const existingSpecifiers = existingPrimerReactImport.specifiers.map(spec => spec.imported.name)
310-
const newSpecifiers = [...existingSpecifiers, ...changes.toMove].filter(
329+
const existingNames = existingPrimerReactImport.specifiers.map(formatSpecifier)
330+
const movedNames = movedSpecifiers.map(formatSpecifier)
331+
const newSpecifiers = [...existingNames, ...movedNames].filter(
311332
(name, index, arr) => arr.indexOf(name) === index,
312333
)
313334

@@ -318,22 +339,22 @@ module.exports = {
318339
),
319340
)
320341

321-
const remainingNames = remainingSpecifiers.map(spec => spec.imported.name)
342+
const remainingNames = remainingSpecifiers.map(formatSpecifier)
322343
fixes.push(
323344
fixer.replaceText(importNode, `import { ${remainingNames.join(', ')} } from '${importSource}'`),
324345
)
325346
} else if (remainingSpecifiers.length === 0) {
326347
// Case: No existing primer-react import, no remaining styled-react imports
327-
const movedComponents = changes.toMove.join(', ')
348+
const movedComponents = movedSpecifiers.map(formatSpecifier).join(', ')
328349
fixes.push(fixer.replaceText(importNode, `import { ${movedComponents} } from '${primerReactPath}'`))
329350
} else {
330351
// Case: No existing primer-react import, some styled-react imports remain
331-
const remainingNames = remainingSpecifiers.map(spec => spec.imported.name)
352+
const remainingNames = remainingSpecifiers.map(formatSpecifier)
332353
fixes.push(
333354
fixer.replaceText(importNode, `import { ${remainingNames.join(', ')} } from '${importSource}'`),
334355
)
335356

336-
const movedComponents = changes.toMove.join(', ')
357+
const movedComponents = movedSpecifiers.map(formatSpecifier).join(', ')
337358
fixes.push(
338359
fixer.insertTextAfter(importNode, `\nimport { ${movedComponents} } from '${primerReactPath}'`),
339360
)
@@ -386,18 +407,14 @@ module.exports = {
386407
// if there are no remaining specifiers, we can remove the whole import
387408
fixes.push(fixer.remove(importNode))
388409
} else {
389-
const remainingNames = remainingSpecifiers.map(spec =>
390-
spec.importKind === 'type' ? `type ${spec.imported.name}` : spec.imported.name,
391-
)
410+
const remainingNames = remainingSpecifiers.map(formatSpecifier)
392411
fixes.push(
393412
fixer.replaceText(importNode, `import { ${remainingNames.join(', ')} } from '${importSource}'`),
394413
)
395414
}
396415

397416
if (specifiersToMove.length > 0) {
398-
const movedComponents = specifiersToMove.map(spec =>
399-
spec.importKind === 'type' ? `type ${spec.imported.name}` : spec.imported.name,
400-
)
417+
const movedComponents = specifiersToMove.map(formatSpecifier)
401418
const onNewLine = remainingSpecifiers.length > 0
402419
fixes.push(
403420
fixer.insertTextAfter(

0 commit comments

Comments
 (0)