test(react-icons-font-subsetting-webpack-plugin): cover async chunks - #1232
Draft
Martin Hochel (Hotell) wants to merge 2 commits into
Draft
test(react-icons-font-subsetting-webpack-plugin): cover async chunks#1232Martin Hochel (Hotell) wants to merge 2 commits into
Martin Hochel (Hotell) wants to merge 2 commits into
Conversation
Every fixture kept its icons in the entry chunk, so nothing verified that a font reachable only through `import()` is still found and subset. The size ceiling cannot police this alone: dropping the async chunk's glyph makes the font smaller, so it slips under any threshold. The two families are therefore also asserted to retain a glyph beyond .notdef, read from the maxp table.
📋 PR Validation SummaryCheck the Build react library job summary for detailed reports:
|
…lit across chunks The lazyAtoms fixture puts its eager and async icons in different font families, so nothing yet proved that a single emitted font can carry glyphs contributed by two different chunks. Subsetting is per family across the whole build, not per chunk, so the eager half must not subset the async half's glyph away. Glyph assertions become counts rather than a mere presence check, since 'at least one glyph' cannot tell two icons from one.
There was a problem hiding this comment.
🟡 Changes recommended
The new glyph-count assertion reads .ttf bytes from disk via readFileSync, which will fail under webpack dev-server’s in-memory output filesystem.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Adds new integration fixtures to the react-icons-font-subsetting-webpack-plugin test suite to guard against regressions where icons are only reachable via dynamic import() (async chunks). This strengthens correctness verification by asserting emitted font glyph counts (via the TTF maxp table) rather than relying solely on size ceilings.
Changes:
- Add two new fixtures (
lazyAtoms,lazySharedFontFamily) to exercise async chunk usage and cross-chunk glyph unioning within a single font family. - Extend the shared test assertion plugin to optionally validate emitted
.ttfglyph counts per font family (in addition to size thresholds).
File summaries
| File | Description |
|---|---|
| packages/react-icons-font-subsetting-webpack-plugin/test/src/lazy-shared-family.js | New eager-entry fixture for “shared font family across eager + async chunks” scenario. |
| packages/react-icons-font-subsetting-webpack-plugin/test/src/lazy-shared-family.async.js | Async half of the shared-family fixture, reachable only via import(). |
| packages/react-icons-font-subsetting-webpack-plugin/test/src/lazy-atoms.js | New eager-entry fixture for “async chunk with different font family” scenario. |
| packages/react-icons-font-subsetting-webpack-plugin/test/src/lazy-atoms.async.js | Async half of the lazy-atoms fixture, reachable only via import(). |
| packages/react-icons-font-subsetting-webpack-plugin/test/make-configs.js | Registers new fixtures and adds optional glyph-count assertions by parsing emitted .ttf files. |
Review details
- Files reviewed: 5/5 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+190
to
+191
| // `afterEmit` downgrades sources to size-only, so the bytes come back off disk. | ||
| const glyphCount = readGlyphCount(readFileSync(join(compiler.outputPath, asset.name))); |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Adds a
lazyAtomsfixture covering icons reachable only throughimport(). Every existing fixture keeps its icons in the entry chunk, so nothing verified that async chunks are handled at all.The behaviour turned out to already be correct — I probed it before writing this — so this is a regression guard, not a fix.
Why the existing threshold is not enough
The suite asserts only a size ceiling. That catches "subsetting never ran" (full fonts are ~2.8 MB), but it is blind to the more dangerous failure: subsetting that runs and drops a glyph it should have kept. That font is smaller, so it sails under any ceiling.
That is not hypothetical — it is exactly the shape of the bug fixed in #1230, where one module's icons subset a shared font down to their own glyphs.
So the fixture also asserts that each expected family retains a glyph beyond
.notdef, read from themaxptable of the emitted.ttf. Byte counts are a proxy; the glyph count says outright whether the icon survived.The fixture
Two icons, deliberately in different font families so they can be told apart:
GamesFilled— eager, lands inFluentSystemIcons-ResizableXboxConsole24Filled— behindimport(), lands inFluentSystemIcons-FilledMeasured, identical on both bundlers (~656–664 B is an empty subset):
FluentSystemIcons-Resizable.ttfFluentSystemIcons-Filled.ttfFluentSystemIcons-Regular.ttfFluentSystemIcons-Light.ttfSo the async-only font is both found (not left at 2.8 MB) and unioned correctly with the eager one.
The assertion bites
Pointed at a family with no icons, it fails as intended:
Also verified while probing (not committed)
Two entrypoints in a single compilation, which is what actually exercises the multi-runtime array path — every fixture here compiles one entry per compiler, so
runtimehas always been a single-element array. rspack reportedalpha[alpha] <async>[beta] beta[beta]and both glyph sets survived, confirminggetUsedExports(m, ['alpha','beta'])unions across runtimes.Not turned into a fixture, since it duplicates this one's coverage at roughly double the build cost. Happy to add it if reviewers would rather have it pinned.
Note
Touches the same
entriesobject as #1230, so whichever merges second will need a trivial conflict resolution.Second fixture: one family split across chunks
lazyAtomsdeliberately puts its two icons in different font families, so it never proved that a single emitted font can carry glyphs contributed by two different chunks — which is the case most likely to regress, since subsetting is per family across the whole build rather than per chunk.lazySharedFontFamilycovers it:Games24Filledeager andXboxConsole24Filledbehindimport(), both sized+Filled, so both land in one file.Measured, identical on webpack and rspack:
FluentSystemIcons-Filled.ttf.notdef+ 1).notdef+ 2)Confirmed exact by over-asserting: demanding 4 fails with "has 3 glyphs, expected at least 4". So the async icon's glyph really is in the same file as the eager one, and dropping either would fail the assertion.
Glyph assertions are therefore counts (
fontGlyphCounts) rather than a presence check — "has at least one glyph" cannot tell two icons from one.How font assets are actually loaded
Worth recording, since it is easy to assume otherwise. The font file is emitted once per family (
ttfCopies=1in every variant), never duplicated per chunk, and its glyph set is the union across the whole build. There is no per-chunk font.When it is fetched depends on where the
@font-facemodule lands:So a build that splits one family across eager and lazy code downloads the lazy glyphs up front — unavoidable with a single shared font file. A family used exclusively behind
import()is genuinely deferred.