From adcea865c729bbd89e7651e4edeef04ca5e4c137 Mon Sep 17 00:00:00 2001 From: Danielle Maywood Date: Mon, 30 Mar 2026 21:41:41 +0100 Subject: [PATCH] fix(site): improve check-compiler.mjs quality and fix bugs (#23812) --- site/scripts/check-compiler.mjs | 205 ++++++++++++++++++++++----- site/scripts/check-compiler.test.mjs | 95 +++++++++++++ site/vite.config.mts | 5 +- 3 files changed, 267 insertions(+), 38 deletions(-) create mode 100644 site/scripts/check-compiler.test.mjs diff --git a/site/scripts/check-compiler.mjs b/site/scripts/check-compiler.mjs index 525f113cc6..793338b43c 100644 --- a/site/scripts/check-compiler.mjs +++ b/site/scripts/check-compiler.mjs @@ -1,18 +1,55 @@ +/** + * React Compiler diagnostic checker. + * + * Runs babel-plugin-react-compiler over every .ts/.tsx file in the + * target directories and reports functions that failed to compile or + * were skipped. Exits with code 1 when any diagnostics are present + * or a target directory is missing. + * + * Usage: node scripts/check-compiler.mjs + */ import { readFileSync, readdirSync } from "node:fs"; import { join, relative } from "node:path"; import { transformSync } from "@babel/core"; +// Resolve the site/ directory (ESM equivalent of __dirname + ".."). const siteDir = new URL("..", import.meta.url).pathname; +// Only AgentsPage is currently opted in to React Compiler. Add new +// directories here as more pages are migrated. const targetDirs = [ "src/pages/AgentsPage", ]; const skipPatterns = [".test.", ".stories.", ".jest."]; +// Maximum length for truncated error messages in the report. +const MAX_ERROR_LENGTH = 120; + +// --------------------------------------------------------------------------- +// File collection +// --------------------------------------------------------------------------- + +/** + * Recursively collect .ts/.tsx files under `dir`, skipping test and + * story files. Returns paths relative to `siteDir`. Sets + * `hadCollectionErrors` and returns an empty array on ENOENT so the + * caller and recursive calls both stay safe. + */ function collectFiles(dir) { + let entries; + try { + entries = readdirSync(dir, { withFileTypes: true }); + } catch (e) { + if (e.code === "ENOENT") { + console.error(`Target directory not found: ${relative(siteDir, dir)}`); + hadCollectionErrors = true; + return []; + } + throw e; + } const results = []; - for (const entry of readdirSync(dir, { withFileTypes: true })) { + for (const entry of entries) { const full = join(dir, entry.name); if (entry.isDirectory()) { results.push(...collectFiles(full)); @@ -26,17 +63,63 @@ function collectFiles(dir) { return results; } -const files = targetDirs.flatMap((d) => collectFiles(join(siteDir, d))); +// --------------------------------------------------------------------------- +// Compilation & diagnostics +// +// We use transformSync deliberately. The React Compiler plugin is +// CPU-bound (parse-only takes ~2s vs ~19s with the compiler over all +// of site/src), so transformAsync + Promise.all gives no speedup — +// Node still runs all transforms on a single thread. Benchmarked +// sync, async-sequential, and async-parallel: all land within noise +// of each other. The sync API keeps the code simple. +// --------------------------------------------------------------------------- -let totalCompiled = 0; -const failures = []; +/** + * Shorten a compiler diagnostic message to its first sentence, stripping + * the leading "Error: " prefix and any trailing URL references so the + * one-line report stays readable. + * + * Example: + * "Error: Ref values are not allowed. Use ref types instead (https://…)." + * → "Ref values are not allowed" + */ +export function shortenMessage(msg) { + const str = typeof msg === "string" ? msg : String(msg); + return str + .replace(/^Error: /, "") + .split(/\.\s/)[0] + .split("(http")[0] + .replace(/\.\s*$/, "") + .trim(); +} -for (const file of files) { - const code = readFileSync(join(siteDir, file), "utf-8"); +/** + * Remove diagnostics that share the same line + message. The compiler + * can emit duplicate events for the same function when it retries + * compilation, so we deduplicate before reporting. + */ +export function deduplicateDiagnostics(diagnostics) { + const seen = new Set(); + return diagnostics.filter((d) => { + const key = `${d.line}:${d.short}`; + if (seen.has(key)) return false; + seen.add(key); + return true; + }); +} + +/** + * Run the React Compiler over a single file and return the number of + * successfully compiled functions plus any diagnostics. Transform + * errors are caught and returned as a diagnostic with line 0 rather + * than thrown, so the caller always gets a result. + */ +function compileFile(file) { const isTSX = file.endsWith(".tsx"); const diagnostics = []; try { + const code = readFileSync(join(siteDir, file), "utf-8"); const result = transformSync(code, { plugins: [ ["@babel/plugin-syntax-typescript", { isTSX }], @@ -44,53 +127,101 @@ for (const file of files) { logger: { logEvent(_filename, event) { if (event.kind === "CompileError" || event.kind === "CompileSkip") { - const msg = event.detail || event.reason || ""; - const short = typeof msg === "string" - ? msg.replace(/^Error: /, "").split(".")[0].split("(http")[0].trim() - : String(msg); - diagnostics.push({ line: event.fnLoc?.start?.line, short }); + const msg = event.detail || event.reason || "(unknown)"; + diagnostics.push({ + line: event.fnLoc?.start?.line ?? 0, + short: shortenMessage(msg), + }); } }, }, }], ], filename: file, + // Skip config-file resolution. No babel.config.js exists in the + // repo, so the search is wasted I/O on every file. + configFile: false, + babelrc: false, }); - const slots = result.code.match(/const \$ = _c\(\d+\)/g) || []; - totalCompiled += slots.length; + // The compiler inserts `const $ = _c(N)` at the top of every + // function it successfully compiles, where N is the number of + // memoization slots. Counting these tells us how many functions + // were compiled in this file. + const compiledCount = result?.code?.match(/const \$ = _c\(\d+\)/g)?.length ?? 0; - if (diagnostics.length) { - const seen = new Set(); - const unique = diagnostics.filter((d) => { - const key = `${d.line}:${d.short}`; - if (seen.has(key)) return false; - seen.add(key); - return true; - }); - failures.push({ file, compiled: slots.length, diagnostics: unique }); - } + return { + compiled: compiledCount, + diagnostics: deduplicateDiagnostics(diagnostics), + }; } catch (e) { - failures.push({ - file, compiled: 0, - diagnostics: [{ line: 0, short: `Transform error: ${String(e.message).substring(0, 120)}` }], - }); + return { + compiled: 0, + diagnostics: [{ + line: 0, + // Truncate to keep the one-line report readable. + short: `Transform error: ${(e instanceof Error ? e.message : String(e)).substring(0, MAX_ERROR_LENGTH)}`, + }], + }; } } -console.log(`\nTotal: ${totalCompiled} functions compiled across ${files.length} files`); -console.log(`Files with diagnostics: ${failures.length}\n`); +// --------------------------------------------------------------------------- +// Report +// --------------------------------------------------------------------------- -for (const f of failures) { - const short = f.file.replace("src/pages/AgentsPage/", ""); - console.log(`✗ ${short} (${f.compiled} compiled)`); - for (const d of f.diagnostics) { - console.log(` line ${d.line}: ${d.short}`); +/** + * Derive a short display path by stripping the first matching target + * dir prefix so the output stays compact. + */ +export function shortPath(file, dirs = targetDirs) { + for (const dir of dirs) { + const prefix = `${dir}/`; + if (file.startsWith(prefix)) { + return file.slice(prefix.length); + } + } + return file; +} + +/** Print a summary of compilation results and per-file diagnostics. */ +function printReport(failures, totalCompiled, fileCount, hadErrors) { + console.log(`\nTotal: ${totalCompiled} functions compiled across ${fileCount} files`); + console.log(`Files with diagnostics: ${failures.length}\n`); + + for (const f of failures) { + console.log(`✗ ${shortPath(f.file)} (${f.compiled} compiled)`); + for (const d of f.diagnostics) { + console.log(` line ${d.line}: ${d.short}`); + } + } + + if (failures.length === 0 && !hadErrors) { + console.log("✓ All files compile cleanly."); } } -if (failures.length === 0) { - console.log("✓ All files compile cleanly."); -} else { +// --------------------------------------------------------------------------- +// Main +// --------------------------------------------------------------------------- + +let hadCollectionErrors = false; + +const files = targetDirs.flatMap((d) => collectFiles(join(siteDir, d))); + +let totalCompiled = 0; +const failures = []; + +for (const file of files) { + const { compiled, diagnostics } = compileFile(file); + totalCompiled += compiled; + if (diagnostics.length > 0) { + failures.push({ file, compiled, diagnostics }); + } +} + +printReport(failures, totalCompiled, files.length, hadCollectionErrors); + +if (failures.length > 0 || hadCollectionErrors) { process.exitCode = 1; } diff --git a/site/scripts/check-compiler.test.mjs b/site/scripts/check-compiler.test.mjs new file mode 100644 index 0000000000..5485f571a3 --- /dev/null +++ b/site/scripts/check-compiler.test.mjs @@ -0,0 +1,95 @@ +import { describe, expect, it } from "vitest"; +import { + deduplicateDiagnostics, + shortPath, + shortenMessage, +} from "./check-compiler.mjs"; + +describe("shortenMessage", () => { + it("strips Error: prefix and takes first sentence", () => { + expect( + shortenMessage( + "Error: Ref values are not allowed. Use ref types instead.", + ), + ).toBe("Ref values are not allowed"); + }); + + it("strips trailing URL references", () => { + expect( + shortenMessage("Mutating a value returned from a hook(https://react.dev/reference)"), + ).toBe("Mutating a value returned from a hook"); + }); + + it("preserves dotted property paths", () => { + expect( + shortenMessage("Cannot destructure props.foo because it is null"), + ).toBe("Cannot destructure props.foo because it is null"); + }); + + it("coerces non-string values", () => { + expect(shortenMessage(42)).toBe("42"); + expect(shortenMessage({ toString: () => "Error: obj. detail" })).toBe("obj"); + }); + + it("normalizes trailing periods", () => { + expect(shortenMessage("Single sentence.")).toBe("Single sentence"); + }); + + it("returns (unknown) for empty input", () => { + expect(shortenMessage("")).toBe(""); + expect(shortenMessage("(unknown)")).toBe("(unknown)"); + }); +}); + +describe("deduplicateDiagnostics", () => { + it("removes duplicates with same line and message", () => { + const input = [ + { line: 1, short: "error A" }, + { line: 1, short: "error A" }, + { line: 2, short: "error B" }, + ]; + expect(deduplicateDiagnostics(input)).toEqual([ + { line: 1, short: "error A" }, + { line: 2, short: "error B" }, + ]); + }); + + it("keeps diagnostics with same message on different lines", () => { + const input = [ + { line: 1, short: "error A" }, + { line: 2, short: "error A" }, + ]; + expect(deduplicateDiagnostics(input)).toEqual(input); + }); + + it("keeps diagnostics with same line but different messages", () => { + const input = [ + { line: 1, short: "error A" }, + { line: 1, short: "error B" }, + ]; + expect(deduplicateDiagnostics(input)).toEqual(input); + }); + + it("returns empty array for empty input", () => { + expect(deduplicateDiagnostics([])).toEqual([]); + }); +}); + +describe("shortPath", () => { + const dirs = ["src/pages/AgentsPage", "src/pages/Other"]; + + it("strips matching target dir prefix", () => { + expect(shortPath("src/pages/AgentsPage/components/Chat.tsx", dirs)) + .toBe("components/Chat.tsx"); + }); + + it("strips first matching prefix when multiple match", () => { + expect(shortPath("src/pages/Other/index.tsx", dirs)) + .toBe("index.tsx"); + }); + + it("returns file unchanged when no prefix matches", () => { + expect(shortPath("src/utils/helper.ts", dirs)) + .toBe("src/utils/helper.ts"); + }); +}); diff --git a/site/vite.config.mts b/site/vite.config.mts index c52013e3eb..8ac7e7807e 100644 --- a/site/vite.config.mts +++ b/site/vite.config.mts @@ -228,7 +228,10 @@ export default defineConfig({ extends: true, test: { name: "unit", - include: ["src/**/*.test.?(m)ts?(x)"], + include: [ + "src/**/*.test.?(m)ts?(x)", + "scripts/**/*.test.?(m)[jt]s?(x)", + ], globals: true, environment: "jsdom", setupFiles: [