fix(createIcon): Fix broken API in createIcon.tsx#12336
fix(createIcon): Fix broken API in createIcon.tsx#12336tlabaj wants to merge 2 commits intopatternfly:mainfrom
Conversation
WalkthroughRefactors createIcon to accept and normalize both legacy flat and new nested icon shapes, splits icon type definitions, and updates rendering to consistently use svgPathData while adding rhUiIcon mapping and related tests for nested SVG and fallback behavior. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
|
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
| /** Icon metadata using the current `svgPathData` field name. */ | ||
| export interface IconDefinitionWithSvgPathData extends IconDefinitionBase { | ||
| svgPathData: string | SVGPathObject[]; | ||
| } | ||
|
|
||
| /** | ||
| * @deprecated Use {@link IconDefinitionWithSvgPathData} with `svgPathData` instead. | ||
| */ | ||
| export interface IconDefinitionWithSvgPath extends IconDefinitionBase { | ||
| svgPath: string | SVGPathObject[]; | ||
| } | ||
|
|
||
| /** Describes SVG path content for one icon variant (default or rh-ui). */ | ||
| export type IconDefinition = IconDefinitionWithSvgPathData | IconDefinitionWithSvgPath; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's examine the file in question
head -100 packages/react-icons/src/createIcon.tsxRepository: patternfly/patternfly-react
Length of output: 3075
🏁 Script executed:
# Search for usages of IconDefinition, IconDefinitionWithSvgPathData, and IconDefinitionWithSvgPath
rg "IconDefinition(WithSvgPathData|WithSvgPath)?" --type typescript --type tsx -A 2 -B 2Repository: patternfly/patternfly-react
Length of output: 105
🏁 Script executed:
# Search for IconDefinition usages across the codebase
rg "IconDefinition" --type-list | head -20Repository: patternfly/patternfly-react
Length of output: 460
🏁 Script executed:
# Try with glob patterns for TypeScript/TSX files
rg "IconDefinition\b" -g "*.ts" -g "*.tsx" -A 3 -B 1 | head -150Repository: patternfly/patternfly-react
Length of output: 7489
🏁 Script executed:
# Search for imports of IconDefinition from external packages
rg "from.*react-icons.*import.*IconDefinition" -g "*.ts" -g "*.tsx" | grep -v "__tests__" | head -30Repository: patternfly/patternfly-react
Length of output: 53
🏁 Script executed:
# Also check if these types are re-exported from main patternfly-react or other packages
rg "export.*IconDefinition" -g "*.ts" -g "*.tsx" | grep -v "createIcon.tsx" | head -20Repository: patternfly/patternfly-react
Length of output: 53
🏁 Script executed:
# Check if there are any direct accesses to .svgPathData or .svgPath on variables typed as IconDefinition
rg "\.svgPathData|\.svgPath" -g "*.ts" -g "*.tsx" -B 3 | head -100Repository: patternfly/patternfly-react
Length of output: 1970
🏁 Script executed:
# Let me examine the test file to see how properties are actually accessed
sed -n '1,80p' packages/react-icons/src/__tests__/createIcon.test.tsxRepository: patternfly/patternfly-react
Length of output: 2361
🏁 Script executed:
# Get more context around the svgPathData access in the test file
rg "svgPathData|svgPath" -g "*.tsx" packages/react-icons/src/__tests__/createIcon.test.tsx -B 5 -A 2Repository: patternfly/patternfly-react
Length of output: 2474
🏁 Script executed:
# Check if there's a tsconfig that shows the TypeScript strictness settings
find . -name "tsconfig*.json" -type f | head -5Repository: patternfly/patternfly-react
Length of output: 280
🏁 Script executed:
# Look at the main tsconfig to see strict settings
cat tsconfig.json | head -50Repository: patternfly/patternfly-react
Length of output: 116
🏁 Script executed:
# Check what TypeScript version is being used
rg "typescript" package.json | head -5Repository: patternfly/patternfly-react
Length of output: 176
🏁 Script executed:
# Check the tsconfig for react-icons package specifically
cat packages/react-icons/tsconfig.jsonRepository: patternfly/patternfly-react
Length of output: 322
🏁 Script executed:
# Check if there's strict mode enabled anywhere
rg '"strict"' . -g "tsconfig*.json" -B 2 -A 2Repository: patternfly/patternfly-react
Length of output: 1164
🏁 Script executed:
# Try to understand what the test is actually doing - it seems to be accessing .svgPathData directly
# Let's see if TypeScript compilation would actually catch this
cat -n packages/react-icons/src/__tests__/createIcon.test.tsx | sed -n '45,55p'Repository: patternfly/patternfly-react
Length of output: 473
🏁 Script executed:
# Check what is exported from the main react-icons entry point
cat packages/react-icons/src/index.ts 2>/dev/null || cat packages/react-icons/index.ts 2>/dev/null || find packages/react-icons -name "index.ts" -o -name "index.tsx" | head -3Repository: patternfly/patternfly-react
Length of output: 104
🏁 Script executed:
# Check the package.json to see what's exported
cat packages/react-icons/package.json | grep -A 20 '"exports"'Repository: patternfly/patternfly-react
Length of output: 53
🏁 Script executed:
# Let's verify the actual TypeScript behavior - can you access svgPathData directly on a union type?
# Create a test file to verify
cat > /tmp/test-union-type.ts << 'EOF'
interface Base {
width: number;
height: number;
}
interface WithA extends Base {
propA: string;
}
interface WithB extends Base {
propB: string;
}
type Union = WithA | WithB;
function testUnion(arg: Union) {
// This should error - propA is not on Union
console.log(arg.propA);
}
// But this should work because TypeScript infers the specific type
const obj: Union = {
width: 10,
height: 20,
propA: 'test'
};
console.log(obj.propA);
EOFRepository: patternfly/patternfly-react
Length of output: 53
🏁 Script executed:
# Check what's exported from the react-icons package
cat packages/react-icons/src/index.tsRepository: patternfly/patternfly-react
Length of output: 104
🏁 Script executed:
# Let's verify if IconDefinition is actually exported from the public API
rg "export.*from.*createIcon" packages/react-icons/src/ -g "*.ts" -g "*.tsx"Repository: patternfly/patternfly-react
Length of output: 53
🏁 Script executed:
# Check if any downstream packages import IconDefinition
rg "IconDefinition" packages/ -g "*.ts" -g "*.tsx" | grep -v "react-icons" | grep -v "__tests__" | head -20Repository: patternfly/patternfly-react
Length of output: 53
🏁 Script executed:
# Let's check the actual exports more carefully - what does icons/index.js export?
cat packages/react-icons/src/icons/index.ts 2>/dev/null | head -30Repository: patternfly/patternfly-react
Length of output: 53
🏁 Script executed:
# Check the actual build/dist structure to see what gets exported
ls -la packages/react-icons/dist/ 2>/dev/null || echo "dist not available in sandbox"Repository: patternfly/patternfly-react
Length of output: 100
🏁 Script executed:
# Let's check if IconDefinition is actually exported from the package by looking at index exports more carefully
cat packages/react-icons/src/index.tsRepository: patternfly/patternfly-react
Length of output: 104
🏁 Script executed:
# Search for any actual exports of IconDefinition, IconDefinitionWithSvgPathData, or CreateIconProps
rg "export\s+(interface|type|class)" packages/react-icons/src/createIcon.tsxRepository: patternfly/patternfly-react
Length of output: 548
🏁 Script executed:
# Let's verify the actual TypeScript compilation behavior
# Check if the test file compiles without errors by looking for any tsconfig overrides
cat packages/react-icons/src/__tests__/createIcon.test.tsx | head -10Repository: patternfly/patternfly-react
Length of output: 504
🏁 Script executed:
# Let's see the complete createIcon.tsx file to understand how properties are actually accessed at runtime
wc -l packages/react-icons/src/createIcon.tsxRepository: patternfly/patternfly-react
Length of output: 114
🏁 Script executed:
# Get the render section where the properties are actually used
sed -n '180,250p' packages/react-icons/src/createIcon.tsxRepository: patternfly/patternfly-react
Length of output: 2180
🏁 Script executed:
# Let's see the complete createIcon.tsx to understand the context better
head -60 packages/react-icons/src/createIcon.tsxRepository: patternfly/patternfly-react
Length of output: 1862
🏁 Script executed:
# Check if there are any actual downstream libraries/packages that depend on `@patternfly/react-icons`
# and might import IconDefinition
find . -name "package.json" -type f -exec grep -l "@patternfly/react-icons" {} \; | head -10Repository: patternfly/patternfly-react
Length of output: 334
🏁 Script executed:
# Let's check if there are any helper functions or utilities that accept IconDefinition as a parameter
rg "function.*IconDefinition|export.*function.*icon.*:" packages/react-icons/src -g "*.ts" -g "*.tsx" -B 1 -A 3Repository: patternfly/patternfly-react
Length of output: 1647
🏁 Script executed:
# Let's verify by checking the actual generated type definitions
# Search for any .d.ts files or check what a TypeScript consumer would see
find packages/react-icons -name "*.d.ts" 2>/dev/null | head -5Repository: patternfly/patternfly-react
Length of output: 53
🏁 Script executed:
# Let's create a simple TypeScript test to verify the union type behavior
cat > /tmp/verify-union.ts << 'EOF'
// Simulating the actual exported types
interface IconDefinitionBase {
name?: string;
width: number;
height: number;
}
interface IconDefinitionWithSvgPathData extends IconDefinitionBase {
svgPathData: string;
}
interface IconDefinitionWithSvgPath extends IconDefinitionBase {
svgPath: string;
}
type IconDefinition = IconDefinitionWithSvgPathData | IconDefinitionWithSvgPath;
// This is what a downstream consumer might try to do
function processIcon(icon: IconDefinition): string {
// Can we access svgPathData directly without narrowing?
return icon.svgPathData; // This should cause a TS error
}
// But this works (from the test)
const icon: IconDefinition = { width: 10, height: 20, svgPathData: 'test' };
console.log(icon.svgPathData); // This works because of type inference from the literal
EOF
cat /tmp/verify-union.tsRepository: patternfly/patternfly-react
Length of output: 889
🏁 Script executed:
# Let's check if there's a create-icon utility or any exported helpers that accept IconDefinition
rg "export" packages/react-icons/src/createIcon.tsx | grep -E "(interface|type|function|class)"Repository: patternfly/patternfly-react
Length of output: 662
🏁 Script executed:
# Let's verify the actual TypeScript behavior by checking if strictNullChecks affects this
cat packages/tsconfig.base.json | grep -A 5 -B 5 "strictNullChecks"Repository: patternfly/patternfly-react
Length of output: 410
🏁 Script executed:
# Let me check if there are any examples or documentation showing how to use IconDefinition
rg "IconDefinition" packages/react-docs -g "*.ts" -g "*.tsx" -g "*.md" -B 2 -A 2 2>/dev/null | head -30Repository: patternfly/patternfly-react
Length of output: 53
🏁 Script executed:
# Check if there's any real-world usage pattern in the docs or examples
find packages -name "*.md" -o -name "*.mdx" | xargs grep -l "IconDefinition" 2>/dev/null | head -5Repository: patternfly/patternfly-react
Length of output: 53
🏁 Script executed:
# Let me check the actual render path more carefully to see if there could be undefined icon issues
sed -n '175,230p' packages/react-icons/src/createIcon.tsxRepository: patternfly/patternfly-react
Length of output: 2105
🏁 Script executed:
# Verify the normalization logic - does it guarantee icon is defined?
sed -n '86,110p' packages/react-icons/src/createIcon.tsxRepository: patternfly/patternfly-react
Length of output: 974
🏁 Script executed:
# Let's check the git history to see if this was a recent change
git log --oneline -20 packages/react-icons/src/createIcon.tsx 2>/dev/null | head -10Repository: patternfly/patternfly-react
Length of output: 129
🏁 Script executed:
# Let's check if there are any CHANGELOG or migration guide references
find . -name "CHANGELOG*" -o -name "MIGRATION*" | xargs grep -l "IconDefinition" 2>/dev/nullRepository: patternfly/patternfly-react
Length of output: 55
🏁 Script executed:
# Final check: verify that the union type actually does cause TypeScript errors in a realistic scenario
cat > /tmp/test-external-helper.ts << 'EOF'
// Simulating external package consuming react-icons
import type { IconDefinition } from '@patternfly/react-icons';
// This is what an external helper might look like
export function extractSvgPath(icon: IconDefinition): string {
// Without type narrowing, TypeScript should error here
// because svgPathData is not guaranteed to exist on all IconDefinition variants
const path = icon.svgPathData; // TS Error: Property 'svgPathData' does not exist on type 'IconDefinition'
return typeof path === 'string' ? path : 'unknown';
}
EOF
cat /tmp/test-external-helper.tsRepository: patternfly/patternfly-react
Length of output: 608
IconDefinition union type requires narrowing for external helper functions—consider a single interface for API simplicity.
The exported IconDefinition as a union (IconDefinitionWithSvgPathData | IconDefinitionWithSvgPath) means that any external consumer writing helpers accepting IconDefinition cannot directly access svgPathData without type narrowing. While the internal codebase handles this correctly via resolveSvgPathData(), the public API surface should be more ergonomic. Prefer a single IconDefinition interface with both properties (marking svgPath as deprecated) over a union type—this eliminates the need for downstream helpers to add type guards.
Suggested refactor
-export interface IconDefinitionWithSvgPathData extends IconDefinitionBase {
- svgPathData: string | SVGPathObject[];
-}
-
-/**
- * `@deprecated` Use {`@link` IconDefinitionWithSvgPathData} with `svgPathData` instead.
- */
-export interface IconDefinitionWithSvgPath extends IconDefinitionBase {
- svgPath: string | SVGPathObject[];
-}
-
-/** Describes SVG path content for one icon variant (default or rh-ui). */
-export type IconDefinition = IconDefinitionWithSvgPathData | IconDefinitionWithSvgPath;
+export interface IconDefinition extends IconDefinitionBase {
+ svgPathData?: string | SVGPathObject[];
+ /** `@deprecated` Use `svgPathData` instead. */
+ svgPath?: string | SVGPathObject[];
+}
+
+export type IconDefinitionWithSvgPathData = Required<Pick<IconDefinition, 'svgPathData'>> & IconDefinition;
+
+/** `@deprecated` Use {`@link` IconDefinitionWithSvgPathData} with `svgPathData` instead. */
+export type IconDefinitionWithSvgPath = Required<Pick<IconDefinition, 'svgPath'>> & IconDefinition;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| /** Icon metadata using the current `svgPathData` field name. */ | |
| export interface IconDefinitionWithSvgPathData extends IconDefinitionBase { | |
| svgPathData: string | SVGPathObject[]; | |
| } | |
| /** | |
| * @deprecated Use {@link IconDefinitionWithSvgPathData} with `svgPathData` instead. | |
| */ | |
| export interface IconDefinitionWithSvgPath extends IconDefinitionBase { | |
| svgPath: string | SVGPathObject[]; | |
| } | |
| /** Describes SVG path content for one icon variant (default or rh-ui). */ | |
| export type IconDefinition = IconDefinitionWithSvgPathData | IconDefinitionWithSvgPath; | |
| export interface IconDefinition extends IconDefinitionBase { | |
| svgPathData?: string | SVGPathObject[]; | |
| /** `@deprecated` Use `svgPathData` instead. */ | |
| svgPath?: string | SVGPathObject[]; | |
| } | |
| export type IconDefinitionWithSvgPathData = Required<Pick<IconDefinition, 'svgPathData'>> & IconDefinition; | |
| /** `@deprecated` Use {`@link` IconDefinitionWithSvgPathData} with `svgPathData` instead. */ | |
| export type IconDefinitionWithSvgPath = Required<Pick<IconDefinition, 'svgPath'>> & IconDefinition; |
🤖 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 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.
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
| // eslint-disable-next-line no-restricted-imports -- test file excluded from package tsconfig; default import satisfies TS/JSX | ||
| import React from 'react'; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "1) React default imports in TSX tests:"
rg -n --type=tsx "^import React from 'react';" packages/react-icons/src/__tests__ -C1 || true
echo
echo "2) JSX runtime settings in tsconfig files:"
fd -a '^tsconfig.*\.json$' | xargs -r rg -n '"jsx"\s*:\s*"[^"]+"' || true
echo
echo "3) ESLint restrictions around React imports:"
fd -a 'eslint.config.*|\.eslintrc.*' | xargs -r rg -n 'no-restricted-imports|react' -C2 || true
echo
echo "4) Jest/Babel transform settings that decide JSX runtime for tests:"
fd -a 'jest.*\.(js|cjs|mjs|ts)|babel.*\.(js|cjs|mjs|json)' | xargs -r rg -n 'transform|tsx|preset-react|runtime|automatic|classic' -C2 || trueRepository: patternfly/patternfly-react
Length of output: 10458
Remove the React default import and ESLint suppression comment; automatic JSX runtime does not require it.
The suppression allows a violation of the repository's no-restricted-imports rule that forbids default imports from React. Since the project uses automatic JSX runtime (configured in babel.config.js with { runtime: 'automatic' } and packages/tsconfig.base.json with "jsx": "react-jsx"), React is available globally and does not need to be imported. No other test files in this repository use default React imports.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/react-icons/src/__tests__/createIcon.test.tsx` around lines 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.
| test('set="rh-ui" with no rhUiIcon mapping falls back to default and warns', () => { | ||
| const warnSpy = jest.spyOn(console, 'warn').mockImplementation(() => {}); | ||
| const IconNoRhMapping = createIcon({ | ||
| name: 'NoRhMappingIcon', | ||
| icon: defaultIconDef, | ||
| rhUiIcon: null | ||
| }); | ||
|
|
||
| render(<IconNoRhMapping set="rh-ui" />); | ||
|
|
||
| expect(warnSpy).toHaveBeenCalledWith( | ||
| 'Set "rh-ui" was provided for NoRhMappingIcon, but no rh-ui icon data exists for this icon. The default icon will be rendered.' | ||
| ); | ||
| const root = screen.getByRole('img', { hidden: true }); | ||
| expect(root.querySelector('path')).toHaveAttribute('d', defaultPath); | ||
| expect(root.querySelectorAll('svg')).toHaveLength(0); | ||
|
|
||
| warnSpy.mockRestore(); | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# First, verify the file exists and locate the specific lines
if [ -f "packages/react-icons/src/__tests__/createIcon.test.tsx" ]; then
echo "=== File found ==="
# Get line count to ensure the lines exist
wc -l "packages/react-icons/src/__tests__/createIcon.test.tsx"
echo ""
echo "=== Lines 190-220 (context around 198-216) ==="
sed -n '190,220p' "packages/react-icons/src/__tests__/createIcon.test.tsx"
else
echo "File not found at expected path"
# Try to find it
find . -name "createIcon.test.tsx" -type f 2>/dev/null
fiRepository: patternfly/patternfly-react
Length of output: 1340
🏁 Script executed:
#!/bin/bash
# Search for other jest.spyOn patterns in test files to see how they handle cleanup
rg "jest\.spyOn\(console" packages/react-icons/src/__tests__/ -A 10 -B 2Repository: patternfly/patternfly-react
Length of output: 1303
🏁 Script executed:
#!/bin/bash
# Look for beforeEach/afterEach patterns in the test file
echo "=== beforeEach/afterEach hooks ==="
rg "beforeEach|afterEach|beforeAll|afterAll" packages/react-icons/src/__tests__/createIcon.test.tsx -B 2 -A 5
echo ""
echo "=== All jest.spyOn calls in the file ==="
rg "jest\.spyOn" packages/react-icons/src/__tests__/createIcon.test.tsx -B 2 -A 15
echo ""
echo "=== Full test function containing the concern ==="
sed -n '198,216p' packages/react-icons/src/__tests__/createIcon.test.tsxRepository: patternfly/patternfly-react
Length of output: 1666
🏁 Script executed:
#!/bin/bash
# Check if there are other tests that handle spies similarly
echo "=== All test cases handling console mocks ==="
rg "jest\.spyOn\(console" packages/react-icons/src/__tests__/ -B 5 -A 20Repository: patternfly/patternfly-react
Length of output: 2326
Wrap spy cleanup in try-finally to ensure restoration even if assertions fail.
If any expect() call before mockRestore() throws an exception, the spy won't be restored and will leak the mocked console.warn state into subsequent tests. Use a try-finally block to guarantee cleanup.
Suggested fix
test('set="rh-ui" with no rhUiIcon mapping falls back to default and warns', () => {
const warnSpy = jest.spyOn(console, 'warn').mockImplementation(() => {});
const IconNoRhMapping = createIcon({
name: 'NoRhMappingIcon',
icon: defaultIconDef,
rhUiIcon: null
});
- render(<IconNoRhMapping set="rh-ui" />);
-
- expect(warnSpy).toHaveBeenCalledWith(
- 'Set "rh-ui" was provided for NoRhMappingIcon, but no rh-ui icon data exists for this icon. The default icon will be rendered.'
- );
- const root = screen.getByRole('img', { hidden: true });
- expect(root.querySelector('path')).toHaveAttribute('d', defaultPath);
- expect(root.querySelectorAll('svg')).toHaveLength(0);
-
- warnSpy.mockRestore();
+ try {
+ render(<IconNoRhMapping set="rh-ui" />);
+
+ expect(warnSpy).toHaveBeenCalledWith(
+ 'Set "rh-ui" was provided for NoRhMappingIcon, but no rh-ui icon data exists for this icon. The default icon will be rendered.'
+ );
+ const root = screen.getByRole('img', { hidden: true });
+ expect(root.querySelector('path')).toHaveAttribute('d', defaultPath);
+ expect(root.querySelectorAll('svg')).toHaveLength(0);
+ } finally {
+ warnSpy.mockRestore();
+ }
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| test('set="rh-ui" with no rhUiIcon mapping falls back to default and warns', () => { | |
| const warnSpy = jest.spyOn(console, 'warn').mockImplementation(() => {}); | |
| const IconNoRhMapping = createIcon({ | |
| name: 'NoRhMappingIcon', | |
| icon: defaultIconDef, | |
| rhUiIcon: null | |
| }); | |
| render(<IconNoRhMapping set="rh-ui" />); | |
| expect(warnSpy).toHaveBeenCalledWith( | |
| 'Set "rh-ui" was provided for NoRhMappingIcon, but no rh-ui icon data exists for this icon. The default icon will be rendered.' | |
| ); | |
| const root = screen.getByRole('img', { hidden: true }); | |
| expect(root.querySelector('path')).toHaveAttribute('d', defaultPath); | |
| expect(root.querySelectorAll('svg')).toHaveLength(0); | |
| warnSpy.mockRestore(); | |
| }); | |
| test('set="rh-ui" with no rhUiIcon mapping falls back to default and warns', () => { | |
| const warnSpy = jest.spyOn(console, 'warn').mockImplementation(() => {}); | |
| const IconNoRhMapping = createIcon({ | |
| name: 'NoRhMappingIcon', | |
| icon: defaultIconDef, | |
| rhUiIcon: null | |
| }); | |
| try { | |
| render(<IconNoRhMapping set="rh-ui" />); | |
| expect(warnSpy).toHaveBeenCalledWith( | |
| 'Set "rh-ui" was provided for NoRhMappingIcon, but no rh-ui icon data exists for this icon. The default icon will be rendered.' | |
| ); | |
| const root = screen.getByRole('img', { hidden: true }); | |
| expect(root.querySelector('path')).toHaveAttribute('d', defaultPath); | |
| expect(root.querySelectorAll('svg')).toHaveLength(0); | |
| } finally { | |
| warnSpy.mockRestore(); | |
| } | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/react-icons/src/__tests__/createIcon.test.tsx` around lines 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.
2 participants
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