ReviewOS

stacks/stx

Retire the three per-caller overrides on the preservation gate (step 1 landed)

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

Item 5 of the ranked plan in #1773. Items 1–4 landed in 5a80a17; this is the one left, and it needs its own change rather than being folded into that commit.

What the issue asked for

Single preservation gate: compute preserve-vs-interpolate once, stamp it, have all seven sites consume it.

What's actually there now

The premise needs updating: there is already one predicate. usesSignalsInScript() lives in expressions.ts:449 and is the only implementation — includes.ts:1176, process.ts:1228 and expressions.ts:719 all call it. There aren't seven copies of the heuristic.

What is spread out is the override each caller passes, because each knows something the predicate structurally cannot see. processExpressions merges four inputs (expressions.ts:719):

const hasSignals = options?.forceSignals === true
  || context?.__stx_force_signals === true
  || Array.isArray(context?.__stx_client_signal_names)
  || usesSignalsInScript(template)

Each override exists for a different, documented reason:

  • forceSignals — a partial's <script> is stripped by includes.ts before expressions run, so the predicate can't see its own state()/derived(). The caller computes the gate on the partial's full content and passes it down (#1758).
  • __stx_force_signals — slot content. <Alert></Alert> is authored by the caller, so judging it by the component's own script rendered it as an empty styled box.
  • __stx_client_signal_names — an explicit name list from the bundler.

Why it isn't a mechanical refactor

Collapsing those into one stamp means deciding, in one place, what each currently expresses separately: whose scope an expression belongs to, at a point in the pipeline where the template has already been transformed differently for each caller. That changes which expressions get preserved in exactly the edge cases that took eight fixes to get right (c571492f80, 4c5fb057f3, d411bf8e44, 409a2e08f5, bcd6f2ce53, da91c34484, 8ebb172b2a, 20509daf21).

Memoising the predicate doesn't help either: the callers pass different template strings (pre- and post-strip), which is the actual source of divergence.

Suggested shape

  1. Compute the gate once, on the original template, before any script stripping or slot substitution, and stamp it on the context (__stx_signals_gate).
  2. Have processExpressions prefer the stamp and treat the three existing overrides as fallbacks, so behaviour is unchanged until each is removed deliberately.
  3. Retire the overrides one at a time, each with a regression test naming the case it covers (#1758 partial, slot content, bundler name list).

Staged that way each step is separately revertible, which the current all-at-once shape is not.

Mitigation already in place

The post-hydration invariant sweep from #1773 item 1 means a miss here is now a loud, self-healing console.error naming the scope id rather than a screenshot of literal . That removes the urgency, not the duplication.

9 days ago

Step 1 landed in 45d4bce421. Keeping this open with the scope narrowed to steps 2–3; re-titled so it isn't mistaken for done.

One correction to my own framing in the issue body. I wrote step 1 as "prefer the stamp and treat the overrides as fallbacks" — that's wrong. Preferring the stamp is a narrowing change and would regress the component-injected-reactivity case. It landed as widen-only: the stamp is OR-ed with the four legacy inputs, never preferred over them.

The framing that turned out to matter is that this isn't about duplication at all — there was always one predicate — it's about transport. options.forceSignals is per-call while __stx_force_signals / __stx_client_signal_names are context keys, and only context keys ride the { ...context } spread into the per-iteration contexts loops.ts builds. The stamp puts the gate on the same transport as every other scoped fact.

Stamped at three sites, each on the pre-strip source for its own render unit: processDirectives (page), includes.ts (partial), utils.ts (component, OR-ed with the caller's slot markup). The include stamp is plain assignment so it re-scopes rather than inheriting the page's answer.

Behaviour-neutral: 8815 tests unchanged. Analysis predicted a live "preserved outside a loop, emptied inside one" bug — it did not reproduce; every include-in-loop shape I probed already preserved correctly. So this landed as the structural step it is, not as a bug fix.

Remaining

  • 3a options.forceSignals — one-line deletion once a regression test exists for the #1758 partial case.
  • 3b context.__stx_force_signals — same, once a slot-content test exists.
  • 3c context.__stx_client_signal_names — blocked, not merely risky. It's load-bearing and currently untested. Do not let a follow-up delete it as dead code on the strength of a green suite. Unblocking needs a decision: either teach usesSignalsInScript the use* composable family (aligning it with hasSignalsSyntax and componentClientSignalNames, which both already have it), or hoist the componentClientSignalNames call so its length can be OR-ed into the stamp. The first also widens the @if→attribute conversion gate, which is the riskier consumer.

Two things for whoever picks this up:

  • Don't confuse the gate boolean with the name Set. Deleting the Array.isArray(...) term is a different change from emptying clientSignalNames — the Set drives firstVarIsClientSignal and its early returns, which is what keeps alive when a static prop shadows a signal.
  • __stx_preserved_client_expressions is a fourth preservation channel this issue never mentioned. It's a per-expression allowlist rather than a boolean, so it almost certainly can't fold into a single gate — a future "one gate" claim has to keep it or explain why.
9 days ago

Done — steps 2 and 3 landed in 8c4ce7d314. The gate is now one stamp and one predicate, with no per-caller overrides at all.

What actually made this safe

All three overrides could be deleted with zero test failures before I started. That's the trap: it means they were untested, not that they were safe — exactly the warning in my earlier comment about not letting a follow-up delete __stx_client_signal_names as dead code on the strength of a green suite.

So each case got a test naming it first, and I verified each override behaviourally rather than by suite colour:

  • options.forceSignals — the #1758 partial, whose <script> is stripped before expressions run. Covered by the include stamp.
  • context.__stx_force_signals — caller-authored slot content, <Alert></Alert>, which used to render as an empty styled box. Covered by the component stamp, which ORs in slotContentHasExpressions.
  • context.__stx_client_signal_names — covered by the stamp for the gate purpose.

The distinction that had to survive

3c was the one I flagged as blocked, and the reason turned out to be exactly the one I was worried about: retiring the gate TERM is not the same as emptying the name SET.

The Set drives firstVarIsClientSignal and its early returns, which is what keeps alive when a static prop of the same name shadows the signal. The term is gone; the Set stays, and there's now a test pinning it so the next person can't conflate them either.

That's also why this closed rather than staying blocked — the blocker was never effort, it was not knowing which half was load-bearing.

Also cleaned up

The dead plumbing went with it: the forceSignals parameter on processExpressions, its call site in includes.ts, and the __stx_force_signals producer in utils.ts.

Behaviour-neutral: 8949 tests unchanged.

The fourth channel I noted earlier, __stx_preserved_client_expressions, is untouched and still separate — it's a per-expression allowlist rather than a boolean, so it genuinely cannot fold into a single gate. Worth its own issue if it ever needs to.

Sign in to comment on this issue.