fix(createIcon): Fix broken API in createIcon.tsx#12336
Open
tlabaj wants to merge 5 commits intopatternfly:mainfrom
Open
fix(createIcon): Fix broken API in createIcon.tsx#12336tlabaj wants to merge 5 commits intopatternfly:mainfrom
tlabaj wants to merge 5 commits intopatternfly:mainfrom
Conversation
WalkthroughAdds runtime normalization to accept both legacy flat and new nested icon shapes, splits icon type definitions, enforces rendering from Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Collaborator
|
Preview: https://pf-react-pr-12336.surge.sh A11y report: https://pf-react-pr-12336-a11y.surge.sh |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/react-icons/src/createIcon.tsx`:
- Around line 86-99: normalizeCreateIconArg currently allows nested
CreateIconProps to return with icon undefined which later causes invalid SVG
output; update normalizeCreateIconArg (the isNestedCreateIconProps branch) to
validate that p.icon is present (not null/undefined) and throw a clear Error
(e.g., "createIcon: missing default 'icon' for nested props 'name'") if it's
missing before calling normalizeIconDefinition; still normalize rhUiIcon as
before using normalizeIconDefinition when non-null. Ensure the thrown message
references the name (p.name) so callers can identify the bad input.
- Around line 17-30: The public IconDefinition union causes consumers to need
type-narrowing; replace the union by exporting a single interface that extends
IconDefinitionBase and includes both svgPathData and the deprecated svgPath
(marked in JSDoc), then update the exported IconDefinition type to that single
interface; specifically, remove IconDefinitionWithSvgPath |
IconDefinitionWithSvgPathData as the union, keep IconDefinitionWithSvgPathData
and IconDefinitionWithSvgPath (or collapse them) but ensure IconDefinition
refers to the unified interface so helpers can access svgPathData directly and
existing internals like resolveSvgPathData() continue to work without changes.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d1c895fb-106c-4779-b20e-c9d2064e6bd2
📒 Files selected for processing (2)
packages/react-icons/src/__tests__/createIcon.test.tsxpackages/react-icons/src/createIcon.tsx
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/react-icons/src/__tests__/createIcon.test.tsx`:
- Around line 1-2: Remove the default React import and the accompanying ESLint
suppression in createIcon.test.tsx: delete the line importing React (the default
identifier "React") and the comment "// eslint-disable-next-line
no-restricted-imports -- test file excluded from package tsconfig; default
import satisfies TS/JSX", since the project uses the automatic JSX runtime and
default React imports are disallowed by the no-restricted-imports rule.
- Around line 198-216: The test 'set="rh-ui" with no rhUiIcon mapping falls back
to default and warns' currently restores the jest.spyOn mock
(warnSpy.mockRestore()) only at the end, which can leak a mocked console.warn if
an assertion throws; wrap the test's execution (render, expects) in a
try-finally and call warnSpy.mockRestore() in the finally block so the spy
created for console.warn is always restored; locate the warnSpy declaration in
this test (const warnSpy = jest.spyOn(console, 'warn').mockImplementation(...))
and move the restore into a finally guarding the render/expect code for
IconNoRhMapping created via createIcon.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: cd163c56-8281-441d-960a-7bf1888bbda2
📒 Files selected for processing (2)
packages/react-icons/src/__tests__/createIcon.test.tsxpackages/react-icons/src/createIcon.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/react-icons/src/createIcon.tsx
tlabaj
requested review from
a team,
kmcfaul and
wise-king-sullyman
and removed request for
a team
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/react-icons/src/createIcon.tsx (1)
199-203: Simplify branch condition and tightenNormalizedCreateIconProps.iconto non-optional.Two small, related cleanups in this block:
- Line 199:
(set === undefined && rhUiIcon === null) || set !== undefinedreduces toset !== undefined || rhUiIcon === null.- Line 200-201:
set !== undefined && set === 'rh-ui'— the first conjunct is redundant;set === 'rh-ui'already impliesset !== undefined.NormalizedCreateIconProps.icon(line 86) is typed optional, butnormalizeCreateIconArgalways assigns it (nested path throws when null at line 97-101; legacy path always callsnormalizeIconDefinition). Making it non-optional lets you drop theiconData ?? ({} as Partial<…>)fallback on line 203 and the| undefinedon line 200, keeping the runtime invariant reflected in the types.♻️ Proposed refactor
On line 86:
- icon?: IconDefinitionWithSvgPathData; + icon: IconDefinitionWithSvgPathData;On lines 199-203:
- if ((set === undefined && rhUiIcon === null) || set !== undefined) { - const iconData: IconDefinitionWithSvgPathData | undefined = - set !== undefined && set === 'rh-ui' && rhUiIcon !== null ? rhUiIcon : icon; - const { xOffset, yOffset, width, height, svgPathData, svgClassName } = - iconData ?? ({} as Partial<IconDefinitionWithSvgPathData>); + if (set !== undefined || rhUiIcon === null) { + const iconData: IconDefinitionWithSvgPathData = + set === 'rh-ui' && rhUiIcon !== null ? rhUiIcon : icon; + const { xOffset, yOffset, width, height, svgPathData, svgClassName } = iconData;You can then drop the
icon && …guard on line 250 as well (still safe, just redundant).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/react-icons/src/createIcon.tsx` around lines 199 - 203, Simplify the conditional and tighten the icon type: change the branch `(set === undefined && rhUiIcon === null) || set !== undefined` to `set !== undefined || rhUiIcon === null`, replace `set !== undefined && set === 'rh-ui'` with `set === 'rh-ui'`, and make NormalizedCreateIconProps.icon non-optional in normalizeCreateIconArg so callers always get an IconDefinition; then remove the `| undefined` from the iconData declaration and drop the fallback `iconData ?? ({} as Partial<...>)` as well as the redundant `icon && …` guard elsewhere (e.g., the check around svg rendering).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/react-icons/src/createIcon.tsx`:
- Around line 199-203: Simplify the conditional and tighten the icon type:
change the branch `(set === undefined && rhUiIcon === null) || set !==
undefined` to `set !== undefined || rhUiIcon === null`, replace `set !==
undefined && set === 'rh-ui'` with `set === 'rh-ui'`, and make
NormalizedCreateIconProps.icon non-optional in normalizeCreateIconArg so callers
always get an IconDefinition; then remove the `| undefined` from the iconData
declaration and drop the fallback `iconData ?? ({} as Partial<...>)` as well as
the redundant `icon && …` guard elsewhere (e.g., the check around svg
rendering).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c738d752-fb52-452d-97ef-f346e6a210a6
📒 Files selected for processing (2)
packages/react-icons/src/__tests__/createIcon.test.tsxpackages/react-icons/src/createIcon.tsx
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/react-icons/src/createIcon.tsx (1)
198-207:⚠️ Potential issue | 🟡 Minor
console.warnfires on every render; also simplify redundant conditions.
- The warning at Lines 200-202 is inside
render(), so every re-render of a parent using<MyIcon set="rh-ui" />against an icon with norhUiIconre-logs the same message — easy to flood the console. Consider gating it with astaticdev-only flag on the class (or moving the check tocomponentDidMount) so it logs at most once per component.- The guard at Line 205 —
(set === undefined && rhUiIcon === null) || set !== undefined— reduces toset !== undefined || rhUiIcon === null.- At Line 207,
set !== undefined && set === 'rh-ui'is equivalent to justset === 'rh-ui'.Proposed readability tweaks (warn dedup left to your discretion)
- if ((set === undefined && rhUiIcon === null) || set !== undefined) { + if (set !== undefined || rhUiIcon === null) { const iconData: IconDefinitionWithSvgPathData | undefined = - set !== undefined && set === 'rh-ui' && rhUiIcon !== null ? rhUiIcon : icon; + set === 'rh-ui' && rhUiIcon !== null ? rhUiIcon : icon;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/react-icons/src/createIcon.tsx` around lines 198 - 207, The console.warn inside render() for when set === 'rh-ui' and rhUiIcon === null causes repeated logs on every render; move that warning to componentDidMount (or gate it with a static dev-only seen flag on the component) so it runs at most once per mounted component, and remove the eslint-disable comment. Also simplify the render guard expression by replacing "(set === undefined && rhUiIcon === null) || set !== undefined" with "set !== undefined || rhUiIcon === null", and replace the ternary condition "set !== undefined && set === 'rh-ui'" with simply "set === 'rh-ui'" when computing iconData (refer to render(), rhUiIcon, set, icon, and the IconDefinitionWithSvgPathData assignment).
🧹 Nitpick comments (2)
packages/react-icons/src/createIcon.tsx (2)
122-152: Consolidate duplicated path/viewBox rendering with the single-SVG branch.Lines 124-150 and the inline single-SVG branch at Lines 208-225 compute the same
viewBox,svgClassNamemerge, andsvgPathsarray/string rendering. Extracting a shared helper (e.g.buildSvgInner(icon)returning{ viewBox, svgClassName, svgPaths }) would remove the duplication and keep future fixes (e.g., accessibility or keying tweaks) in one place.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/react-icons/src/createIcon.tsx` around lines 122 - 152, The createSvg implementation duplicates viewBox, className merging, and svgPaths construction that are also used in the single-SVG rendering branch; extract that shared logic into a new helper (e.g., buildSvgInner(icon)) which returns { viewBox, svgClassName, svgPaths } and have both createSvg and the single-SVG render call buildSvgInner instead of duplicating the code; ensure the helper uses the same symbol names (xOffset, yOffset, width, height, svgPathData, svgClassName) and preserves path keying/className behavior so both createSvg and the single-SVG branch reuse identical viewBox, class merging, and path rendering.
89-120: TightenNormalizedCreateIconProps.iconto required.Both branches of
normalizeCreateIconArgguaranteeiconis set (legacy always normalizesarg; nested throws if missing). Makingiconrequired on the internal type removes the need for theiconData ?? ({} as Partial<…>)fallback at Line 209 and theicon ?? {}at Line 124, and eliminates the| undefinedoniconDataat Line 206.Proposed type tightening
interface NormalizedCreateIconProps { name?: string; - icon?: IconDefinitionWithSvgPathData; + icon: IconDefinitionWithSvgPathData; rhUiIcon: IconDefinitionWithSvgPathData | null; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/react-icons/src/createIcon.tsx` around lines 89 - 120, The NormalizedCreateIconProps.interface currently marks `icon` optional but both branches of `normalizeCreateIconArg` always provide a normalized icon; update the interface so `icon` is required (change `icon?: IconDefinitionWithSvgPathData` to `icon: IconDefinitionWithSvgPathData`) and set `rhUiIcon: IconDefinitionWithSvgPathData | null` remains; then update any callers that previously used `icon ?? {}` or `iconData ?? ({} as Partial<...>)` (references: `normalizeCreateIconArg`, usages around the former `icon ?? {}` and `iconData` handling) to rely on the now-required `icon` without fallback, removing the `| undefined` from `iconData` types and the fallback expressions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@packages/react-icons/src/createIcon.tsx`:
- Around line 198-207: The console.warn inside render() for when set === 'rh-ui'
and rhUiIcon === null causes repeated logs on every render; move that warning to
componentDidMount (or gate it with a static dev-only seen flag on the component)
so it runs at most once per mounted component, and remove the eslint-disable
comment. Also simplify the render guard expression by replacing "(set ===
undefined && rhUiIcon === null) || set !== undefined" with "set !== undefined ||
rhUiIcon === null", and replace the ternary condition "set !== undefined && set
=== 'rh-ui'" with simply "set === 'rh-ui'" when computing iconData (refer to
render(), rhUiIcon, set, icon, and the IconDefinitionWithSvgPathData
assignment).
---
Nitpick comments:
In `@packages/react-icons/src/createIcon.tsx`:
- Around line 122-152: The createSvg implementation duplicates viewBox,
className merging, and svgPaths construction that are also used in the
single-SVG rendering branch; extract that shared logic into a new helper (e.g.,
buildSvgInner(icon)) which returns { viewBox, svgClassName, svgPaths } and have
both createSvg and the single-SVG render call buildSvgInner instead of
duplicating the code; ensure the helper uses the same symbol names (xOffset,
yOffset, width, height, svgPathData, svgClassName) and preserves path
keying/className behavior so both createSvg and the single-SVG branch reuse
identical viewBox, class merging, and path rendering.
- Around line 89-120: The NormalizedCreateIconProps.interface currently marks
`icon` optional but both branches of `normalizeCreateIconArg` always provide a
normalized icon; update the interface so `icon` is required (change `icon?:
IconDefinitionWithSvgPathData` to `icon: IconDefinitionWithSvgPathData`) and set
`rhUiIcon: IconDefinitionWithSvgPathData | null` remains; then update any
callers that previously used `icon ?? {}` or `iconData ?? ({} as Partial<...>)`
(references: `normalizeCreateIconArg`, usages around the former `icon ?? {}` and
`iconData` handling) to rely on the now-required `icon` without fallback,
removing the `| undefined` from `iconData` types and the fallback expressions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 991b1b0d-13d1-4420-84cb-4800498423f9
📒 Files selected for processing (1)
packages/react-icons/src/createIcon.tsx
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What: Closes #12328
What was changed:
CreateIconPropswithicon/rhUiIcon) and the legacy flatcreateIcon({ … svgPath … })by normalizing at runtime (detect viaicon/rhUiIconkeys).svgPathorsvgPathDataonIconDefinition; resolve tosvgPathDatainternally (svgPathDatawins if both are set).LegacyFlatIconDefinitionas an alias for migration/typing.createIconshapes; RH UI (nested swap markup,setprop, missing-map warning).Summary by CodeRabbit
New Features
Bug Fixes
Tests