Conversation
|
|
commit: |
There was a problem hiding this comment.
🟡 Changes recommended
There are correctness/type-safety issues that can break TS consumption and HMR behavior (type mismatch in options typing, overly-permissive HMR loader detection, and a Vite watch/meta safety concern).
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR introduces a new filesystem-backed custom collection loader that can participate in HMR by tracking resolved SVG paths and invalidating the corresponding ~icons/... virtual modules across supported bundlers.
Changes:
- Added
FileSystemHMRIconLoader(and supporting HMR plumbing) to enable HMR for filesystem custom collections. - Updated the unplugin entry to register watched SVG files (non-Vite) and to invalidate Vite modules via
handleHotUpdate. - Updated multiple examples (Vite, Webpack, Rspack, Nuxt) to demonstrate the new loader.
File summaries
| File | Description |
|---|---|
| src/types.ts | Introduces CustomHMRIconLoader / CustomCollectionIconLoader types (but options typing still needs alignment). |
| src/loaders.ts | Adds FileSystemHMRIconLoader implementation with path/name tracking for HMR. |
| src/index.ts | Integrates watch-file registration and Vite handleHotUpdate invalidation hook. |
| src/core/options.ts | Transforms HMR-aware custom collections into iconify-compatible loaders and exposes HMR helpers. |
| src/core/loader.ts | Adds HMR loader type guard and adjusts iconify loader option shaping. |
| src/core/hmr.ts | New helper to map changed SVG paths to virtual icon module IDs for invalidation. |
| examples/webpack/webpack.config.js | Demonstrates FileSystemHMRIconLoader usage in Webpack and adds a minimal HTML emitter plugin. |
| examples/webpack/src/main.js | Uses ?raw icon imports and renders both built-in and custom icons. |
| examples/webpack/package.json | Adds a dev script for webpack serve. |
| examples/webpack/custom-a/car-a.svg | Adds a sample custom SVG icon for the Webpack example. |
| examples/vite-vue3/vite.config.ts | Switches the custom collection example to FileSystemHMRIconLoader. |
| examples/rspack-vue3/src/App.vue | Demonstrates importing and rendering a custom icon in Rspack example. |
| examples/rspack-vue3/rspack.config.mjs | Configures custom collections via FileSystemHMRIconLoader in Rspack example. |
| examples/rspack-vue3/custom-a/car-a.svg | Adds a sample custom SVG icon for the Rspack example. |
| examples/nuxt4/nuxt.config.ts | Wires FileSystemHMRIconLoader into Nuxt module config and icon resolver config (currently mismatched). |
| examples/nuxt4/app/custom-a/SteeringWheel.svg | Adds a sample custom SVG icon for the Nuxt example. |
| examples/nuxt4/app/custom-a/car-a.svg | Adds a sample custom SVG icon for the Nuxt example. |
| examples/nuxt4/app/app.vue | Demonstrates importing and rendering a custom icon in Nuxt app. |
Review details
- Files reviewed: 14/18 changed files
- Comments generated: 5
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
chore: remove async from `collectVirtualIconModuleNames` chore: return undefined when no modules returned
…d helper to hmr module
There was a problem hiding this comment.
🟡 Changes recommended
There are confirmed HMR correctness gaps (Vite watch registration/invalidation ID mismatches) and a TypeScript API typing mismatch that will block correct usage without casts.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (2)
Previously missed (1) — in code that hasn't changed since the last review.
src/loaders.ts:70
resolveIcon()usesfs.lstat(). For symlinked SVGs,lstat().isFile()is false, so icons behind symlinks will never load. Usingfs.stat()(follows symlinks) avoids this class of false negatives while still letting you checkisFile().
src/types.ts:12
CustomHMRIconLoaderis now part of the public types andFileSystemHMRIconLoader()returns it, butOptions.customCollectionsis still typed asRecord<string, CustomIconLoader | InlineCollection>. This makes TypeScript configs unable to pass...FileSystemHMRIconLoader(...)without casts, even though runtime support exists inresolveOptions().
export interface CustomHMRIconLoader {
/**
* The name of the custom collection.
*/
name: string
- Files reviewed: 14/18 changed files
- Comments generated: 3
- Review effort level: Lite
There was a problem hiding this comment.
🟡 Changes recommended
There are correctness/API issues in the new loader implementation (symlink handling and alias-to-path mapping) and a TypeScript typing mismatch that will break consumption without casts.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (2)
Previously missed (2) — in code that hasn't changed since the last review.
src/loaders.ts:74
- Using
fs.lstat()here prevents loading icons that are symlinked (common in monorepos/workspaces), becauselstat().isFile()is false for symbolic links.fs.stat()follows symlinks and keeps the behavior closer to typical file loaders while still ensuring the target is a file.
src/loaders.ts:76 pathToNamestores only one icon name per SVG path. If the same file is resolved through different name variants (e.g.SteeringWheel.svgimported as bothSteeringWheelandsteering-wheel), the later import overwrites the mapping and HMR invalidation will only refresh one of the virtual ids. To make HMR reliable, consider tracking a set/list of icon names per path and invalidating all matching virtual modules.
- Files reviewed: 14/18 changed files
- Comments generated: 1
- Review effort level: Lite
|
My plan:
|
|
Published new version of Iconify Utils with new loader. |
|
superseded by #440 |
Description
This PR adds a new
FileSystemHMRIconLoaderto allow use HMR when using a custom collection from filesystem:server.moduleGraphwith env. api)it seems there was a bug fixed atv3.3.0 has the fix 🤞unplugin.webpacklogic inthis.addWatchFile=> rspack should use the same change applied in this PRFileSystemHMRIconLoaderat README fileLinked Issues
closes #424
supersedes ##425
Additional context
I need to check it here, once tested will be moved to iconify utils (the new
FileSystemHMRIconLoaderloader).NOTE: we need to update some dependencies here adding some new stuff (will try to send some PR this weekend 🤞 ):
@unvuetify)/cc @cyberalien @antfu