From 001de3074a6e3439183bf437b7cac29092d669d4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Artur=20M=C4=99dryga=C5=82?= Date: Thu, 20 Aug 2026 15:45:52 +0200 Subject: [PATCH 1/2] fix(runner): resolve the compiler chunk's real module shape on retry (DEV-2569) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The retry mechanism shipped in #235 recovers the fetch and then cannot compile: `Failed to transpile /index.js for the parcel sandbox: e.transform is not a function`. Verification on deployed master c83975c2 found it; the two import sites in transpile.ts are byte-identical in source and are not identical in the bundle. Vite rewrites only the bare specifier, and @babel/standalone is CJS, so the primary path gets a CJS-interop hop the `@vite-ignore` retry does not. Measured in the deployed bundle: primary: import("./babel-.js").then(t => t.b).then(t => t.default ?? t) retry: import(`${url}?hotRetry=1`).then(o => o.default ?? o) chunk: export { Hke as b } // Hke = _mergeNamespaces({default: babel}, [cjs]) so the retry resolved the raw module record `{b: {…}}`, `m.default` was undefined, and the loader returned the record. `asBabel` replaces the fixed `default ?? m` unwrap at both sites with a shape check over the record, its `default`, and one wrapper level, preferring `default` at each — so the primary keeps resolving exactly what it resolves today, and the retry stops depending on an interop hop it never gets. Walking values rather than a hardcoded `b` survives Rollup renaming that export. A miss throws, from inside the loaders, so it becomes a CompilerUnavailableError and is latched, carded and filed under ["tier1-compiler-asset"]. Left un-thrown it surfaced downstream inside babel.transform and landed in the visitor-source bucket as if it were the visitor's typo — the other half of the defect. The URL rides in the thrown message because assetUrl is recovered by regexing the cause. Also removes the exposure that made a retry necessary: the compiler now ships as `assets/compiler-babel.js` with no content hash. Workers Assets serves this app with `not_found_handling: "single-page-application"`, so a deploy rotated the hashed chunk out and its path answered `200 text/html` — a stranded tab could never succeed. No cache-header change goes with it; Workers Assets already serves every asset as `max-age=0, must-revalidate` with an ETag (measured on prod). Renaming only, no `manualChunks`: assigning the package to a named chunk was measured to pull Rollup's shared getDefaultExportFromCjs helper in with it, which made two ordinary chunks import 2.3 MB statically and modulepreload it. Tests. The existing loader suite imports packages/runtime/dist, where both paths really are equivalent, which is why #235's ten tests were green over a broken retry — so the shape rule is pinned three ways instead. Five unit cases fix the three measured namespace shapes and the compiler-error routing. A deterministic Playwright case in preview-recovery.spec.ts blocks the chunk against a real `vite build`, asserts the card copy and the two bounded attempts, then lifts the block and requires the recovered compiler to hand a sandbox to the bundler — an attempted bundler request, which `buildSetup` never reaches when the transpile throws (measured: 2 attempts with this fix, 0 without). `data-preview-status` is deliberately not that oracle: `booting` precedes `error`, so asserting it passed with the fix reverted. Everything off-localhost is aborted, because a `parcel` sandbox loads its bundler from a versioned host stubShell's globs never matched. `scripts/check-compiler-chunk.mjs`, run in the authoring CI job, pins the hash-free name and that the chunk stays lazy — verified red against the eager-chunk regression above. Co-Authored-By: Claude Opus 5 --- .github/workflows/ci.yml | 7 ++ runner/apps/authoring/vite.config.ts | 35 +++++++ runner/e2e/preview-recovery.spec.ts | 116 ++++++++++++++++++++++ runner/package.json | 1 + runner/packages/runtime/src/transpile.ts | 68 ++++++++++++- runner/pipeline/transpile-loader.test.mjs | 91 +++++++++++++++++ runner/scripts/check-compiler-chunk.mjs | 77 ++++++++++++++ 7 files changed, 391 insertions(+), 4 deletions(-) create mode 100644 runner/scripts/check-compiler-chunk.mjs diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index a01a26956..fca79c39c 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -147,6 +147,13 @@ jobs: - name: Build authoring (build smoke + e2e target) run: pnpm --filter @handsontable/demo-authoring build + # Two properties of the *bundle* that no unit test can see: the compiler chunk keeps a + # hash-free path (a rotated hashed chunk answers the SPA fallback and strands open tabs + # — DEV-2569 / DEMOS-15), and it stays lazily imported rather than joining the initial + # load. Checked here because this is the job that has the build. + - name: Check the compiler chunk (DEV-2569) + run: node scripts/check-compiler-chunk.mjs + - name: Upload the authoring build uses: actions/upload-artifact@v4 with: diff --git a/runner/apps/authoring/vite.config.ts b/runner/apps/authoring/vite.config.ts index 9051c3ff1..e3a898018 100644 --- a/runner/apps/authoring/vite.config.ts +++ b/runner/apps/authoring/vite.config.ts @@ -33,6 +33,41 @@ 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", + }, + }, }, plugins: [ react(), diff --git a/runner/e2e/preview-recovery.spec.ts b/runner/e2e/preview-recovery.spec.ts index 3018267db..82bd79e31 100644 --- a/runner/e2e/preview-recovery.spec.ts +++ b/runner/e2e/preview-recovery.spec.ts @@ -1,4 +1,5 @@ import { test, expect } from "@playwright/test"; +import { expectGridRendered, stubShell } from "./helpers"; // The preview must be able to come back. A runtime error in edited code puts the // pane into `error` (the "The preview could not start" card); fixing the code has @@ -278,3 +279,118 @@ test("live: fixing a Vue template error clears the preview error card", async ({ timeout: 60_000, }); }); + +// DEV-2569 / Sentry DEMOS-15 — the compiler chunk, and the only place its failure is real. +// +// Tier 1 pre-transpiles sources for the classic `parcel` bundler, which needs +// @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. +// +// 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 +// specifier and inserts a CJS-interop hop the `@vite-ignore` retry does not get — so the +// retry used to resolve the chunk's raw module record and die on the next compile with +// `e.transform is not a function`. `pipeline/transpile-loader.test.mjs` imports +// `packages/runtime/dist`, where both paths really are equivalent, and was green over +// exactly that. Playwright runs against a real `vite build` (see playwright.config.ts), so +// here the divergence exists. +// +// Verified red: with the two `asBabel` calls in transpile.ts reverted, the recovery +// assertion below fails — the status stays `error` after Restart, which is what production +// did. + +/** 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*"; + +test("a blocked compiler chunk cards, and Restart preview really recovers", async ({ page }) => { + test.setTimeout(120_000); + // Deterministic in the strict sense: `stubShell` alone is not enough here. A `parcel` + // sandbox loads its bundler from a *versioned* host (measured: 2-19-8-sandpack.codesandbox.io) + // plus jsdelivr and prod-packager-packages, none of which stubShell's two globs match — so + // this case would have quietly depended on the external bundler. Everything off-localhost is + // aborted instead, which costs the `ready` end-state (that one is the E2E_LIVE case below) + // and keeps a sharper oracle: with the bundler unreachable, `booting` means our transpile + // finished and handed the sandbox over, and `error` means it did not. + await stubShell(page); + await page.route((url) => url.hostname !== "localhost", (route) => route.abort()); + + // The oracle. Handing the compiled sandbox to the bundler is the first thing that happens + // *after* our transpile succeeds, and `buildSetup` throws ahead of it when the transpile + // fails — so an attempted bundler request is a positive signal that the compiler produced + // usable output. Measured both ways on this build: 2 attempts with the fix, 0 without. + // The requests are aborted by the route above, so nothing leaves the machine. + const bundlerAttempts: string[] = []; + page.on("request", (r) => { + const { hostname } = new URL(r.url()); + if (/sandpack/.test(hostname)) bundlerAttempts.push(hostname); + }); + + const asked: string[] = []; + let blocked = true; + await page.route(COMPILER_CHUNK, (route) => { + asked.push(new URL(route.request().url()).search || "(bare)"); + return blocked ? route.abort() : route.fallback(); + }); + + // `javascript` is Tier 1 on the `parcel` environment (catalog.json) — the one engine that + // needs the compiler at all. + await page.goto("/?example=javascript"); + await expect(previewStatus(page)).toHaveAttribute("data-preview-status", "error", { + timeout: 60_000, + }); + await expect(page.getByText("The preview could not start")).toBeVisible(); + await expect(page.locator("pre")).toContainText("The preview compiler could not be downloaded"); + const restart = page.getByRole("button", { name: "Restart preview" }); + await expect(restart, "the card has to offer the action its copy names").toBeVisible(); + expect(asked, "two bounded attempts, and the second must ask for a URL of its own").toEqual([ + "(bare)", + "?hotRetry=1", + ]); + + expect(bundlerAttempts, "a failed transpile never reaches the bundler").toEqual([]); + + blocked = false; + await restart.click(); + + // The whole fix. Before it the retry resolved the chunk's raw module record and the card + // flipped to the exact string production showed — "Failed to transpile /index.js for the + // parcel sandbox: e.transform is not a function" — with the status stuck on `error` and no + // bundler request ever attempted. Note `data-preview-status` is deliberately *not* the + // oracle here: `booting` is on the failure path too (it precedes `error`), so asserting it + // passes with the fix reverted. + await expect + .poll(() => bundlerAttempts.length, { timeout: 60_000, intervals: [250] }) + .toBeGreaterThan(0); + await expect(page.getByText("The preview could not start")).toHaveCount(0); + // A browser caches a failed module fetch in the document's module map, so re-importing the + // *same* specifier never touches the network again — which is why the remount's bare import + // files no third request, and why `rearm` has to mint a fresh query to be honest. + expect(asked).toEqual(["(bare)", "?hotRetry=1", "?hotRetry=2"]); +}); + +test("live: the grid renders after a compiler-chunk recovery", async ({ page }) => { + test.skip(process.env.E2E_LIVE !== "1", "set E2E_LIVE=1 to run live-render checks"); + test.setTimeout(180_000); + // The half that was never clicked through before shipping. `booting` above is our own + // signal; a real bundler behind it is what proves the recovered compiler's output builds. + + let blocked = true; + await page.route(COMPILER_CHUNK, (route) => (blocked ? route.abort() : route.fallback())); + + await page.goto("/?example=javascript"); + await expect(previewStatus(page)).toHaveAttribute("data-preview-status", "error", { + timeout: 60_000, + }); + + blocked = false; + await page.getByRole("button", { name: "Restart preview" }).click(); + await expect(previewStatus(page)).toHaveAttribute("data-preview-status", "ready", { + timeout: 120_000, + }); + await expectGridRendered(page); +}); diff --git a/runner/package.json b/runner/package.json index 620421cdb..61bee34c8 100644 --- a/runner/package.json +++ b/runner/package.json @@ -16,6 +16,7 @@ "typecheck": "pnpm -r run typecheck", "test": "pnpm --filter @handsontable/demo-runtime build && node --experimental-strip-types --test pipeline/*.test.mjs", "e2e": "playwright test", + "check:compiler-chunk": "node scripts/check-compiler-chunk.mjs", "e2e:matrix": "E2E_STARTER_MATRIX=1 PLAYWRIGHT_JSON_OUTPUT_NAME=test-results/starter-matrix.json playwright test e2e/starter-matrix.spec.ts --workers=2 --retries=2 --reporter=list,json", "e2e:matrix:report": "node scripts/starter-matrix-report.mjs" }, diff --git a/runner/packages/runtime/src/transpile.ts b/runner/packages/runtime/src/transpile.ts index ff6482295..f91a9690c 100644 --- a/runner/packages/runtime/src/transpile.ts +++ b/runner/packages/runtime/src/transpile.ts @@ -181,8 +181,69 @@ export function createRetryingLoader( }; } +/** + * Resolve the babel object out of whatever a dynamic import actually handed back + * (DEV-2569, second pass). + * + * The two import sites in this file are byte-identical in source and *not* identical in the + * shipped bundle. Vite rewrites only the bare specifier, and `@babel/standalone` is CJS, so + * the primary site gets an interop hop the `@vite-ignore` retry does not. Measured in the + * deployed bundle (`/assets/index-uxATgr1X.js`, 2026-08-20): + * + * primary: import("./babel-.js").then(t => t.b).then(t => t.default ?? t) + * retry: import(`${url}?hotRetry=1`).then(o => o.default ?? o) + * + * and the chunk's only export is that wrapper — `export { Hke as b }`, where `Hke` is Vite's + * `_mergeNamespaces({__proto__: null, default: babel}, [cjs])`. So the retry resolved the raw + * module record `{b: {…}}`, `m.default` was `undefined`, and the loader returned the record: + * the retry "succeeded" and the next compile died as `e.transform is not a function`. + * + * Hence a shape check rather than a fixed unwrap. The three shapes this has to accept, all + * measured: + * + * Node / `dist` (what `pipeline/*.test.mjs` sees) { default: babel, …named } + * bundled primary, after Vite's hop { __proto__: null, default: babel } + * bundled retry, no hop { b: { default: babel, transform } } + * + * `default` is preferred at every level, so the primary path keeps resolving exactly the + * object it resolves today. Walking *values* rather than a hardcoded `b` is what survives + * Rollup renaming that export — the name is bundler-generated and pinned by nothing. + * + * A miss **throws**, and the throw carries the URL: this runs inside the loaders, so the + * rejection reaches `createRetryingLoader` and becomes a `CompilerUnavailableError` — latched, + * carded, and reported as our own infrastructure failure. Left un-thrown it would surface + * downstream inside `babel.transform` and be filed in the visitor-source Sentry bucket as if + * it were the visitor's typo, which is the other half of what this defect was. `assetUrl` is + * recovered by `assetUrlFrom` regexing the cause's *message*, so the URL has to be in it. + */ +export function asBabel(ns: unknown, url?: string | null): Babel { + const babel = findBabel(ns, 2); + if (babel) return babel; + throw new TypeError( + `the in-browser compiler module exported no transform()${url ? `: ${url}` : ""}`, + ); +} + +/** Depth 2 is what the shapes above need: the record, its `default`, and one wrapper level. */ +function findBabel(value: unknown, depth: number): Babel | null { + if (value === null || (typeof value !== "object" && typeof value !== "function")) return null; + const record = value as Record; + if (depth > 0) { + const viaDefault = findBabel(record.default, depth - 1); + if (viaDefault) return viaDefault; + } + if (typeof record.transform === "function") return record as unknown as Babel; + if (depth > 0) { + for (const nested of Object.values(record)) { + const hit = findBabel(nested, depth - 1); + if (hit) return hit; + } + } + return null; +} + const loadBabelChunk = createLazyLoader(() => - import("@babel/standalone").then((m) => ((m as { default?: Babel }).default ?? m) as Babel), + import("@babel/standalone").then((m) => asBabel(m)), ); /** The retry's URL: the failed chunk with a query the module map has not seen. `@vite-ignore` @@ -192,9 +253,8 @@ function retryBabelChunk(cause: unknown, generation: number): Promise | n const url = assetUrlFrom(cause); if (!url) return null; const sep = url.includes("?") ? "&" : "?"; - return import(/* @vite-ignore */ `${url}${sep}hotRetry=${generation + 1}`).then( - (m) => ((m as { default?: Babel }).default ?? m) as Babel, - ); + const spec = `${url}${sep}hotRetry=${generation + 1}`; + return import(/* @vite-ignore */ spec).then((m) => asBabel(m, spec)); } const babelLoader = createRetryingLoader( diff --git a/runner/pipeline/transpile-loader.test.mjs b/runner/pipeline/transpile-loader.test.mjs index 12cb1c8f6..40f4a30b2 100644 --- a/runner/pipeline/transpile-loader.test.mjs +++ b/runner/pipeline/transpile-loader.test.mjs @@ -3,6 +3,7 @@ import assert from "node:assert/strict"; import { COMPILER_UNAVAILABLE_MESSAGE, CompilerUnavailableError, + asBabel, assetUrlFrom, createLazyLoader, createRetryingLoader, @@ -218,3 +219,93 @@ test("isCompilerUnavailable is a marker check, not a message sniff", () => { assert.equal(isCompilerUnavailable(null), false); assert.equal(isCompilerUnavailable("compiler"), false); }); + +// DEV-2569, second pass. The retry mechanism above was shipped and did not recover: it +// resolved, and the next compile died as `e.transform is not a function`. +// +// The two import sites in transpile.ts are byte-identical in source and are not identical in +// the bundle. Vite rewrites only the bare specifier, and @babel/standalone is CJS, so the +// primary gets an interop hop that `@vite-ignore` suppresses on the retry. Measured in the +// deployed bundle (/assets/index-uxATgr1X.js, 2026-08-20): +// +// primary: import("./babel-.js").then(t => t.b).then(t => t.default ?? t) +// retry: import(`${url}?hotRetry=1`).then(o => o.default ?? o) +// +// and the chunk exported only that wrapper — `export { Hke as b }`, Hke being Vite's +// `_mergeNamespaces({__proto__: null, default: babel}, [cjs])`. So the retry resolved the raw +// record `{b: {…}}` and `m.default ?? m` returned the record itself. In-page probe against +// that bundle: nsKeys ["b"], hasTransform "undefined", viaB "function". +// +// None of this is visible from here — these tests import `packages/runtime/dist`, where both +// paths really are equivalent, which is why the ten tests above were green over a broken +// retry. What *is* testable from here is the rule that makes the shapes interchangeable, so +// these cases fix the three measured shapes as the contract. The bundle itself is pinned by +// `scripts/check-compiler-chunk.mjs` and by preview-recovery.spec.ts, which run against a build. + +/** Stands in for the babel object. Identity is the assertion: `asBabel` must return *this*, + * not some wrapper that merely happens to expose a `transform`. */ +const babelStub = { transform: () => ({ code: "" }) }; + +/** A module namespace: null-prototype and frozen, the way both bundlers and the engine make + * them — a plain object would let a `default` lookup fall through to Object.prototype. */ +const ns = (props) => Object.freeze({ __proto__: null, ...props }); + +test("asBabel resolves the shape the bundled retry receives (no interop hop)", () => { + const record = ns({ b: ns({ default: babelStub, transform: babelStub.transform }) }); + assert.equal(asBabel(record), babelStub, "the wrapper is not the compiler"); +}); + +test("asBabel does not depend on the bundler's export name", () => { + // `b` is Rollup's, pinned by nothing. A rename must not strand the retry again. + assert.equal(asBabel(ns({ zQ7: ns({ default: babelStub }) })), babelStub); +}); + +test("asBabel keeps resolving the two shapes that already worked", () => { + // The bundled primary, after Vite's `.then(t => t.b)` hop. + assert.equal(asBabel(ns({ default: babelStub })), babelStub); + // Node/dist, where the namespace re-exports the CJS members alongside `default`. + assert.equal( + asBabel(ns({ default: babelStub, transform: babelStub.transform })), + babelStub, + "`default` is preferred at every level, so the primary path resolves what it does today", + ); +}); + +test("a module with no transform() is a compiler failure, and keeps its URL", async () => { + // The SPA fallback answers a rotated chunk with `200 text/html`; a module that parses but + // exposes no compiler is the same class of event. It must not be left to surface downstream + // inside babel.transform, where tier1Report files it in the visitor-source bucket as if it + // were a typo — that is what this defect did in production. + const { load } = createRetryingLoader( + async () => asBabel(ns({}), CHUNK), + () => null, + wrap, + ); + + const e = await load().catch((err) => err); + assert.ok(isCompilerUnavailable(e), "ours to fix, not the visitor's"); + assert.equal(e.message, COMPILER_UNAVAILABLE_MESSAGE); + assert.equal(e.assetUrl, CHUNK, "the URL has to ride in the thrown message to survive here"); +}); + +test("a bad-shape resolution is retried against a fresh URL and recovers", async () => { + // The whole point: the retry now yields a usable compiler instead of a wrapper. + const asked = []; + const { load } = createRetryingLoader( + async () => { + asked.push(CHUNK); + return asBabel(ns({}), CHUNK); + }, + (cause, generation) => { + const url = assetUrlFrom(cause); + if (!url) return null; + const spec = `${url}?hotRetry=${generation + 1}`; + asked.push(spec); + return Promise.resolve(asBabel(ns({ b: ns({ default: babelStub }) }), spec)); + }, + wrap, + ); + + assert.equal(await load(), babelStub); + assert.deepEqual(asked, [CHUNK, `${CHUNK}?hotRetry=1`]); +}); diff --git a/runner/scripts/check-compiler-chunk.mjs b/runner/scripts/check-compiler-chunk.mjs new file mode 100644 index 000000000..b7f452d3f --- /dev/null +++ b/runner/scripts/check-compiler-chunk.mjs @@ -0,0 +1,77 @@ +#!/usr/bin/env node +// The compiler chunk must keep a hash-free path and stay lazily loaded (DEV-2569). +// +// Two build-shaped promises no unit test can see, both of which failed in production once: +// +// 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. +// +// 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. +// +// Run against a real `vite build` output — the `authoring` job in ci.yml does, 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"); +const assets = readdirSync(assetsDir); + +if (!assets.includes(CHUNK)) { + const hashed = assets.filter((f) => /^(compiler-)?babel-.*\.js$/.test(f)); + 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` : ""}`, + ); +} + +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`); +} + +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 (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 (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`); From 75a2a8df5be66f30c9d50a09201e70f47ef35b22 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Artur=20M=C4=99dryga=C5=82?= Date: Thu, 20 Aug 2026 16:12:58 +0200 Subject: [PATCH 2/2] fix(runner): address the review on #249 (DEV-2569) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Five findings, all real. The new deterministic e2e aborted everything off-localhost, and this file is listed in e2e-live.yml with `E2E_BASE_URL` pointed at a deployment for the nightly canary — where `baseURL` is off-localhost, so `page.goto` itself was aborted and the canary would have gone red every night. Gated to the local build: the test's premise is the artifact the run just built, and the deployed target may predate the stable chunk name. Its E2E_LIVE companion now asserts the chunk was actually requested before waiting on the card, so a target whose build predates that rename fails saying so instead of as an opaque 60 s timeout. `asBabel` is called without a URL at the primary site, and that is where a genuine shape change is met first — so `assetUrl` is null there and no retry is attempted. Both are correct (nothing at that site knows the hashed path, and refetching the same URL returns the same bytes and the same shape), but the unit test claimed otherwise: it passed CHUNK into the *primary* loader and asserted `assetUrl === CHUNK`, a property the shipped path does not have. The test now models the URL-less primary — asserting the null and the declined retry — with a separate case for the retry site, which does keep its URL. Writing that second case turned up a real fragility: `retryOf` was called *outside* the try in `createRetryingLoader`, so a synchronous throw from it escaped un-wrapped, and an error that is not a CompilerUnavailableError is filed as the visitor's own compile failure — the mis-routing half of this very defect. Production is safe today (the real `retryOf` normalises inside `.then`, so it rejects), but it was one refactor from regressing. Now inside the try, covered for both failure modes, and verified red without the change. `check-compiler-chunk.mjs` guarded `dist/` but not `dist/assets/`, so a half-cleaned build printed a raw ENOENT stack in place of the script's own message. And it ran only in ci.yml's PR build, so the hash silently returning could still ship from a push or workflow_dispatch that never had one — master.yml now runs it on the deploy build too, before the upload. Verified: pnpm test 847 pass / 0 fail, typecheck clean, chunk check green, preview-recovery.spec.ts green on the ungated path (4 passed) and correctly skipping the local-build case when E2E_BASE_URL is set. Co-Authored-By: Claude Opus 5 --- .github/workflows/master.yml | 8 ++++ runner/e2e/preview-recovery.spec.ts | 20 +++++++++- runner/packages/runtime/src/transpile.ts | 24 ++++++++---- runner/pipeline/transpile-loader.test.mjs | 45 +++++++++++++++++++++-- runner/scripts/check-compiler-chunk.mjs | 6 +++ 5 files changed, 91 insertions(+), 12 deletions(-) diff --git a/.github/workflows/master.yml b/.github/workflows/master.yml index 7ee0e8414..d4c75c2f8 100644 --- a/.github/workflows/master.yml +++ b/.github/workflows/master.yml @@ -137,6 +137,14 @@ jobs: SENTRY_PROJECT: ${{ vars.SENTRY_PROJECT }} GITHUB_SHA: ${{ github.sha }} + # The one regression that only shows up in a deploy: the compiler chunk's content hash + # coming back, which strands every tab opened before the next deploy (DEV-2569). ci.yml + # checks the PR build; a push or workflow_dispatch that never had a PR build reaches this + # job instead, so it is checked here too — before the upload, not after. + - name: Check the compiler chunk (DEV-2569) + if: needs.changes.outputs.authoring == 'true' + run: node scripts/check-compiler-chunk.mjs + - name: Upload the authoring build if: needs.changes.outputs.authoring == 'true' uses: actions/upload-artifact@v4 diff --git a/runner/e2e/preview-recovery.spec.ts b/runner/e2e/preview-recovery.spec.ts index 82bd79e31..876613042 100644 --- a/runner/e2e/preview-recovery.spec.ts +++ b/runner/e2e/preview-recovery.spec.ts @@ -308,6 +308,15 @@ test("live: fixing a Vue template error clears the preview error card", async ({ const COMPILER_CHUNK = "**/assets/compiler-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 + // run just built. `e2e-live.yml` runs this file with `E2E_BASE_URL` pointed at a deployment + // for the nightly canary, where `baseURL` is off-localhost — the blanket abort below would + // kill `page.goto` itself, and the deployed build may predate the stable chunk name. The + // live half of this pair is the test after it. + test.skip( + Boolean(process.env.E2E_BASE_URL), + "runs against the locally built app — unset E2E_BASE_URL to run it", + ); test.setTimeout(120_000); // Deterministic in the strict sense: `stubShell` alone is not enough here. A `parcel` // sandbox loads its bundler from a *versioned* host (measured: 2-19-8-sandpack.codesandbox.io) @@ -380,9 +389,18 @@ test("live: the grid renders after a compiler-chunk recovery", async ({ page }) // signal; a real bundler behind it is what proves the recovered compiler's output builds. let blocked = true; - await page.route(COMPILER_CHUNK, (route) => (blocked ? route.abort() : route.fallback())); + const seen: string[] = []; + await page.route(COMPILER_CHUNK, (route) => { + seen.push(route.request().url()); + return blocked ? route.abort() : route.fallback(); + }); await page.goto("/?example=javascript"); + // Named before the status wait so a target whose build predates the stable chunk name fails + // saying so, rather than as an opaque 60 s timeout on an error card that never appears. + await expect + .poll(() => seen.length, { timeout: 60_000, intervals: [500] }) + .toBeGreaterThan(0); await expect(previewStatus(page)).toHaveAttribute("data-preview-status", "error", { timeout: 60_000, }); diff --git a/runner/packages/runtime/src/transpile.ts b/runner/packages/runtime/src/transpile.ts index f91a9690c..bad006abd 100644 --- a/runner/packages/runtime/src/transpile.ts +++ b/runner/packages/runtime/src/transpile.ts @@ -163,13 +163,16 @@ export function createRetryingLoader( } catch (cause) { first = cause; } - const retry = retryOf(first, generation); - if (retry) { - try { - return await retry; - } catch (cause) { - first = cause; - } + try { + // `retryOf` is inside the try, not just its promise: the real one builds the URL and + // normalises the module inside `.then`, so it rejects — but a synchronous throw from + // it (a bad URL, a shape check moved out of the `.then`) would otherwise escape + // un-wrapped, and an error that is not a `CompilerUnavailableError` is filed as the + // visitor's own compile failure. Half of DEV-2569 was exactly that mis-routing. + const retry = retryOf(first, generation); + if (retry) return await retry; + } catch (cause) { + first = cause; } terminal = wrap(first, {}); throw terminal; @@ -242,6 +245,13 @@ function findBabel(value: unknown, depth: number): Babel | null { return null; } +/** No URL is passed: the primary specifier is rewritten to a hashed path at build time, so + * 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`. */ const loadBabelChunk = createLazyLoader(() => import("@babel/standalone").then((m) => asBabel(m)), ); diff --git a/runner/pipeline/transpile-loader.test.mjs b/runner/pipeline/transpile-loader.test.mjs index 40f4a30b2..9d0b36e61 100644 --- a/runner/pipeline/transpile-loader.test.mjs +++ b/runner/pipeline/transpile-loader.test.mjs @@ -271,23 +271,60 @@ test("asBabel keeps resolving the two shapes that already worked", () => { ); }); -test("a module with no transform() is a compiler failure, and keeps its URL", async () => { +test("a module with no transform() is a compiler failure, not a visitor typo", async () => { // The SPA fallback answers a rotated chunk with `200 text/html`; a module that parses but // exposes no compiler is the same class of event. It must not be left to surface downstream // inside babel.transform, where tier1Report files it in the visitor-source bucket as if it // were a typo — that is what this defect did in production. + // + // Modelled on the *primary* site, which is where a genuine shape change would be met first + // and which passes no URL — the specifier is rewritten to a hashed path at build time, so + // nothing there knows it. So `assetUrl` is null and the retry is declined, both of which the + // real loader does: refetching the same URL returns the same bytes and the same shape. + let retries = 0; const { load } = createRetryingLoader( - async () => asBabel(ns({}), CHUNK), - () => null, + async () => asBabel(ns({})), + () => { + retries += 1; + return null; + }, wrap, ); const e = await load().catch((err) => err); assert.ok(isCompilerUnavailable(e), "ours to fix, not the visitor's"); assert.equal(e.message, COMPILER_UNAVAILABLE_MESSAGE); - assert.equal(e.assetUrl, CHUNK, "the URL has to ride in the thrown message to survive here"); + assert.equal(e.assetUrl, null, "the primary site has no URL to name — see loadBabelChunk"); + assert.equal(retries, 1, "the retry was offered the cause and declined it — not skipped"); + assert.match(e.cause.message, /exported no transform/); }); +// The retry *does* know its URL, and that is the one Sentry gets as `extra.assetUrl`. It only +// survives because `asBabel` puts it in the thrown message — `assetUrlFrom` regexes the cause. +// +// Run for both ways `retryOf` can fail. The real one normalises inside `.then`, so it rejects; +// a synchronous throw is what a later refactor would produce, and it has to be wrapped just the +// same — an error that is not a `CompilerUnavailableError` is filed as the visitor's own compile +// failure, which is the mis-routing half of this defect. +for (const [how, failing] of [ + ["rejects", (url) => Promise.resolve(ns({})).then((m) => asBabel(m, url))], + ["throws synchronously", (url) => asBabel(ns({}), url)], +]) { + test(`a shape miss on a retry that ${how} keeps the URL it asked for`, async () => { + const { load } = createRetryingLoader( + async () => { + throw new TypeError(`Failed to fetch dynamically imported module: ${CHUNK}`); + }, + (cause) => failing(`${assetUrlFrom(cause)}?hotRetry=1`), + wrap, + ); + + const e = await load().catch((err) => err); + assert.ok(isCompilerUnavailable(e), "our asset, not the visitor's source"); + assert.equal(e.assetUrl, `${CHUNK}?hotRetry=1`); + }); +} + test("a bad-shape resolution is retried against a fresh URL and recovers", async () => { // The whole point: the retry now yields a usable compiler instead of a wrapper. const asked = []; diff --git a/runner/scripts/check-compiler-chunk.mjs b/runner/scripts/check-compiler-chunk.mjs index b7f452d3f..3a527eccb 100644 --- a/runner/scripts/check-compiler-chunk.mjs +++ b/runner/scripts/check-compiler-chunk.mjs @@ -37,6 +37,12 @@ if (!existsSync(dist)) { 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. +if (!existsSync(assetsDir)) { + console.error(`${assetsDir} does not exist — the build did not finish`); + process.exit(1); +} const assets = readdirSync(assetsDir); if (!assets.includes(CHUNK)) {