From b7c2dfa49b1cb4b3cb22d9b043e833589f9845cb Mon Sep 17 00:00:00 2001 From: meh Date: Tue, 8 Sep 2026 05:36:21 +0700 Subject: [PATCH] fix(icons): draw everything from one Iconify set A consumer installs icon sets itself, so every set this library reaches for is a set the consumer has to know to install. Nothing stated the requirement and nothing failed loudly when it was unmet: the build printed `Cannot load icon set for "mdi"` among its warnings and the icon rendered as empty space. crates.vip installed `@iconify-json/lucide`, which is what the fleet uses. Five icons were `mdi` -- in `LanguageSwitcher`, `ThemeColorPicker`, `MobileListView` and the Firefox banner -- and all five were blank. This library develops against `@iconify/json`, the whole collection, so it could never see that. Four had direct lucide equivalents and were converted. The fifth is the Firefox brand mark, and lucide has no brand glyphs, so it becomes a prop on `FirefoxPWABanner` with no default: a consumer who wants it supplies it and installs the set it needs, and everyone else pays nothing for a banner they never render. The verifier walks the source for `icon-[set--name]` and fails on any set but `lucide`, skipping comment lines so the one that names `mdi` in prose does not trip it. Reintroducing a single `mdi` icon fails it by file name. --- docs/api-contract.md | 1 + .../components/FirefoxPWABanner.tsx | 18 ++--- src/components/immersive-landing/types.ts | 15 ++++ .../LanguageSwitcher.layout.tsx | 2 +- .../table/MobileListView.layout.tsx | 2 +- .../ThemeColorPicker.layout.tsx | 2 +- tests/components/single-icon-set.test.ts | 69 +++++++++++++++++++ 7 files changed, 98 insertions(+), 11 deletions(-) create mode 100644 tests/components/single-icon-set.test.ts diff --git a/docs/api-contract.md b/docs/api-contract.md index 0353b649..84023811 100644 --- a/docs/api-contract.md +++ b/docs/api-contract.md @@ -936,6 +936,7 @@ _No props beyond HTML attributes and `UIBaseProps`._ ```ts extensionUrl?: string +icon?: string | JSX.Element onDismiss?: () => void onInstall?: () => void storageKey?: string diff --git a/src/components/immersive-landing/components/FirefoxPWABanner.tsx b/src/components/immersive-landing/components/FirefoxPWABanner.tsx index a3dde2da..d72b6ad4 100644 --- a/src/components/immersive-landing/components/FirefoxPWABanner.tsx +++ b/src/components/immersive-landing/components/FirefoxPWABanner.tsx @@ -135,7 +135,7 @@ export const FirefoxPWABanner: Component = (props) => { aria-label={texts().closeLabel} > @@ -148,13 +148,15 @@ export const FirefoxPWABanner: Component = (props) => { {...{ class: CLASSES.firefoxBanner.media }} >
- - + + {(icon) => ( + + )}
diff --git a/src/components/immersive-landing/types.ts b/src/components/immersive-landing/types.ts index 47c1dda5..c64f480f 100644 --- a/src/components/immersive-landing/types.ts +++ b/src/components/immersive-landing/types.ts @@ -138,6 +138,21 @@ export interface FirefoxPWABannerProps { extensionUrl?: string; storageKey?: string; texts?: FirefoxPWABannerTexts; + /** + * The browser mark shown beside the text. Omit it and the banner renders + * without one. + * + * A default lived here as `icon-[mdi--firefox]`, and it was the only reason + * this library needed a second Iconify set. Everything else it draws is + * `lucide`, which has no brand glyphs, so one banner in one optional + * component obliged every consumer to install all of `-json/mdi` -- + * and a consumer who installed only `lucide` got build warnings and a blank + * space, which is what happened on crates.vip. + * + * Accepts what `Icon` accepts: an Iconify class such as + * `"icon-[mdi--firefox]"`, or an inline SVG element. + */ + icon?: string | JSX.Element; onInstall?: () => void; onDismiss?: () => void; } diff --git a/src/components/language-switcher/LanguageSwitcher.layout.tsx b/src/components/language-switcher/LanguageSwitcher.layout.tsx index 64738b9e..ae327565 100644 --- a/src/components/language-switcher/LanguageSwitcher.layout.tsx +++ b/src/components/language-switcher/LanguageSwitcher.layout.tsx @@ -88,7 +88,7 @@ const LanguageSwitcher: Layout< when={!props.i18n.isLoading} fallback={ diff --git a/src/components/theme-color-picker/ThemeColorPicker.layout.tsx b/src/components/theme-color-picker/ThemeColorPicker.layout.tsx index 1ab1d262..53e2e139 100644 --- a/src/components/theme-color-picker/ThemeColorPicker.layout.tsx +++ b/src/components/theme-color-picker/ThemeColorPicker.layout.tsx @@ -211,7 +211,7 @@ const ThemeColorPicker: Layout = > {props.children ?? ( { + const files: string[] = []; + const walk = (dir: string) => { + for (const entry of readdirSync(dir)) { + const path = join(dir, entry); + if (statSync(path).isDirectory()) walk(path); + // Generated twins mirror their layout source, so a finding in one is the + // same finding in the other. + else if (/\.(ts|tsx|css)$/.test(entry) && !entry.includes(".generated.")) + files.push(path); + } + }; + walk(SRC); + + // `icon-[set--name]` as it is written in a class or an `src`. Prose in a + // comment is not a reference, so the match has to be anchored to the + // delimiter a real one carries. + const references = new Map(); + for (const file of files) { + const text = readFileSync(file, "utf8"); + for (const line of text.split("\n")) { + if (/^\s*(\*|\/\/)/.test(line)) continue; + for (const match of line.matchAll(/icon-\[([a-z0-9]+)--[a-z0-9-]+\]/g)) { + const set = match[1]; + if (!references.has(set)) references.set(set, []); + references.get(set)?.push(file.replace(SRC, "src")); + } + } + } + + it("finds icon references, so a broken walk cannot pass silently", () => { + expect(references.get(ALLOWED)?.length ?? 0).toBeGreaterThan(10); + }); + + it("uses no set other than the one consumers are told to install", () => { + const strays = [...references] + .filter(([set]) => set !== ALLOWED) + .map(([set, where]) => `${set}: ${[...new Set(where)].join(", ")}`); + expect(strays).toEqual([]); + }); +});