feat: tree-shakeable string icons <Button icon="menu" />#627
feat: tree-shakeable string icons <Button icon="menu" />#627netchampfaris wants to merge 1 commit intomainfrom
Conversation
Rewrite static Button icon string props to direct Lucide imports in the frappe-ui Vite plugin, and remove FeatherIcon runtime handling from Button. Runtime string values now warn in development so consumers can switch to the Vite transform or explicit Lucide component bindings.
netchampfaris
changed the title
netchampfaris
changed the title
WalkthroughThis change introduces a build-time transformation system for Button icon props using Lucide icons. A new Vite plugin analyzes Vue single-file components and rewrites static string icon props (icon, iconLeft, iconRight) on Button elements to reference lazily-imported Lucide icon components. The Button component is refactored to use computed icon component bindings and includes runtime warnings for non-transformed string icons. Documentation updates explain the transformation behavior for static versus dynamic icon values. Tests validate the plugin's template transformation logic. 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
![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
🧹 Nitpick comments (6)
vite/lucideIcons.test.ts (2)
4-8: Guard against a silentundefinedplugin.If
pluginisundefined(e.g., a future rename offrappe-ui-button-lucide-icon-props), every test here fails with a confusingCannot read properties of undefined (reading 'transform')rather than a clear message. A singleexpect(plugin).toBeDefined()at the top of each test — or inside the helper — keeps failures diagnosable.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@vite/lucideIcons.test.ts` around lines 4 - 8, The helper getButtonIconTransform may return undefined, causing ambiguous test failures; update getButtonIconTransform (or the tests that call it) to assert the plugin exists before using its transform: locate the lucideIcons() lookup in getButtonIconTransform and add a guard such as expect(plugin).toBeDefined() (or throw a clear Error if plugin is undefined) so tests fail with a descriptive message instead of a "Cannot read properties of undefined" when the 'frappe-ui-button-lucide-icon-props' plugin is missing or renamed.
38-58: Add an assertion that the existing<script lang="ts">block is preserved.The test name claims the existing script lang is preserved, but assertions only verify a new
<script setup lang="ts">is created. A regression that strips/replaces the original<script lang="ts">export default {}</script>would still pass.✅ Proposed assertion
expect(result.code).toContain('<script setup lang="ts">') + expect(result.code).toContain('<script lang="ts">') + expect(result.code).toContain('export default {}') expect(result.code).toContain( "import __FrappeUiLucideIcon0 from '~icons/lucide/chevron-down'", )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@vite/lucideIcons.test.ts` around lines 38 - 58, The test for getButtonIconTransform currently only asserts a new '<script setup lang="ts">' block is inserted but doesn't verify the original '<script lang="ts">...export default {}</script>' remains; update the test (in vite/lucideIcons.test.ts) to also assert result.code contains the original script block (e.g., the exact string '<script lang="ts">export default {}</script>' or at least '<script lang="ts">' and 'export default {}') after calling plugin.transform so regressions that remove/replace the original script will fail.vite/README.md (1)
109-119: Optional: mention the runtime consequence of non-transformable string values.The note says dynamic string values “are not transformed,” but does not state what happens at runtime. Because
Button.vue'sgetRenderableIconreturnsnullfor any string, a dynamic:icon="'menu'"or prop-bound string will render nothing (only a DEV console warning). A one-line callout here would save downstream users a debugging session.📝 Suggested addition
This only applies to static string literals in `.vue` templates. Dynamic string values are not transformed; prefer passing the imported Lucide component in those cases. + +> At runtime, Button treats any string `icon`/`icon-left`/`icon-right` as +> empty (a DEV-only warning is logged). Bind a resolved component instead, +> e.g. `:icon="LucideMenu"`.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@vite/README.md` around lines 109 - 119, Add a one-line runtime note clarifying that dynamic string icon props are not transformed and will render no icon at runtime because Button.vue's getRenderableIcon returns null for non-literal strings; mention using a bound string like :icon="'menu'" or a prop-bound value will produce no icon (and only a DEV console warning) and recommend passing the imported Lucide component or using a static literal instead.vite/lucideIcons.js (2)
139-163:findTagEndquote escaping won't survive a value ending in\\"— unlikely in practice but worth a note.
template[index - 1] !== '\\'treats\\"(escaped backslash before a closing quote) as escaped and fails to close the attribute. HTML/Vue attribute values don't use backslash escaping, so this only matters for pathological inputs. If you'd like to keep it robust, count trailing backslashes and check parity.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@vite/lucideIcons.js` around lines 139 - 163, findTagEnd can mis-detect a closing quote when the character before it is a backslash (e.g. '\\"'); replace the naive check template[index - 1] !== '\\' with a trailing-backslash parity test: when encountering a quote in findTagEnd, walk backwards from index-1 counting consecutive '\' characters and treat the quote as escaped only if the count is odd (count % 2 === 1), otherwise close the quote; update the logic around the quote variable handling in findTagEnd (using template, startIndex, quote, index) to use this parity check so escaped backslashes are handled correctly.
101-137: Edge case:<Button …>inside HTML comments or fenced<code>blocks is transformed.
transformButtonIconPropsTemplatewalks the template via a bare<Buttonregex, so an example like this inside a docs/story.vue:<template> <!-- <Button icon="menu" /> --> <pre><code><Button icon="menu" /></code></pre> </template>would (a) rewrite the attributes inside the comment and (b) inject a
~icons/lucide/menuimport even though no real component uses it. The rewritten comment is still a valid comment, but the injected import pulls an unused icon into the bundle, defeating the tree-shaking goal for that file.Consider stripping HTML comments before the scan (or tracking comment ranges and skipping matches that fall inside them). Low priority; mostly bites docs/story SFCs.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@vite/lucideIcons.js` around lines 101 - 137, transformButtonIconPropsTemplate currently matches <Button ...> anywhere (using BUTTON_TAG_RE) causing false positives inside HTML comments or fenced code blocks; fix by detecting and skipping ranges that are inside HTML comments or code blocks before transforming. Modify transformButtonIconPropsTemplate to first scan template and record skip ranges for HTML comments (<!-- ... -->) and common code containers like <pre><code>...</code></pre> (store start/end offsets), then in the main while loop check if match.index falls inside any skip range and if so advance BUTTON_TAG_RE.lastIndex past that range (or continue) instead of calling transformButtonTag; ensure imports are only added for real transforms and behavior of findTagEnd, transformButtonTag, output, lastIndex and changed remains unchanged.src/components/Button/Button.vue (1)
65-65: Nit: module-scopedwarnedRuntimeStringIconsnever releases memory.The Set grows unbounded across HMR cycles and across every
(propName, stringValue)pair ever observed. In practice the cardinality is tiny, so this is fine, but aWeakRef-based or per-instance guard would be more hygienic if this ever sees high-churn icon names.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/Button/Button.vue` at line 65, The module-scoped Set warnedRuntimeStringIcons holds every observed (propName, stringValue) pair indefinitely and can leak across HMR/component lifecycles; change this to a per-component guard (e.g., create a Set inside the component's setup or use a WeakMap keyed by the component instance/props so entries are GC'able) and ensure you remove or let it be collected on unmount, or if you need a global cache for many transient keys use a WeakRef-based approach instead; update references to warnedRuntimeStringIcons in Button.vue to use the new per-instance/weak keyed store.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/components/Button/Button.vue`:
- Around line 207-218: hasLucideIconInDefaultSlot can throw when slots.default
returns an empty array because firstVNode is undefined; update the guard so you
don't access properties on undefined: check that slotContent[0] (firstVNode)
exists before reading its type/name (e.g., return false if !firstVNode), or use
optional chaining starting at firstVNode (firstVNode?.type?.name) and then test
startsWith; reference hasLucideIconInDefaultSlot, slots.default, slotContent,
and firstVNode when applying the change.
- Around line 220-246: getRenderableIcon currently returns null for string icons
and warnForRuntimeStringIcon only warns in DEV, so dynamic string icon usage
(e.g., in CalendarDaily/CalendarWeekly) silently renders nothing in production;
change the Button API and implementation to disallow runtime strings: update the
ButtonProps types (icon, iconLeft, iconRight) to be Component | undefined
(remove string), update getRenderableIcon to accept only Component and return it
directly, and remove or narrow DEV-only warnForRuntimeStringIcon usage; migrate
call sites (CalendarDaily, CalendarWeekly) to pass actual component references
instead of strings so the type system prevents regressions.
---
Nitpick comments:
In `@src/components/Button/Button.vue`:
- Line 65: The module-scoped Set warnedRuntimeStringIcons holds every observed
(propName, stringValue) pair indefinitely and can leak across HMR/component
lifecycles; change this to a per-component guard (e.g., create a Set inside the
component's setup or use a WeakMap keyed by the component instance/props so
entries are GC'able) and ensure you remove or let it be collected on unmount, or
if you need a global cache for many transient keys use a WeakRef-based approach
instead; update references to warnedRuntimeStringIcons in Button.vue to use the
new per-instance/weak keyed store.
In `@vite/lucideIcons.js`:
- Around line 139-163: findTagEnd can mis-detect a closing quote when the
character before it is a backslash (e.g. '\\"'); replace the naive check
template[index - 1] !== '\\' with a trailing-backslash parity test: when
encountering a quote in findTagEnd, walk backwards from index-1 counting
consecutive '\' characters and treat the quote as escaped only if the count is
odd (count % 2 === 1), otherwise close the quote; update the logic around the
quote variable handling in findTagEnd (using template, startIndex, quote, index)
to use this parity check so escaped backslashes are handled correctly.
- Around line 101-137: transformButtonIconPropsTemplate currently matches
<Button ...> anywhere (using BUTTON_TAG_RE) causing false positives inside HTML
comments or fenced code blocks; fix by detecting and skipping ranges that are
inside HTML comments or code blocks before transforming. Modify
transformButtonIconPropsTemplate to first scan template and record skip ranges
for HTML comments (<!-- ... -->) and common code containers like
<pre><code>...</code></pre> (store start/end offsets), then in the main while
loop check if match.index falls inside any skip range and if so advance
BUTTON_TAG_RE.lastIndex past that range (or continue) instead of calling
transformButtonTag; ensure imports are only added for real transforms and
behavior of findTagEnd, transformButtonTag, output, lastIndex and changed
remains unchanged.
In `@vite/lucideIcons.test.ts`:
- Around line 4-8: The helper getButtonIconTransform may return undefined,
causing ambiguous test failures; update getButtonIconTransform (or the tests
that call it) to assert the plugin exists before using its transform: locate the
lucideIcons() lookup in getButtonIconTransform and add a guard such as
expect(plugin).toBeDefined() (or throw a clear Error if plugin is undefined) so
tests fail with a descriptive message instead of a "Cannot read properties of
undefined" when the 'frappe-ui-button-lucide-icon-props' plugin is missing or
renamed.
- Around line 38-58: The test for getButtonIconTransform currently only asserts
a new '<script setup lang="ts">' block is inserted but doesn't verify the
original '<script lang="ts">...export default {}</script>' remains; update the
test (in vite/lucideIcons.test.ts) to also assert result.code contains the
original script block (e.g., the exact string '<script lang="ts">export default
{}</script>' or at least '<script lang="ts">' and 'export default {}') after
calling plugin.transform so regressions that remove/replace the original script
will fail.
In `@vite/README.md`:
- Around line 109-119: Add a one-line runtime note clarifying that dynamic
string icon props are not transformed and will render no icon at runtime because
Button.vue's getRenderableIcon returns null for non-literal strings; mention
using a bound string like :icon="'menu'" or a prop-bound value will produce no
icon (and only a DEV console warning) and recommend passing the imported Lucide
component or using a static literal instead.
🪄 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: afa71324-85f0-4cab-ac53-78da00fd47cd
📒 Files selected for processing (6)
src/components/Button/Button.cy.tssrc/components/Button/Button.vuesrc/components/Button/types.tsvite/README.mdvite/lucideIcons.jsvite/lucideIcons.test.ts
| const hasLucideIconInDefaultSlot = computed(() => { | ||
| if (!slots.default) return false | ||
|
|
||
| const slotContent = slots.default() | ||
| if (!Array.isArray(slotContent)) return false | ||
| // if the slot contains only one element and it's a lucide icon | ||
| // render it as an icon button | ||
| let firstVNode = slotContent[0] | ||
| if ( | ||
| typeof firstVNode.type?.name == 'string' && | ||
| firstVNode.type?.name?.startsWith('lucide-') | ||
| ) { | ||
| return true | ||
| } | ||
| return false | ||
|
|
||
| const firstVNode = slotContent[0] | ||
| return ( | ||
| typeof firstVNode.type?.name === 'string' && | ||
| firstVNode.type.name.startsWith('lucide-') | ||
| ) | ||
| }) |
There was a problem hiding this comment.
Potential TypeError when default slot returns an empty array.
slotContent[0] is undefined when a consumer passes an empty-returning default slot (e.g. default: () => [] or <template #default />). Accessing firstVNode.type?.name then throws Cannot read properties of undefined. The optional chain needs to start at firstVNode itself.
🛡️ Proposed fix
const firstVNode = slotContent[0]
return (
- typeof firstVNode.type?.name === 'string' &&
- firstVNode.type.name.startsWith('lucide-')
+ typeof firstVNode?.type?.name === 'string' &&
+ firstVNode.type.name.startsWith('lucide-')
)📝 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.
| const hasLucideIconInDefaultSlot = computed(() => { | |
| if (!slots.default) return false | |
| const slotContent = slots.default() | |
| if (!Array.isArray(slotContent)) return false | |
| // if the slot contains only one element and it's a lucide icon | |
| // render it as an icon button | |
| let firstVNode = slotContent[0] | |
| if ( | |
| typeof firstVNode.type?.name == 'string' && | |
| firstVNode.type?.name?.startsWith('lucide-') | |
| ) { | |
| return true | |
| } | |
| return false | |
| const firstVNode = slotContent[0] | |
| return ( | |
| typeof firstVNode.type?.name === 'string' && | |
| firstVNode.type.name.startsWith('lucide-') | |
| ) | |
| }) | |
| const hasLucideIconInDefaultSlot = computed(() => { | |
| if (!slots.default) return false | |
| const slotContent = slots.default() | |
| if (!Array.isArray(slotContent)) return false | |
| const firstVNode = slotContent[0] | |
| return ( | |
| typeof firstVNode?.type?.name === 'string' && | |
| firstVNode.type.name.startsWith('lucide-') | |
| ) | |
| }) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/Button/Button.vue` around lines 207 - 218,
hasLucideIconInDefaultSlot can throw when slots.default returns an empty array
because firstVNode is undefined; update the guard so you don't access properties
on undefined: check that slotContent[0] (firstVNode) exists before reading its
type/name (e.g., return false if !firstVNode), or use optional chaining starting
at firstVNode (firstVNode?.type?.name) and then test startsWith; reference
hasLucideIconInDefaultSlot, slots.default, slotContent, and firstVNode when
applying the change.
| if (import.meta.env.DEV) { | ||
| watchEffect(() => { | ||
| warnForRuntimeStringIcon('icon', props.icon) | ||
| warnForRuntimeStringIcon('iconLeft', props.iconLeft) | ||
| warnForRuntimeStringIcon('iconRight', props.iconRight) | ||
| }) | ||
| } | ||
|
|
||
| function getRenderableIcon(icon?: ButtonProps['icon']) { | ||
| if (!icon || typeof icon === 'string') return null | ||
| return icon | ||
| } | ||
|
|
||
| function warnForRuntimeStringIcon( | ||
| propName: 'icon' | 'iconLeft' | 'iconRight', | ||
| icon?: string | ButtonProps['icon'], | ||
| ) { | ||
| if (!icon || typeof icon !== 'string') return | ||
|
|
||
| const key = `${propName}:${icon}` | ||
| if (warnedRuntimeStringIcons.has(key)) return | ||
|
|
||
| warnedRuntimeStringIcons.add(key) | ||
| console.warn( | ||
| `[frappe-ui] Button ${propName}="${icon}" requires the frappe-ui/vite transform or a component binding like :${propName}="LucideX".`, | ||
| ) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find dynamic (v-bind / :icon) usages on Button that are not component references
rg -nP --type=vue -C2 '<Button\b[^>]*\s(?::|v-bind:)(?:icon|icon-left|icon-right|iconLeft|iconRight)\s*='
# Find call sites that forward a string-typed prop into Button icon props
rg -nP --type=vue -C2 '<Button\b[^>]*\s(?::|v-bind:)(?:icon|icon-left|icon-right|iconLeft|iconRight)\s*="[a-zA-Z_$][\w.$]*"'Repository: frappe/frappe-ui
Length of output: 115
🏁 Script executed:
#!/bin/bash
# Find all .vue files and search for Button component with icon bindings
fd -e vue -x rg -l '<Button\b' {} \; | head -20
# Then search specifically for dynamic icon bindings in those files
echo "=== Dynamic icon bindings in Button usages ==="
fd -e vue -x grep -l '<Button' {} \; | xargs rg -C2 ':(icon|iconLeft|iconRight|icon-left|icon-right)\s*=' 2>/dev/null | head -50
# Also search for string-typed icon props being forwarded
echo "=== String icon prop forwarding patterns ==="
fd -e vue -x grep -E '<Button[^>]*(icon|iconLeft|iconRight)' {} \; 2>/dev/null | grep -E '"[a-zA-Z_]' | head -30Repository: frappe/frappe-ui
Length of output: 4991
🏁 Script executed:
#!/bin/bash
# Find ButtonProps definition
rg 'interface ButtonProps|type ButtonProps' -A 15 src/components/Button/Repository: frappe/frappe-ui
Length of output: 961
🏁 Script executed:
#!/bin/bash
# Get full ButtonProps icon property definition
rg 'icon\?' src/components/Button/types.ts -A 3Repository: frappe/frappe-ui
Length of output: 128
🏁 Script executed:
#!/bin/bash
# Check CalendarWeekly and CalendarDaily for context
cat -n src/components/Calendar/CalendarWeekly.vue | sed -n '1,30p'
cat -n src/components/Calendar/CalendarDaily.vue | sed -n '1,30p'Repository: frappe/frappe-ui
Length of output: 2625
🏁 Script executed:
#!/bin/bash
# Search for Button.vue to find what Phase 2/3 components are mentioned or planned
rg 'Phase|TODO|FIXME|migration' src/components/Button/ -i
rg 'DropdownOption|Input|Switch' src/components/ --type=vue -l | head -10Repository: frappe/frappe-ui
Length of output: 87
🏁 Script executed:
#!/bin/bash
# Find all Button usages with string literal icons (not component references)
rg -U '<Button[^>]*\s(?:icon|iconLeft|iconRight|icon-left|icon-right)\s*=\s*["\x27][a-z\-]+["\x27]' --type vue -c
# Also find computed/dynamic string icons
rg -U 'iconRight\s*=\s*".*\?" ' src/components/ --type vue -lRepository: frappe/frappe-ui
Length of output: 115
🏁 Script executed:
#!/bin/bash
# Search for any TODO, FIXME, or comments about the string icon deprecation
rg 'string.*icon|icon.*string|migration|Phase' src/components/Button/ -i -A 2 -B 2Repository: frappe/frappe-ui
Length of output: 2754
🏁 Script executed:
#!/bin/bash
# List all components that use Button with potential string icons
fd -e vue -path "*/src/components/*" -x grep -l '<Button' {} \; | head -20Repository: frappe/frappe-ui
Length of output: 228
Dynamic string icons in Button cause silent null rendering in production — migrate CalendarDaily and CalendarWeekly to component references.
getRenderableIcon returns null for any string, and the warning only fires in DEV builds. CalendarDaily.vue and CalendarWeekly.vue still pass dynamic strings (:iconRight="showCollapsable ? (isCollapsed ? 'chevron-down' : 'chevron-up') : ''"), which silently render nothing in production with no diagnostic.
The type signature still allows icon?: string | Component, enabling this unsafe pattern. Consider tightening ButtonProps.{icon,iconLeft,iconRight} to Component | undefined once all call sites are migrated, making the string path a compile error rather than a silent null.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/Button/Button.vue` around lines 220 - 246, getRenderableIcon
currently returns null for string icons and warnForRuntimeStringIcon only warns
in DEV, so dynamic string icon usage (e.g., in CalendarDaily/CalendarWeekly)
silently renders nothing in production; change the Button API and implementation
to disallow runtime strings: update the ButtonProps types (icon, iconLeft,
iconRight) to be Component | undefined (remove string), update getRenderableIcon
to accept only Component and return it directly, and remove or narrow DEV-only
warnForRuntimeStringIcon usage; migrate call sites (CalendarDaily,
CalendarWeekly) to pass actual component references instead of strings so the
type system prevents regressions.
1 participant
Summary
Buttonicon string props to direct Lucide imports infrappe-ui/viteFeatherIconruntime handling fromButton.vueButton icon="x"transform behavior and add transform testsThis keeps the nice DX of:
<Button icon="x" />while making the transformed path tree-shakeable.
How it works
When
frappe-ui/viteis enabled, static string literals onButtonprops are rewritten at build time:<Button icon="menu" icon-left="search" icon-right="chevron-down" />becomes direct per-icon Lucide imports, so only the used icons are pulled into the bundle.
Button.vueno longer renders string icon props throughFeatherIconat runtime. In development, runtime string values now warn so consumers can switch to the Vite transform or explicit Lucide component bindings for dynamic cases.Verification
yarn vitest run vite/lucideIcons.test.tsyarn buildFollow-up migration plan for removing
FeatherIconfrom frappe-uiThis PR only removes
FeatherIconfromButtonruntime and adds the Lucide transform path. To fully removeFeatherIconfrom frappe-ui, the next steps are:Phase 1: direct component replacements
<FeatherIcon ... />usages in core components and frappe module components with direct Lucide importsFeatherIconimports in stories/tests while touching those filesPhase 2: public API cleanup
DropdownOption.iconInput.iconLeftSwitch.icontypeof icon === 'string'Buttonon the compile-time transform path for static template literalsPhase 3: consumer migration
Phase 4: final removal
src/components/FeatherIcon.vueFeatherIconexport fromsrc/index.tsfeather-iconspackage dependencyfeather-*classes in icon components/docsThat plan can be handled incrementally in follow-up PRs.
Summary by CodeRabbit
New Features
Documentation