Skip to content

Rename Icon path property to svgPath to fix type collision#1347

Open
josemontespg wants to merge 18 commits intogoogle:mainfrom
josemontespg:rename-icon-path
Open

Rename Icon path property to svgPath to fix type collision#1347
josemontespg wants to merge 18 commits intogoogle:mainfrom
josemontespg:rename-icon-path

Conversation

@josemontespg
Copy link
Copy Markdown
Collaborator

@josemontespg josemontespg commented May 6, 2026

Problem

The Icon component's name property accepts either a string (for standard icons) or an object with a path property (for custom SVG paths):

name: z.union([
  z.enum(ICON_NAMES),
  z.object({ path: z.string() }).strict(),
])

However, the Angular renderer uses a utility type to resolve properties that strips away shapes matching DataBindingType (which is defined as { path: string }). Because of this structural match, the { path: string } branch was being incorrectly excluded from the resolved type, leaving only the string enum and forcing developers to use as any casts when dealing with custom SVG paths.

Solution

This PR renames the property inside the object from path to svgPath. This breaks the structural match with DataBindingType and preserves the type in the union, allowing for proper type inference without forced casts.

name: z.union([
  z.enum(ICON_NAMES),
  z.object({ svgPath: z.string() }).strict(),
])

Changes

  • Schema: Updated IconApi schema in web_core to use svgPath and added documentation description.
  • Angular: Updated IconComponent to check for and use svgPath. Updated tests to use svgPath.
  • React: Updated Icon component to use svgPath and correctly render an <svg> element instead of a ligature span.
  • Lit: Updated Icon component to use svgPath and correctly render an <svg> element.
  • Cleanup: Removed unnecessary as any casts in the Angular icon component.

Rename the property inside the object from path to svgPath in IconApi schema. This breaks the structural match with DataBindingType and preserves the type in the union, avoiding the need for as any casts in renderers.

TAG=agy

CONV=2bdb6c04-2ba5-495e-8b59-e9981dd0ce1c
Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request renames the path property to svgPath across the Angular, React, and Web Core implementations of the Icon component to clarify that it expects SVG path data. While the property rename is consistent, the React implementation needs to be updated to correctly render an <svg> element when svgPath is provided, as it currently incorrectly treats the path data as a Material Symbol ligature. Additionally, the unintentional pinning of the dompurify dependency in the React package lock should be reverted, and a description should be added to the svgPath Zod schema for better documentation.

Comment thread renderers/react/src/v0_9/catalog/basic/components/Icon.tsx Outdated
Comment thread renderers/react/package-lock.json Outdated
Comment thread renderers/web_core/src/v0_9/basic_catalog/components/basic_components.ts Outdated
…file change, and add schema description

- Update React Icon component to render SVG when svgPath is provided.

- Revert unintentional pinning of dompurify in React package-lock.json.

- Add description to svgPath in IconApi schema.

TAG=agy

CONV=2bdb6c04-2ba5-495e-8b59-e9981dd0ce1c
- Update Lit Icon component to render SVG when svgPath is provided.

- Update usages from path to svgPath.

TAG=agy

CONV=2bdb6c04-2ba5-495e-8b59-e9981dd0ce1c
TAG=agy

CONV=2bdb6c04-2ba5-495e-8b59-e9981dd0ce1c
TAG=agy

CONV=2bdb6c04-2ba5-495e-8b59-e9981dd0ce1c
TAG=agy

CONV=2bdb6c04-2ba5-495e-8b59-e9981dd0ce1c
@josemontespg josemontespg marked this pull request as ready for review May 7, 2026 00:16
Copy link
Copy Markdown
Collaborator

@ditman ditman left a comment

Choose a reason for hiding this comment

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

This is unfortunate, but makes sense. We should have a way to reserve these internal keywords at the protocol level, so this gets flagged earlier (and on custom components) (/cc @gspencergoog ?)

Also, IMO this is a technically breaking change, even though it never worked in the first place, it'll break cached responses; we should mark and release this as a breaking change.

TAG=agy

CONV=2bdb6c04-2ba5-495e-8b59-e9981dd0ce1c
TAG=agy

CONV=2bdb6c04-2ba5-495e-8b59-e9981dd0ce1c
# Conflicts:
#	renderers/angular/src/v0_9/catalog/basic/simple-components.spec.ts
TAG=agy

CONV=2bdb6c04-2ba5-495e-8b59-e9981dd0ce1c
@josemontespg josemontespg requested a review from ditman May 8, 2026 19:01
@josemontespg
Copy link
Copy Markdown
Collaborator Author

This is unfortunate, but makes sense. We should have a way to reserve these internal keywords at the protocol level, so this gets flagged earlier (and on custom components) (/cc @gspencergoog ?)

I agree. Issue #1376 created to track

Also updated the changelogs to mark this as a breaking change.

@josemontespg josemontespg enabled auto-merge (squash) May 8, 2026 19:48
@ditman
Copy link
Copy Markdown
Collaborator

ditman commented May 8, 2026

There's some conflicts, but I think this is one of the last breaking changes that I see incoming; I'll use this as a break point to do a "next" release!

# Conflicts:
#	renderers/lit/CHANGELOG.md
#	renderers/react/CHANGELOG.md
#	renderers/web_core/CHANGELOG.md
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Todo

Development

Successfully merging this pull request may close these issues.

2 participants