Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
121 changes: 119 additions & 2 deletions packages/react-icons/src/__tests__/createIcon.test.tsx
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
// eslint-disable-next-line no-restricted-imports -- test file excluded from package tsconfig; default import satisfies TS/JSX
import React from 'react';
Comment on lines +1 to +2
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

🧩 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 || true

Repository: 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.

import { render, screen } from '@testing-library/react';
import { IconDefinition, CreateIconProps, createIcon, SVGPathObject } from '../createIcon';
import { IconDefinition, CreateIconProps, createIcon, LegacyFlatIconDefinition, SVGPathObject } from '../createIcon';

const multiPathIcon: IconDefinition = {
name: 'IconName',
Expand Down Expand Up @@ -43,7 +45,48 @@ test('sets correct viewBox', () => {

test('sets correct svgPath if string', () => {
render(<SVGIcon />);
expect(screen.getByRole('img', { hidden: true }).querySelector('path')).toHaveAttribute('d', iconDef.svgPath);
expect(screen.getByRole('img', { hidden: true }).querySelector('path')).toHaveAttribute(
'd',
singlePathIcon.svgPathData
);
});

test('accepts legacy flat createIcon({ svgPath }) shape', () => {
const legacyDef: LegacyFlatIconDefinition = {
name: 'LegacyIcon',
width: 10,
height: 20,
svgPath: 'legacy-path',
svgClassName: 'legacy-svg'
};
const LegacySVGIcon = createIcon(legacyDef);
render(<LegacySVGIcon />);
expect(screen.getByRole('img', { hidden: true }).querySelector('path')).toHaveAttribute('d', 'legacy-path');
});

test('accepts CreateIconProps with nested icon using deprecated svgPath field', () => {
const nestedLegacyPath: CreateIconProps = {
name: 'NestedLegacyPathIcon',
icon: {
width: 8,
height: 8,
svgPath: 'nested-legacy-d'
}
};
const NestedIcon = createIcon(nestedLegacyPath);
render(<NestedIcon />);
expect(screen.getByRole('img', { hidden: true }).querySelector('path')).toHaveAttribute('d', 'nested-legacy-d');
});

test('throws when nested CreateIconProps omits icon', () => {
expect(() =>
createIcon({
name: 'MissingDefaultIcon',
rhUiIcon: null
})
).toThrow(
'@patternfly/react-icons: createIcon requires an `icon` definition when using nested CreateIconProps (name: MissingDefaultIcon).'
);
});

test('sets correct svgPath if array', () => {
Expand Down Expand Up @@ -98,3 +141,77 @@ test('additional props should be spread to the root svg element', () => {
render(<SVGIcon data-testid="icon" />);
expect(screen.getByTestId('icon')).toBeInTheDocument();
});

describe('rh-ui mapping: nested SVGs, set prop, and warnings', () => {
const defaultPath = 'M0 0-default';
const rhUiPath = 'M0 0-rh-ui';

const defaultIconDef: IconDefinition = {
name: 'DefaultVariant',
width: 16,
height: 16,
svgPathData: defaultPath
};

const rhUiIconDef: IconDefinition = {
name: 'RhUiVariant',
width: 16,
height: 16,
svgPathData: rhUiPath
};

const dualConfig: CreateIconProps = {
name: 'DualMappedIcon',
icon: defaultIconDef,
rhUiIcon: rhUiIconDef
};

const DualMappedIcon = createIcon(dualConfig);

test('renders two nested inner svgs when rhUiIcon is set and `set` is omitted (swap layout)', () => {
render(<DualMappedIcon />);
const root = screen.getByRole('img', { hidden: true });
expect(root).toHaveClass('pf-v6-svg');
const innerSvgs = root.querySelectorAll(':scope > svg');
expect(innerSvgs).toHaveLength(2);
expect(root?.querySelector('.pf-v6-icon-default path')).toHaveAttribute('d', defaultPath);
expect(root?.querySelector('.pf-v6-icon-rh-ui path')).toHaveAttribute('d', rhUiPath);
});

test('set="default" renders a single flat svg using the default icon paths', () => {
render(<DualMappedIcon set="default" />);
const root = screen.getByRole('img', { hidden: true });
expect(root.querySelectorAll(':scope > svg')).toHaveLength(0);
expect(root).toHaveAttribute('viewBox', '0 0 16 16');
expect(root.querySelector('path')).toHaveAttribute('d', defaultPath);
expect(root.querySelectorAll('svg')).toHaveLength(0);
});

test('set="rh-ui" renders a single flat svg using the rh-ui icon paths', () => {
render(<DualMappedIcon set="rh-ui" />);
const root = screen.getByRole('img', { hidden: true });
expect(root.querySelectorAll(':scope > svg')).toHaveLength(0);
expect(root.querySelector('path')).toHaveAttribute('d', rhUiPath);
expect(root.querySelectorAll('svg')).toHaveLength(0);
});

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();
});
Comment on lines +198 to +216
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

🧩 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
fi

Repository: 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 2

Repository: 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.tsx

Repository: 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 20

Repository: 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.

Suggested change
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.

});
119 changes: 108 additions & 11 deletions packages/react-icons/src/createIcon.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -5,22 +5,42 @@ export interface SVGPathObject {
className?: string;
}

export interface IconDefinition {
export interface IconDefinitionBase {
name?: string;
width: number;
height: number;
svgPathData: string | SVGPathObject[];
xOffset?: number;
yOffset?: number;
svgClassName?: string;
}

/** 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;
Comment on lines +17 to +30
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

# First, let's examine the file in question
head -100 packages/react-icons/src/createIcon.tsx

Repository: 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 2

Repository: patternfly/patternfly-react

Length of output: 105


🏁 Script executed:

# Search for IconDefinition usages across the codebase
rg "IconDefinition" --type-list | head -20

Repository: 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 -150

Repository: 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 -30

Repository: 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 -20

Repository: 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 -100

Repository: 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.tsx

Repository: 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 2

Repository: 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 -5

Repository: patternfly/patternfly-react

Length of output: 280


🏁 Script executed:

# Look at the main tsconfig to see strict settings
cat tsconfig.json | head -50

Repository: patternfly/patternfly-react

Length of output: 116


🏁 Script executed:

# Check what TypeScript version is being used
rg "typescript" package.json | head -5

Repository: patternfly/patternfly-react

Length of output: 176


🏁 Script executed:

# Check the tsconfig for react-icons package specifically
cat packages/react-icons/tsconfig.json

Repository: 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 2

Repository: 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 -3

Repository: 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);
EOF

Repository: patternfly/patternfly-react

Length of output: 53


🏁 Script executed:

# Check what's exported from the react-icons package
cat packages/react-icons/src/index.ts

Repository: 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 -20

Repository: 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 -30

Repository: 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.ts

Repository: 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.tsx

Repository: 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 -10

Repository: 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.tsx

Repository: 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.tsx

Repository: 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.tsx

Repository: 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 -10

Repository: 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 3

Repository: 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 -5

Repository: 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.ts

Repository: 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 -30

Repository: 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 -5

Repository: 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.tsx

Repository: 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.tsx

Repository: 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 -10

Repository: 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/null

Repository: 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.ts

Repository: 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.

Suggested change
/** 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.


export interface CreateIconProps {
name?: string;
icon?: IconDefinition;
rhUiIcon?: IconDefinition | null;
}

/**
* @deprecated The previous `createIcon` accepted a flat {@link IconDefinition} with top-level
* `svgPath`. Pass {@link CreateIconProps} with a nested `icon` field instead.
*/
export type LegacyFlatIconDefinition = IconDefinition;

export interface SVGIconProps extends Omit<React.HTMLProps<SVGElement>, 'ref'> {
title?: string;
className?: string;
Expand All @@ -30,7 +50,62 @@ export interface SVGIconProps extends Omit<React.HTMLProps<SVGElement>, 'ref'> {

let currentId = 0;

const createSvg = (icon: IconDefinition, iconClassName: string) => {
function resolveSvgPathData(icon: IconDefinition): string | SVGPathObject[] {
if ('svgPathData' in icon && icon.svgPathData !== undefined) {
return icon.svgPathData;
}
if ('svgPath' in icon && icon.svgPath !== undefined) {
return icon.svgPath;
}
throw new Error('@patternfly/react-icons: IconDefinition must define svgPathData or svgPath');
}

function normalizeIconDefinition(icon: IconDefinition): IconDefinitionWithSvgPathData {
return {
name: icon.name,
width: icon.width,
height: icon.height,
svgPathData: resolveSvgPathData(icon),
xOffset: icon.xOffset,
yOffset: icon.yOffset,
svgClassName: icon.svgClassName
};
}

function isNestedCreateIconProps(arg: object): arg is CreateIconProps {
return 'icon' in arg || 'rhUiIcon' in arg;
}

/** Props after resolving legacy `svgPath` and flat `createIcon` arguments. */
interface NormalizedCreateIconProps {
name?: string;
icon?: IconDefinitionWithSvgPathData;
rhUiIcon: IconDefinitionWithSvgPathData | null;
}

function normalizeCreateIconArg(arg: CreateIconProps | LegacyFlatIconDefinition): NormalizedCreateIconProps {
if (isNestedCreateIconProps(arg)) {
const p = arg as CreateIconProps;
if (p.icon == null) {
const label = p.name != null ? ` (name: ${String(p.name)})` : '';
throw new Error(
`@patternfly/react-icons: createIcon requires an \`icon\` definition when using nested CreateIconProps${label}.`
);
}
return {
name: p.name,
icon: normalizeIconDefinition(p.icon),
rhUiIcon: p.rhUiIcon != null ? normalizeIconDefinition(p.rhUiIcon) : null
};
}
return {
name: (arg as LegacyFlatIconDefinition).name,
icon: normalizeIconDefinition(arg as IconDefinition),
rhUiIcon: null
};
}

const createSvg = (icon: IconDefinitionWithSvgPathData, iconClassName: string) => {
const { xOffset, yOffset, width, height, svgPathData, svgClassName } = icon ?? {};
const _xOffset = xOffset ?? 0;
const _yOffset = yOffset ?? 0;
Expand Down Expand Up @@ -62,18 +137,38 @@ const createSvg = (icon: IconDefinition, iconClassName: string) => {
};

/**
* Factory to create Icon class components for consumers
* Builds a React **class** component that renders a PatternFly SVG icon (`role="img"`, optional `<title>` for a11y).
*
* **Argument shape — pick one:**
*
* 1. **`CreateIconProps` (preferred)** — `{ name?, icon?, rhUiIcon? }`. Dimensions and path data sit on `icon`
* (and optionally on `rhUiIcon` for Red Hat UI–mapped icons). If the object **has an `icon` or `rhUiIcon` key**
* (including `rhUiIcon: null`), this shape is assumed.
*
* 2. **Legacy flat `IconDefinition`** — the same fields as `icon`, but at the **top level** (no nested `icon`).
* Still accepted so existing callers are not broken. Prefer migrating to `CreateIconProps`.
*
* **Path data on each `IconDefinition`:** use `svgPathData` (string or {@link SVGPathObject}[]). The old name
* `svgPath` is deprecated but still read; `svgPathData` wins if both are present.
*
* **Default vs RH UI rendering:** If `rhUiIcon` is set and the consumer does **not** pass `set` on the component,
* the output is an outer `<svg.pf-v6-svg>` containing **two** inner `<svg>`s (default + rh-ui) so CSS can swap
* which variant is visible. If `set` is `"default"` or `"rh-ui"`, a **single** flat `<svg>` is rendered for that
* variant. Requesting `set="rh-ui"` when there is no `rhUiIcon` falls back to the default glyph and logs a
* `console.warn` (see implementation).
*
* @param arg Icon configuration: either {@link CreateIconProps} (nested `icon` / `rhUiIcon`) or a legacy flat
* {@link LegacyFlatIconDefinition}. Runtime detection follows the rules in **Argument shape** above.
* @returns A `ComponentClass<SVGIconProps>` — render it as `<YourIcon />` or with `title`, `className`, `set`, etc.
*/
export function createIcon({ name, icon, rhUiIcon = null }: CreateIconProps): React.ComponentClass<SVGIconProps> {
export function createIcon(arg: CreateIconProps | LegacyFlatIconDefinition): React.ComponentClass<SVGIconProps> {
const { name, icon, rhUiIcon = null } = normalizeCreateIconArg(arg);

return class SVGIcon extends Component<SVGIconProps> {
static displayName = name;

id = `icon-title-${currentId++}`;

constructor(props: SVGIconProps) {
super(props);
}

render() {
const { title, className: propsClassName, set, ...props } = this.props;

Expand All @@ -92,8 +187,10 @@ export function createIcon({ name, icon, rhUiIcon = null }: CreateIconProps): Re
}

if ((set === undefined && rhUiIcon === null) || set !== undefined) {
const iconData = set !== undefined && set === 'rh-ui' && rhUiIcon !== null ? rhUiIcon : icon;
const { xOffset, yOffset, width, height, svgPathData, svgClassName } = iconData ?? {};
const iconData: IconDefinitionWithSvgPathData | undefined =
set !== undefined && set === 'rh-ui' && rhUiIcon !== null ? rhUiIcon : icon;
const { xOffset, yOffset, width, height, svgPathData, svgClassName } =
iconData ?? ({} as Partial<IconDefinitionWithSvgPathData>);
const _xOffset = xOffset ?? 0;
const _yOffset = yOffset ?? 0;
const viewBox = [_xOffset, _yOffset, width, height].join(' ');
Expand Down
Loading