diff --git a/runner/apps/authoring/vite.config.ts b/runner/apps/authoring/vite.config.ts index e3a89801..c4406096 100644 --- a/runner/apps/authoring/vite.config.ts +++ b/runner/apps/authoring/vite.config.ts @@ -33,41 +33,27 @@ export default defineConfig({ // plugin's post-upload cleanup never runs to remove — and a manual // `wrangler deploy` would publish them. sourcemap: uploadEnabled, - rollupOptions: { - output: { - // Keep @babel/standalone in a chunk whose path does not change per build - // (DEV-2569). This app is served from Workers Assets with - // `not_found_handling: "single-page-application"` (see wrangler.jsonc), so a - // deploy removes the previous build's hashed chunks and their paths answer - // `200 text/html` instead of a 404. A tab that had not yet fetched the ~2.3 MB - // compiler when a deploy landed was then asking for a file that no longer - // existed — forever, since an HTML body is not a module. A stable path always - // exists in the deploy that is currently live. - // - // No cache-header work goes with this: Workers Assets already serves every - // asset, hashed or not, as `cache-control: public, max-age=0, must-revalidate` - // with an ETag (measured on prod 2026-08-20), so the stable URL revalidates on - // each use and picks up the new bytes. - // - // Two accepted consequences. A tab open across a deploy that fetches the - // compiler afterwards gets the *new* build's babel — benign, we only call - // `transform`, and strictly better than a permanent failure. And the Sentry - // plugin matches sourcemaps by the embedded debug id rather than by filename, - // so reusing a name across releases does not confuse symbolication. - // - // Renaming only, deliberately: no `manualChunks`. Assigning @babel/standalone to - // a named chunk was measured to pull Rollup's shared `getDefaultExportFromCjs` - // helper in with it (`export { … as b, … as g }`), which made two ordinary chunks - // import the 2.3 MB compiler *statically* and put a `modulepreload` for it in - // index.html — the opposite of lazy. Rollup's own split of the dynamic import in - // `packages/runtime/src/transpile.ts` already isolates it under the implicit name - // `babel`; all this does is take the hash off that one file. If a Rollup version - // ever renames that chunk, `scripts/check-compiler-chunk.mjs` goes red in CI rather - // than the hash quietly coming back. - chunkFileNames: (chunk) => - chunk.name === "babel" ? "assets/compiler-babel.js" : "assets/[name]-[hash].js", - }, - }, + // ⚠ Do not give the @babel/standalone chunk a hash-free name (reverted from #249, + // DEV-2569). The intent was sound — Workers Assets serves this app with + // `not_found_handling: "single-page-application"`, so a deploy rotates the hashed chunk + // out and its path answers `200 text/html`, which strands a tab that had not fetched the + // compiler yet. But the chunk is *not* self-contained: Rollup hoists the shared CJS + // interop helpers into the entry, so the emitted chunk opens with + // + // import { c as SD, g as Nke } from "./index-.js"; + // + // and that path is content-hashed. Measured on the deployed build: a stable + // `compiler-babel.js` therefore pulls the *new* build's 1.3 MB entry into an old tab, and + // that entry's top level is `createRoot(document.getElementById("root")).render(…)` plus + // `Sentry.init`. React 18 clears the root container, so the visitor's workspace is + // detached and silently remounted from a different build — unsaved edits gone, no card, + // two Sentry clients. That is strictly worse than the carded failure it replaced, which + // tells the visitor to reload (`describeRuntimeError`, and rearmCompilerLoad's docblock). + // + // A stable path is still the right end state; it needs the compiler built as its own + // self-contained artifact (or the SPA fallback stopped from answering /assets/*) rather + // than a `chunkFileNames` rename. Until then the hash is load-bearing: it is what makes a + // rotated chunk fail loudly. }, plugins: [ react(), diff --git a/runner/e2e/preview-recovery.spec.ts b/runner/e2e/preview-recovery.spec.ts index 87661304..6596c6f0 100644 --- a/runner/e2e/preview-recovery.spec.ts +++ b/runner/e2e/preview-recovery.spec.ts @@ -286,9 +286,10 @@ test("live: fixing a Vue template error clears the preview error card", async ({ // @babel/standalone: ~2.3 MB, code-split, fetched on first compile. It fails for ordinary // reasons (offline, a blocked request, an extension) and it used to fail permanently for // one of ours — a deploy rotating the chunk out from under a tab, which Workers Assets -// answers with `200 text/html` rather than a 404. `assets/compiler-babel.js` is hash-free -// now so that population is gone (`scripts/check-compiler-chunk.mjs` pins the name), and -// what is left is the transient case these two tests drive. +// answers with `200 text/html` rather than a 404. The hash-free chunk name meant to close that +// population is reverted (see vite.config.ts — a stable path with a content-hashed dependency +// drags a second copy of the app into an old tab), so a rotated chunk is still cured only by a +// reload and the transient case is what these two tests drive. // // This spec is the *only* place the loader can be tested honestly. Its two import sites are // byte-identical in source and are not identical in the bundle — Vite rewrites the bare @@ -305,7 +306,7 @@ test("live: fixing a Vue template error clears the preview error card", async ({ /** The compiler chunk plus any retry query. The trailing `*` is load-bearing: without it * the `?hotRetry=n` requests slip past the block and the test passes for the wrong reason. */ -const COMPILER_CHUNK = "**/assets/compiler-babel.js*"; +const COMPILER_CHUNK = "**/assets/babel-*.js*"; test("a blocked compiler chunk cards, and Restart preview really recovers", async ({ page }) => { // Local-build only, and not because it is slow or flaky: the premise is the artifact this diff --git a/runner/packages/runtime/src/transpile.ts b/runner/packages/runtime/src/transpile.ts index bad006ab..68216c0a 100644 --- a/runner/packages/runtime/src/transpile.ts +++ b/runner/packages/runtime/src/transpile.ts @@ -249,9 +249,10 @@ function findBabel(value: unknown, depth: number): Babel | null { * nothing here knows it (that is the whole reason `retryBabelChunk` reads it out of the * engine's error text). A shape mismatch discovered here therefore reaches * `CompilerUnavailableError` with `assetUrl: null` and no retry — correct on both counts: - * refetching the same URL returns the same bytes and the same shape, and the chunk now has - * one fixed path (`assets/compiler-babel.js`), so naming it adds nothing the fingerprint - * does not already say. The engine's own wording still rides in `extra.cause`. */ + * refetching the same URL returns the same bytes and the same shape, so there is nothing for + * a retry to do. Only the URL is lost, and `extra.cause` still carries the engine's own + * wording — a shape mismatch is a build-shape bug, which the hashed path does not help + * diagnose anyway. */ const loadBabelChunk = createLazyLoader(() => import("@babel/standalone").then((m) => asBabel(m)), ); diff --git a/runner/scripts/check-compiler-chunk.mjs b/runner/scripts/check-compiler-chunk.mjs index 3a527ecc..63f3ac4e 100644 --- a/runner/scripts/check-compiler-chunk.mjs +++ b/runner/scripts/check-compiler-chunk.mjs @@ -1,41 +1,39 @@ #!/usr/bin/env node -// The compiler chunk must keep a hash-free path and stay lazily loaded (DEV-2569). +// The compiler chunk must stay lazily loaded, and self-contained enough to be renameable +// (DEV-2569). // -// Two build-shaped promises no unit test can see, both of which failed in production once: +// `@babel/standalone` is ~2.3 MB and only Tier-1 examples ever compile, so Rollup's split of +// the dynamic import in `packages/runtime/src/transpile.ts` is what keeps it off the initial +// load. Two ways that has been lost or nearly lost, both measured, neither visible to any +// unit test: // -// 1. `@babel/standalone` ships in its own chunk named `assets/compiler-babel.js`, with no -// content hash. `apps/authoring/wrangler.jsonc` serves this app from Workers Assets with -// `not_found_handling: "single-page-application"`, so a deploy removes the previous -// build's hashed chunks and their paths answer `200 text/html` instead of a 404. A tab -// that had not yet fetched the ~2.3 MB compiler when a deploy landed was asking for a -// file that no longer existed — forever, since an HTML body is not a module (Sentry -// DEMOS-15). The rename is a one-line `chunkFileNames` in `vite.config.ts`, and it rests -// on Rollup's implicit chunk name for the dynamic import in -// `packages/runtime/src/transpile.ts`. If a Rollup version renames that chunk, the hash -// comes back silently — this is what makes it loud instead. +// 1. **It went eager.** Naming the chunk via `manualChunks` pulled Rollup's shared +// `getDefaultExportFromCjs` helper in with it (`export { … as b, … as g }`), after which +// two ordinary chunks imported the 2.3 MB compiler *statically* and index.html gained a +// `modulepreload` for it. `manualChunks` assigns a module to a chunk; it does not keep the +// chunk lazy. // -// 2. It stays *dynamically* imported. Naming it via `manualChunks` was measured to pull -// Rollup's shared `getDefaultExportFromCjs` helper into the same chunk, which made two -// ordinary chunks import 2.3 MB of compiler statically and put a `modulepreload` for it -// in index.html. Every visitor would have paid for a compiler that only Tier-1 examples -// use, and nothing else in the suite would have noticed. +// 2. **It stopped being self-contained.** The emitted chunk opens with +// `import { c, g } from "./index-.js"` — the content-hashed *entry*, whose top level +// renders the app. That is why #249's hash-free `compiler-babel.js` had to be reverted: a +// stable path with a hashed dependency drags a whole second copy of the app into an old +// tab. The count below is a tripwire, not a ban: if it ever reaches zero the chunk can be +// given a stable name, which is the fix DEV-2569 actually wants. // -// Run against a real `vite build` output — the `authoring` job in ci.yml does, right after -// building it. +// Run against a real `vite build` output — ci.yml's `authoring` job and master.yml's deploy +// build both do, right after building it. import { readFileSync, readdirSync, existsSync } from "node:fs"; import path from "node:path"; import { fileURLToPath } from "node:url"; const dist = path.resolve(path.dirname(fileURLToPath(import.meta.url)), "../apps/authoring/dist"); -const CHUNK = "compiler-babel.js"; if (!existsSync(dist)) { console.error(`no build to check at ${dist} — run \`pnpm --filter @handsontable/demo-authoring build\` first`); process.exit(1); } -const failures = []; const assetsDir = path.join(dist, "assets"); // Guarded like `dist` itself: a half-cleaned build would otherwise crash with a raw ENOENT // stack in place of the message this script exists to print. @@ -43,41 +41,58 @@ if (!existsSync(assetsDir)) { console.error(`${assetsDir} does not exist — the build did not finish`); process.exit(1); } + +const failures = []; const assets = readdirSync(assetsDir); -if (!assets.includes(CHUNK)) { - const hashed = assets.filter((f) => /^(compiler-)?babel-.*\.js$/.test(f)); +// Rollup names the chunk after the dynamically imported package's entry. +const chunk = assets.find((f) => /^babel-[^/]*\.js$/.test(f)); +if (!chunk) { failures.push( - `assets/${CHUNK} is missing${hashed.length ? ` — found ${hashed.join(", ")} instead, so the chunk was renamed and \`chunkFileNames\` in vite.config.ts no longer matches it` : ""}`, + `no assets/babel-.js chunk — @babel/standalone is no longer code-split, so every visitor now downloads ~2.3 MB of compiler (found: ${assets.filter((f) => f.endsWith(".js")).join(", ")})`, ); } const html = readFileSync(path.join(dist, "index.html"), "utf8"); -if (html.includes(CHUNK)) { - failures.push(`index.html references ${CHUNK} (a preload or a script tag) — the compiler must not be part of the initial load`); +if (chunk && html.includes(chunk)) { + failures.push(`index.html references ${chunk} (a preload or a script tag) — the compiler must not be part of the initial load`); } const dynamic = []; -for (const file of assets.filter((f) => f.endsWith(".js") && f !== CHUNK)) { - const code = readFileSync(path.join(assetsDir, file), "utf8"); - // A static `import … from "./compiler-babel.js"` is the failure mode: it makes the chunk - // eager even though the source only ever writes `import(…)`. - if (/\bfrom\s*["']\.\/compiler-babel\.js["']/.test(code)) { - failures.push(`assets/${file} imports ${CHUNK} statically — the chunk is no longer lazy`); +if (chunk) { + for (const file of assets.filter((f) => f.endsWith(".js") && f !== chunk)) { + const code = readFileSync(path.join(assetsDir, file), "utf8"); + // Both static forms. `from "./chunk"` is the ordinary one; a bare `import"./chunk"` is what + // Rollup emits when the importer uses none of the chunk's exports, and it makes the chunk + // just as eager while matching no `from` pattern. + const spec = `["']\\./${chunk.replace(/\./g, "\\.")}["']`; + if (new RegExp(`\\bfrom\\s*${spec}`).test(code) || new RegExp(`\\bimport\\s*${spec}`).test(code)) { + failures.push(`assets/${file} imports ${chunk} statically — the chunk is no longer lazy`); + } + if (code.includes(`import("./${chunk}")`)) dynamic.push(file); } - if (code.includes(`import("./${CHUNK}")`)) dynamic.push(file); -} -if (dynamic.length !== 1) { - failures.push( - `expected exactly one chunk to dynamically import ${CHUNK}, found ${dynamic.length}${dynamic.length ? ` (${dynamic.join(", ")})` : " — the lazy load is gone"}`, - ); + if (dynamic.length !== 1) { + failures.push( + `expected exactly one chunk to dynamically import ${chunk}, found ${dynamic.length}${dynamic.length ? ` (${dynamic.join(", ")})` : " — the lazy load is gone"}`, + ); + } } +// Reported, not failed: this is the number that has to reach zero before the chunk can carry a +// stable name (see the header). Printing it keeps the reason for the hash in front of whoever +// next reads this output. +const crossBuild = chunk + ? [...readFileSync(path.join(assetsDir, chunk), "utf8").matchAll(/from"\.\/([^"]+)"/g)].map((m) => m[1]) + : []; + if (failures.length) { console.error("compiler chunk check failed:"); for (const f of failures) console.error(` - ${f}`); process.exit(1); } -console.log(`compiler chunk ok: assets/${CHUNK}, lazily imported by ${dynamic[0]} only`); +console.log( + `compiler chunk ok: assets/${chunk}, lazily imported by ${dynamic[0]} only; ` + + `${crossBuild.length} hashed dependency/ies (${crossBuild.join(", ") || "none"}) — must be 0 before it can be renamed`, +);