Skip to content

fix(bin): Don't crash only-include-used-icons when public/dsfr exists without an index.html #506

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
kevbarns merged 5 commits into from
Aug 19, 2026
Merged

fix(bin): Don't crash only-include-used-icons when public/dsfr exists without an index.html #506

Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
c279bd8
Don't crash when public/dsfr exists without an index.html
kevbarns Aug 19, 2026
fa0dbe1
Address review of #506
kevbarns Aug 19, 2026
7ea46f8
Stop wiping the build cache on every only-include-used-icons run
kevbarns Aug 19, 2026
aa428f3
Drop copy-static-assets from the htmlFilePath comment
kevbarns Aug 19, 2026
38dc8c4
Drop the monorepo case from the htmlFilePath comment
kevbarns Aug 19, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
132 changes: 74 additions & 58 deletions src/bin/only-include-used-icons.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -116,7 +116,13 @@ type CommandContext = {
spaParams:
| {
dsfrDirPath_static: string;
htmlFilePath: string;

@lsagetlethias lsagetlethias Aug 19, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@kevbarns kevbarns Aug 19, 2026

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

// Undefined whenever public/dsfr exists but no index.html was found to add
// a cache busting query parameter to: 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, which
// resolves index.html through the same two paths as this script and asserts
// "Can't locate your index.html file." before creating anything.
htmlFilePath: string | undefined;
}
| undefined;
isSilent: boolean;
Expand Down Expand Up @@ -401,8 +407,6 @@ async function getCommandContext(args: string[]): Promise<CommandContext> {
return undefined;
}

assert(htmlFilePath !== undefined);

return {
dsfrDirPath_static,
htmlFilePath
Expand Down Expand Up @@ -475,7 +479,12 @@ export async function main(args: string[]) {
})
);

return { "usedIconClassNames": Array.from(setUsedIconClassNames) };
// NOTE: The set is filled from a Promise.all over the source files, so its
// insertion order follows I/O completion order and varies between runs. Sorting
// makes the generated stylesheet byte stable, which is what the `hasChanged`
// comparison below relies on. Rule order carries no meaning here: every rule
// targets a distinct `.fr-icon-*::before` / `.ri-*::before` selector.
return { "usedIconClassNames": Array.from(setUsedIconClassNames).sort() };
})();

if (usedIconClassNames.length > 300) {
Expand Down Expand Up @@ -529,22 +538,50 @@ export async function main(args: string[]) {
})
);

if (!hasChanged) {
log?.("No change since last run");
return;
}

// NOTE: Deliberately outside of the `hasChanged` guard below. These three writes are
// idempotent, and inside the guard a stale or hand reverted output could never be
// repaired as long as icons.min.css itself did not change. Nothing else writes them:
// copy-dsfr-to-public builds its keep list from the url() of dsfr.min.css, so the
// icons and the hash query parameter are this script's responsibility alone.
await Promise.all([
(async function addHashQueryParameterInIndexHtml() {
const htmlFilePath = commandContext.spaParams?.htmlFilePath;

if (htmlFilePath === undefined) {
return;
}

const html = (await readFile(htmlFilePath)).toString("utf8");

const { modifiedHtml } = modifyHtmlHrefs({
"html": html,
"getModifiedHref": href => {
if (!href.includes(iconsMinCssRelativePath.replace(/\\/g, "/"))) {
return href;
}

const [urlWithoutQuery] = href.split("?");

return `${urlWithoutQuery}?hash=${fnv1aHashToHex(
rawIconCssCodeBuffer.toString("utf8")
)}`;
}
});

if (modifiedHtml === html) {
return;
}

await writeFile(htmlFilePath, Buffer.from(modifiedHtml, "utf8"));
})(),
(async function generateUsedRemixiconFiles() {
await Promise.all(
[commandContext.dsfrDirPath, commandContext.spaParams?.dsfrDirPath_static]
.filter(exclude(undefined))
.map(async dsfrDistDirPath => {
const remixiconDirPath = pathJoin(dsfrDistDirPath, "icons", "remixicon");

if (!fs.existsSync(remixiconDirPath)) {
fs.mkdirSync(remixiconDirPath);
}
fs.mkdirSync(remixiconDirPath, { "recursive": true });

await Promise.all(
usedIcons
Expand Down Expand Up @@ -583,56 +620,35 @@ export async function main(args: string[]) {
)
.map(([srcFilePath, destFilePath]) => cp(srcFilePath, destFilePath))

@lsagetlethias lsagetlethias Aug 19, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@kevbarns kevbarns Aug 19, 2026

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

);
})(),
(async function addHashQueryParameterInIndexHtml() {
if (commandContext.spaParams === undefined) {
return;
}
})()
]);

const html = (await readFile(commandContext.spaParams.htmlFilePath)).toString("utf8");
if (!hasChanged) {
log?.("No change since last run");
return;
}

const { modifiedHtml } = modifyHtmlHrefs({
"html": html,
"getModifiedHref": href => {
if (!href.includes(iconsMinCssRelativePath.replace(/\\/g, "/"))) {
return href;
await (async function clearCache() {
await Promise.all(
[
pathJoin(".next", "cache"),
pathJoin(".vite"),
pathJoin(".cache", "storybook"),
pathJoin(".cache", "babel-loader"),
pathJoin(".cache", "default-development")
]
.map(relativeDirPath =>
pathJoin(commandContext.projectDirPath, "node_modules", relativeDirPath)
)
.map(async dirPath => {
if (!(await existsAsync(dirPath))) {
return;
}

const [urlWithoutQuery] = href.split("?");

return `${urlWithoutQuery}?hash=${fnv1aHashToHex(
rawIconCssCodeBuffer.toString("utf8")
)}`;
}
});

await writeFile(
commandContext.spaParams.htmlFilePath,
Buffer.from(modifiedHtml, "utf8")
);
})(),
(async function clearCache() {
await Promise.all(
[
pathJoin(".next", "cache"),
pathJoin(".vite"),
pathJoin(".cache", "storybook"),
pathJoin(".cache", "babel-loader"),
pathJoin(".cache", "default-development")
]
.map(relativeDirPath =>
pathJoin(commandContext.projectDirPath, "node_modules", relativeDirPath)
)
.map(async dirPath => {
if (!(await existsAsync(dirPath))) {
return;
}

await rm(dirPath, { "recursive": true, "force": true });
})
);
})()
]);
await rm(dirPath, { "recursive": true, "force": true });
})
);
})();
}

if (require.main === module) {
Expand Down