refactor!: v4 — restructure library internals, drop v2 emit shim - #9
Conversation
Plugin emit specs gain `emit` + `inject: 'embed'` (+ SvgSpec `encoding`) to inline ICO/PNG/SVG bytes into the HTML `<link>` instead of, or alongside, writing files. `svg-to-ico inject` mirrors it with `--embed` / `--encoding` / `--asset-dir`. New pure `src/data-uri.ts`; data hrefs skip cache-busting and the dev HMR client; no-output specs warn once. Also migrate CLI help text to ANSI colors via `colors.ts` (TTY + NO_COLOR/FORCE_COLOR guard, auto-reset) and drop a redundant `as InjectMode`. Rides along with already-staged build/CI tweaks (dprint, autofix workflow, bunfig, preload removal).
BREAKING: `emit` accepts only `EmitSpec[]`; removed the v2
`{ source, sizes, inject }` object shape and its exported types
(LegacyEmitOptions, EmitOptions, isLegacyEmit, NormalizedEmit,
IncludeSourceOptions, EmitSizesFormat, EMIT_SIZES_FORMATS).
All option parsing + validation collapses into one pure
`parseConfig(opts) -> ResolvedConfig` boundary; the config hook
only applies Vite root/base. normalize-emit.ts deleted.
Byte production + caching + embed-URI memoization move into a
testable AssetProducer class; dev client snippets move to
dev-client.ts. index.ts drops from 603 to ~298 lines and only
wires parseConfig → resolveSpecs → producer/builder into the
three Vite plugins. Also removes the `as { size }` assertions
via clean discriminated narrowing.
Public option size fields are plain `number`; the validated `IconSize` brand is produced only by `parseSize(raw, max)` in resolve-specs as it builds the internal size structures.
Bump to 4.0.0, CHANGELOG release notes with the v2→v3 migration table, README updated to EmitSpec[]-only.
The `--mode`/`-m` flag never affected output (the CLI emits ICO + optional SVG links regardless of minimal/full). Drops it plus the now-dead `InjectMode` / `INJECT_MODES` exports.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
📜 Recent review details⏰ Context from checks skipped due to timeout. (2)
🧰 Additional context used🧠 Learnings (1)📚 Learning: 2026-05-12T20:04:37.791ZApplied to files:
🔍 Remote MCP GitHub GrepAdditional review context
🔇 Additional comments (2)
📝 WalkthroughRefactor
Verification reported: 176 passing tests, clean typecheck/lint/format, and successful WalkthroughThis PR bumps the package to v4.0.0, removes the legacy v2 Sequence Diagram(s)sequenceDiagram
participant Vite
participant svgToIco
participant parseConfig
participant AssetProducer
participant buildFaviconTags
participant buildShimScript
Vite->>svgToIco: initialise plugin
svgToIco->>parseConfig: parseConfig(opts)
parseConfig-->>svgToIco: ResolvedConfig
svgToIco->>AssetProducer: new AssetProducer(cfg, sizes)
Vite->>svgToIco: transformIndexHtml / build hooks
svgToIco->>buildFaviconTags: buildFaviconTags(injections, ctx)
buildFaviconTags-->>svgToIco: HtmlTagDescriptor[]
svgToIco->>buildShimScript: buildShimScript(tags, hmr)
buildShimScript-->>svgToIco: shim script
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 5 | ❌ 3❌ Failed checks (3 inconclusive)
✅ Passed checks (5 passed)
Comment |
The smoke fixture is no longer a workspace member resolving to a volatile (clean-on-build) dist/. A `pretest` lifecycle script packs the plugin to a tarball inside the fixture and installs it, so CI's `test` step builds+packs before running — the smoke test exercises the real published artifact. The standalone consumer fixture is excluded from the library typecheck.
- config: validate emit/inject/encoding shapes and dev flags at the parse boundary; reject empty PNG inject.sizes subsets (silent no-output) - favicon-tags: fragment-aware cacheBust (?v= before #frag); widen embed resolver return type to Promise<string | undefined> - dev-client: HMR re-busts only icon/shortcut-icon links, not apple-touch-icon - raster: reject ICO layers above 256px before packing - cli inject: preload embed assets from resolved injections, not --source - tests: cover the new validation, cacheBust fragments, ICO range guard; await fixture restores in integration finally blocks Skipped (deliberate): autofix.yml hard-fail (dismissed on PR), data-uri SVG escaping (intentional mini-svg-data-uri behavior, test-covered), raster path passthrough (CR's suggestion regresses to N reads for N sizes).
CI: the smoke fixture's bun.lock pinned the packed tarball's integrity, but pretest repacks it from changing source every run, so CI's CI=true (implicit frozen) install hit IntegrityCheckFailed. Stop tracking the fixture lockfile and install it --no-frozen-lockfile so it reconciles to the fresh tarball. data-uri: escapeSvgUtf8 swapped " -> ' and collapsed whitespace, mutating CDATA / xml:space="preserve" / <style> content so the embedded favicon could render differently from source. Now percent-encode only what the URI/attribute requires; the SVG round-trips byte-for-byte. raster: generateIco also rejects sizes below 1 (was >256 only); await the rejection assertion in its test (both per CodeRabbit). README: document the v4 inject:'embed' / encoding feature, fix PngSpec sizes (1-4096, not 1-256), add emit/encoding rows to the spec tables.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/data-uri.ts`:
- Around line 39-46: The escapeSvgUtf8 helper in src/data-uri.ts is still
leaving raw carriage returns unencoded, which breaks the byte-for-byte
round-trip for Windows-style line endings. Update escapeSvgUtf8 to
percent-encode \r alongside the existing percent-encoding logic, and add a
regression test in the data URI/SVG test coverage that includes a \r\n case to
verify the href preserves the original bytes.
🪄 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: ASSERTIVE
Plan: Pro
Run ID: 85c3bd59-6b8f-49e3-ad26-b6bf7e112396
⛔ Files ignored due to path filters (1)
tests/smoke/fixture/bun.lockis excluded by!**/*.lock
📒 Files selected for processing (8)
.gitignoreREADME.mdpackage.jsonsrc/data-uri.tssrc/raster.tstests/cli.test.tstests/data-uri.test.tstests/raster.test.ts
📜 Review details
⏰ Context from checks skipped due to timeout. (3)
- GitHub Check: Socket Security: Pull Request Alerts
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Analyze (actions)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2026-05-12T20:04:37.791Z
Learnt from: kjanat
Repo: kjanat/vite-svg-to-ico PR: 7
File: tests/plugin.test.ts:24-32
Timestamp: 2026-05-12T20:04:37.791Z
Learning: For Vite plugins, the `configResolved` hook must return `void` or `Promise<void>` (per Vite’s `ObjectHook` type). Do not return boolean values from `configResolved`. If you need boolean control flow, use the appropriate hook types (e.g., other hooks like `config`/`configureServer`/`transformIndexHtml` may support different return shapes) rather than returning a boolean from `configResolved`.
Applied to files:
tests/raster.test.tssrc/data-uri.tstests/cli.test.tstests/data-uri.test.tssrc/raster.ts
🪛 LanguageTool
README.md
[uncategorized] ~277-~277: Loose punctuation mark.
Context: ... | | inject | boolean \| 'embed' \| { sizes?, embed? } | false | `tru...
(UNLIKELY_OPENING_PUNCTUATION)
🔍 Remote MCP GitHub Grep
Relevant external code patterns found:
- SVG data URIs are commonly generated with
data:image/svg+xml,...plusencodeURIComponent(svg); I found this inadobe/react-spectrum,mapbox/geojson.io,graphif/project-graph, and others. Some code also uses thecharset=utf-8variant. - Base64 SVG embedding is also used in public code, e.g.
data:image/svg+xml;base64,viabtoa(unescape(encodeURIComponent(svg)))inanyproto/anytype-tsand related repos. - Cache-busting with
url.searchParams.set('v', ...)is a common pattern in public code, includingtravis-web,directus,nextcloud, andGhost. - Favicon handling commonly targets
rel="shortcut icon"/rel="icon"separately fromapple-touch-icon; examples includeroundcubemail,firefox-ios,senna.js, andmarkuplint. Some code explicitly replaces the whole<link rel="shortcut icon">element rather than just mutatinghref.
The WHATWG URL parser strips raw \t/\n/\r from any URL (HTML normalizes CR/CRLF to LF before that), so an unencoded line ending silently vanished from the decoded SVG — breaking the byte-for-byte round-trip for multi-line or CRLF sources. CodeRabbit flagged \r; \n and \t fail identically. Encode all three, and add a regression that decodes through `new URL()` (not just `decodeURIComponent`, which never exercised the stripping).
What & why
Restructures
src/*.ts(library internals; CLI rewired only where shared code moved) from a 603-line closure god-file + dual tag models + fake-constraint types into focused, pipeline-stage modules. Ships as 4.0.0 with the v2emitshim removed.The three problems this fixes:
index.tswas a 603-line closure-as-object — 9 mutable vars + ~12 inner functions, none testable in isolation. Now 298 lines of Vite glue over an extracted, testableAssetProducer.faviconTags()vshtml.ts'sbuildFaviconTags(), withwithBaseduplicated). Now onebuildFaviconTags(injections, ctx)serves both plugin and CLI.IconSizewas a fake constraint ((number & {})accepts anything). Now oneparseConfig()boundary;IconSizeis a branded type minted only byparseSize().New shape (by pipeline stage)
config.ts·size.ts·load-input.tsresolve-specs.tsraster.ts(sharp) ·ico.ts(packing) ·assets.ts(AssetProducer) ·data-uri.tsfavicon-tags.ts(one builder) ·inject-html.ts·dev-client.tsindex.ts(thin) ·types.tsDeleted:
normalize-emit.ts,html.ts. Net −228 lines in the library.BREAKING (v4)
emitaccepts onlyEmitSpec[]— the v2{ source, sizes, inject }object shape is removed.LegacyEmitOptions,EmitOptions,isLegacyEmit,NormalizedEmit,IncludeSourceOptions,EmitSizesFormat,EMIT_SIZES_FORMATS,InjectMode,INJECT_MODES.svg-to-ico inject --mode/-mflag (never affected output).Migration table in
CHANGELOG.md+README.md.Verification
tsdownbuild passesattw(types) +publint(packaging)Each commit is a self-contained, green phase — reviewable in order.
The
v4.0.0signed tag is intentionally not in this branch (per repo convention, tag the work commit after merge).