[Project Solar / Phase 1 / Foundations] Flight icons &Icon carbonization#3584
[Project Solar / Phase 1 / Foundations] Flight icons &Icon carbonization#3584zamoore wants to merge 20 commits intoproject-solar/phase-1-main-feature-branchfrom
Icon carbonization#3584Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Icon carbonization
didoo
left a comment
There was a problem hiding this comment.
I have left a few comments/suggestions. I haven't thoroughly reviewed the hds-icon-registry.ts file, because one of the suggestions revolves around moving some logic onto this service.
I also have a couple of extra high-level/general questions:
- has this latest implementation already been tested in a couple of consuming apps?
- how does the
flightIconsSpriteLazyEmbedlogic works now? inshowcase/config/environment.jsI still see it set to true, but the SVG sprite is not injected anymore; should consumers remove this from their settings? should we remove other code involved with this setting, if it's not needed anymore?
packages/components/src/instance-initializers/load-sprite-empty.ts
Outdated
Show resolved
Hide resolved
packages/components/src/instance-initializers/load-sprite-empty.ts
Outdated
Show resolved
Hide resolved
similar to what Zack has done in #3584
37131cf to
f30a05a
Compare
similar to what Zack has done in #3584
similar to what Zack has done in #3584
didoo
left a comment
There was a problem hiding this comment.
I have left a bunch of comments/suggestions, and one large comment about further possible performance improvements to the hds-icon-registry service.
I'll set up a sync meeting later in the day, so we can try to get this PR over the line today (at this point no major architectural changes are needed, only small refinements here and there)
packages/flight-icons/scripts/build-parts/generateBundleSymbolJS.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
I asked Copilot (Claude Opus 4.6) to make me an analysis of this file file and look for possible optimizations (both in JS execution and DOM access/manipulation), mostly to confirm a couple of instincts I had around hasDOM, _symbolExists and document.getElementById(symbolId) and it confirmed them, but also provided a list of interesting possible improvements that I'm adding below, for your evaluation (for context: happy to defer these to a follow-up task)
Code Simplification
Repeated _entries.set(...) pattern
The this._entries.set(key, { symbolId, status, ... }) pattern appears 6 times across the file. A small _setStatus(key, symbolId, status, error?) helper would collapse each to a one-liner and make status transitions easier to audit.
hasDOM() called repeatedly, but result never changes
hasDOM() is a pure function of globals (window, document) that's called in _symbolExists, requestLoad, and _scheduleFlush. The result is constant at runtime — it could be a module-scope constant (const HAS_DOM = ...).
Spread operator creating redundant copies
In _runQueuedLoad and _scheduleFlush, { ...entry, status } spreads a 2–3 field object just to change status. Since symbolId is the only other field, explicit assignment ({ symbolId: entry.symbolId, status }) avoids the spread iterator machinery and is more readable.
Redundant _symbolExists checks
_symbolExists is called in 3 places: requestLoad (microtask), _runQueuedLoad (before fetch), and _scheduleFlush (before injection). The check in requestLoad guards against pre-populated sprites — but the sprite container starts empty (per load-sprite-empty). If pre-populated sprites aren't a supported use case, that first check is unnecessary.
Dead fallback in concurrency cap
The || MAX_CONCURRENT_LOADS_CAP at the end of the MAX_CONCURRENT_LOADS expression is dead code — Math.max(8, ...) can never return 0 or NaN. Harmless, but misleading.
JS Performance
Biggest win: skip the Loading status write ⚠️
In _runQueuedLoad, the status is set to Loading in the TrackedMap. But no consumer reads this state — the Hds::Icon component only checks for Loaded. This write dirties Glimmer tracking tags and triggers unnecessary scheduling work for every single icon. Removing it would halve the number of TrackedMap writes in the happy path (4 → 2–3 per icon).
Array.shift() is O(n)
_drainQueue uses this._queue.shift(). For n icons, total drain cost is O(n²). For typical loads (20–100 icons) this is negligible, but an index-pointer approach (this._queue[this._queueHead++] with periodic trimming) would make it O(n) with zero API change.
String concatenation in _scheduleFlush
combined += markup inside the loop creates n-1 intermediate strings. Using Array.push() + .join('') is more predictable for larger batches (100+ icons flushing in one frame).
Promise.resolve().then(...) deferral
This is necessary — without it, writing to the TrackedMap during render (from the loadIcon modifier) would trigger a Glimmer backtracking assertion. The cost (one microtask per icon) is acceptable, but a comment explaining why it's needed would improve maintainability.
DOM Performance
Sprite root element not cached
_scheduleFlush calls document.getElementById(ROOT_ID) on every rAF callback. The root element never changes — it could be cached as a class property after first lookup.
document.getElementById called 3× per icon
_symbolExists calls getElementById in three separate code paths per icon. Since the service already tracks state in its TrackedMap, these DOM lookups are redundant if external DOM mutation of the sprite container isn't a concern. Trusting the service's own state would eliminate most of these.
What's already excellent ✅
insertAdjacentHTMLwith a single combined string — one DOM parse + one reflow per flush. Optimal.- rAF coalescing with the
_flushScheduledflag — correctly prevents stacked callbacks. - No layout thrashing — no layout property reads (
offsetWidth, etc.) between DOM writes.
Robustness Gap
There's no error handling around root.insertAdjacentHTML(combined) in _scheduleFlush. If the markup is malformed, the entire batch fails and those icons remain in Loading status forever (they'll never transition to Loaded or Error).
Prioritized Impact Summary
| Finding | Category | Impact |
|---|---|---|
Eliminate Loading status write to TrackedMap |
JS Perf | High — removes unnecessary Glimmer invalidations |
| Cache sprite root element | DOM Perf | Medium — eliminates repeated DOM lookup |
Reduce redundant _symbolExists DOM checks |
DOM Perf | Medium — up to 2 fewer getElementById per icon |
Extract _setStatus helper |
Simplification | Low — removes 6× repetition |
hasDOM() → module constant |
Simplification | Low — cleaner, one fewer function call per path |
Array.shift() → index pointer |
JS Perf | Low — O(n) vs O(n²), matters at scale |
Add try/catch around insertAdjacentHTML |
Robustness | Medium — prevents silent batch failures |
|
@didoo I finished responding to your individual comments (thanks!). I'm going to address the Copilot feedback tomorrow morning. Thanks again |
📌 Summary
If merged, this PR introduces an async-loading architecture for the HDS icon system, replacing the legacy monolithic SVG sprite.
By shifting from a single large sprite to granular, on-demand module fetching, we enable a unified strategy for the IBM Carbon migration while significantly reducing initial bundle sizes. Additionally, this PR implements concurrency management and DOM batching to ensure that icon-dense pages remain performant and responsive.
🛠️ Detailed description
Architectural Shift: From Monolithic Sprite to Async Modules
We have moved away from loading a massive SVG sprite containing the entire Flight icon set.
HdsIconcomponent fetches these modules asynchronously at runtime, effectively "tree-shaking" the icon library over the network.Performance & Loading Optimization
To prevent "rerender storms" and network bottlenecks on icon-heavy screens (like the HDS Gallery), we’ve implemented several orchestration improvements:
<defs>host in batches. This drastically reduces DOM churn and parsing overhead compared to per-icon insertion.HdsIconnow utilizes ember-concurrency to manage the lifecycle of icon requests, ensuring reactive mode switching (Flight ↔ Carbon) is handled cleanly without race conditions.New Build Pipeline & Registry
packages/flight-icons: Added generateBundleSymbolJS to produce granular files for symbol-js/flight and symbol-js/carbon.🔗 External links
Jira ticket: HDS-5198
👀 Component checklist
💬 Please consider using conventional comments when reviewing this PR.
📋 PCI review checklist
Examples of changes to controls include access controls, encryption, logging, etc.
Examples include changes to operating systems, ports, protocols, services, cryptography-related components, PII processing code, etc.