-
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?
Changes from 2 commits
3e7d162
d35c34c
5f920de
ba61171
ab189c5
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,188 @@ | ||
'use strict' | ||
|
||
const {RuleTester} = require('eslint') | ||
const rule = require('../no-deprecated-octicon') | ||
|
||
const ruleTester = new RuleTester({ | ||
parserOptions: { | ||
ecmaVersion: 'latest', | ||
sourceType: 'module', | ||
ecmaFeatures: { | ||
jsx: true, | ||
}, | ||
}, | ||
}) | ||
|
||
ruleTester.run('no-deprecated-octicon', rule, { | ||
valid: [ | ||
// Not an Octicon component | ||
{ | ||
code: `import {Button} from '@primer/react' | ||
export default function App() { | ||
return <Button>Click me</Button> | ||
}`, | ||
}, | ||
|
||
// Already using direct icon import | ||
{ | ||
code: `import {XIcon} from '@primer/octicons-react' | ||
export default function App() { | ||
return <XIcon /> | ||
}`, | ||
}, | ||
|
||
// Octicon without icon prop (edge case - can't transform) | ||
{ | ||
code: `import {Octicon} from '@primer/react/deprecated' | ||
export default function App() { | ||
return <Octicon /> | ||
}`, | ||
}, | ||
], | ||
|
||
invalid: [ | ||
// Basic case: simple Octicon with icon prop | ||
{ | ||
code: `import {Octicon} from '@primer/react/deprecated' | ||
import {XIcon} from '@primer/octicons-react' | ||
export default function App() { | ||
return <Octicon icon={XIcon} /> | ||
}`, | ||
output: `import {Octicon} from '@primer/react/deprecated' | ||
import {XIcon} from '@primer/octicons-react' | ||
export default function App() { | ||
return <XIcon /> | ||
}`, | ||
errors: [ | ||
{ | ||
messageId: 'replaceDeprecatedOcticon', | ||
}, | ||
], | ||
}, | ||
|
||
// Octicon with additional props | ||
{ | ||
code: `import {Octicon} from '@primer/react/deprecated' | ||
import {XIcon} from '@primer/octicons-react' | ||
export default function App() { | ||
return <Octicon icon={XIcon} size={16} className="test" /> | ||
}`, | ||
output: `import {Octicon} from '@primer/react/deprecated' | ||
import {XIcon} from '@primer/octicons-react' | ||
export default function App() { | ||
return <XIcon size={16} className="test" /> | ||
}`, | ||
errors: [ | ||
{ | ||
messageId: 'replaceDeprecatedOcticon', | ||
}, | ||
], | ||
}, | ||
|
||
// Octicon with spread props | ||
{ | ||
code: `import {Octicon} from '@primer/react/deprecated' | ||
import {XIcon} from '@primer/octicons-react' | ||
export default function App() { | ||
const props = { size: 16 } | ||
return <Octicon {...props} icon={XIcon} className="test" /> | ||
}`, | ||
output: `import {Octicon} from '@primer/react/deprecated' | ||
import {XIcon} from '@primer/octicons-react' | ||
export default function App() { | ||
const props = { size: 16 } | ||
return <XIcon {...props} className="test" /> | ||
}`, | ||
errors: [ | ||
{ | ||
messageId: 'replaceDeprecatedOcticon', | ||
}, | ||
], | ||
}, | ||
|
||
// Octicon with closing tag | ||
{ | ||
code: `import {Octicon} from '@primer/react/deprecated' | ||
import {XIcon} from '@primer/octicons-react' | ||
export default function App() { | ||
return <Octicon icon={XIcon}> | ||
<span>Content</span> | ||
</Octicon> | ||
}`, | ||
output: `import {Octicon} from '@primer/react/deprecated' | ||
import {XIcon} from '@primer/octicons-react' | ||
export default function App() { | ||
return <XIcon> | ||
<span>Content</span> | ||
</XIcon> | ||
}`, | ||
errors: [ | ||
{ | ||
messageId: 'replaceDeprecatedOcticon', | ||
}, | ||
], | ||
}, | ||
|
||
// Multiple Octicons | ||
{ | ||
code: `import {Octicon} from '@primer/react/deprecated' | ||
import {XIcon, CheckIcon} from '@primer/octicons-react' | ||
export default function App() { | ||
return ( | ||
<div> | ||
<Octicon icon={XIcon} /> | ||
<Octicon icon={CheckIcon} size={24} /> | ||
</div> | ||
) | ||
}`, | ||
output: `import {Octicon} from '@primer/react/deprecated' | ||
import {XIcon, CheckIcon} from '@primer/octicons-react' | ||
export default function App() { | ||
return ( | ||
<div> | ||
<XIcon /> | ||
<CheckIcon size={24} /> | ||
</div> | ||
) | ||
}`, | ||
errors: [ | ||
{ | ||
messageId: 'replaceDeprecatedOcticon', | ||
}, | ||
{ | ||
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 <Octicon icon={condition ? XIcon : CheckIcon} /> | ||
}`, | ||
output: null, | ||
errors: [ | ||
{ | ||
messageId: 'replaceDeprecatedOcticon', | ||
}, | ||
], | ||
}, | ||
|
||
// Dynamic icon access - should report but not autofix | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Added! The rule now autofixes dynamic icon access scenarios by transforming |
||
{ | ||
code: `import {Octicon} from '@primer/react/deprecated' | ||
export default function App() { | ||
const icons = { x: XIcon } | ||
return <Octicon icon={icons.x} /> | ||
}`, | ||
output: null, | ||
errors: [ | ||
{ | ||
messageId: 'replaceDeprecatedOcticon', | ||
}, | ||
], | ||
}, | ||
], | ||
}) |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,124 @@ | ||
'use strict' | ||
|
||
const {getJSXOpeningElementAttribute} = require('../utils/get-jsx-opening-element-attribute') | ||
const {getJSXOpeningElementName} = require('../utils/get-jsx-opening-element-name') | ||
const url = require('../url') | ||
|
||
/** | ||
* @type {import('eslint').Rule.RuleModule} | ||
*/ | ||
module.exports = { | ||
meta: { | ||
type: 'problem', | ||
docs: { | ||
description: 'Replace deprecated `Octicon` component with specific icon imports from `@primer/octicons-react`', | ||
recommended: true, | ||
url: url(module), | ||
}, | ||
fixable: 'code', | ||
schema: [], | ||
messages: { | ||
replaceDeprecatedOcticon: | ||
'Replace deprecated `Octicon` component with the specific icon from `@primer/octicons-react`', | ||
}, | ||
}, | ||
create(context) { | ||
const sourceCode = context.getSourceCode() | ||
|
||
return { | ||
JSXElement(node) { | ||
const {openingElement, closingElement} = node | ||
const elementName = getJSXOpeningElementName(openingElement) | ||
|
||
if (elementName !== 'Octicon') { | ||
return | ||
} | ||
|
||
// Get the icon prop | ||
const iconProp = getJSXOpeningElementAttribute(openingElement, 'icon') | ||
if (!iconProp) { | ||
// No icon prop - can't determine what to replace with | ||
return | ||
} | ||
|
||
let iconName = null | ||
let isDynamic = false | ||
|
||
// Analyze the icon prop to determine the icon name | ||
if (iconProp.value?.type === 'JSXExpressionContainer') { | ||
const expression = iconProp.value.expression | ||
|
||
if (expression.type === 'Identifier') { | ||
// Simple case: icon={XIcon} | ||
iconName = expression.name | ||
} else if (expression.type === 'ConditionalExpression') { | ||
// Conditional case: icon={condition ? XIcon : YIcon} | ||
// For now, we'll skip auto-fixing complex conditionals | ||
isDynamic = true | ||
} else if (expression.type === 'MemberExpression') { | ||
// Dynamic lookup: icon={icons.x} | ||
isDynamic = true | ||
} | ||
} | ||
|
||
if (!iconName && !isDynamic) { | ||
return | ||
} | ||
|
||
// For simple cases, we can provide an autofix | ||
if (iconName && !isDynamic) { | ||
context.report({ | ||
node: openingElement, | ||
messageId: 'replaceDeprecatedOcticon', | ||
*fix(fixer) { | ||
// Replace opening element name | ||
yield fixer.replaceText(openingElement.name, iconName) | ||
|
||
// Replace closing element name if it exists | ||
if (closingElement) { | ||
yield fixer.replaceText(closingElement.name, iconName) | ||
} | ||
|
||
// Remove the icon prop with proper whitespace handling | ||
// Use the JSXAttribute node's properties to determine proper removal boundaries | ||
const attributes = openingElement.attributes | ||
const iconIndex = attributes.indexOf(iconProp) | ||
|
||
if (iconIndex === 0 && attributes.length === 1) { | ||
// Only attribute: remove with leading space | ||
const beforeIcon = sourceCode.getTokenBefore(iconProp) | ||
const startPos = | ||
beforeIcon && /\s/.test(sourceCode.getText().substring(beforeIcon.range[1], iconProp.range[0])) | ||
? beforeIcon.range[1] | ||
: iconProp.range[0] | ||
yield fixer.removeRange([startPos, iconProp.range[1]]) | ||
} else if (iconIndex === 0) { | ||
// First attribute: remove including trailing whitespace/comma | ||
const afterIcon = attributes[1] | ||
const afterPos = sourceCode.getText().substring(iconProp.range[1], afterIcon.range[0]) | ||
const whitespaceMatch = /^\s*/.exec(afterPos) | ||
const endPos = whitespaceMatch ? iconProp.range[1] + whitespaceMatch[0].length : iconProp.range[1] | ||
yield fixer.removeRange([iconProp.range[0], endPos]) | ||
} else { | ||
// Not first attribute: remove including leading whitespace/comma | ||
const beforeIcon = attributes[iconIndex - 1] | ||
const beforePos = sourceCode.getText().substring(beforeIcon.range[1], iconProp.range[0]) | ||
const whitespaceMatch = /\s*$/.exec(beforePos) | ||
const startPos = whitespaceMatch | ||
? beforeIcon.range[1] + beforePos.length - whitespaceMatch[0].length | ||
: iconProp.range[0] | ||
yield fixer.removeRange([startPos, iconProp.range[1]]) | ||
} | ||
}, | ||
}) | ||
} else { | ||
// For complex cases, just report without autofix | ||
context.report({ | ||
node: openingElement, | ||
messageId: 'replaceDeprecatedOcticon', | ||
}) | ||
} | ||
}, | ||
} | ||
}, | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 💥 |
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:
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} />
tocondition ? <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