ReviewOS

stacks/stx

SPA nav: runsAlways fallback dedups unmarked scoped scripts (leading comment / non-'(' start) → scope never re-registers

#1828
Closed glennmichael123 opened this 9 days ago ·
9 days ago

Summary

On SPA navigation, a scoped component script that the emitter did not mark data-stx-run="always" can be silently deduplicated by execScripts, because the runsAlways fallback heuristic only re-runs scripts whose trimmed body starts with ( or ;. When that happens the component's scope is never re-registered and it fails to hydrate — the same class as #1773 / "content vanishes on SPA nav."

Background

execScripts dedups already-executed scripts by content hash, except scoped scripts, which must re-run every navigation (the prior scope was deleted by _cleanupContainer). The exemption is decided by runsAlways:

function runsAlways(declared, code){
  if(declared==='always')return true;
  if(declared==='once')return false;
  var head=code.trimStart();
  return head.charAt(0)==='('||head.charAt(0)===';'||code.indexOf('window.stx.mount')>-1;
}

The primary path is the emitter mark (data-stx-run="always"). The fallback sniff is the safety net when the mark is missing — and it's brittle.

Repro (unmarked scoped scripts, declared="")

(function(){ /*scope*/ })()          → re-runs ✅
// scope for X\n(function(){})()     → DEDUPED ❌   (leading // comment)
/* scope */(function(){})()          → DEDUPED ❌   (leading /* */ comment)
const s = stx.state(0); /*...*/      → DEDUPED ❌   (starts with const)
!function(){}()                      → DEDUPED ❌   (bang-IIFE)

Each DEDUPED case is skipped on the 2nd+ navigation → scope never re-registers → :if/@event/ under that scope never re-bind → the component (including its static markup) disappears on SPA nav.

Impact

The entire re-hydration safety rests on the emitter marking every scoped script correctly. Any gap in the emitter (a new code path, a wrapper change) falls through to a fallback that misses common forms (leading comments, const-first, !function). Failures are silent — no error, just missing UI after navigation.

Suggested fix

  • Strip leading // and /* … */ comments (and whitespace) before the start-char check.
  • Broaden the heuristic (!function, void function, const/let/var … stx.).
  • Or make the emitter mark authoritative and fail loudly (dev warning) if a data-stx-scope script arrives without a data-stx-run declaration, so the brittle sniff is never the deciding factor.

Location

packages/router/src/client.ts — runsAlways + the execScripts hash-dedup guard.

9 days ago

Fixed in 338e4ba — though the reachable failure turned out not to be the one in the repro, and the suggested fix would have made things worse. Both worth writing down.

The reachable bug is a repeated component

The repro cases are all real properties of runsAlways in isolation, but the emitters that actually hit them are mostly harmless — the reactive bridge misclassifies but never reaches this code (the fragment builder drops it, and the full-document path hardcodes runAlways), and setup scripts are exempt by the separate isSetup check.

What does hit it: a component used more than once on a page.

Two or more instances take the shared-factory path — one prelude that registers the factory, then one call per instance that registers that instance's scope:

<script data-stx-scoped data-stx-run="always" data-stx-component-factories>(function(){ const factories = ... })()</script>
<script data-stx-scoped>window.__stxComponentFactories["6e1e4d3e93e6517d"]("stx_counter_1_3ysmg1");</script>
<script data-stx-scoped>window.__stxComponentFactories["6e1e4d3e93e6517d"]("stx_counter_2_af33xo");</script>

The prelude was already stamped always — but nothing invokes the prelude, so marking it was never enough. The calls begin window., so runsAlways returns false, and from the first SPA return onward each was hash-deduped and its scope never put back. _handleStxLoad then bails at if (!scopeVars) return, so the component renders with empty :text and inert @click, silently. A component used once was fine — it inlines as (factoryBody)(scopeId) and survives on its leading paren.

Two details worth knowing: it breaks on the first SPA return, not the second, because executedScriptHashes is seeded from the initial document; and :for/:if-driven repeats have a rescue path via hydrateComponentScopes, so it is specific to statically repeated components.

Why I did not strip leading comments

That is the natural reading of the report, and it is wrong in the expensive direction. Three framework scripts open with a comment and must not re-run — comment-stripping flips all three, trading a silent missing component for a silent leak.

So the fallback now asks what a script registers rather than how it is spelled (_scopes[, initScope(, stx.mount, __stxComponentFactories[), and every scoped emitter is stamped so the sniff is no longer what decides.

One place the old answer was wrong, and I changed it

The scroll observer and the animation group are now always. Both re-query the document when they run and remove the stx-out class that hides their targets, so being deduped did not merely drop an animation — content swapped in by an SPA navigation stayed hidden. That was already broken before this change; stamping them once would have made it permanent. Cost is one extra IntersectionObserver per hop, which is the cheaper side of that trade.

Everything else preserves current behaviour exactly; I checked each mark against what the old sniff produced rather than assuming.

Tests

packages/router/test/client-scoped-script-rerun.test.ts (classification + end-to-end revisits) and packages/stx/test/signal-processing/scoped-script-run-marks.test.ts (every emitted scoped script declares a mark, and the factory calls are pinned to the scope roots they register).

Four existing assertions anchored on an exact script tag and had to become attribute-tolerant. One of those mattered: component-edge-cases matched <script data-stx-scoped client> in order to execute the factory calls, so the added attribute made it match nothing, register no scope, and fail on undefined — the same brittleness in the test that this fix removes from the router.

Full suite 10626 pass in a clean worktree; the two failures are the pre-existing crosswind-arbitrary-values case and a package-exports timeout that passes in isolation.

Sign in to comment on this issue.