ReviewOS

stacks/stx

Bridge drops an explicitly declared payload name when a nested scope reuses it: runtime ReferenceError, no diagnostic

#1953
Closed glennmichael123 opened this 10 days ago ·
10 days ago

Summary

defineClientPayload({ range }) is dropped from a <script client> block if any nested scope in that block declares a local named range. The bridge emits nothing for the name, the compiler and stx typecheck say nothing, and the first top-level use throws ReferenceError: range is not defined at runtime.

The skip itself is deliberate for a top-level re-declaration, where emitting var range beside const range would be a duplicate-binding SyntaxError; test/client-script/declared-payload.test.ts pins exactly that case ("still refuses to clobber a name the client declares itself"). But declaresClientIdentifier (packages/stx/src/client-script.ts:871 on main) decides ownership with a textual regex:

if (new RegExp(`(?:const|let|var|function|class)\\s+${name}\\b`).test(code))
  return true

so a declaration three scopes deep counts the same as one at the top. For a nested declaration, emitting var range at the top is perfectly legal (the local merely shadows it inside its own block), and withholding it is what breaks the page.

Reproduction on 0.2.286

Same helper shape as declared-payload.test.ts:

import { extractBridgeData, generateServerDataBridge } from '@stacksjs/stx'

const version = JSON.parse(readFileSync('node_modules/@stacksjs/stx/package.json', 'utf8')).version
// The exact shape upstream's declared-payload.test.ts uses.
const bridge = (ctx: Record<string, unknown>, code: string) => generateServerDataBridge(code, extractBridgeData(ctx))
const ctx = { range: '30d', __stxClientPayload: { range: '30d' } }

const cases: Record<string, string> = {
  'A  control: range only used':
    'console.log(range)',
  'B  top-level const range (skip is intentional, tested upstream)':
    'const range = "7d"\nconsole.log(range)',
  'C  nested const range inside a function, top-level use':
    'function later() {\n  const range = "7d"\n  return range\n}\nconsole.log(range)',
  'D  nested const range inside an else-if, top-level use (analyticshq dashboard shape)':
    'if (first.ok) {\n}\nelse if (first.status === 413) {\n  const range = first.data.range\n}\nnavigate(`?range=${encodeURIComponent(range)}`)',
}

Output:

@stacksjs/stx 0.2.286

A  control: range only used
   bridge emits: var range = "30d";
B  top-level const range (skip is intentional, tested upstream)
   bridge emits: (nothing)
C  nested const range inside a function, top-level use
   bridge emits: (nothing)
D  nested const range inside an else-if, top-level use (analyticshq dashboard shape)
   bridge emits: (nothing)

runtime, bridge output + client code executed together:
   A: returned "30d"
   C: ReferenceError: range is not defined

Cases C and D should emit var range = "30d"; like A does. B is the only case where withholding is correct.

How it was found

A real app: analyticshq's dashboard declares defineClientPayload({ …, range, … }) and its switchSite handler reads range at the top of the client block. A later feature added const range = first.data.range inside an else if two functions away in the same block. Every site switch in production then threw ReferenceError: range is not defined, with nothing at build time to point at it. The rendered page made the mechanism visible: the compiled <script client> block received six of the seven declared names, while a sibling <script type="module"> block on the same page, which declares nothing, received all seven including var range. Fixed app-side by renaming the local (stacksjs/analyticshq#54), plus a unit test that greps each view's client block for re-declarations of its payload names, which is the check the compiler is doing but without telling anyone.

Suggested fix

  1. Make the ownership check scope-aware: only a depth-0 declaration (brace depth zero in the comment/literal-stripped source, which stripCommentsAndLiterals already provides for the auto-import scan at ~line 394) should suppress emission. Nested declarations should not.
  2. For a name that is both explicitly declared in defineClientPayload and declared at depth 0 by the client, report it. The page asked for the value and then made it unreachable; that is a conflict worth a diagnostic, not a silent no-op. The typecheck warning path added for #1868 ("N values reach this block on the implicit bridge…") is a natural home.
  3. Add the nested case to declared-payload.test.ts beside the top-level one, so the two behaviours are pinned as a pair.
  • #1868 introduced defineClientPayload so a page could state what crosses; this is a way a declared name still fails to arrive.
  • #1831 is the mirror image (a name crossing when it should not); this is a name not crossing when it should.
  • #1908 covers accuracy of the implicit-bridge warning; the diagnostic suggested in (2) would live next to it.
10 days ago

Fixed in 700a219cc7.

declaresClientIdentifier now strips comments and literals, then requires bracket depth 0. Parens and square brackets count as nesting too, so for (const range of rows) does not claim the top-level name either.

With { range } declared:

client codebeforenow
const range = "7d" (top level)skipsskips — correct, would be a duplicate binding
const { range } = opts (top level)skipsskips
function range() {} (top level)skipsskips
function later() { const range = … }skipsemits
else if (…) { const range = … } — the shape you hitskipsemits
for (const range of rows)skipsemits
console.log("const range = 1")skipsemits

One detail worth recording: depth has to be read at the keyword, not at the regex match start. Measured at the match start, the loop-head case stayed broken — the character the word boundary consumed was the ( whose own depth is still 0, so the fix looked right while the case it was written for still failed.

Suggestion (3) is done: the nested cases sit beside the top-level one in declared-payload.test.ts, and both fail against the previous implementation.

Suggestion (2) is done differently from what you proposed. A declared name that IS re-declared at depth 0 now emits a console.warn from the bridge naming the conflict, rather than going through the #1868 typecheck warning path. That path is still the better long-term home — it reports at build time rather than render time — so if you want it moved there, it is a small follow-up.

🤖 Generated with Claude Code

Sign in to comment on this issue.