From 85826662b3f7606c37cf70272882cec4b867e257 Mon Sep 17 00:00:00 2001 From: Vivek Date: Sat, 1 Aug 2026 23:46:55 +0530 Subject: [PATCH 1/3] fix(server): memoize facade action wrapping and refine export classification Refine export symbol classification in extractExportNames() to properly recognize non-function variables declared locally in export lists (export { a, b }). Memoize action function resolution in generated buildFacade() via module-scoped variables (_fn_), avoiding redundant __w() lookups and Proxy instantiations on repeated calls. Remove unused import in Bun circular re-export test. --- packages/server/src/action-seed.js | 63 ++++++++++++++++--- .../server/test/seed/action-seed-unit.test.js | 12 +++- test/bun/action-seed-circular.test.mjs | 1 - 3 files changed, 67 insertions(+), 9 deletions(-) diff --git a/packages/server/src/action-seed.js b/packages/server/src/action-seed.js index a9a3c5bc9..8b6243ca5 100644 --- a/packages/server/src/action-seed.js +++ b/packages/server/src/action-seed.js @@ -288,28 +288,75 @@ function seedProxy(file, fnName, orig) { export function extractExportNames(src) { const fnNames = new Set(); const valNames = new Set(); + + // Find local function declarations & function variable assignments + const localFns = new Set(); let m; + const reLocalFn = /\b(?:async\s+)?function\s*\*?\s+([A-Za-z_$][\w$]*)/g; + while ((m = reLocalFn.exec(src))) localFns.add(m[1]); + + const reLocalFnVar = /\b(?:const|let|var)\s+([A-Za-z_$][\w$]*)\s*=\s*(?:async\s*)?(?:function|\([^)]*\)\s*=>|[\w$]+\s*=>)/g; + while ((m = reLocalFnVar.exec(src))) localFns.add(m[1]); + + // Find direct function exports const reFn = /\bexport\s+(?:async\s+)?function\s*\*?\s+([A-Za-z_$][\w$]*)/g; while ((m = reFn.exec(src))) fnNames.add(m[1]); + const reFnVar = /\bexport\s+(?:const|let|var)\s+([A-Za-z_$][\w$]*)\s*=\s*(?:async\s*)?(?:function|\([^)]*\)\s*=>|[\w$]+\s*=>)/g; while ((m = reFnVar.exec(src))) fnNames.add(m[1]); + + // Find local non-function variables and classes + const localVals = new Set(); + const reLocalClass = /\b(?:abstract\s+)?class\s+([A-Za-z_$][\w$]*)/g; + while ((m = reLocalClass.exec(src))) localVals.add(m[1]); + + const reLocalVarStmt = /\b(?:const|let|var)\s+([^;\n]+)/g; + while ((m = reLocalVarStmt.exec(src))) { + const stmt = m[1]; + const idRe = /\b([A-Za-z_$][\w$]*)\s*(?:=|,|;|$)/g; + let idM; + while ((idM = idRe.exec(stmt))) { + const name = idM[1]; + if (name !== 'const' && name !== 'let' && name !== 'var' && name !== 'async' && name !== 'function') { + if (!localFns.has(name)) localVals.add(name); + } + } + } + + // Direct class exports const reClass = /\bexport\s+(?:abstract\s+)?class\s+([A-Za-z_$][\w$]*)/g; while ((m = reClass.exec(src))) valNames.add(m[1]); + + // Direct variable exports (not function assignments) + const reVar = /\bexport\s+(?:const|let|var)\s+([A-Za-z_$][\w$]*)/g; + while ((m = reVar.exec(src))) { + if (!fnNames.has(m[1])) valNames.add(m[1]); + } + + // Export list: export { a, b as bee } const reList = /\bexport\s*\{([^}]*)\}/g; while ((m = reList.exec(src))) { for (const part of m[1].split(',')) { const seg = part.trim(); if (!seg) continue; const as = seg.split(/\s+as\s+/); + const local = as[0].trim(); const exported = (as[1] || as[0]).trim(); - if (/^[A-Za-z_$][\w$]*$/.test(exported) && exported !== 'default') fnNames.add(exported); - else if (exported === 'default') fnNames.add('__default__'); + if (!/^[A-Za-z_$][\w$]*$/.test(exported)) continue; + + if (exported === 'default' || local === 'default') { + fnNames.add('__default__'); + } else if (localVals.has(local) && !localFns.has(local)) { + valNames.add(exported); + } else { + fnNames.add(exported); + } } } - const reVar = /\bexport\s+(?:const|let|var)\s+([A-Za-z_$][\w$]*)/g; - while ((m = reVar.exec(src))) { - if (!fnNames.has(m[1])) valNames.add(m[1]); - } + + // Clean up any overlap if an item ended up in both (fnNames wins if it's a function) + for (const fn of fnNames) valNames.delete(fn); + const hasDefault = /\bexport\s+default\b/.test(src) || fnNames.delete('__default__') || valNames.delete('__default__'); return { fnNames: [...fnNames], valNames: [...valNames], names: [...fnNames, ...valNames], hasDefault }; } @@ -332,8 +379,10 @@ function buildFacade(origUrl, absPath, exports) { out += `export * from ${origSpec};\n`; for (const n of exports.fnNames) { const k = JSON.stringify(n); + const v = `_fn_${n}`; + out += `let ${v};\n`; out += `export function ${n}(...args) {\n`; - out += ` const fn = __w(${file}, ${k}, __orig[${k}]);\n`; + out += ` const fn = ${v} || (${v} = __w(${file}, ${k}, __orig[${k}]));\n`; out += ` return typeof fn === 'function' ? fn.apply(this, args) : fn;\n`; out += `}\n`; out += `__w(${file}, ${k}, ${n});\n`; diff --git a/packages/server/test/seed/action-seed-unit.test.js b/packages/server/test/seed/action-seed-unit.test.js index cdb170a3a..04ca69dde 100644 --- a/packages/server/test/seed/action-seed-unit.test.js +++ b/packages/server/test/seed/action-seed-unit.test.js @@ -46,7 +46,7 @@ test('extractExportNames finds function / const / class / list / default exports export { a, b as bee }; export default function () {} `; - const { names, hasDefault } = extractExportNames(src); + const { fnNames, valNames, names, hasDefault } = extractExportNames(src); assert.ok(names.includes('getUser')); assert.ok(names.includes('getPosts')); assert.ok(names.includes('VERSION')); @@ -56,6 +56,16 @@ test('extractExportNames finds function / const / class / list / default exports assert.ok(names.includes('bee'), 'the EXPORTED name of `b as bee` is `bee`'); assert.ok(!names.includes('b'), 'the local name is not the exported binding'); assert.equal(hasDefault, true); + + assert.deepEqual(fnNames.sort(), ['getUser', 'getPosts'].sort()); + assert.deepEqual(valNames.sort(), ['VERSION', 'counter', 'Thing', 'a', 'bee'].sort()); +}); + +test('buildSeedFacade memoizes action wrap lookups and generates hoisted function facades', () => { + const src = `'use server';\nexport async function submitData(d) { return d; }\n`; + const facade = buildSeedFacade('file:///app/s.server.js', '/app/s.server.js', src); + assert.match(facade, /let _fn_submitData;/); + assert.match(facade, /const fn = _fn_submitData \|\| \(_fn_submitData = __w\(/); }); test('a star re-export is FACETED, not passed through (#1155)', () => { diff --git a/test/bun/action-seed-circular.test.mjs b/test/bun/action-seed-circular.test.mjs index 875d33655..61d3a1296 100644 --- a/test/bun/action-seed-circular.test.mjs +++ b/test/bun/action-seed-circular.test.mjs @@ -11,7 +11,6 @@ import { tmpdir } from 'node:os'; import { join } from 'node:path'; import { pathToFileURL } from 'node:url'; -import { installBunSeedPlugin } from '../../packages/server/src/action-seed-bun.js'; import { seedingEnabled, registerActionHooks } from '../../packages/server/src/action-seed.js'; test('circular re-export between use-server modules loads on Bun / Node (#1208)', async () => { From 441508019623efce768d8aaa9a4938e422a06f41 Mon Sep 17 00:00:00 2001 From: Vivek Date: Wed, 5 Aug 2026 14:28:59 +0530 Subject: [PATCH 2/3] fix(server): hoist the facade memo and harden export classification The memoization added for the action facade kept its per-function cache in a module-scope `let`. The exported wrapper is a hoisted function precisely so a circular re-export between two 'use server' modules can call it before the facade body has run (#1208), but a `let` is in TDZ at that moment, so such a call threw "Cannot access '_fn_x' before initialization" and took the whole module load down. That is the same failure #1208 was filed for, moved from one binding to another. `var` hoists initialized to undefined and keeps the memoization intact. The export classifier is reworked around which way it is safe to guess. The two buckets fail in opposite directions: a value emitted as a hoisted function reaches importers as a callable, while a function emitted as a const loses the hoisting the cycle depends on. So a name is demoted to a value only on positive evidence (a literal, object, array, `new`, tagged template, or class), and anything undecidable stays a function. Previously any identifier that merely appeared before an `=` or `,` anywhere in a declaration line became a value, which swept up call arguments and object members, and could demote a real action. Scanning now runs over the shared js-scan redaction, so a declaration written in a comment or quoted in a string can no longer register as real. Also: `export { default as X } from` no longer claims a default export the module does not have, which was fabricating `export default undefined`; and the memo variable name is chosen so it cannot collide with a real export, which would emit a duplicate declaration and crash the load with a SyntaxError the hook cannot catch, rather than degrading fail-open. --- packages/server/src/action-seed.js | 202 +++++++++++++----- .../server/test/seed/action-seed-unit.test.js | 84 +++++++- packages/server/test/seed/seed-hook.test.js | 87 +++++++- test/bun/action-seed-circular.test.mjs | 71 ++++++ 4 files changed, 392 insertions(+), 52 deletions(-) diff --git a/packages/server/src/action-seed.js b/packages/server/src/action-seed.js index 8b6243ca5..a913ed66c 100644 --- a/packages/server/src/action-seed.js +++ b/packages/server/src/action-seed.js @@ -68,6 +68,7 @@ import { stringify } from '@webjsdev/core'; import { hashFile } from './actions.js'; import { isStreamable } from './action-stream.js'; import { serverRuntime } from './listener-core.js'; +import { redactStringsAndTemplates } from './js-scan.js'; /** Ambient per-render seed collector. `Map` or undefined. */ const als = new AsyncLocalStorage(); @@ -275,67 +276,144 @@ function seedProxy(file, fnName, orig) { }); } +/** A declarator right-hand side that is unambiguously a function. */ +const RHS_FN_RE = /^(?:async\s+)?function\b|^(?:async\s*)?(?:<[^>]*>\s*)?\([^)]*\)\s*=>|^(?:async\s*)?[A-Za-z_$][\w$]*\s*=>/; /** - * Extract the names of every named export from an action module's source, used - * to generate the facade's `export const NAME = wrap(...)` lines. Conservative: - * a name it misses simply is not wrapped (no seed for it, RPC fallback). A - * `export *` re-export cannot be enumerated statically and is not reported - * here: the facade re-exports it wholesale through its own `export * from` - * catch-all (#538), so nothing about it needs a decision. + * A declarator right-hand side that is unambiguously NOT a function: a string / + * template / numeric literal, an object or array literal, a `new` expression, a + * tagged template, or a keyword literal. Runs on REDACTED source, so a literal + * body is blank but its delimiters survive, which is all this needs. + */ +const RHS_VAL_RE = /^['"`]|^[+-]?\d|^[{[]|^new\s|^(?:true|false|null|undefined)\s*[;,]?\s*$|^[A-Za-z_$][\w$]*\s*`/; + +/** + * Split a declaration statement into its declarators on TOP-LEVEL commas, so + * `const a = 1, b = f(x, y)` yields two parts rather than three. Depth-tracking + * is enough because the input is redacted (no string / template / regex body can + * carry an unbalanced bracket). + * @param {string} stmt + * @returns {string[]} + */ +function splitDeclarators(stmt) { + const parts = []; + let depth = 0; + let start = 0; + for (let i = 0; i < stmt.length; i++) { + const c = stmt[i]; + if (c === '(' || c === '[' || c === '{') depth++; + else if (c === ')' || c === ']' || c === '}') depth--; + else if (c === ',' && depth === 0) { + parts.push(stmt.slice(start, i)); + start = i + 1; + } + } + parts.push(stmt.slice(start)); + return parts; +} + +/** + * Extract the names of every named export from an action module's source, split + * into the ones that must be emitted as HOISTED function declarations and the + * ones that must stay plain value bindings. Conservative: a name it misses + * simply is not wrapped (no seed for it, RPC fallback). A `export *` re-export + * cannot be enumerated statically and is not reported here: the facade + * re-exports it wholesale through its own `export * from` catch-all (#538), so + * nothing about it needs a decision. + * + * ## Why the split matters, and which way to guess + * + * The two buckets get structurally different facade code, and each is wrong for + * the other's members in a DIFFERENT way, so the fallback direction is a real + * decision rather than a detail: + * + * - `fnNames` emits a hoisted `export function n(...)`. Hoisting is what makes + * a circular re-export between two `'use server'` modules load (#1208), but + * a VALUE emitted this way is silently handed to importers as a callable. + * - `valNames` emits `export const n = __w(...)`. Correct for any value, but a + * `const` is in TDZ until the facade body runs, so a FUNCTION emitted this + * way re-breaks the #1208 cycle. + * + * So a name is classified as a value only on POSITIVE evidence (a literal / + * object / array / `new` / tagged-template right-hand side, or a `class`), and + * everything undecidable falls back to `fnNames`. That keeps the cycle-critical + * cases safe: a name re-exported from another module (`export { helper }`, the + * #1208 fixture) is never locally declared at all, so it can never look like a + * value, and a higher-order-wrapped action (`const post = withAuth(...)`) has a + * call-expression right-hand side that is likewise undecidable. + * + * The residue is a list-exported `const x = someCall()` returning a non-function, + * which is still emitted as a function. That is unchanged from before the split + * existed (every list export used to land in `fnNames` unconditionally), so the + * classification is a strict improvement rather than a new trade. + * + * Scanning runs over a REDACTED copy (string / template / regex / comment bodies + * blanked by the shared `js-scan` lexer), so a `const` written in a doc comment + * or quoted in a string cannot register as a real declaration. + * * @param {string} src * @returns {{ fnNames: string[], valNames: string[], names: string[], hasDefault: boolean }} */ export function extractExportNames(src) { const fnNames = new Set(); const valNames = new Set(); + // Blank every literal body (`blankStrings`), so only code position is read. + const code = redactStringsAndTemplates(src, true); - // Find local function declarations & function variable assignments + // Locally declared functions: declarations, and declarators whose right-hand + // side is unambiguously a function. const localFns = new Set(); + // Locally declared non-functions, on POSITIVE evidence only (see above). + const localVals = new Set(); let m; + const reLocalFn = /\b(?:async\s+)?function\s*\*?\s+([A-Za-z_$][\w$]*)/g; - while ((m = reLocalFn.exec(src))) localFns.add(m[1]); + while ((m = reLocalFn.exec(code))) localFns.add(m[1]); - const reLocalFnVar = /\b(?:const|let|var)\s+([A-Za-z_$][\w$]*)\s*=\s*(?:async\s*)?(?:function|\([^)]*\)\s*=>|[\w$]+\s*=>)/g; - while ((m = reLocalFnVar.exec(src))) localFns.add(m[1]); + const reLocalClass = /\b(?:abstract\s+)?class\s+([A-Za-z_$][\w$]*)/g; + while ((m = reLocalClass.exec(code))) localVals.add(m[1]); + + const reDeclStmt = /\b(?:const|let|var)\s+([^;\n]+)/g; + while ((m = reDeclStmt.exec(code))) { + for (const part of splitDeclarators(m[1])) { + // `name = rhs`, tolerating a TS type annotation. A destructuring head or a + // bare `let x;` does not match and is left undecided on purpose. + const d = /^\s*([A-Za-z_$][\w$]*)\s*(?::[^=]*)?=\s*([\s\S]*)$/.exec(part); + if (!d) continue; + const [, name, rhs] = d; + const body = rhs.trim(); + if (RHS_FN_RE.test(body)) localFns.add(name); + else if (RHS_VAL_RE.test(body)) localVals.add(name); + // Otherwise undecided: left out of both, so a list export of it falls + // through to `fnNames` (hoisted, cycle-safe). + } + } - // Find direct function exports + // Direct function exports. const reFn = /\bexport\s+(?:async\s+)?function\s*\*?\s+([A-Za-z_$][\w$]*)/g; - while ((m = reFn.exec(src))) fnNames.add(m[1]); + while ((m = reFn.exec(code))) fnNames.add(m[1]); const reFnVar = /\bexport\s+(?:const|let|var)\s+([A-Za-z_$][\w$]*)\s*=\s*(?:async\s*)?(?:function|\([^)]*\)\s*=>|[\w$]+\s*=>)/g; - while ((m = reFnVar.exec(src))) fnNames.add(m[1]); - - // Find local non-function variables and classes - const localVals = new Set(); - const reLocalClass = /\b(?:abstract\s+)?class\s+([A-Za-z_$][\w$]*)/g; - while ((m = reLocalClass.exec(src))) localVals.add(m[1]); - - const reLocalVarStmt = /\b(?:const|let|var)\s+([^;\n]+)/g; - while ((m = reLocalVarStmt.exec(src))) { - const stmt = m[1]; - const idRe = /\b([A-Za-z_$][\w$]*)\s*(?:=|,|;|$)/g; - let idM; - while ((idM = idRe.exec(stmt))) { - const name = idM[1]; - if (name !== 'const' && name !== 'let' && name !== 'var' && name !== 'async' && name !== 'function') { - if (!localFns.has(name)) localVals.add(name); - } - } - } + while ((m = reFnVar.exec(code))) fnNames.add(m[1]); - // Direct class exports + // Direct class exports. const reClass = /\bexport\s+(?:abstract\s+)?class\s+([A-Za-z_$][\w$]*)/g; - while ((m = reClass.exec(src))) valNames.add(m[1]); + while ((m = reClass.exec(code))) valNames.add(m[1]); - // Direct variable exports (not function assignments) + // Direct variable exports that are not function assignments. const reVar = /\bexport\s+(?:const|let|var)\s+([A-Za-z_$][\w$]*)/g; - while ((m = reVar.exec(src))) { + while ((m = reVar.exec(code))) { if (!fnNames.has(m[1])) valNames.add(m[1]); } - // Export list: export { a, b as bee } + // Export list: `export { a, b as bee }`. const reList = /\bexport\s*\{([^}]*)\}/g; - while ((m = reList.exec(src))) { + while ((m = reList.exec(code))) { + // `export { default as X } from './other.js'` re-exports ANOTHER module's + // default under a named binding. It gives this module no default export, so + // it must not set `hasDefault` (that would fabricate `export default + // undefined` on a module that has none). `X` itself needs no entry here: it + // is a named export of the re-export target, which the facade's own + // `export * from` catch-all already carries. for (const part of m[1].split(',')) { const seg = part.trim(); if (!seg) continue; @@ -343,24 +421,44 @@ export function extractExportNames(src) { const local = as[0].trim(); const exported = (as[1] || as[0]).trim(); if (!/^[A-Za-z_$][\w$]*$/.test(exported)) continue; + if (local === 'default') continue; - if (exported === 'default' || local === 'default') { - fnNames.add('__default__'); - } else if (localVals.has(local) && !localFns.has(local)) { - valNames.add(exported); - } else { - fnNames.add(exported); - } + if (exported === 'default') fnNames.add('__default__'); + else if (localVals.has(local) && !localFns.has(local)) valNames.add(exported); + else fnNames.add(exported); } } - // Clean up any overlap if an item ended up in both (fnNames wins if it's a function) + // A name reaching both buckets (e.g. `export const x = () => {}` also seen as + // a plain variable export) is a function: the hoisted form is the safe one. for (const fn of fnNames) valNames.delete(fn); - const hasDefault = /\bexport\s+default\b/.test(src) || fnNames.delete('__default__') || valNames.delete('__default__'); + const hasDefault = /\bexport\s+default\b/.test(code) || fnNames.delete('__default__') || valNames.delete('__default__'); return { fnNames: [...fnNames], valNames: [...valNames], names: [...fnNames, ...valNames], hasDefault }; } +/** + * Pick a prefix for the facade's per-function memo variables that cannot collide + * with anything else the facade declares. + * + * The memo for export `n` is declared at module scope as `n`, so a module + * exporting both `ping` and `_fn_ping` would emit `var _fn_ping` alongside + * `export function _fn_ping`, a duplicate declaration. That is a SyntaxError in + * generated source, which the load hook's try/catch cannot contain (the parse + * happens after the hook returns), so it would crash the module load outright + * instead of degrading to no seeding. Pathological naming, but the feature's + * contract is that any failure is fail-open, and a hard crash is not. + * + * @param {{ fnNames: string[], valNames: string[] }} exports + * @returns {string} + */ +function memoPrefix(exports) { + const taken = new Set([...exports.fnNames, ...exports.valNames, '__orig', '__w']); + let prefix = '_fn_'; + while (exports.fnNames.some((n) => taken.has(prefix + n))) prefix = `_${prefix}`; + return prefix; +} + /** * Build the facade module source for a `'use server'` action module: it imports * the REAL module via a `?webjs-seed-orig` query (which the hook passes through @@ -374,13 +472,21 @@ function buildFacade(origUrl, absPath, exports) { const sep = origUrl.includes('?') ? '&' : '?'; const origSpec = JSON.stringify(origUrl + sep + 'webjs-seed-orig'); const file = JSON.stringify(absPath); + const memo = memoPrefix(exports); let out = `import * as __orig from ${origSpec};\n`; out += `import { __actionWrap as __w } from ${JSON.stringify(SELF_URL)};\n`; out += `export * from ${origSpec};\n`; for (const n of exports.fnNames) { const k = JSON.stringify(n); - const v = `_fn_${n}`; - out += `let ${v};\n`; + const v = `${memo}${n}`; + // `var`, NOT `let`. The exported function is hoisted, which is the whole + // reason a circular re-export between two `'use server'` modules loads + // (#1208): the other module in the cycle can call it before this facade's + // body has run. A `let` memo would be in TDZ at that moment, so the call + // would throw `Cannot access '...' before initialization`, re-breaking the + // very cycle the hoisting exists to survive. `var` hoists initialized to + // undefined, so the first call falls through to the lookup as intended. + out += `var ${v};\n`; out += `export function ${n}(...args) {\n`; out += ` const fn = ${v} || (${v} = __w(${file}, ${k}, __orig[${k}]));\n`; out += ` return typeof fn === 'function' ? fn.apply(this, args) : fn;\n`; diff --git a/packages/server/test/seed/action-seed-unit.test.js b/packages/server/test/seed/action-seed-unit.test.js index 04ca69dde..934907e90 100644 --- a/packages/server/test/seed/action-seed-unit.test.js +++ b/packages/server/test/seed/action-seed-unit.test.js @@ -61,11 +61,89 @@ test('extractExportNames finds function / const / class / list / default exports assert.deepEqual(valNames.sort(), ['VERSION', 'counter', 'Thing', 'a', 'bee'].sort()); }); -test('buildSeedFacade memoizes action wrap lookups and generates hoisted function facades', () => { +test('buildSeedFacade memoizes the action wrap behind a HOISTED (var) memo', () => { const src = `'use server';\nexport async function submitData(d) { return d; }\n`; const facade = buildSeedFacade('file:///app/s.server.js', '/app/s.server.js', src); - assert.match(facade, /let _fn_submitData;/); - assert.match(facade, /const fn = _fn_submitData \|\| \(_fn_submitData = __w\(/); + assert.match(facade, /const fn = _fn_submitData \|\| \(_fn_submitData = __w\(/, 'the wrap is memoized, not redone per call'); + // `var`, not `let`. The exported function is hoisted so a circular + // `'use server'` pair can call it before this facade's body runs (#1208); a + // `let` memo would be in TDZ at that moment and throw. The runtime proof is + // in seed-hook.test.js, but pin the emitted keyword here too, because this is + // the line that silently re-breaks the cycle if someone "modernises" it. + assert.match(facade, /var _fn_submitData;/); + assert.doesNotMatch(facade, /let _fn_submitData;/); +}); + +test('a memo variable never collides with a real export name', () => { + // A module exporting both `ping` and `_fn_ping` would emit `var _fn_ping` + // beside `export function _fn_ping`: a duplicate declaration, i.e. a + // SyntaxError in generated source. The load hook's try/catch cannot contain + // that (the parse happens after the hook returns), so it would be a hard + // crash rather than the fail-open degradation the feature promises. + const src = `'use server';\nexport async function ping() {}\nexport async function _fn_ping() {}\n`; + const facade = buildSeedFacade('file:///app/y.server.js', '/app/y.server.js', src); + const declared = [...facade.matchAll(/^(?:var|export function) ([A-Za-z_$][\w$]*)/gm)].map((m) => m[1]); + assert.equal(new Set(declared).size, declared.length, `no duplicate declaration, got: ${declared.join(', ')}`); +}); + +test('export classification: value only on positive evidence, undecidable falls back to a function', () => { + // The two buckets fail in opposite directions, so the fallback direction is + // the decision: a VALUE emitted as a function is handed to importers as a + // callable, while a FUNCTION emitted as a const loses the hoisting that makes + // a circular re-export load (#1208). Only positive value evidence (literal / + // object / `new` / class) may demote a name. + const val = extractExportNames( + `'use server';\nconst VERSION = '1.0';\nconst cache = new Map();\nconst cfg = { a: 1 };\nexport { VERSION, cache, cfg };\n`, + ); + assert.deepEqual(val.fnNames, [], 'literal / new / object-literal consts are values, not callables'); + assert.deepEqual(val.valNames.sort(), ['VERSION', 'cache', 'cfg']); + + // A higher-order-wrapped action has a call-expression right-hand side, which + // is undecidable, so it must stay in the hoisted bucket. + const hof = extractExportNames( + `'use server';\nconst createPost = withAuth(async (input) => input);\nexport { createPost };\n`, + ); + assert.deepEqual(hof.fnNames, ['createPost']); + assert.deepEqual(hof.valNames, []); + + // A name re-exported from ANOTHER module is never locally declared, so it can + // never look like a value. This is the #1208 shape and must stay hoisted. + const reexport = extractExportNames( + `'use server';\nexport { helper } from './c2.server.js';\nexport async function ring(x) { return x + 1; }\n`, + ); + assert.deepEqual(reexport.fnNames.sort(), ['helper', 'ring']); + assert.deepEqual(reexport.valNames, []); +}); + +test('extraction reads code position only, not comments or strings', () => { + // The scan runs over a redacted copy, so a declaration written in prose + // cannot demote a real exported function to a value binding. + const src = + `'use server';\n` + + `// const submitOrder = 'not a real declaration';\n` + + `const note = 'const submitOrder = 1';\n` + + `export async function submitOrder(o) { return o; }\n` + + `export { submitOrder as submit };\n`; + const { fnNames, valNames } = extractExportNames(src); + assert.ok(fnNames.includes('submit'), 'the commented-out const must not demote the export'); + assert.ok(!valNames.includes('submit')); +}); + +test('`export { default as X } from` does not fabricate a default export', () => { + // It re-exports ANOTHER module's default under a named binding, so this + // module has no default of its own. Claiming one makes the facade emit + // `export default __w(..., __orig.default)`, i.e. `export default undefined`, + // turning what was a loud link-time error for importers into a silent one. + const { hasDefault } = extractExportNames( + `'use server';\nexport { default as Helper } from './h.server.js';\nexport async function go() {}\n`, + ); + assert.equal(hasDefault, false); + const facade = buildSeedFacade( + 'file:///app/x.server.js', + '/app/x.server.js', + `'use server';\nexport { default as Helper } from './h.server.js';\nexport async function go() {}\n`, + ); + assert.doesNotMatch(facade, /export default/); }); test('a star re-export is FACETED, not passed through (#1155)', () => { diff --git a/packages/server/test/seed/seed-hook.test.js b/packages/server/test/seed/seed-hook.test.js index bf344d125..a7966c03c 100644 --- a/packages/server/test/seed/seed-hook.test.js +++ b/packages/server/test/seed/seed-hook.test.js @@ -21,7 +21,7 @@ import { hashFile } from '../../src/actions.js'; import { stringify } from '@webjsdev/core'; let dir; -let actionUrl, utilUrl, exoticUrl, c1Url, c2Url; +let actionUrl, utilUrl, exoticUrl, c1Url, c2Url, eagerUrl, eager2Url, collideUrl, constsUrl; before(async () => { dir = mkdtempSync(join(tmpdir(), 'webjs-seedhook-')); @@ -67,6 +67,53 @@ before(async () => { c1Url = pathToFileURL(c1).toString(); c2Url = pathToFileURL(c2).toString(); + // A circular pair where one module CALLS the other during its own module-body + // evaluation, i.e. while the callee's facade body has not run yet. Only the + // hoisted binding exists at that moment, so anything the exported function + // reads from facade module scope must be hoisted too. + const e1 = join(dir, 'e1.server.js'); + const e2 = join(dir, 'e2.server.js'); + writeFileSync( + e1, + `'use server';\n` + + `export { helper } from './e2.server.js';\n` + + `export async function ring(x) { return x + 1; }\n`, + ); + writeFileSync( + e2, + `'use server';\n` + + `import { ring } from './e1.server.js';\n` + + `export const EAGER = ring(1);\n` + + `export async function helper(x) { return x * 2; }\n`, + ); + eagerUrl = pathToFileURL(e1).toString(); + eager2Url = pathToFileURL(e2).toString(); + + // A module exporting a name that collides with the facade's memo-variable + // naming scheme. A collision is a duplicate declaration in generated source, + // which is a SyntaxError the load hook cannot catch. + const collide = join(dir, 'collide.server.js'); + writeFileSync( + collide, + `'use server';\n` + + `export async function ping() { return 1; }\n` + + `export async function _fn_ping() { return 2; }\n`, + ); + collideUrl = pathToFileURL(collide).toString(); + + // A `'use server'` module whose non-function exports leave via an export LIST + // rather than an inline `export const`. + const consts = join(dir, 'consts.server.js'); + writeFileSync( + consts, + `'use server';\n` + + `const VERSION = '2.0';\n` + + `const LIMITS = { max: 10 };\n` + + `async function fetchThing(id) { return id; }\n` + + `export { VERSION, LIMITS, fetchThing };\n`, + ); + constsUrl = pathToFileURL(consts).toString(); + // Install the global hook BEFORE importing the fixtures (ESM caches by URL). await registerActionHooks({ seed: true }); }); @@ -122,3 +169,41 @@ test('circular re-export between two use-server modules loads without throwing ( assert.equal(await mod1.helper(5), 10); }); +test('a circular action CALLED during module evaluation loads without throwing (#1208)', async () => { + // The stricter half of #1208. The test above imports the cycle and calls + // afterwards, by which point every facade body has run, so it passes even if + // the facade keeps per-function state in a `let`. Here `e2` calls back into + // `e1.ring()` while `e1`'s facade body is still suspended on its own import, + // so ONLY the hoisted function declaration exists. Any facade module-scope + // binding the function body reads must therefore be hoisted as well: a `let` + // memo throws `Cannot access '_fn_ring' before initialization` here and turns + // the whole module load into a ReferenceError, which is precisely the failure + // #1208 was filed for. + const mod = await import(eagerUrl); + assert.equal(await mod.ring(5), 6, 'the action still works after the cycle settles'); + // `EAGER` holds whatever the module-body call to `ring(1)` produced, so it is + // the proof the call actually went through the facade rather than throwing. + const mod2 = await import(eager2Url); + assert.equal(await mod2.EAGER, 2, 'the load-time call resolved through the facade'); +}); + +test('an export colliding with the memo naming scheme still loads (fail-open, no SyntaxError)', async () => { + const mod = await import(collideUrl); + assert.equal(await mod.ping(), 1); + assert.equal(await mod._fn_ping(), 2); +}); + +test('a list-exported const stays a value, and a list-exported function stays callable', async () => { + // The classification bug this pins: a non-function exported via `export { ... }` + // used to land in the function bucket, so the facade emitted + // `export function VERSION(...)` and importers received a callable instead of + // the string. The sibling function in the same list must keep working. + const mod = await import(constsUrl); + assert.equal(mod.VERSION, '2.0', 'a list-exported string is a string, not a function'); + assert.deepEqual(mod.LIMITS, { max: 10 }, 'a list-exported object is the object'); + assert.equal(typeof mod.fetchThing, 'function'); + const { value, collector } = await collectSeeds(async () => mod.fetchThing(7)); + assert.equal(value, 7); + assert.equal(collector.size, 1, 'the list-exported action is still faceted and seeds'); +}); + diff --git a/test/bun/action-seed-circular.test.mjs b/test/bun/action-seed-circular.test.mjs index 61d3a1296..940eb0037 100644 --- a/test/bun/action-seed-circular.test.mjs +++ b/test/bun/action-seed-circular.test.mjs @@ -3,6 +3,14 @@ * * Proves that two 'use server' modules that re-export from each other load * without throwing ReferenceError on Bun as well as Node. + * + * The facade SOURCE is runtime-neutral (Bun and Node both call + * `buildSeedFacade`), so a hoisting or export-classification mistake in it is a + * cross-runtime bug. Only the INSTALL differs: Bun goes through a `Bun.plugin` + * `onLoad`, which must return contents for EVERY filter match, so a facade + * change that throws degrades differently there than under Node's `nextLoad`. + * That is why the classification and load-time-call cases are asserted on Bun + * too rather than trusted to the Node suite. */ import { test } from 'node:test'; import assert from 'node:assert/strict'; @@ -43,3 +51,66 @@ test('circular re-export between use-server modules loads on Bun / Node (#1208)' rmSync(dir, { recursive: true, force: true }); } }); + +test('a circular action CALLED during module evaluation loads on Bun / Node (#1208)', async () => { + // The stricter half: `e2` calls back into `e1.ring()` while `e1`'s facade body + // is still suspended on its own import, so only the hoisted function + // declaration exists. Any facade module-scope binding the function body reads + // must be hoisted too, or the whole load is a ReferenceError. + const dir = mkdtempSync(join(tmpdir(), 'webjs-bun-eager-')); + try { + const e1 = join(dir, 'e1.server.js'); + const e2 = join(dir, 'e2.server.js'); + writeFileSync( + e1, + `'use server';\n` + + `export { helper } from './e2.server.js';\n` + + `export async function ring(x) { return x + 1; }\n`, + ); + writeFileSync( + e2, + `'use server';\n` + + `import { ring } from './e1.server.js';\n` + + `export const EAGER = ring(1);\n` + + `export async function helper(x) { return x * 2; }\n`, + ); + + if (!seedingEnabled()) await registerActionHooks({ seed: true }); + + const mod1 = await import(pathToFileURL(e1).toString()); + assert.equal(await mod1.ring(5), 6); + const mod2 = await import(pathToFileURL(e2).toString()); + assert.equal(await mod2.EAGER, 2, 'the load-time call resolved through the facade'); + } finally { + rmSync(dir, { recursive: true, force: true }); + } +}); + +test('list-exported values stay values on Bun / Node', async () => { + // The facade classifies each export into a hoisted function or a plain value + // binding. A value misclassified as a function reaches importers as a + // callable, which is a wrong-data bug rather than a crash, so it needs a + // runtime assertion on both runtimes. + const dir = mkdtempSync(join(tmpdir(), 'webjs-bun-consts-')); + try { + const f = join(dir, 'consts.server.js'); + writeFileSync( + f, + `'use server';\n` + + `const VERSION = '2.0';\n` + + `const LIMITS = { max: 10 };\n` + + `async function fetchThing(id) { return id; }\n` + + `export { VERSION, LIMITS, fetchThing };\n`, + ); + + if (!seedingEnabled()) await registerActionHooks({ seed: true }); + + const mod = await import(pathToFileURL(f).toString()); + assert.equal(mod.VERSION, '2.0', 'a list-exported string is a string, not a function'); + assert.deepEqual(mod.LIMITS, { max: 10 }); + assert.equal(typeof mod.fetchThing, 'function'); + assert.equal(await mod.fetchThing(7), 7); + } finally { + rmSync(dir, { recursive: true, force: true }); + } +}); From 78d15a4e4c4fd4944e0cdc472a0e177cc5928807 Mon Sep 17 00:00:00 2001 From: Vivek Date: Wed, 5 Aug 2026 15:03:51 +0530 Subject: [PATCH 3/3] fix(server): hoist a type-annotated export const action The direct `export const` path recognised only a bare `function` or arrow right-hand side, so `export const create: Handler = async (i) => ...`, an ordinary shape, fell to the value bucket and lost the hoisting a circular import depends on (#1208). It now consults the declarator classifier, which tolerates the annotation. The promotion needs positive function evidence, so a genuine value export is untouched. Also corrects the classification docstring, which claimed a higher-order-wrapped `export const` landed in the function bucket. It does not, and never did; the residues in both directions are now stated accurately instead. --- packages/server/src/action-seed.js | 44 +++++++++++++------ .../server/test/seed/action-seed-unit.test.js | 18 ++++++++ 2 files changed, 49 insertions(+), 13 deletions(-) diff --git a/packages/server/src/action-seed.js b/packages/server/src/action-seed.js index a913ed66c..4e68122b1 100644 --- a/packages/server/src/action-seed.js +++ b/packages/server/src/action-seed.js @@ -333,18 +333,27 @@ function splitDeclarators(stmt) { * `const` is in TDZ until the facade body runs, so a FUNCTION emitted this * way re-breaks the #1208 cycle. * - * So a name is classified as a value only on POSITIVE evidence (a literal / - * object / array / `new` / tagged-template right-hand side, or a `class`), and - * everything undecidable falls back to `fnNames`. That keeps the cycle-critical - * cases safe: a name re-exported from another module (`export { helper }`, the - * #1208 fixture) is never locally declared at all, so it can never look like a - * value, and a higher-order-wrapped action (`const post = withAuth(...)`) has a - * call-expression right-hand side that is likewise undecidable. + * The two export FORMS are therefore defaulted in opposite directions, because + * each starts from a different prior: * - * The residue is a list-exported `const x = someCall()` returning a non-function, - * which is still emitted as a function. That is unchanged from before the split - * existed (every list export used to land in `fnNames` unconditionally), so the - * classification is a strict improvement rather than a new trade. + * - An `export { ... }` LIST is the cycle-critical form: its local may not be + * declared in this file at all (`export { helper } from './c2.server.js'` is + * the #1208 fixture). So a listed name is demoted to a value only on POSITIVE + * evidence (a literal / object / array / `new` / tagged-template right-hand + * side, or a `class`), and anything undecidable stays a function. + * - A direct `export const` always HAS its initializer right here, and is much + * more often a genuine value, so it defaults to `valNames` and is promoted to + * `fnNames` only on positive function evidence. + * + * Both residues are wrong in the value-as-function direction and both PREDATE + * this split, so it is a strict improvement rather than a new trade, but neither + * is eliminated. A list-exported name whose right-hand side is computed rather + * than literal (`const limit = Number(env.L)`, `const x = cond ? a : b`, + * `const x = someCall()`, or a value whose TS annotation contains a generic + * comma, which splits the declarator) is still emitted as a function. And a + * direct `export const` whose right-hand side is a call (`export const post = + * withAuth(...)`) is still emitted as a `const`, so it keeps the TDZ exposure in + * a circular import that every direct value export has always had. * * Scanning runs over a REDACTED copy (string / template / regex / comment bodies * blanked by the shared `js-scan` lexer), so a `const` written in a doc comment @@ -399,10 +408,19 @@ export function extractExportNames(src) { const reClass = /\bexport\s+(?:abstract\s+)?class\s+([A-Za-z_$][\w$]*)/g; while ((m = reClass.exec(code))) valNames.add(m[1]); - // Direct variable exports that are not function assignments. + // Direct variable exports that are not function assignments. `reFnVar` above + // only sees a bare `function` / arrow right-hand side, so consult `localFns` + // too: it ran the declarator through `RHS_FN_RE`, which tolerates a TS type + // annotation, and so recognises the very common + // `export const create: Handler = async (i) => ...`. This only ever PROMOTES + // a name to the hoisted bucket on positive function evidence; the default for + // a direct `export const` stays the value binding. const reVar = /\bexport\s+(?:const|let|var)\s+([A-Za-z_$][\w$]*)/g; while ((m = reVar.exec(code))) { - if (!fnNames.has(m[1])) valNames.add(m[1]); + const n = m[1]; + if (fnNames.has(n)) continue; + if (localFns.has(n)) fnNames.add(n); + else valNames.add(n); } // Export list: `export { a, b as bee }`. diff --git a/packages/server/test/seed/action-seed-unit.test.js b/packages/server/test/seed/action-seed-unit.test.js index 934907e90..54520bd2e 100644 --- a/packages/server/test/seed/action-seed-unit.test.js +++ b/packages/server/test/seed/action-seed-unit.test.js @@ -115,6 +115,24 @@ test('export classification: value only on positive evidence, undecidable falls assert.deepEqual(reexport.valNames, []); }); +test('a TS-annotated direct export const arrow is a function, not a value', () => { + // `export const create: Handler = async (i) => ...` is an ordinary shape, and + // the direct-export regex only recognises a BARE `function` / arrow right-hand + // side, so the annotation used to push it into the value bucket and cost it + // the hoisting a circular import needs (#1208). + const { fnNames, valNames } = extractExportNames( + `'use server';\nexport const createTodo: Handler = async (i) => i;\n`, + ); + assert.deepEqual(fnNames, ['createTodo']); + assert.deepEqual(valNames, []); + + // The promotion is on positive function evidence only: a genuine value export + // must not be dragged along with it. + const plain = extractExportNames(`'use server';\nexport const VERSION = '1.0';\nexport const CFG = { a: 1 };\n`); + assert.deepEqual(plain.fnNames, []); + assert.deepEqual(plain.valNames.sort(), ['CFG', 'VERSION']); +}); + test('extraction reads code position only, not comments or strings', () => { // The scan runs over a redacted copy, so a declaration written in prose // cannot demote a real exported function to a value binding.