-
Notifications
You must be signed in to change notification settings - Fork 10
Add ESLint rule to replace deprecated Octicon component with specific icons and remove unused imports #376
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
@joshblack 👋 This repository doesn't have Copilot instructions. With Copilot instructions, I can understand the repository better, work faster and produce higher quality PRs. I can generate a .github/copilot-instructions.md file for you automatically. Click here to open a pre-filled issue and assign it to me. I'll write the instructions, and then tag you for review. |
|
…c icons Co-authored-by: joshblack <[email protected]>
@primer/octicons-react
// Complex conditional case - should report but not autofix | ||
{ | ||
code: `import {Octicon} from '@primer/react/deprecated' | ||
import {XIcon, CheckIcon} from '@primer/octicons-react' | ||
export default function App() { | ||
return <Octicon icon={condition ? XIcon : CheckIcon} /> | ||
}`, | ||
output: null, | ||
errors: [ | ||
{ | ||
messageId: 'replaceDeprecatedOcticon', | ||
}, | ||
], | ||
}, |
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.
Update this rule to autofix like:
// Complex conditional case - should report but not autofix | |
{ | |
code: `import {Octicon} from '@primer/react/deprecated' | |
import {XIcon, CheckIcon} from '@primer/octicons-react' | |
export default function App() { | |
return <Octicon icon={condition ? XIcon : CheckIcon} /> | |
}`, | |
output: null, | |
errors: [ | |
{ | |
messageId: 'replaceDeprecatedOcticon', | |
}, | |
], | |
}, | |
// Complex conditional case - should report but not autofix | |
{ | |
code: `import {Octicon} from '@primer/react/deprecated' | |
import {XIcon, CheckIcon} from '@primer/octicons-react' | |
export default function App() { | |
return condition ? <XIcon /> : <CheckIcon /> | |
}`, | |
output: null, | |
errors: [ | |
{ | |
messageId: 'replaceDeprecatedOcticon', | |
}, | |
], | |
}, |
It should also apply the same props from Octicon to each component in the condition
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.
@copilot make this change
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.
Done! I've updated the rule to autofix conditional expressions by transforming <Octicon icon={condition ? XIcon : CheckIcon} />
to condition ? <XIcon /> : <CheckIcon />
and properly apply all props from Octicon to each component in the condition. Added test cases covering both scenarios with and without additional props. Commit: d35c34c
], | ||
}, | ||
|
||
// Dynamic icon access - should report but not autofix |
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.
Add the autofix for this scenario, it should create a React element from this icon like React.createElement(icons.x)
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.
Added! The rule now autofixes dynamic icon access scenarios by transforming <Octicon icon={icons.x} />
to React.createElement(icons.x, {})
and properly handles all props. Added test cases for both with and without props scenarios. Commit: d35c34c
Co-authored-by: joshblack <[email protected]>
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.
@copilot update this rule to also include removing the import to Octicon
if all Octicon
s have been replaced
Co-authored-by: joshblack <[email protected]>
Added import removal functionality! The rule now automatically removes the |
Co-authored-by: joshblack <[email protected]>
}, | ||
} | ||
}, | ||
} |
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 PR adds a new ESLint rule
no-deprecated-octicon
that automatically transforms deprecatedOcticon
component usage to use specific icon components directly from@primer/octicons-react
. The rule also removes unusedOcticon
imports when all usages have been replaced.What it does
The rule transforms various usage patterns and cleans up imports:
Basic transformation with import removal
Partial import cleanup
Advanced transformations
The rule also supports:
Conditional expressions:
Dynamic icon access:
Key features
<Octicon icon={IconName} />
with<IconName />
Octicon
from imports when no longer neededReact.createElement
for dynamic icon access patternsImplementation details
The rule uses AST analysis to identify JSX elements using the
Octicon
component, extract and analyze theicon
prop to determine the appropriate transformation strategy, and generate fixes that include both JSX transformation and import cleanup when appropriate.The import removal logic carefully handles comma placement and whitespace to ensure clean output in all scenarios.
Fixes #375.
💬 Share your feedback on Copilot coding agent for the chance to win a $200 gift card! Click here to start the survey.