fix(bin): Don't crash only-include-used-icons when public/dsfr exists without an index.html - #506
Conversation
`assert(htmlFilePath !== undefined)` threw a message-less AssertionError as soon as public/dsfr existed while neither <projectDir>/index.html nor <publicDir>/index.html did. A monorepo invoked with --projectDir reproduces it, as does a project that lost its index.html. Nothing else in the script needs the html file, so make `spaParams.htmlFilePath` optional and skip only the cache busting query parameter when it is missing. Move that rewrite outside of the `hasChanged` early return while at it. It is idempotent, and inside the guard a stale or hand reverted hash could never be repaired as long as icons.min.css itself did not change.
There was a problem hiding this comment.
Copilot wasn't able to review any files in this pull request.
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
The section banners do exist in dsfr.main.css, my test docblock claimed otherwise. They are however a defective index, which is the real reason to prefer the source map: 44 banners for 45 components, `badge`, `consent`, `notice` and `radio` have none, and `notice` is labelled ALERT so the list contains `alert` twice. The map only misses `radio`. Docblock corrected to say that instead. Fail the cascade order test when dsfr.main.css.map is missing, instead of warning and reporting success. @gouvfr/dsfr is a direct dependency of this repo so there is no legitimate skip case, and a green skip would let the guard silently evaporate on a future DSFR that stops shipping source maps. Also apply here the fix sent for only-include-used-icons in codegouvfr#506, since this file does not exist on main yet: `spaParams.htmlFilePath` becomes optional so a public/dsfr without a findable index.html no longer throws a message-less AssertionError, and the cache busting rewrite moves out of the `hasChanged` early return so a stale or hand reverted hash is repairable.
| spaParams: | ||
| | { | ||
| dsfrDirPath_static: string; | ||
| htmlFilePath: string; |
There was a problem hiding this comment.
This is wrong, and it is my fault for starting it. Next.js has no public/dsfr in the documented setup: copy-dsfr-to-public.ts:7 says "not in Next.js for example", and this repo's own integration apps confirm it, next-appdir and next-pagesdir run only-include-used-icons alone while vite and cra run copy-dsfr-to-public && only-include-used-icons. A canonical Next.js fixture exits 0 on main too, so there is no crash there. My original repro created public/dsfr by hand, which is not the Next.js path, and you took my word for it in #505. Your PR body is accurate as written (monorepo, ex-SPA), it is just this comment and the "unusable in Next.js today" line. Accurate framing: any project with a public/dsfr and no index.html, which includes a Next.js app that opted into copy-static-assets.
There was a problem hiding this comment.
You're right, and it's worth spelling out the evidence since it also settles part of my #505 write-up: test/integration/next-appdir/package.json and next-pagesdir/package.json run only-include-used-icons alone, while vite and cra run copy-dsfr-to-public first. So in the documented Next.js setup there is no public/dsfr, dsfrDirPath_static is undefined, spaParams is undefined, and the assertion is never reached. No crash there, as you say.
One correction to the framing you propose, though: it does not include a Next.js app that opted into copy-static-assets. That command asserts "Can't locate your index.html file." at copy-dsfr-to-public.ts:60, before the mkdirSync(dsfrDirPath) at :95, so a project with no index.html never gets a public/dsfr out of it either. Confirmed on a fixture: it exits non-zero and public/ stays empty. That is the same point I made in #505 point 6 and then contradicted in this comment.
Reworded in fa0dbe1, then corrected in f0297d7 once I checked that copy-static-assets claim, down to the states that actually reach this branch:
// Undefined whenever public/dsfr exists but no index.html was found to add
// a cache busting query parameter to: a monorepo invoked with --projectDir
// where the html is not where either branch looks, a project that used to be
// a Vite/CRA app and lost its index.html, or a public/dsfr that was committed
// or restored by other means. Not reachable through copy-static-assets: it
// asserts "Can't locate your index.html file." before creating anything.
The same comment had been copied into #505; corrected there too (a84376f).
| @@ -584,33 +618,6 @@ export async function main(args: string[]) { | |||
| .map(([srcFilePath, destFilePath]) => cp(srcFilePath, destFilePath)) | |||
There was a problem hiding this comment.
This copy and its sibling generateUsedRemixiconFiles stayed inside the hasChanged guard while the html rewrite moved out, and your own NOTE argues for both. copy-dsfr-to-public does not copy public/dsfr/icons/arrows/arrow-down-circle-fill.svg, only this script does. Delete it, leave the CSS alone, re-run: No change since last run, and the icon stays missing until something else changes the stylesheet. Same shape as the asset bug you fixed in #505.
Unrelated nit two functions up, since it is not in the diff and I cannot anchor it: fs.mkdirSync(remixiconDirPath) has no { recursive: true }. It only fires on a public/dsfr that exists without public/dsfr/icons, which is the kind of odd state this PR is about tolerating.
There was a problem hiding this comment.
Both confirmed, fixed in fa0dbe1.
The guard. My own NOTE argued for it and I moved only half of it. generateUsedRemixiconFiles and copyUsedDsfrIconsToStatic now sit in the same Promise.all as the html rewrite, ahead of the guard. Only clearCache stays behind it, which is the one thing that genuinely depends on the stylesheet having changed.
The nit. fs.mkdirSync(remixiconDirPath, { "recursive": true }), and the existsSync above it is gone since it no longer guards anything.
What your repro turned up. It took several attempts to reproduce, because it only fails about half the time, and the reason is a second bug. setUsedIconClassNames is filled from a Promise.all over the source files, so its insertion order follows I/O completion order and changes between runs. The rules of icons.min.css were emitted in that order, so the file differed byte-for-byte from one run to the next for an unchanged icon set, and hasChanged flipped at random: needless CSS rewrites, needless ?hash= churn, node_modules/.next/cache wiped for nothing, and No change since last run reported only by luck. It was also masking the bug you describe half the time. Sorted in be8e28f — rule order carries no meaning here, every rule targets a distinct .fr-icon-*::before or .ri-*::before selector. Happy to split that commit into its own PR if you would rather keep this one to the review points.
Verified on a fixture (public/dsfr populated, no index.html), deleting public/dsfr/icons/arrows/arrow-down-circle-fill.svg and public/dsfr/icons/remixicon/ before each run:
| before | after | |
|---|---|---|
| deleted assets repaired | 4 failures / 8 runs | 0 / 16 |
public/dsfr/icons absent → mkdirSync |
ENOENT | recreated |
No change since last run over 6 consecutive runs |
random | 6 / 6 |
- Correct the `htmlFilePath` comment. Next.js has no `public/dsfr` in the
documented setup, so it never reaches the assertion that used to fire here.
The real trigger is any project with a `public/dsfr` and no `index.html`.
- Move `generateUsedRemixiconFiles` and `copyUsedDsfrIconsToStatic` out of the
`hasChanged` guard alongside the html rewrite. `copy-dsfr-to-public` builds
its keep list from the `url()` of `dsfr.min.css`, so these icons are written
by this script alone. A deleted svg was never repaired as long as
`icons.min.css` itself did not change.
- `fs.mkdirSync(remixiconDirPath, { recursive: true })`, so a `public/dsfr`
without an `icons` directory no longer throws ENOENT.
Only `clearCache` stays behind the guard, which is the one thing that really
depends on the stylesheet having changed.
…able `setUsedIconClassNames` is filled from a `Promise.all` over the source files, so its insertion order follows I/O completion order and changes between runs. The rules of `icons.min.css` were emitted in that order, so the file differed from one run to the next for an unchanged set of icons. `hasChanged` compares the generated buffer against the file on disk, so it flipped at random: `icons.min.css` and the `?hash=` query parameter were rewritten and `node_modules/.next/cache` was wiped on runs where nothing had actually changed, and "No change since last run" was reported only by luck. Rule order carries no meaning here, every rule targets a distinct `.fr-icon-*::before` or `.ri-*::before` selector, so sorting is safe. Found while verifying the `hasChanged` guard for the review of codegouvfr#506: the nondeterminism was masking the missing asset bug half the time.
lsagetlethias
left a comment
lsagetlethias
left a comment
There was a problem hiding this comment.
LGTM. Verified by running it: deleting public/dsfr/icons/arrows/arrow-down-circle-fill.svg and re-running without touching the CSS now prints No change since last run and restores the svg. mkdirSync recursive, only clearCache left behind the guard, 75 tests + tsc + eslint clean.
You are right about copy-static-assets and I was wrong: copy-dsfr-to-public.ts:60 asserts before the mkdirSync at :95, so it can never leave a public/dsfr without an index.html. Thanks for checking rather than taking my word for it twice.
Your sort commit is undersold. I failed to reproduce the nondeterminism twice, both times on my own broken fixtures. With a correct one (12 files, one distinct icon each, increasing sizes), six runs on the pre-sort code give five different rule orders and No change since last run never fires, six out of six. So on main this script rewrites the CSS, changes the ?hash= and wipes node_modules/.next/cache on every prebuild for any project with icons spread across several files. That is a build time regression worth naming in the commit message, not just a determinism cleanup.
Nit, non blocking: the monorepo bullet in the new comment is not an independent trigger, both scripts resolve index.html through the same two paths, so it collapses into your "committed or restored by other means" case.
|
Sorry my MacBook is being serviced, I'll review this once I get it back. |
`setUsedIconClassNames` is filled from a `Promise.all` over the source files, so its insertion order follows I/O completion order and changes between runs. The rules of `icons.min.css` were emitted in that order, so the generated stylesheet differed from one run to the next for an unchanged set of icons. `hasChanged` compares that buffer against the file on disk, so it was true almost every time. For any project whose icons are spread across several source files, `icons.min.css` is rewritten, the `?hash=` query parameter in index.html is changed, and `node_modules/.next/cache`, `.vite`, the storybook and the babel-loader caches are wiped on every prebuild. "No change since last run" was reported only by luck. Measured on a 40 file fixture: 15 runs, 15 different outputs, and the message never fired once. That makes this a build time regression, not just a determinism cleanup. Sorting fixes it, and is safe: rule order carries no meaning here, every rule targets a distinct `.fr-icon-*::before` or `.ri-*::before` selector. Found while verifying the `hasChanged` guard for the review of codegouvfr#506, where the nondeterminism was masking the missing asset bug half the time.
It cannot produce this state: copy-dsfr-to-public.ts:60 asserts "Can't locate your index.html file." before the mkdirSync at :95, so a project with no index.html never gets a public/dsfr out of it, Next.js included.
It is not an independent trigger: both this script and copy-dsfr-to-public resolve index.html through the same two paths, <projectDir>/index.html and <publicDir>/index.html. If copy-static-assets was able to create public/dsfr, an index.html was found at one of them and this script finds it too. The case collapses into "the index.html was removed afterwards".
f0297d7 to
38dc8c4
Compare
|
Thanks @lsagetlethias — both notes taken, force-pushed. The sort commit is reworded ( The monorepo case is dropped from the Follow-up, unrelated to the review: the npm publish job has been failing since npm 12 (node 20 + |
Don't crash
only-include-used-iconswhenpublic/dsfrexists without anindex.htmlSpun off from the review of #505 (points 6a and 6b of @lsagetlethias), since the fix belongs to
only-include-used-iconsand is unrelated to that PR.Problem
getCommandContextbuildsspaParamsas soon aspublic/dsfrexists, and asserts anindex.htmlalongside it:https://github.com/codegouvfr/react-dsfr/blob/main/src/bin/only-include-used-icons.ts#L404
The assertion has no message, so when it fires the user gets a bare
AssertionError: Wrong assertion encounteredand no indication of what to do.It fires whenever
public/dsfrexists while neither<projectDir>/index.html(Vite) nor<publicDir>/index.html(CRA) does. Two setups I could reproduce:only-include-used-icons --projectDir apps/webfrom the workspace root, where the html file is not where either branch looks;index.html.Nothing else in the script needs the html file — it is only used to append a
?hash=cache busting query parameter — so the run has no reason to abort.Changes
spaParams.htmlFilePathbecomesstring | undefined, andaddHashQueryParameterInIndexHtmlreturns early when there is nothing to rewrite. The icons are still trimmed and still copied topublic/dsfr.hasChangedearly return. It is idempotent, and inside the guard a stale or hand-reverted hash could never be repaired as long asicons.min.cssitself did not change. It now also skips the write when the html is unchanged, so no needless mtime bump.Verification
Monorepo repro above,
public/dsfr/utility/iconspopulated, noindex.html:AssertionError: Wrong assertion encountered, nothing written;public/dsfr/utility/icons/icons.min.csstrimmed to the 6 used icons, second run printsNo change since last run.Full suite passes (75 tests),
yarn build, eslint and prettier clean.