Skip to content

Commit 190ffc7

Browse files
authored
Merge pull request #515 from codegouvfr/fix/content-detection-over-inclusion
fix(only-include-used-components): guard both maps against silent drift
2 parents 621ee4a + 588780d commit 190ffc7

3 files changed

Lines changed: 332 additions & 2 deletions

File tree

src/bin/only-include-used-components.ts

Lines changed: 23 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -189,11 +189,29 @@ export const REACT_DSFR_MODULE_TO_DSFR_COMPONENTS: Record<string, DsfrComponentN
189189
"shared": ["form", "radio", "checkbox"]
190190
};
191191

192+
/**
193+
* The stylesheet a DSFR component ships under `component/<name>/<name>.<ext>`, by order of
194+
* preference. Not every component has every variant: `download` for instance only ships
195+
* `download.css` and `download.min.css`, no `.main.` ones.
196+
*/
197+
export const DSFR_COMPONENT_CSS_FILE_EXTENSIONS = [
198+
"main.min.css",
199+
"min.css",
200+
"main.css",
201+
"css"
202+
] as const;
203+
192204
/**
193205
* CSS class name prefixes that reveal a direct usage of a DSFR component in the
194206
* sources (when raw fr-* classes are used without importing the React component).
195207
* Substring matching is intentional and fail-safe: matching too much only means
196208
* including a component's CSS that may not be needed.
209+
*
210+
* A prefix only belongs here if it opens a selector in that component's own stylesheet.
211+
* A class that the component merely *styles as a descendant* is not a usage signal: its
212+
* base rules live elsewhere (usually in the always included core), so detecting on it
213+
* pulls the whole component in for nothing. dsfrComponentDetectionClassPrefixes.test.ts
214+
* re-derives this rule against the installed @gouvfr/dsfr.
197215
*/
198216
export const DSFR_COMPONENT_DETECTION_CLASS_PREFIXES: Record<DsfrComponentName, string[]> = {
199217
"accordion": ["fr-accordion"],
@@ -206,7 +224,10 @@ export const DSFR_COMPONENT_DETECTION_CLASS_PREFIXES: Record<DsfrComponentName,
206224
"checkbox": ["fr-checkbox"],
207225
"connect": ["fr-connect"],
208226
"consent": ["fr-consent"],
209-
"content": ["fr-content-media", "fr-responsive-img", "fr-responsive-vid"],
227+
// NOTE: deliberately not fr-responsive-img / fr-responsive-vid. Their base rules
228+
// live in the always included core, and every rule content.css has for them is
229+
// scoped under .fr-content-media, which is already the prefix detected here.
230+
"content": ["fr-content-media"],
210231
"download": ["fr-download"],
211232
"follow": ["fr-follow"],
212233
"footer": ["fr-footer"],
@@ -878,7 +899,7 @@ export async function main(args: string[]) {
878899
.filter(dirent => dirent.isDirectory())
879900
.map(dirent => dirent.name)
880901
.filter(componentName =>
881-
["main.min.css", "min.css", "main.css", "css"].some(ext =>
902+
DSFR_COMPONENT_CSS_FILE_EXTENSIONS.some(ext =>
882903
fs.existsSync(
883904
pathJoin(
884905
commandContext.dsfrDirPath,
Lines changed: 118 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,118 @@
1+
import { it, expect, describe } from "vitest";
2+
import * as fs from "fs";
3+
import { join as pathJoin } from "path";
4+
import {
5+
DSFR_COMPONENT_DETECTION_CLASS_PREFIXES,
6+
DSFR_COMPONENT_CSS_FILE_EXTENSIONS
7+
} from "../../../../src/bin/only-include-used-components";
8+
9+
/**
10+
* DSFR_COMPONENT_DETECTION_CLASS_PREFIXES is hand written, and a wrong entry is invisible:
11+
* it does not break a build, it just quietly includes a component's CSS in every project
12+
* that happens to use the class. This test re-derives it from the installed @gouvfr/dsfr.
13+
*
14+
* The rule: a prefix earns its place only if it *opens a selector* in that component's own
15+
* stylesheet. Merely appearing there is not enough, because a component also styles classes
16+
* it does not own: content.css has rules for fr-responsive-img, but all of them are scoped
17+
* under `.fr-content-media`, and the base `.fr-responsive-img` rule lives in core, which is
18+
* always included. Detecting `content` on fr-responsive-img therefore pulled in ~2.8 KB of
19+
* minified CSS for nothing, in every project using that core utility class.
20+
*
21+
* This is intentionally the one direction that is safe to assert. The opposite check, that
22+
* every DSFR class maps back to a component, would be a fail-safe over-inclusion, and the
23+
* union of the prefixes is deliberately not exhaustive over the DSFR class vocabulary.
24+
*/
25+
describe("DSFR_COMPONENT_DETECTION_CLASS_PREFIXES", () => {
26+
const componentsDirPath = pathJoin(
27+
process.cwd(),
28+
"node_modules",
29+
"@gouvfr",
30+
"dsfr",
31+
"dist",
32+
"component"
33+
);
34+
35+
/** Same extension list, in the same order, as the availableDsfrComponents filter. */
36+
const getComponentCssFilePath = (componentName: string): string | undefined =>
37+
DSFR_COMPONENT_CSS_FILE_EXTENSIONS.map(ext =>
38+
pathJoin(componentsDirPath, componentName, `${componentName}.${ext}`)
39+
).find(filePath => fs.existsSync(filePath));
40+
41+
/**
42+
* True when `.<classPrefix>` starts a compound selector, i.e. at the very beginning of
43+
* the stylesheet or right after a `,` `{` `}`. Anchoring this way is what separates
44+
* `.fr-download {` from `.fr-content-media [class^=fr-responsive-img]`, where the class
45+
* only ever appears as a descendant of another component's class.
46+
*/
47+
const doesClassPrefixOpenASelector = (params: {
48+
rawCssCode: string;
49+
classPrefix: string;
50+
}): boolean => {
51+
const { rawCssCode, classPrefix } = params;
52+
53+
return new RegExp(
54+
`(?:^|[,{}]\\s*)\\.${classPrefix.replace(/[.*+?^${}()|[\]\\]/g, "\\$&")}`,
55+
"m"
56+
).test(rawCssCode);
57+
};
58+
59+
it("only detects on classes each component actually owns", () => {
60+
// Deliberately an assertion and not a skip: @gouvfr/dsfr is a direct dependency, so
61+
// there is no legitimate case where the stylesheets are absent. Skipping would let
62+
// this guard silently evaporate on a future layout change upstream.
63+
expect(fs.existsSync(componentsDirPath), `${componentsDirPath} not found`).toBe(true);
64+
65+
const problems: string[] = [];
66+
67+
for (const [componentName, classPrefixes] of Object.entries(
68+
DSFR_COMPONENT_DETECTION_CLASS_PREFIXES
69+
)) {
70+
const cssFilePath = getComponentCssFilePath(componentName);
71+
72+
if (cssFilePath === undefined) {
73+
problems.push(`${componentName}: no stylesheet found in ${componentsDirPath}`);
74+
continue;
75+
}
76+
77+
const rawCssCode = fs.readFileSync(cssFilePath).toString("utf8");
78+
79+
for (const classPrefix of classPrefixes) {
80+
if (doesClassPrefixOpenASelector({ rawCssCode, classPrefix })) {
81+
continue;
82+
}
83+
84+
problems.push(
85+
`${componentName}: "${classPrefix}" never opens a selector in ${componentName}.css`
86+
);
87+
}
88+
}
89+
90+
expect(
91+
problems,
92+
[
93+
`These detection prefixes do not identify the component they are mapped to,`,
94+
`so matching one pulls in that component's CSS for nothing:`,
95+
...problems.map(entry => ` - ${entry}`),
96+
``,
97+
`Either the prefix belongs to another component (or to the always included`,
98+
`core), or it is only styled as a descendant. Drop it from`,
99+
`DSFR_COMPONENT_DETECTION_CLASS_PREFIXES in`,
100+
`src/bin/only-include-used-components.ts.`
101+
].join("\n")
102+
).toStrictEqual([]);
103+
});
104+
105+
it("checks a meaningful number of prefixes", () => {
106+
// Guards the loop above: a wrong componentsDirPath, or an upstream rename, would
107+
// otherwise leave it iterating over stylesheets it never actually reads.
108+
const classPrefixes = Object.values(DSFR_COMPONENT_DETECTION_CLASS_PREFIXES).flat();
109+
110+
expect(classPrefixes.length).toBeGreaterThan(40);
111+
112+
expect(
113+
Object.keys(DSFR_COMPONENT_DETECTION_CLASS_PREFIXES).filter(
114+
componentName => getComponentCssFilePath(componentName) === undefined
115+
)
116+
).toStrictEqual([]);
117+
});
118+
});
Lines changed: 191 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,191 @@
1+
import { it, expect, describe } from "vitest";
2+
import * as fs from "fs";
3+
import { join as pathJoin } from "path";
4+
import {
5+
getReactDsfrImportedModuleIds,
6+
resolveModuleIdToDsfrComponents
7+
} from "../../../../src/bin/only-include-used-components";
8+
9+
/**
10+
* `only-include-used-components` maps every imported react-dsfr module to the DSFR CSS
11+
* components it renders. A module that is in neither REACT_DSFR_MODULE_TO_DSFR_COMPONENTS
12+
* nor NON_COMPONENT_MODULE_IDS makes resolveModuleIdToDsfrComponents() return undefined,
13+
* which trips the include-everything fail-safe: the script warns and exits 0, so a
14+
* consumer importing a component added after the map was written silently stops getting
15+
* any CSS trimmed at all. Nothing else in the test suite notices.
16+
*
17+
* This test is the guard: adding a component to `src/` without adding it to one of the
18+
* two maps fails here, naming the module.
19+
*
20+
* On what counts as a public module: package.json has NO `exports` field (checked below),
21+
* so the published subpaths are not an explicit list to compare against. They are exactly
22+
* whatever `tsc -p src` emits: the publish job runs denoify's `enable_short_npm_import_path`
23+
* (.github/workflows/ci.yaml), which moves the content of the tsconfig `outDir` up one level
24+
* onto the package root, so `@codegouvfr/react-dsfr/<subpath>` resolves straight into it.
25+
* The enumeration below therefore replays that emission: the entries of `src/`, minus what
26+
* `src/tsconfig.json` excludes (read from the file, not hardcoded, so a future exclude stays
27+
* in sync) and minus the files tsc does not emit a module for.
28+
*/
29+
describe("REACT_DSFR_MODULE_TO_DSFR_COMPONENTS exhaustiveness", () => {
30+
const projectRootDirPath = process.cwd();
31+
const srcDirPath = pathJoin(projectRootDirPath, "src");
32+
33+
/**
34+
* Top level names excluded from the `tsc -p src` emission, e.g. "bin".
35+
* Only the first path segment is kept, so a pattern is still understood if it is ever
36+
* widened to a glob ("./bin" and "./bin/**" both yield "bin").
37+
*/
38+
const getSrcTsconfigExcludedNames = (): Set<string> => {
39+
const { exclude }: { exclude?: string[] } = JSON.parse(
40+
fs.readFileSync(pathJoin(srcDirPath, "tsconfig.json")).toString("utf8")
41+
);
42+
43+
return new Set((exclude ?? []).map(pattern => pattern.replace(/^\.\//, "").split("/")[0]));
44+
};
45+
46+
const isModuleSourceFileName = (fileName: string): boolean =>
47+
/\.tsx?$/.test(fileName) && !fileName.endsWith(".d.ts");
48+
49+
const removeExtension = (fileName: string): string => fileName.replace(/\.tsx?$/, "");
50+
51+
const hasAnIndex = (dirPath: string): boolean =>
52+
fs.readdirSync(dirPath).some(childName => /^index\.tsx?$/.test(childName));
53+
54+
/**
55+
* The `@codegouvfr/react-dsfr/<subpath>` a consumer can import, one per module the
56+
* script has to resolve. Not every importable path: `tools/powerhooks/useConst` is
57+
* importable too, but yields the same module id as `tools/cx`, so listing the
58+
* shallowest path per module is enough and keeps failure messages readable.
59+
*
60+
* A directory with an index is importable as is. One without (Chart, blocks, shared,
61+
* tools...) is only reachable deeper, so its children are listed instead, which is what
62+
* produces the two segment "blocks/PasswordInput". A child directory is importable in
63+
* turn only if it has an index of its own: `tools/StatefulObservable` has one,
64+
* `tools/powerhooks` does not.
65+
*/
66+
const getPublicSubpaths = (): string[] => {
67+
const excludedNames = getSrcTsconfigExcludedNames();
68+
69+
const subpaths: string[] = [];
70+
71+
for (const dirent of fs.readdirSync(srcDirPath, { "withFileTypes": true })) {
72+
if (excludedNames.has(dirent.name)) {
73+
continue;
74+
}
75+
76+
if (!dirent.isDirectory()) {
77+
if (isModuleSourceFileName(dirent.name)) {
78+
subpaths.push(removeExtension(dirent.name));
79+
}
80+
continue;
81+
}
82+
83+
const dirPath = pathJoin(srcDirPath, dirent.name);
84+
85+
if (hasAnIndex(dirPath)) {
86+
subpaths.push(dirent.name);
87+
continue;
88+
}
89+
90+
for (const childDirent of fs.readdirSync(dirPath, { "withFileTypes": true })) {
91+
// Files are kept whatever their extension: src/assets holds only .svg and
92+
// .css, and skipping those would drop the "assets" module id altogether,
93+
// silently leaving it unchecked.
94+
if (childDirent.isDirectory() && !hasAnIndex(pathJoin(dirPath, childDirent.name))) {
95+
continue;
96+
}
97+
98+
subpaths.push(`${dirent.name}/${removeExtension(childDirent.name)}`);
99+
}
100+
}
101+
102+
return subpaths;
103+
};
104+
105+
/**
106+
* Goes through the production extraction rather than reimplementing it, so the module
107+
* ids asserted here are the ones the script will actually resolve at run time.
108+
*/
109+
const getModuleIdBySubpath = (): Map<string, string[]> =>
110+
new Map(
111+
getPublicSubpaths().map(subpath => [
112+
subpath,
113+
getReactDsfrImportedModuleIds({
114+
"rawFileContent": `import "@codegouvfr/react-dsfr/${subpath}";`
115+
})
116+
])
117+
);
118+
119+
it("covers every module publicly exposed by src/", () => {
120+
// One list, one assertion: every problem found in this run is reported at once.
121+
// Asserting inside the loop, or once per category, would abort on the first
122+
// offender and hide the rest, turning a single fix-and-rerun cycle into several.
123+
const problems: string[] = [];
124+
125+
for (const [subpath, moduleIds] of getModuleIdBySubpath()) {
126+
// A public subpath must yield exactly one module id, otherwise the extraction
127+
// dropped it and its coverage would go unchecked, vacuously passing.
128+
if (moduleIds.length !== 1) {
129+
problems.push(
130+
`src/${subpath}: extraction yielded ${moduleIds.length} module ids, expected 1`
131+
);
132+
continue;
133+
}
134+
135+
const [moduleId] = moduleIds;
136+
137+
if (resolveModuleIdToDsfrComponents({ moduleId }) === undefined) {
138+
problems.push(`${moduleId} (from src/${subpath}): resolves to undefined`);
139+
}
140+
}
141+
142+
expect(
143+
problems,
144+
[
145+
`only-include-used-components would fall back to including every DSFR`,
146+
`component because of:`,
147+
...problems.map(entry => ` - ${entry}`),
148+
``,
149+
`Add each unresolved module to REACT_DSFR_MODULE_TO_DSFR_COMPONENTS (with the`,
150+
`DSFR components its markup renders, transitive dependencies included) or to`,
151+
`NON_COMPONENT_MODULE_IDS if it renders no DSFR markup, in`,
152+
`src/bin/only-include-used-components.ts.`
153+
].join("\n")
154+
).toStrictEqual([]);
155+
});
156+
157+
it("enumerates the public modules from the real src/ layout", () => {
158+
// Guards the enumeration itself: were getPublicSubpaths() to return nothing, or to
159+
// quietly stop covering a module, the assertion above would pass on what it no
160+
// longer looks at. Narrowing the enumeration has to fail here, not go unnoticed.
161+
const subpaths = getPublicSubpaths();
162+
163+
expect(subpaths.length).toBeGreaterThan(40);
164+
165+
for (const expected of ["Header", "Highlight", "blocks/PasswordInput", "i18n"]) {
166+
expect(subpaths).toContain(expected);
167+
}
168+
169+
// One representative per shape: a component directory with an index, a flat file,
170+
// index-less directories reached through a child, and a directory whose children
171+
// are not TypeScript at all (src/assets holds only .svg and .css).
172+
const moduleIds = new Set(Array.from(getModuleIdBySubpath().values()).flat());
173+
174+
for (const expected of ["Header", "Alert", "Chart", "tools", "shared", "assets"]) {
175+
expect(moduleIds).toContain(expected);
176+
}
177+
178+
// `src/tsconfig.json` excludes `./bin`: the CLI is not a public import subpath.
179+
expect(subpaths.some(subpath => subpath.startsWith("bin"))).toBe(false);
180+
});
181+
182+
it("has no package.json exports field to compare against", () => {
183+
// The premise of the enumeration above. If an `exports` map is ever added, this
184+
// fails and the public subpaths must be read from it instead of from src/.
185+
const packageJsonParsed = JSON.parse(
186+
fs.readFileSync(pathJoin(projectRootDirPath, "package.json")).toString("utf8")
187+
);
188+
189+
expect(packageJsonParsed["exports"]).toBe(undefined);
190+
});
191+
});

0 commit comments

Comments
 (0)