From 5961269cb5e7b6dfb0e2a7a7febb80d6d89c4b58 Mon Sep 17 00:00:00 2001 From: Paul Bakaus Date: Thu, 13 Aug 2026 23:18:29 -0400 Subject: [PATCH 1/2] Stop emitting a JSDoc cast into every Svelte variant (fixes #580) Live mode scaffolds each Svelte variant with a props script that annotated the declaration: /** @type {{ title: string; }} */ let { title } = $props(); A JSDoc `@type` written directly before a value is also JSDoc's cast syntax, and esrap 2.3.3, the printer Svelte emits JS through, moves that annotation onto the template's own declaration: var /** @type {{ title: string; }} */ (h1) = root(); `var (h1) = ...` does not parse. The .svelte source is valid, the compile succeeds, and the failure lands in the browser's dynamic import as "Unexpected token '('": the variant never mounts and the session shows nothing. `@typedef` carries the same shape without being a cast, so both builders emit that. This is not test-only. Every Svelte variant we generate carried the construct, so live mode was broken for any user whose install resolved esrap 2.3.3. Svelte declares `esrap: ^2.2.12`, so a fresh install takes it; this repo's lockfile pins 2.3.0, which is why unit tests stayed green while the fixture, which installs into a temp dir, did not. Two reasons the existing pre-publish guard could not have caught it, now recorded next to it: - `compileCheckVariants` compiles with `generate: false`, so there is no emitted JS to inspect. - `loadSvelteCompiler` resolves the compiler through createRequire, which Svelte's export map routes to a prebuilt CJS build. A dev server imports `src/compiler`, and only that path runs the app's installed printer. The guard was checking a different compiler than the browser runs. The new suite therefore imports the compiler as ESM and asserts the emitted JavaScript parses, rather than pinning the comment style: a future printer that mangles some other construct fails it too. The first draft used createRequire and reported green against the exact input that breaks in a browser, which is the mistake worth not repeating. Verified against svelte 5.56.9 with esrap 2.3.3. Full live-e2e sweep green, 26 fixtures. Written with AI assistance (Claude Code). Co-Authored-By: Claude Opus 5 (1M context) --- scripts/test-suites.mjs | 1 + skill/scripts/live/svelte-ast.mjs | 12 +- skill/scripts/live/svelte-component.mjs | 28 +++- tests/live-svelte-props-script.test.mjs | 163 ++++++++++++++++++++++++ 4 files changed, 200 insertions(+), 4 deletions(-) create mode 100644 tests/live-svelte-props-script.test.mjs diff --git a/scripts/test-suites.mjs b/scripts/test-suites.mjs index c488973e1..7f726fa6e 100644 --- a/scripts/test-suites.mjs +++ b/scripts/test-suites.mjs @@ -161,6 +161,7 @@ export const SUITES = { 'tests/live-source-search.test.mjs', 'tests/live-svelte-ast.test.mjs', 'tests/live-svelte-component-accept.test.mjs', + 'tests/live-svelte-props-script.test.mjs', 'tests/live-tanstack-adapter.test.mjs', 'tests/live-target-context.test.mjs', 'tests/live-ui-surfaces.test.mjs', diff --git a/skill/scripts/live/svelte-ast.mjs b/skill/scripts/live/svelte-ast.mjs index 06e18b62e..dae6f8312 100644 --- a/skill/scripts/live/svelte-ast.mjs +++ b/skill/scripts/live/svelte-ast.mjs @@ -933,9 +933,17 @@ function collectFreeIdentifierRanges(node, scopes, emit) { * Build the preview component's script block from a v2 contract, with * defaults that keep an unhydrated mount rendering instead of crashing. */ +// `/** @type {...} */` directly before a destructuring declaration is also +// JSDoc's cast syntax, and Svelte 5.50+ re-emits the annotation in cast form +// onto the template's own declaration: `var /** @type {...} */ (h1) = root()`. +// That is a syntax error, so the browser's dynamic import of the variant dies +// with "Unexpected token '('" and nothing renders. `@typedef` carries the same +// shape without being a cast. Keep it a typedef; see PROPS_SCRIPT_SHAPES in +// tests/live-svelte-props-script.test.mjs, which compiles what these builders +// emit and parses the result. export function buildPropsScriptV2(contract) { if (!contract || contract.length === 0) { - return '\n'; + return '\n'; } const defaults = { text: "''", @@ -957,5 +965,5 @@ export function buildPropsScriptV2(contract) { const typeFields = contract .map((c) => ` ${c.prop}?: ${types[c.kind] ?? 'string'};`) .join('\n'); - return `\n`; + return `\n`; } diff --git a/skill/scripts/live/svelte-component.mjs b/skill/scripts/live/svelte-component.mjs index 4993453a7..8e2c33deb 100644 --- a/skill/scripts/live/svelte-component.mjs +++ b/skill/scripts/live/svelte-component.mjs @@ -154,13 +154,19 @@ export function parseSvelteComponentFile(content) { return { markup, cssLines, styleBlock }; } +// A JSDoc `@type` directly before a destructuring declaration is JSDoc cast +// syntax, and Svelte 5.50+ re-emits it onto the template's own declaration as +// `var /** @type {...} */ (h1) = root()`, which does not parse. The browser's +// import of the variant then fails with "Unexpected token '('" and the session +// shows nothing. `@typedef` documents the same shape without being a cast. +// tests/live-svelte-props-script.test.mjs compiles and parses what this emits. function buildPropsScript(contract) { if (contract.length === 0) { - return '\n'; + return '\n'; } const names = contract.map((c) => c.prop).join(', '); const typeFields = contract.map((c) => ` ${c.prop}: string;`).join('\n'); - return `\n`; + return `\n`; } function buildVariantStub(variantNum, originalWithProps, contract) { @@ -1111,6 +1117,24 @@ export function removeSvelteComponentSession(id, cwd = process.cwd()) { * seeded one) used to surface as a red Vite overlay in the user's page plus * a mount-failure round trip; bounced at publish time it is a private * agent-side fix with the exact file and line. + * + * What this deliberately does NOT prove is that the emitted module is valid + * JavaScript, and issue #580 was exactly that gap: valid .svelte source whose + * generated JS did not parse, so this check passed and the browser's import + * failed with "Unexpected token '('". Two reasons it cannot close the gap, both + * structural rather than oversights: + * + * - `generate: false` produces no JS to inspect, and generating it here would + * spend a full codegen per variant on the publish path. + * - `loadSvelteCompiler` reaches the compiler through createRequire, which + * Svelte's export map routes to a prebuilt CJS build. The dev server + * imports `src/compiler` instead, and only that path uses the app's + * installed printer (esrap). The two can disagree, so even a generated + * check here would be checking a different compiler than the one whose + * output the browser runs. + * + * tests/live-svelte-props-script.test.mjs covers the emitted JS, importing the + * compiler as ESM so it sees what the dev server sees. */ export function compileCheckVariants(id, cwd = process.cwd()) { const manifest = findSvelteComponentManifest(id, cwd); diff --git a/tests/live-svelte-props-script.test.mjs b/tests/live-svelte-props-script.test.mjs new file mode 100644 index 000000000..e662a276d --- /dev/null +++ b/tests/live-svelte-props-script.test.mjs @@ -0,0 +1,163 @@ +/** + * The variant components live mode scaffolds must survive the app's own Svelte + * compiler AND parse as JavaScript afterwards. Run with: + * node --test tests/live-svelte-props-script.test.mjs + * + * `compileCheckVariants` already compiles each variant with `generate: false`, + * which proves the .svelte source parses. It cannot prove the emitted module + * parses, and that is the gap this suite covers. In issue #580, esrap 2.3.3 + * (Svelte's JS printer, pulled in by `esrap: ^2.2.12`) printed a + * `/** @type {...} *\/` written directly before a destructuring declaration as + * JSDoc cast syntax on the template's own declaration, + * + * var /** @type {{ title: string; }} *\/ (h1) = root(); + * + * which compiles without complaint and then dies in the browser's dynamic + * import with "Unexpected token '('". Nothing rendered, and the failure + * surfaced two layers away from the comment that caused it. + * + * Two things follow, and both shape this file: + * + * 1. The assertion is on the emitted JavaScript, never on the comment style. A + * future printer that mangles some other construct fails here too, which a + * test pinned to `@typedef` would not. + * 2. The compiler is imported as ESM, because that is what the dev server + * resolves. Svelte's export map sends `require` to a prebuilt CJS compiler + * and `import` to `src/compiler`, and only the latter goes through the + * installed esrap. Reaching for `createRequire` here (as `loadSvelteCompiler` + * does) compiles with a different printer than the browser ever sees, and + * the guard passes while the product is broken. That is not hypothetical: + * the first draft of this suite did exactly that and reported green. + */ + +import { describe, it, before, beforeEach, afterEach } from 'node:test'; +import assert from 'node:assert/strict'; +import fs from 'node:fs'; +import path from 'node:path'; +import os from 'node:os'; +import { createRequire } from 'node:module'; +import { fileURLToPath } from 'node:url'; + +import { buildPropsScriptV2 } from '../skill/scripts/live/svelte-ast.mjs'; +import { scaffoldSvelteComponentSession } from '../skill/scripts/live/svelte-component.mjs'; + +const require = createRequire(import.meta.url); +const REPO_ROOT = path.resolve(path.dirname(fileURLToPath(import.meta.url)), '..'); + +let compile; +let acornParse; +let svelteVersion; +let printerVersion; + +before(async () => { + // ESM import, not createRequire: see note 2 in the header. This is the build + // a Vite dev server loads, and the only one that uses the installed esrap. + const compiler = await import('svelte/compiler'); + compile = compiler.compile; + svelteVersion = compiler.VERSION; + acornParse = require('acorn').parse; + try { + printerVersion = require('esrap/package.json').version; + } catch { + printerVersion = 'unknown'; + } +}); + +function assertEmittedJsParses(svelteSource, label) { + const { js } = compile(svelteSource, { + generate: 'client', + // dev:true is what a dev server uses, and it is the mode that carries the + // defect: the extra location metadata is where the stray annotation lands. + dev: true, + filename: 'v1.svelte', + }); + try { + acornParse(js.code, { ecmaVersion: 'latest', sourceType: 'module' }); + } catch (err) { + const line = js.code.split('\n')[(err.loc?.line ?? 1) - 1] || ''; + assert.fail( + `${label}: svelte ${svelteVersion} (printer esrap ${printerVersion}) emitted JavaScript that does not parse.\n` + + ` ${err.message}\n` + + ` offending line: ${line.trim()}\n` + + ` The browser reports this as a mount failure, not as a compile error, ` + + `because the .svelte source is valid and only the emitted module is not.\n` + + ` source:\n${svelteSource}`, + ); + } +} + +describe('scaffolded props scripts emit parseable JavaScript', () => { + // The empty contract is the shape the CI fixture hit: a picked element with + // no dynamic values at all. + const CONTRACTS = { + 'no props': [], + 'one text prop': [{ prop: 'title', expr: 'title', kind: 'text' }], + 'every prop kind': [ + { prop: 'title', expr: 'title', kind: 'text' }, + { prop: 'body', expr: 'post.body', kind: 'raw' }, + { prop: 'isOpen', expr: 'open', kind: 'condition' }, + { prop: 'items', expr: 'stages', kind: 'collection' }, + { prop: 'onSelect', expr: 'select', kind: 'handler' }, + ], + }; + + for (const [label, contract] of Object.entries(CONTRACTS)) { + it(`buildPropsScriptV2: ${label}`, () => { + const source = `${buildPropsScriptV2(contract)}\n

Fixture

\n`; + assertEmittedJsParses(source, `buildPropsScriptV2 (${label})`); + }); + } + + // Deliberately NOT asserted here: that the old `@type` shape still breaks. + // Whether it breaks depends on the installed printer (esrap 2.3.3 yes, 2.3.2 + // no), and this repo's lockfile carries a good one while a fresh fixture + // install pulls the bad one. An assertion that upstream is still broken would + // fail in this repo and pass in CI, which is the wrong way round for a guard. + // The live-e2e suite installs fresh and is where the real printer gets + // exercised; these cases pin what we emit, on whatever printer is present. +}); + +describe('the real scaffolder writes variants that parse', () => { + let scratch; + + beforeEach(() => { + scratch = fs.mkdtempSync(path.join(os.tmpdir(), 'impeccable-svelte-scaffold-')); + // The scaffolder resolves the compiler from the app root, so the staged app + // needs a package.json and a reachable svelte. Symlinking the repo's + // node_modules is what the static fixture sweep already does. + fs.writeFileSync(path.join(scratch, 'package.json'), JSON.stringify({ name: 'app', type: 'module' })); + fs.symlinkSync(path.join(REPO_ROOT, 'node_modules'), path.join(scratch, 'node_modules'), 'dir'); + }); + + afterEach(() => { + fs.rmSync(scratch, { recursive: true, force: true }); + }); + + const CASES = { + 'static markup (the #580 shape)': ['

Vite 8 + SvelteKit Fixture

'], + 'markup with a free expression': ['

{headline}

'], + }; + + for (const [label, originalLines] of Object.entries(CASES)) { + it(label, () => { + const result = scaffoldSvelteComponentSession({ + id: 'testsession', + count: 3, + sourceFile: 'src/routes/+page.svelte', + sourceStartLine: 1, + sourceEndLine: originalLines.length, + originalLines, + cwd: scratch, + }); + assert.equal(result.fallback, undefined, `scaffold fell back: ${result.reason}`); + + const dir = path.join(scratch, 'node_modules', '.impeccable-live', 'testsession'); + const variants = fs.readdirSync(dir).filter((name) => /^v\d+\.svelte$/.test(name)); + assert.ok(variants.length > 0, 'scaffolder wrote no variant files'); + + for (const name of variants) { + assertEmittedJsParses(fs.readFileSync(path.join(dir, name), 'utf-8'), `${label} / ${name}`); + } + }); + } +}); From b7960ecde3e9021feac7ea3d3853d2a6c6e04be1 Mon Sep 17 00:00:00 2001 From: Paul Bakaus Date: Thu, 13 Aug 2026 23:38:35 -0400 Subject: [PATCH 2/2] Keep the scaffolder test inside its own workspace Two review findings on #581, both fair. The scratch app symlinked the whole of the repo's node_modules, so the scaffolder's output directory, `node_modules/.impeccable-live`, resolved to the REPO's copy. Variants were written there and survived `afterEach`, which only removed the temp dir; the next case reused the session id, and the scaffolder keeps existing variant files, so a case could parse a previous case's source against a fresh manifest. Now only `svelte` is linked, into a node_modules the workspace owns, and each case gets its own session id. Svelte's own dependencies still resolve, because node follows the link to its real path before looking for them. The comment also pointed at a `PROPS_SCRIPT_SHAPES` symbol that does not exist in the test file. Dropped the name and kept the file reference. Written with AI assistance (Claude Code). Co-Authored-By: Claude Opus 5 (1M context) --- skill/scripts/live/svelte-ast.mjs | 6 +++--- tests/live-svelte-props-script.test.mjs | 26 +++++++++++++++++++------ 2 files changed, 23 insertions(+), 9 deletions(-) diff --git a/skill/scripts/live/svelte-ast.mjs b/skill/scripts/live/svelte-ast.mjs index dae6f8312..7481b8730 100644 --- a/skill/scripts/live/svelte-ast.mjs +++ b/skill/scripts/live/svelte-ast.mjs @@ -938,9 +938,9 @@ function collectFreeIdentifierRanges(node, scopes, emit) { // onto the template's own declaration: `var /** @type {...} */ (h1) = root()`. // That is a syntax error, so the browser's dynamic import of the variant dies // with "Unexpected token '('" and nothing renders. `@typedef` carries the same -// shape without being a cast. Keep it a typedef; see PROPS_SCRIPT_SHAPES in -// tests/live-svelte-props-script.test.mjs, which compiles what these builders -// emit and parses the result. +// shape without being a cast. Keep it a typedef; +// tests/live-svelte-props-script.test.mjs compiles what these builders emit +// and parses the result. export function buildPropsScriptV2(contract) { if (!contract || contract.length === 0) { return '\n'; diff --git a/tests/live-svelte-props-script.test.mjs b/tests/live-svelte-props-script.test.mjs index e662a276d..3ace60c70 100644 --- a/tests/live-svelte-props-script.test.mjs +++ b/tests/live-svelte-props-script.test.mjs @@ -122,11 +122,20 @@ describe('the real scaffolder writes variants that parse', () => { beforeEach(() => { scratch = fs.mkdtempSync(path.join(os.tmpdir(), 'impeccable-svelte-scaffold-')); - // The scaffolder resolves the compiler from the app root, so the staged app - // needs a package.json and a reachable svelte. Symlinking the repo's - // node_modules is what the static fixture sweep already does. fs.writeFileSync(path.join(scratch, 'package.json'), JSON.stringify({ name: 'app', type: 'module' })); - fs.symlinkSync(path.join(REPO_ROOT, 'node_modules'), path.join(scratch, 'node_modules'), 'dir'); + // Only `svelte` is linked, into a node_modules this workspace owns. + // Symlinking the whole directory pointed `node_modules/.impeccable-live` at + // the REPO's node_modules, so the scaffolder wrote its variants there: + // afterEach cleaned the temp dir and left them behind, the next case reused + // the session id, and a stale variant could be parsed against a fresh + // manifest. Svelte's own dependencies still resolve, because node follows + // the link to its real path before looking for them. + fs.mkdirSync(path.join(scratch, 'node_modules'), { recursive: true }); + fs.symlinkSync( + path.join(REPO_ROOT, 'node_modules', 'svelte'), + path.join(scratch, 'node_modules', 'svelte'), + 'dir', + ); }); afterEach(() => { @@ -138,10 +147,15 @@ describe('the real scaffolder writes variants that parse', () => { 'markup with a free expression': ['

{headline}

'], }; + let caseIndex = 0; for (const [label, originalLines] of Object.entries(CASES)) { it(label, () => { + // Distinct per case as well: an id shared across cases is only safe while + // the output directory is genuinely per-case, and that coupling is the + // kind that quietly breaks again. + const sessionId = `testsession${caseIndex++}`; const result = scaffoldSvelteComponentSession({ - id: 'testsession', + id: sessionId, count: 3, sourceFile: 'src/routes/+page.svelte', sourceStartLine: 1, @@ -151,7 +165,7 @@ describe('the real scaffolder writes variants that parse', () => { }); assert.equal(result.fallback, undefined, `scaffold fell back: ${result.reason}`); - const dir = path.join(scratch, 'node_modules', '.impeccable-live', 'testsession'); + const dir = path.join(scratch, 'node_modules', '.impeccable-live', sessionId); const variants = fs.readdirSync(dir).filter((name) => /^v\d+\.svelte$/.test(name)); assert.ok(variants.length > 0, 'scaffolder wrote no variant files');