From 3e7d16203d8c0df36ee4e5bebca93c457832568f Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Wed, 6 Aug 2025 20:21:11 +0000 Subject: [PATCH 01/10] Initial plan From d35c34c7ad843fc6eb5b5b24c9ee7c63922a243c Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Wed, 6 Aug 2025 20:37:14 +0000 Subject: [PATCH 02/10] Add no-deprecated-octicon ESLint rule to replace Octicon with specific icons Co-authored-by: joshblack <3901764+joshblack@users.noreply.github.com> --- src/index.js | 1 + .../__tests__/no-deprecated-octicon.test.js | 188 ++++++++++++++++++ src/rules/no-deprecated-octicon.js | 124 ++++++++++++ 3 files changed, 313 insertions(+) create mode 100644 src/rules/__tests__/no-deprecated-octicon.test.js create mode 100644 src/rules/no-deprecated-octicon.js diff --git a/src/index.js b/src/index.js index 68de6f2..a8c51a4 100644 --- a/src/index.js +++ b/src/index.js @@ -19,6 +19,7 @@ module.exports = { 'prefer-action-list-item-onselect': require('./rules/prefer-action-list-item-onselect'), 'enforce-css-module-identifier-casing': require('./rules/enforce-css-module-identifier-casing'), 'enforce-css-module-default-import': require('./rules/enforce-css-module-default-import'), + 'no-deprecated-octicon': require('./rules/no-deprecated-octicon'), }, configs: { recommended: require('./configs/recommended'), diff --git a/src/rules/__tests__/no-deprecated-octicon.test.js b/src/rules/__tests__/no-deprecated-octicon.test.js new file mode 100644 index 0000000..af5418c --- /dev/null +++ b/src/rules/__tests__/no-deprecated-octicon.test.js @@ -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 +}`, + }, + + // Already using direct icon import + { + code: `import {XIcon} from '@primer/octicons-react' +export default function App() { + return +}`, + }, + + // Octicon without icon prop (edge case - can't transform) + { + code: `import {Octicon} from '@primer/react/deprecated' +export default function App() { + return +}`, + }, + ], + + 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 +}`, + output: `import {Octicon} from '@primer/react/deprecated' +import {XIcon} from '@primer/octicons-react' +export default function App() { + return +}`, + 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 +}`, + output: `import {Octicon} from '@primer/react/deprecated' +import {XIcon} from '@primer/octicons-react' +export default function App() { + return +}`, + 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 +}`, + output: `import {Octicon} from '@primer/react/deprecated' +import {XIcon} from '@primer/octicons-react' +export default function App() { + const props = { size: 16 } + return +}`, + 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 + Content + +}`, + output: `import {Octicon} from '@primer/react/deprecated' +import {XIcon} from '@primer/octicons-react' +export default function App() { + return + Content + +}`, + errors: [ + { + messageId: 'replaceDeprecatedOcticon', + }, + ], + }, + + // Multiple Octicons + { + code: `import {Octicon} from '@primer/react/deprecated' +import {XIcon, CheckIcon} from '@primer/octicons-react' +export default function App() { + return ( +
+ + +
+ ) +}`, + output: `import {Octicon} from '@primer/react/deprecated' +import {XIcon, CheckIcon} from '@primer/octicons-react' +export default function App() { + return ( +
+ + +
+ ) +}`, + 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 +}`, + output: null, + errors: [ + { + messageId: 'replaceDeprecatedOcticon', + }, + ], + }, + + // Dynamic icon access - should report but not autofix + { + code: `import {Octicon} from '@primer/react/deprecated' +export default function App() { + const icons = { x: XIcon } + return +}`, + output: null, + errors: [ + { + messageId: 'replaceDeprecatedOcticon', + }, + ], + }, + ], +}) diff --git a/src/rules/no-deprecated-octicon.js b/src/rules/no-deprecated-octicon.js new file mode 100644 index 0000000..887d773 --- /dev/null +++ b/src/rules/no-deprecated-octicon.js @@ -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', + }) + } + }, + } + }, +} From 5f920de26cf2cb03fca0034e460612fb3a83185b Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Wed, 6 Aug 2025 21:17:14 +0000 Subject: [PATCH 03/10] Add autofix support for conditional and dynamic icon usage Co-authored-by: joshblack <3901764+joshblack@users.noreply.github.com> --- .../__tests__/no-deprecated-octicon.test.js | 54 +++++++++- src/rules/no-deprecated-octicon.js | 102 ++++++++++++++++-- 2 files changed, 144 insertions(+), 12 deletions(-) diff --git a/src/rules/__tests__/no-deprecated-octicon.test.js b/src/rules/__tests__/no-deprecated-octicon.test.js index af5418c..ceca22b 100644 --- a/src/rules/__tests__/no-deprecated-octicon.test.js +++ b/src/rules/__tests__/no-deprecated-octicon.test.js @@ -155,14 +155,37 @@ export default function App() { ], }, - // Complex conditional case - should report but not autofix + // Complex conditional case - now provides autofix { code: `import {Octicon} from '@primer/react/deprecated' import {XIcon, CheckIcon} from '@primer/octicons-react' export default function App() { return }`, - output: null, + output: `import {Octicon} from '@primer/react/deprecated' +import {XIcon, CheckIcon} from '@primer/octicons-react' +export default function App() { + return condition ? : +}`, + errors: [ + { + messageId: 'replaceDeprecatedOcticon', + }, + ], + }, + + // Complex conditional case with props - applies props to both components + { + code: `import {Octicon} from '@primer/react/deprecated' +import {XIcon, CheckIcon} from '@primer/octicons-react' +export default function App() { + return +}`, + output: `import {Octicon} from '@primer/react/deprecated' +import {XIcon, CheckIcon} from '@primer/octicons-react' +export default function App() { + return condition ? : +}`, errors: [ { messageId: 'replaceDeprecatedOcticon', @@ -170,14 +193,37 @@ export default function App() { ], }, - // Dynamic icon access - should report but not autofix + // Dynamic icon access - now provides autofix { code: `import {Octicon} from '@primer/react/deprecated' export default function App() { const icons = { x: XIcon } return }`, - output: null, + output: `import {Octicon} from '@primer/react/deprecated' +export default function App() { + const icons = { x: XIcon } + return React.createElement(icons.x, {}) +}`, + errors: [ + { + messageId: 'replaceDeprecatedOcticon', + }, + ], + }, + + // Dynamic icon access with props + { + code: `import {Octicon} from '@primer/react/deprecated' +export default function App() { + const icons = { x: XIcon } + return +}`, + output: `import {Octicon} from '@primer/react/deprecated' +export default function App() { + const icons = { x: XIcon } + return React.createElement(icons.x, {size: 16, className: "btn-icon"}) +}`, errors: [ { messageId: 'replaceDeprecatedOcticon', diff --git a/src/rules/no-deprecated-octicon.js b/src/rules/no-deprecated-octicon.js index 887d773..8f8ffbc 100644 --- a/src/rules/no-deprecated-octicon.js +++ b/src/rules/no-deprecated-octicon.js @@ -42,9 +42,12 @@ module.exports = { } let iconName = null - let isDynamic = false + let isConditional = false + let isMemberExpression = false + let conditionalExpression = null + let memberExpression = null - // Analyze the icon prop to determine the icon name + // Analyze the icon prop to determine the icon name and type if (iconProp.value?.type === 'JSXExpressionContainer') { const expression = iconProp.value.expression @@ -53,20 +56,25 @@ module.exports = { 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 + isConditional = true + conditionalExpression = expression } else if (expression.type === 'MemberExpression') { // Dynamic lookup: icon={icons.x} - isDynamic = true + isMemberExpression = true + memberExpression = expression } } - if (!iconName && !isDynamic) { + if (!iconName && !isConditional && !isMemberExpression) { return } + // Get all props except the icon prop to preserve them + const otherProps = openingElement.attributes.filter(attr => attr !== iconProp) + const propsText = otherProps.map(attr => sourceCode.getText(attr)).join(' ') + // For simple cases, we can provide an autofix - if (iconName && !isDynamic) { + if (iconName) { context.report({ node: openingElement, messageId: 'replaceDeprecatedOcticon', @@ -111,8 +119,86 @@ module.exports = { } }, }) + } else if (isConditional) { + // Handle conditional expressions: icon={condition ? XIcon : YIcon} + // Transform to: condition ? : + context.report({ + node: openingElement, + messageId: 'replaceDeprecatedOcticon', + *fix(fixer) { + const test = sourceCode.getText(conditionalExpression.test) + const consequentName = conditionalExpression.consequent.type === 'Identifier' + ? conditionalExpression.consequent.name + : sourceCode.getText(conditionalExpression.consequent) + const alternateName = conditionalExpression.alternate.type === 'Identifier' + ? conditionalExpression.alternate.name + : sourceCode.getText(conditionalExpression.alternate) + + const propsString = propsText ? ` ${propsText}` : '' + let replacement = `${test} ? <${consequentName}${propsString} /> : <${alternateName}${propsString} />` + + // If it has children, we need to include them in both branches + if (node.children && node.children.length > 0) { + const childrenText = node.children.map(child => sourceCode.getText(child)).join('') + replacement = `${test} ? <${consequentName}${propsString}>${childrenText} : <${alternateName}${propsString}>${childrenText}` + } + + yield fixer.replaceText(node, replacement) + }, + }) + } else if (isMemberExpression) { + // Handle member expressions: icon={icons.x} + // Transform to: React.createElement(icons.x, otherProps) + context.report({ + node: openingElement, + messageId: 'replaceDeprecatedOcticon', + *fix(fixer) { + const memberText = sourceCode.getText(memberExpression) + + // Build props object + let propsObject = '{}' + if (otherProps.length > 0) { + const propStrings = otherProps.map(attr => { + if (attr.type === 'JSXSpreadAttribute') { + return `...${sourceCode.getText(attr.argument)}` + } else { + const name = attr.name.name + const value = attr.value + if (!value) { + return `${name}: true` + } else if (value.type === 'Literal') { + return `${name}: ${JSON.stringify(value.value)}` + } else if (value.type === 'JSXExpressionContainer') { + return `${name}: ${sourceCode.getText(value.expression)}` + } + return `${name}: ${sourceCode.getText(value)}` + } + }) + propsObject = `{${propStrings.join(', ')}}` + } + + let replacement = `React.createElement(${memberText}, ${propsObject})` + + // If it has children, include them as additional arguments + if (node.children && node.children.length > 0) { + const childrenArgs = node.children.map(child => { + if (child.type === 'JSXText') { + return JSON.stringify(child.value.trim()).replace(/\n\s*/g, ' ') + } else { + return sourceCode.getText(child) + } + }).filter(child => child !== '""') // Filter out empty text nodes + + if (childrenArgs.length > 0) { + replacement = `React.createElement(${memberText}, ${propsObject}, ${childrenArgs.join(', ')})` + } + } + + yield fixer.replaceText(node, replacement) + }, + }) } else { - // For complex cases, just report without autofix + // For other complex cases, just report without autofix context.report({ node: openingElement, messageId: 'replaceDeprecatedOcticon', From ba61171cb43a4c3ef6202476da1970cf6ed0e532 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Thu, 7 Aug 2025 21:42:56 +0000 Subject: [PATCH 04/10] Add import removal functionality to no-deprecated-octicon rule Co-authored-by: joshblack <3901764+joshblack@users.noreply.github.com> --- .../__tests__/no-deprecated-octicon.test.js | 83 +++++++++++++---- src/rules/no-deprecated-octicon.js | 88 +++++++++++++++++++ 2 files changed, 153 insertions(+), 18 deletions(-) diff --git a/src/rules/__tests__/no-deprecated-octicon.test.js b/src/rules/__tests__/no-deprecated-octicon.test.js index ceca22b..341be4e 100644 --- a/src/rules/__tests__/no-deprecated-octicon.test.js +++ b/src/rules/__tests__/no-deprecated-octicon.test.js @@ -48,8 +48,7 @@ import {XIcon} from '@primer/octicons-react' export default function App() { return }`, - output: `import {Octicon} from '@primer/react/deprecated' -import {XIcon} from '@primer/octicons-react' + output: `import {XIcon} from '@primer/octicons-react' export default function App() { return }`, @@ -67,8 +66,7 @@ import {XIcon} from '@primer/octicons-react' export default function App() { return }`, - output: `import {Octicon} from '@primer/react/deprecated' -import {XIcon} from '@primer/octicons-react' + output: `import {XIcon} from '@primer/octicons-react' export default function App() { return }`, @@ -87,8 +85,7 @@ export default function App() { const props = { size: 16 } return }`, - output: `import {Octicon} from '@primer/react/deprecated' -import {XIcon} from '@primer/octicons-react' + output: `import {XIcon} from '@primer/octicons-react' export default function App() { const props = { size: 16 } return @@ -109,8 +106,7 @@ export default function App() { Content }`, - output: `import {Octicon} from '@primer/react/deprecated' -import {XIcon} from '@primer/octicons-react' + output: `import {XIcon} from '@primer/octicons-react' export default function App() { return Content @@ -135,8 +131,7 @@ export default function App() { ) }`, - output: `import {Octicon} from '@primer/react/deprecated' -import {XIcon, CheckIcon} from '@primer/octicons-react' + output: `import {XIcon, CheckIcon} from '@primer/octicons-react' export default function App() { return (
@@ -162,8 +157,7 @@ import {XIcon, CheckIcon} from '@primer/octicons-react' export default function App() { return }`, - output: `import {Octicon} from '@primer/react/deprecated' -import {XIcon, CheckIcon} from '@primer/octicons-react' + output: `import {XIcon, CheckIcon} from '@primer/octicons-react' export default function App() { return condition ? : }`, @@ -181,8 +175,7 @@ import {XIcon, CheckIcon} from '@primer/octicons-react' export default function App() { return }`, - output: `import {Octicon} from '@primer/react/deprecated' -import {XIcon, CheckIcon} from '@primer/octicons-react' + output: `import {XIcon, CheckIcon} from '@primer/octicons-react' export default function App() { return condition ? : }`, @@ -200,8 +193,7 @@ export default function App() { const icons = { x: XIcon } return }`, - output: `import {Octicon} from '@primer/react/deprecated' -export default function App() { + output: `export default function App() { const icons = { x: XIcon } return React.createElement(icons.x, {}) }`, @@ -219,10 +211,65 @@ export default function App() { const icons = { x: XIcon } return }`, - output: `import {Octicon} from '@primer/react/deprecated' -export default function App() { + output: `export default function App() { const icons = { x: XIcon } return React.createElement(icons.x, {size: 16, className: "btn-icon"}) +}`, + errors: [ + { + messageId: 'replaceDeprecatedOcticon', + }, + ], + }, + + // Test import removal - single Octicon import gets removed + { + code: `import {Octicon} from '@primer/react/deprecated' +import {XIcon} from '@primer/octicons-react' +export default function App() { + return +}`, + output: `import {XIcon} from '@primer/octicons-react' +export default function App() { + return +}`, + errors: [ + { + messageId: 'replaceDeprecatedOcticon', + }, + ], + }, + + // Test partial import removal - Octicon removed but other imports remain + { + code: `import {Octicon, Button} from '@primer/react/deprecated' +import {XIcon} from '@primer/octicons-react' +export default function App() { + return +}`, + output: `import {Button} from '@primer/react/deprecated' +import {XIcon} from '@primer/octicons-react' +export default function App() { + return +}`, + errors: [ + { + messageId: 'replaceDeprecatedOcticon', + }, + ], + }, + + // Test partial import removal - Octicon in middle of import list + { + code: `import {Button, Octicon, TextField} from '@primer/react/deprecated' +import {XIcon} from '@primer/octicons-react' +export default function App() { + return +}`, + output: `import {Button, TextField} from '@primer/react/deprecated' +import {XIcon} from '@primer/octicons-react' +export default function App() { + return }`, errors: [ { diff --git a/src/rules/no-deprecated-octicon.js b/src/rules/no-deprecated-octicon.js index 8f8ffbc..2f32633 100644 --- a/src/rules/no-deprecated-octicon.js +++ b/src/rules/no-deprecated-octicon.js @@ -24,8 +24,25 @@ module.exports = { }, create(context) { const sourceCode = context.getSourceCode() + + // Track Octicon imports + const octiconImports = [] return { + ImportDeclaration(node) { + if (node.source.value !== '@primer/react/deprecated') { + return + } + + const hasOcticon = node.specifiers.some( + specifier => specifier.imported && specifier.imported.name === 'Octicon', + ) + + if (hasOcticon) { + octiconImports.push(node) + } + }, + JSXElement(node) { const {openingElement, closingElement} = node const elementName = getJSXOpeningElementName(openingElement) @@ -73,6 +90,68 @@ module.exports = { const otherProps = openingElement.attributes.filter(attr => attr !== iconProp) const propsText = otherProps.map(attr => sourceCode.getText(attr)).join(' ') + // Helper function to determine if this is the last Octicon in the file + function isLastOcticon() { + // Get all Octicon elements using the source code + const sourceText = sourceCode.getText() + const octiconMatches = [...sourceText.matchAll(/ match.index > currentNodeStart) + return laterOcticons.length === 0 + } + + // Helper function to generate import fixes if this is the last Octicon usage + function* generateImportFixes(fixer) { + if (isLastOcticon() && octiconImports.length > 0) { + const importNode = octiconImports[0] + const octiconSpecifier = importNode.specifiers.find( + specifier => specifier.imported && specifier.imported.name === 'Octicon', + ) + + if (importNode.specifiers.length === 1) { + // Octicon is the only import, remove the entire import statement + // Also remove trailing newline if present + const nextToken = sourceCode.getTokenAfter(importNode) + const importEnd = importNode.range[1] + const nextStart = nextToken ? nextToken.range[0] : sourceCode.getText().length + const textBetween = sourceCode.getText().substring(importEnd, nextStart) + const hasTrailingNewline = /^\s*\n/.test(textBetween) + + if (hasTrailingNewline) { + const newlineMatch = textBetween.match(/^\s*\n/) + const endRange = importEnd + newlineMatch[0].length + yield fixer.removeRange([importNode.range[0], endRange]) + } else { + yield fixer.remove(importNode) + } + } else { + // Remove just the Octicon specifier from the import + const previousToken = sourceCode.getTokenBefore(octiconSpecifier) + const nextToken = sourceCode.getTokenAfter(octiconSpecifier) + const hasTrailingComma = nextToken && nextToken.value === ',' + const hasLeadingComma = previousToken && previousToken.value === ',' + + let rangeToRemove + if (hasTrailingComma) { + rangeToRemove = [octiconSpecifier.range[0], nextToken.range[1] + 1] + } else if (hasLeadingComma) { + rangeToRemove = [previousToken.range[0], octiconSpecifier.range[1]] + } else { + rangeToRemove = [octiconSpecifier.range[0], octiconSpecifier.range[1]] + } + yield fixer.removeRange(rangeToRemove) + } + } + } + // For simple cases, we can provide an autofix if (iconName) { context.report({ @@ -117,6 +196,9 @@ module.exports = { : iconProp.range[0] yield fixer.removeRange([startPos, iconProp.range[1]]) } + + // Handle import removal if this is the last Octicon usage + yield* generateImportFixes(fixer) }, }) } else if (isConditional) { @@ -144,6 +226,9 @@ module.exports = { } yield fixer.replaceText(node, replacement) + + // Handle import removal if this is the last Octicon usage + yield* generateImportFixes(fixer) }, }) } else if (isMemberExpression) { @@ -195,6 +280,9 @@ module.exports = { } yield fixer.replaceText(node, replacement) + + // Handle import removal if this is the last Octicon usage + yield* generateImportFixes(fixer) }, }) } else { From ab189c58f3da65ea3e0c3d52aa9a4fcaeac81ad7 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Thu, 7 Aug 2025 21:51:23 +0000 Subject: [PATCH 05/10] Implement import removal with edge case documented Co-authored-by: joshblack <3901764+joshblack@users.noreply.github.com> --- src/rules/no-deprecated-octicon.js | 30 +++++++++++++++++------------- 1 file changed, 17 insertions(+), 13 deletions(-) diff --git a/src/rules/no-deprecated-octicon.js b/src/rules/no-deprecated-octicon.js index 2f32633..9314e7d 100644 --- a/src/rules/no-deprecated-octicon.js +++ b/src/rules/no-deprecated-octicon.js @@ -90,27 +90,31 @@ module.exports = { const otherProps = openingElement.attributes.filter(attr => attr !== iconProp) const propsText = otherProps.map(attr => sourceCode.getText(attr)).join(' ') - // Helper function to determine if this is the last Octicon in the file - function isLastOcticon() { - // Get all Octicon elements using the source code + // Helper function to determine if this is the last Octicon in the file that needs fixing + function isLastOcticonToFix() { + // Get all JSX elements in the source code that are Octicons with icon props const sourceText = sourceCode.getText() - const octiconMatches = [...sourceText.matchAll(/ { + if (line.includes(' Date: Thu, 21 Aug 2025 23:52:32 -0400 Subject: [PATCH 06/10] Create tidy-beds-juggle.md --- .changeset/tidy-beds-juggle.md | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 .changeset/tidy-beds-juggle.md diff --git a/.changeset/tidy-beds-juggle.md b/.changeset/tidy-beds-juggle.md new file mode 100644 index 0000000..1982ff5 --- /dev/null +++ b/.changeset/tidy-beds-juggle.md @@ -0,0 +1,5 @@ +--- +"eslint-plugin-primer-react": minor +--- + +Add ESLint rule to replace deprecated Octicon component with specific icons and remove unused imports From e92b1e35528336500af5f3ccdfc9f5db26a51e28 Mon Sep 17 00:00:00 2001 From: Marie Lucca Date: Thu, 21 Aug 2025 23:59:11 -0400 Subject: [PATCH 07/10] lint/format --- src/rules/no-deprecated-octicon.js | 76 ++++++++++++++++-------------- 1 file changed, 40 insertions(+), 36 deletions(-) diff --git a/src/rules/no-deprecated-octicon.js b/src/rules/no-deprecated-octicon.js index 9314e7d..ae8c70b 100644 --- a/src/rules/no-deprecated-octicon.js +++ b/src/rules/no-deprecated-octicon.js @@ -24,7 +24,7 @@ module.exports = { }, create(context) { const sourceCode = context.getSourceCode() - + // Track Octicon imports const octiconImports = [] @@ -33,16 +33,16 @@ module.exports = { if (node.source.value !== '@primer/react/deprecated') { return } - + const hasOcticon = node.specifiers.some( specifier => specifier.imported && specifier.imported.name === 'Octicon', ) - + if (hasOcticon) { octiconImports.push(node) } }, - + JSXElement(node) { const {openingElement, closingElement} = node const elementName = getJSXOpeningElementName(openingElement) @@ -95,18 +95,18 @@ module.exports = { // Get all JSX elements in the source code that are Octicons with icon props const sourceText = sourceCode.getText() const lines = sourceText.split('\n') - + // Find all potential Octicon lines const octiconLines = [] - lines.forEach((line, index) => { + for (const [index, line] of lines.entries()) { if (line.includes(' : <${alternateName}${propsString} />` - + // If it has children, we need to include them in both branches if (node.children && node.children.length > 0) { const childrenText = node.children.map(child => sourceCode.getText(child)).join('') replacement = `${test} ? <${consequentName}${propsString}>${childrenText} : <${alternateName}${propsString}>${childrenText}` } - + yield fixer.replaceText(node, replacement) - + // Handle import removal if this is the last Octicon usage yield* generateImportFixes(fixer) }, @@ -243,7 +245,7 @@ module.exports = { messageId: 'replaceDeprecatedOcticon', *fix(fixer) { const memberText = sourceCode.getText(memberExpression) - + // Build props object let propsObject = '{}' if (otherProps.length > 0) { @@ -265,26 +267,28 @@ module.exports = { }) propsObject = `{${propStrings.join(', ')}}` } - + let replacement = `React.createElement(${memberText}, ${propsObject})` - + // If it has children, include them as additional arguments if (node.children && node.children.length > 0) { - const childrenArgs = node.children.map(child => { - if (child.type === 'JSXText') { - return JSON.stringify(child.value.trim()).replace(/\n\s*/g, ' ') - } else { - return sourceCode.getText(child) - } - }).filter(child => child !== '""') // Filter out empty text nodes - + const childrenArgs = node.children + .map(child => { + if (child.type === 'JSXText') { + return JSON.stringify(child.value.trim()).replace(/\n\s*/g, ' ') + } else { + return sourceCode.getText(child) + } + }) + .filter(child => child !== '""') // Filter out empty text nodes + if (childrenArgs.length > 0) { replacement = `React.createElement(${memberText}, ${propsObject}, ${childrenArgs.join(', ')})` } } - + yield fixer.replaceText(node, replacement) - + // Handle import removal if this is the last Octicon usage yield* generateImportFixes(fixer) }, From af5a2965f668850ea849b6a2a09d896c2d139e59 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Fri, 22 Aug 2025 04:12:11 +0000 Subject: [PATCH 08/10] Fix test configuration to use flat config format Co-authored-by: francinelucca <40550942+francinelucca@users.noreply.github.com> --- .../__tests__/no-deprecated-octicon.test.js | 26 +-- src/rules/no-deprecated-octicon.js | 202 ++++++++++++------ 2 files changed, 137 insertions(+), 91 deletions(-) diff --git a/src/rules/__tests__/no-deprecated-octicon.test.js b/src/rules/__tests__/no-deprecated-octicon.test.js index 341be4e..dfe5279 100644 --- a/src/rules/__tests__/no-deprecated-octicon.test.js +++ b/src/rules/__tests__/no-deprecated-octicon.test.js @@ -4,11 +4,13 @@ const {RuleTester} = require('eslint') const rule = require('../no-deprecated-octicon') const ruleTester = new RuleTester({ - parserOptions: { + languageOptions: { ecmaVersion: 'latest', sourceType: 'module', - ecmaFeatures: { - jsx: true, + parserOptions: { + ecmaFeatures: { + jsx: true, + }, }, }, }) @@ -222,24 +224,6 @@ export default function App() { ], }, - // Test import removal - single Octicon import gets removed - { - code: `import {Octicon} from '@primer/react/deprecated' -import {XIcon} from '@primer/octicons-react' -export default function App() { - return -}`, - output: `import {XIcon} from '@primer/octicons-react' -export default function App() { - return -}`, - errors: [ - { - messageId: 'replaceDeprecatedOcticon', - }, - ], - }, - // Test partial import removal - Octicon removed but other imports remain { code: `import {Octicon, Button} from '@primer/react/deprecated' diff --git a/src/rules/no-deprecated-octicon.js b/src/rules/no-deprecated-octicon.js index ae8c70b..762ac93 100644 --- a/src/rules/no-deprecated-octicon.js +++ b/src/rules/no-deprecated-octicon.js @@ -25,8 +25,15 @@ module.exports = { create(context) { const sourceCode = context.getSourceCode() - // Track Octicon imports + // Track Octicon imports and usages const octiconImports = [] + let totalOcticonUsages = 0 + let processedOcticonUsages = 0 + + // Count total Octicon usages with icon props at the start + const sourceText = sourceCode.getText() + const octiconMatches = sourceText.match(/]*icon=/g) + totalOcticonUsages = octiconMatches ? octiconMatches.length : 0 return { ImportDeclaration(node) { @@ -90,72 +97,6 @@ module.exports = { const otherProps = openingElement.attributes.filter(attr => attr !== iconProp) const propsText = otherProps.map(attr => sourceCode.getText(attr)).join(' ') - // Helper function to determine if this is the last Octicon in the file that needs fixing - function isLastOcticonToFix() { - // Get all JSX elements in the source code that are Octicons with icon props - const sourceText = sourceCode.getText() - const lines = sourceText.split('\n') - - // Find all potential Octicon lines - const octiconLines = [] - for (const [index, line] of lines.entries()) { - if (line.includes(' 0) { + const importNode = octiconImports[0] + const octiconSpecifier = importNode.specifiers.find( + specifier => specifier.imported && specifier.imported.name === 'Octicon', + ) + + if (importNode.specifiers.length === 1) { + // Octicon is the only import, remove the entire import statement + // Also remove trailing newline if present + const nextToken = sourceCode.getTokenAfter(importNode) + const importEnd = importNode.range[1] + const nextStart = nextToken ? nextToken.range[0] : sourceCode.getText().length + const textBetween = sourceCode.getText().substring(importEnd, nextStart) + const hasTrailingNewline = /^\s*\n/.test(textBetween) + + if (hasTrailingNewline) { + const newlineMatch = textBetween.match(/^\s*\n/) + const endRange = importEnd + newlineMatch[0].length + yield fixer.removeRange([importNode.range[0], endRange]) + } else { + yield fixer.remove(importNode) + } + } else { + // Remove just the Octicon specifier from the import + const previousToken = sourceCode.getTokenBefore(octiconSpecifier) + const nextToken = sourceCode.getTokenAfter(octiconSpecifier) + const hasTrailingComma = nextToken && nextToken.value === ',' + const hasLeadingComma = previousToken && previousToken.value === ',' + + let rangeToRemove + if (hasTrailingComma) { + rangeToRemove = [octiconSpecifier.range[0], nextToken.range[1] + 1] + } else if (hasLeadingComma) { + rangeToRemove = [previousToken.range[0], octiconSpecifier.range[1]] + } else { + rangeToRemove = [octiconSpecifier.range[0], octiconSpecifier.range[1]] + } + yield fixer.removeRange(rangeToRemove) + } + } }, }) } else if (isConditional) { @@ -234,7 +216,47 @@ module.exports = { yield fixer.replaceText(node, replacement) // Handle import removal if this is the last Octicon usage - yield* generateImportFixes(fixer) + processedOcticonUsages++ + if (processedOcticonUsages === totalOcticonUsages && octiconImports.length > 0) { + const importNode = octiconImports[0] + const octiconSpecifier = importNode.specifiers.find( + specifier => specifier.imported && specifier.imported.name === 'Octicon', + ) + + if (importNode.specifiers.length === 1) { + // Octicon is the only import, remove the entire import statement + // Also remove trailing newline if present + const nextToken = sourceCode.getTokenAfter(importNode) + const importEnd = importNode.range[1] + const nextStart = nextToken ? nextToken.range[0] : sourceCode.getText().length + const textBetween = sourceCode.getText().substring(importEnd, nextStart) + const hasTrailingNewline = /^\s*\n/.test(textBetween) + + if (hasTrailingNewline) { + const newlineMatch = textBetween.match(/^\s*\n/) + const endRange = importEnd + newlineMatch[0].length + yield fixer.removeRange([importNode.range[0], endRange]) + } else { + yield fixer.remove(importNode) + } + } else { + // Remove just the Octicon specifier from the import + const previousToken = sourceCode.getTokenBefore(octiconSpecifier) + const nextToken = sourceCode.getTokenAfter(octiconSpecifier) + const hasTrailingComma = nextToken && nextToken.value === ',' + const hasLeadingComma = previousToken && previousToken.value === ',' + + let rangeToRemove + if (hasTrailingComma) { + rangeToRemove = [octiconSpecifier.range[0], nextToken.range[1] + 1] + } else if (hasLeadingComma) { + rangeToRemove = [previousToken.range[0], octiconSpecifier.range[1]] + } else { + rangeToRemove = [octiconSpecifier.range[0], octiconSpecifier.range[1]] + } + yield fixer.removeRange(rangeToRemove) + } + } }, }) } else if (isMemberExpression) { @@ -290,7 +312,47 @@ module.exports = { yield fixer.replaceText(node, replacement) // Handle import removal if this is the last Octicon usage - yield* generateImportFixes(fixer) + processedOcticonUsages++ + if (processedOcticonUsages === totalOcticonUsages && octiconImports.length > 0) { + const importNode = octiconImports[0] + const octiconSpecifier = importNode.specifiers.find( + specifier => specifier.imported && specifier.imported.name === 'Octicon', + ) + + if (importNode.specifiers.length === 1) { + // Octicon is the only import, remove the entire import statement + // Also remove trailing newline if present + const nextToken = sourceCode.getTokenAfter(importNode) + const importEnd = importNode.range[1] + const nextStart = nextToken ? nextToken.range[0] : sourceCode.getText().length + const textBetween = sourceCode.getText().substring(importEnd, nextStart) + const hasTrailingNewline = /^\s*\n/.test(textBetween) + + if (hasTrailingNewline) { + const newlineMatch = textBetween.match(/^\s*\n/) + const endRange = importEnd + newlineMatch[0].length + yield fixer.removeRange([importNode.range[0], endRange]) + } else { + yield fixer.remove(importNode) + } + } else { + // Remove just the Octicon specifier from the import + const previousToken = sourceCode.getTokenBefore(octiconSpecifier) + const nextToken = sourceCode.getTokenAfter(octiconSpecifier) + const hasTrailingComma = nextToken && nextToken.value === ',' + const hasLeadingComma = previousToken && previousToken.value === ',' + + let rangeToRemove + if (hasTrailingComma) { + rangeToRemove = [octiconSpecifier.range[0], nextToken.range[1] + 1] + } else if (hasLeadingComma) { + rangeToRemove = [previousToken.range[0], octiconSpecifier.range[1]] + } else { + rangeToRemove = [octiconSpecifier.range[0], octiconSpecifier.range[1]] + } + yield fixer.removeRange(rangeToRemove) + } + } }, }) } else { From f98df37263c03469d5346221e4f6f7b72da2399e Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Fri, 22 Aug 2025 04:25:47 +0000 Subject: [PATCH 09/10] Fix multiple Octicons transformation issue and address PR comments Co-authored-by: francinelucca <40550942+francinelucca@users.noreply.github.com> --- .../__tests__/no-deprecated-octicon.test.js | 3 ++- src/rules/no-deprecated-octicon.js | 19 ++++++++++++++++--- 2 files changed, 18 insertions(+), 4 deletions(-) diff --git a/src/rules/__tests__/no-deprecated-octicon.test.js b/src/rules/__tests__/no-deprecated-octicon.test.js index dfe5279..6c9055b 100644 --- a/src/rules/__tests__/no-deprecated-octicon.test.js +++ b/src/rules/__tests__/no-deprecated-octicon.test.js @@ -133,7 +133,8 @@ export default function App() {
) }`, - output: `import {XIcon, CheckIcon} from '@primer/octicons-react' + output: `import {Octicon} from '@primer/react/deprecated' +import {XIcon, CheckIcon} from '@primer/octicons-react' export default function App() { return (
diff --git a/src/rules/no-deprecated-octicon.js b/src/rules/no-deprecated-octicon.js index 762ac93..c9b63d3 100644 --- a/src/rules/no-deprecated-octicon.js +++ b/src/rules/no-deprecated-octicon.js @@ -145,7 +145,12 @@ module.exports = { // Handle import removal if this is the last Octicon usage // Only check for last octicon after incrementing counter to ensure accurate count processedOcticonUsages++ - if (processedOcticonUsages === totalOcticonUsages && octiconImports.length > 0) { + // Disable import removal for multiple Octicons to avoid fix conflicts (temporary fix) + if ( + processedOcticonUsages === totalOcticonUsages && + totalOcticonUsages === 1 && + octiconImports.length > 0 + ) { const importNode = octiconImports[0] const octiconSpecifier = importNode.specifiers.find( specifier => specifier.imported && specifier.imported.name === 'Octicon', @@ -217,7 +222,11 @@ module.exports = { // Handle import removal if this is the last Octicon usage processedOcticonUsages++ - if (processedOcticonUsages === totalOcticonUsages && octiconImports.length > 0) { + if ( + processedOcticonUsages === totalOcticonUsages && + totalOcticonUsages === 1 && + octiconImports.length > 0 + ) { const importNode = octiconImports[0] const octiconSpecifier = importNode.specifiers.find( specifier => specifier.imported && specifier.imported.name === 'Octicon', @@ -313,7 +322,11 @@ module.exports = { // Handle import removal if this is the last Octicon usage processedOcticonUsages++ - if (processedOcticonUsages === totalOcticonUsages && octiconImports.length > 0) { + if ( + processedOcticonUsages === totalOcticonUsages && + totalOcticonUsages === 1 && + octiconImports.length > 0 + ) { const importNode = octiconImports[0] const octiconSpecifier = importNode.specifiers.find( specifier => specifier.imported && specifier.imported.name === 'Octicon', From 292afead9928c7298c363db600d930be9595f4e0 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Tue, 26 Aug 2025 02:09:00 +0000 Subject: [PATCH 10/10] Clean up import removal logic comments and clarify intended behavior Co-authored-by: francinelucca <40550942+francinelucca@users.noreply.github.com> --- src/rules/no-deprecated-octicon.js | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/src/rules/no-deprecated-octicon.js b/src/rules/no-deprecated-octicon.js index c9b63d3..d57d606 100644 --- a/src/rules/no-deprecated-octicon.js +++ b/src/rules/no-deprecated-octicon.js @@ -142,10 +142,9 @@ module.exports = { yield fixer.removeRange([startPos, iconProp.range[1]]) } - // Handle import removal if this is the last Octicon usage - // Only check for last octicon after incrementing counter to ensure accurate count + // Import removal: only enabled for single Octicon cases to prevent ESLint fix conflicts + // For multiple Octicons, JSX transformation works but import remains (can be cleaned up manually) processedOcticonUsages++ - // Disable import removal for multiple Octicons to avoid fix conflicts (temporary fix) if ( processedOcticonUsages === totalOcticonUsages && totalOcticonUsages === 1 && @@ -220,7 +219,8 @@ module.exports = { yield fixer.replaceText(node, replacement) - // Handle import removal if this is the last Octicon usage + // Import removal: only enabled for single Octicon cases to prevent ESLint fix conflicts + // For multiple Octicons, JSX transformation works but import remains (can be cleaned up manually) processedOcticonUsages++ if ( processedOcticonUsages === totalOcticonUsages && @@ -320,7 +320,8 @@ module.exports = { yield fixer.replaceText(node, replacement) - // Handle import removal if this is the last Octicon usage + // Import removal: only enabled for single Octicon cases to prevent ESLint fix conflicts + // For multiple Octicons, JSX transformation works but import remains (can be cleaned up manually) processedOcticonUsages++ if ( processedOcticonUsages === totalOcticonUsages &&