ReviewOS

stacks/stx

A third drifted signals gate in events.ts strips @click from useStore-only pages, and the imperative fallback it hands off to cannot resolve the handler (ReferenceError)

#1824
Closed glennmichael123 opened this 10 days ago · {{ comments.length }} {{ comments.length === 1 ? 'comment' : 'comments' }}
10 days ago

Follow-up to #1819 / #1820. Those fixed two hand-maintained "does this template use signals" tests by deriving them from STX_RUNTIME_GLOBALS. There is a third copy, in events.ts, which was not covered — and the path it falls through to cannot resolve a named handler from a page-level client block, so the handler throws ReferenceError on click.

Net effect after the #1820 fix: a page whose client block uses useStore (or any reactive API other than state/derived/effect) now correctly gets the signals runtime, and then has all of its @click handlers stripped and rebound to code that cannot find them. The symptom moved from TypeError: …reading 'mount' to ReferenceError: toggleTheme is not defined.

1. The third gate

packages/stx/src/events.ts:598:

// When signals are used, skip event processing - the signals runtime handles events
const usesSignals = /\b(?:state|derived|effect)\s*(?:<[^<>()]*>)?\s*\(/.test(template)
  || /@(?:model|show|for|if)\s*=/.test(template)
  || /data-stx(?:-auto)?(?![-\w])/.test(template)

if (usesSignals) {
  return template
}

Three names out of 71. When it returns early, @click="toggleTheme" survives into the HTML and the signals runtime binds it declaratively — which works. When it does not, the template goes down the stripping path and the attribute is replaced by an imperative binding.

Two pages from the same app, same framework build:

client block@ attr in HTML__stx_evt_result
/loginstate('')@submit="handleSubmit" survives0works
/projectsuseStore('theme') onlystripped4ReferenceError

The only difference is which reactive API the block happens to call.

2. The path it falls through to cannot resolve the handler

client-script.ts:960-970 (and the parallel block at 1010-1020) emits:

var $el = document.getElementById('__stx_evt_0')
if ($el) $el.addEventListener('click', function($event) {
  var scope = $el.closest('[data-stx-scope]');
  if (scope && scope.__stx_execute) {
    scope.__stx_execute('toggleTheme', $event, $el);
  }
  else {
    toggleTheme          // bare identifier
  }
})

Both branches fail for a page-level <script client>:

  • [data-stx-scope] is not emitted for it. Grepping the emitters, that attribute is produced only by includes.ts:372 (for @include partials) and by runtime scope remapping (signals.ts:3065, 3159, 3172). A page client block registers on <body data-stx="__stx_setup_…"> instead — a different attribute. Measured on the page above: document.querySelectorAll('[data-stx-scope]').length === 0, while [data-stx] matches 1 (the body).
  • __stx_execute is assigned in exactly one place — reactive.ts:644, on an x-data scope element. document.body.__stx_execute is undefined.
  • So the else branch runs, and it is a bare identifier. toggleTheme is defined inside the function __stx_setup_…() closure the client block compiles to, not as a global, so evaluating the name throws.
ReferenceError: toggleTheme is not defined
    at HTMLButtonElement.<anonymous> (http://localhost:3100/projects:203:7)

The else branch also just evaluates the identifier rather than calling it — even if the name resolved, toggleTheme on its own is a no-op expression, not an invocation. For the @click="doThing()" call form that is fine; for the @click="doThing" reference form it would silently do nothing.

Repro

<script client>
const themeStore = useStore('theme')
function toggleTheme(): void { themeStore.toggle() }
</script>

<button @click="toggleTheme">Toggle</button>

Click the button: ReferenceError: toggleTheme is not defined.

Change the block's first line to const x = state(0) and it works — because that trips the regex in (1) and the event is never stripped.

Suggested direction

For (1): derive it from STX_RUNTIME_GLOBALS like #1819 and #1820 now do. This is the third instance of the same drift, which suggests the shared helper those two fixes introduced should simply be reused here rather than a fourth list maintained.

For (2): the imperative fallback is worth a second look regardless of (1), since it is reachable whenever a template genuinely has no signals. Either it should look for the attribute that page-level client scopes actually carry ([data-stx]) and call through window.__stx_latestSetup, or — probably better — the setup closure should expose its named handlers on the scope element the same way reactive.ts does, so both binding systems resolve handlers through one mechanism instead of two that disagree.

A test that clicks a @click="namedHandler" button on a page whose client block uses only useStore would cover both halves. The existing tests presumably all use state(), which is exactly why this survived.

  • #1820 — the gate in signal-processing.ts; fixing it is what exposed this.
  • #1819 — the gate in client-script.ts.
  • #1804 — the fix that established deriving these lists from the runtime.
9 days ago

Part 2 done in 92527f1 — the imperative fallback, which 88c5da0 deliberately left for a second look.

Two defects there, both silent, and the second one was not on the list.

1. The reference form was never invoked

As the report predicted: @click="doThing" compiled to else { doThing } — the identifier is evaluated and discarded. The declarative runtime learned this in #1695; the imperative paths never did. So which behaviour a page got depended on which binding path it took, which depended on which reactive API its client block happened to call:

client block          binding path    @click="doThing"
state()               declarative     works  (#1695)
plain functions only  imperative      dead   (silent)

Three emitters had it — client-script.ts's inline else, __stx_runHandler, and __stx_execute in the x-data bridge. All three receive the handler as text that is already known at emit time, so one normalisation upstream covers all three and none of the generated runtimes had to be touched:

if (typeof doThing === 'function' && !doThing._isSignal) doThing($event);

Restricted to a plain identifier and excluding signals, matching signals.ts exactly. The _isSignal part matters: signals are functions, so without it @click="count" would start reading the signal instead of doing nothing.

Incidentally the report's ReferenceError diagnosis was right for the pre-#1820 world but no longer reproduces — the emitted binding now sits inside the mount closure (return { doThing } follows it), so the bare name does resolve. What was left was purely the no-op.

2. The generated event script ended itself, in a comment

Found while writing an execution test — document.write refused to parse the output. The script carried a raw </script>, inside a comment, as an example of what must never appear in it:

// the surrounding <script> can never contain raw markup (e.g. </script>); we

A browser ends the element at the first closing tag it sees. Measured on real output: the element terminated 723 characters in, and the 1206 characters after it — __stx_runHandler and every event listener — were emitted as page text. So the standalone imperative path could not work in a browser at all, and printed its own source onto the page.

Invisible from the server: the string is valid JavaScript, the existing tests parsed it happily, and only an HTML parser disagrees. The comment about escaping was the thing that needed escaping. There is now a guard asserting the generated script contains exactly one closing tag.

Tests

packages/stx/test/directives/event-reference-form.test.ts — these execute the emitted code and click the button rather than asserting on its text. That is the point: the bug was code that parsed fine and did nothing, which no string assertion can see. It is also why the existing suite missed it — every fixture used the call form doThing(), for which evaluating and invoking are the same thing.

Both defects verified present at HEAD (2 closing tags; handler emitted as the bare word doThing) and absent after. Full suite 10606 pass; the one failure is the pre-existing crosswind-arbitrary-values case.

Note I ran this in a clean worktree — there was unrelated in-flight work in the tree that made failures hard to attribute.

Sign in to comment on this issue.