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..7481b8730 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; +// 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'; + 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..3ace60c70 --- /dev/null +++ b/tests/live-svelte-props-script.test.mjs @@ -0,0 +1,177 @@ +/** + * 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-')); + fs.writeFileSync(path.join(scratch, 'package.json'), JSON.stringify({ name: 'app', type: 'module' })); + // 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(() => { + fs.rmSync(scratch, { recursive: true, force: true }); + }); + + const CASES = { + 'static markup (the #580 shape)': ['

Vite 8 + SvelteKit Fixture

'], + '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: sessionId, + 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', sessionId); + 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}`); + } + }); + } +});