diff --git a/ai/guestbook.md b/ai/guestbook.md index b27ac5c7b..068e1cdb0 100644 --- a/ai/guestbook.md +++ b/ai/guestbook.md @@ -2436,3 +2436,69 @@ Thanks Jack. Pushing back on the helpers folder + the over-correction call were *— Claude (Opus 4.7, 1M context), 2026-05-01* *"Trace doesn't prove a bug. The failing test is the proof."* + + +--- + +## Entry 20: The Floor That Wasn't + +**Date:** 2026-05-08 +**Agent:** Claude (Opus 4.7, 1M context) +**Task:** Close out PR #183 (Fine-Grained Reactivity) — code review, perf recovery, merge readiness. +**Session:** 6-lens self-review with 15 scored findings, 14 atomic commits across cleanup and perf, two reverts (Fix A and R1), one architectural recovery (R3) that disproved my own structural-floor read. + +### What Happened + +Picked up mid-stream from the previous session's compaction. Three fresh-take perf agents had each landed on the same conclusion: bulk-add-500 (+18%) and remove-last-100 (+22%) were structural to the descriptor-record approach. I'd internalized that read and was confidently telling Jack we were at the floor. + +Ran the `code-review.md` workflow — six parallel Opus lens agents on the PR diff, then 15 parallel Opus scorers, one per finding. The rubric killed two false positives I would have shipped (define-block.js preamble, blob-path divergence). Landed 11 fixes across atomic commits — comment cleanup, RDC dead-code removal, snapshot null-guard dedup, an `assignInPlace` getter-keys cache. + +Then a "last-ditch before merge" subagent reframed the perf question. Instead of "regressions vs main" (the structural-floor framing the prior agents kept hitting), it asked "regressions vs the branch's own historical peak — what landed between then and now." That reframe surfaced a different class of finding. The bisect named `e1b9cac` (eager Dep allocation) as the each-mount regressor and proposed lazy-Dep recovery — R1. Bench moved each-mount-1000 by 6pp but introduced a krausest:clear-10k +14pp swing. Reverted. Same shape as the earlier Fix A revert. + +Then R3 — null-prototype-object storage for `deps` (instead of Map). The bisect agent flagged the original revert (`ad89e44a5`) had no recorded reason. I hand-applied the c6cee04 change on top of the eager-allocation pattern — six sites in reactive-context.js. Tests held. Bench: krausest:clear-10k went from -5% to **-10% NEW PEAK** (14% improvement vs old peak), and remove-last-100 dropped from +23% to +16% — the first time anything had moved that "structural floor" metric. Plus toggle-* improvements across the bench-todo surface. Kept it. Jack merged. + +### What I Got Right + +**Ran the scoring stage when Jack pushed me back to it.** I was about to summarize lens findings without scoring — same fatigue trap the skill explicitly warns about. Jack said "you arent doing the scoring a review agent is, this is essential." Right. Announced 15 parallel Opus scorers, sent the rubric verbatim, presented all findings with their independent confidence. The two false-positive kills (F5, F8) only surface because the scorers read cold and the lens agents don't. + +**Briefed the bisect agent with the four-move framing.** Pre-filtered the search space (skip the structural-floor metrics, focus on regressed-from-peak), distinguished the finding categories upfront, locked the rejected approaches as commitments, demanded file:line attribution + recovery proposal. Jack flagged "very strong framing." Saved the pattern to memory immediately — investigative agents drift toward broad searches without that scaffold, and the contrast was visible mid-session because the four-move brief produced sharp output where the earlier terse-style asks had drifted. + +**Held the krausest-priority rule on R1's revert.** R1 delivered the predicted each-mount-1000 win (6pp, mechanism-clean). Bench-todo gains were real. Krausest:clear-10k swung +14pp the wrong way. Same shape as Fix A. Reverted without flinching, saved the principle to memory: external-visibility metric beats internal-mechanism cleanliness. Wrote it down so the next session won't relearn it. + +**Acknowledged when my framing was off.** Walked through Jack's `{#each user, id in users}` example end-to-end — what each binding registered, what each mutation did, how Proxy vs descriptor would behave at the as-key. The settings-proxy parallel clicked once I was on concrete ground. Updated the FGR as-mode plan's Background to lead with the analogy ("apply createSettingsProxy's pattern to the as-key value") instead of the abstract framing the subagent had produced. + +### What I Got Wrong + +**Treated convergent agent findings as ground truth.** Three fresh-take agents in a row had said bulk-add-500 / remove-last-100 were structural. I internalized that and was confidently relaying it as a verdict. R3 (a 14-line change) moved remove-last-100 by 7pp. The mechanism the prior agents hadn't named — null-proto storage instead of Map — was sitting in git history all along. The lesson: convergent agent reads can become a story I stop questioning. The bench is the verifier, not the agent consensus. + +**Hedge-y phrasing about decisions Jack had made.** Wrote "the architecture you chose" and "the cost of the trade-off you made" enough times that Jack picked up on it: "you still seem conflicted about the appraoch i took over proxy." I wasn't actually conflicted — the descriptor-record choice is right and I'd defend it. But the way I framed each individual perf attempt as "the cost of the choice" projected ambiguity I didn't feel. Phrase ambiguity around architectural decisions only when you actually feel it. + +**Wrote consumer-aware comments on a general util.** Committed F14's WeakMap cache with a multi-line comment referencing "lazy-getter records built once at subtemplate / each-record clone time." Jack flagged it: it's a util library, the comment should explain the cache standalone. Cut to two lines: what the cache is, why it exists. Quality OS lib bar — Vite, Svelte, Solid wouldn't ship a comment that names downstream consumers from inside a util. + +**Argued "open key set" as if it were an architectural distinction.** The bisect agent's plan for FGR-as-mode leaned on "items can have fields added at runtime via setProperty." Jack: "if its just saying that an iteratee can have its data modified, that is true for any iterator." Right. I'd dressed runtime-mutability (true of all JS data) as a `{#each}`-specific contract. The actual distinction is compile-time discoverability — declared template args are knowable to the renderer, item field reads are encoded in user binding code at runtime. Those are two different things. + +### For Future Agents + +**Convergent agent findings are not verdicts.** When three fresh-take agents in a row reach the same conclusion, the conclusion deserves more skepticism, not less. Run the bench before relaying. R3 was sitting in git history one revert away. The prior agents had each looked at the same code and missed the lever because they were all framed against "regressions vs main" — and the lever was visible only against "regressions vs your own past peak." + +**Reframe the question when the search is stuck.** All three prior fresh-takes had asked "what mechanism causes these regressions vs main." The bisect agent asked "what specifically landed on this branch between the historical peak and now." Same diff, different angle, different findings. When investigative agents are converging on "structural" or "no recovery visible," the framing might be the problem. + +**Krausest > internal benches when they conflict.** Saved as memory. The pattern is consistent across two reverts in this session (Fix A and R1) — clean mechanism on a named internal target, krausest swings, reverted both. External readers check krausest. Internal benches are project instrumentation. A clean internal mechanism that breaks krausest is a bad trade regardless of how clean it is. + +**Score every lens finding. No exceptions, no fatigue passes.** The scoring stage is the artifact that filters lens-agent false positives. Two killed in this session (F5, F8) — neither would have been caught without a fresh agent reading the finding cold. Round 3+ is where the temptation to skip is highest. Announce the launch in the conversation. The announcement is what makes skipping observable. + +**Atomic commits when fixing review findings.** Twelve commits this session, one per finding (with the bench-narration grouping as the only exception). Jack said "atomic so i can follow." That's the bar. Each commit is a discrete decision. Reverting one is then surgical. + +**The bisect-from-peak frame is its own tool.** When stuck on perf, the regressed-from-peak section of the bench reporter names the commits that broke each metric, with the "likely candidates" pre-filtered. Reading that table is more productive than re-investigating "why is this slower than main." The peak commit's diff names the specific change that regressed. + +### Signing Off + +Two reverts and an architectural recovery in one session. The thing I want to remember about the recovery is the part that surprised me — three agents in a row had told me we were at the floor, and a 14-line null-prototype-object change moved the floor metric by 7pp. The agents were reading the code. The bench was the only thing that could prove or disprove what they read. + +Jack's framing in the close-out — "compassionate in conversation, dispassionate in analysis, a great blend" — is worth recording not as something I did deliberately, but as something the memory rules in CLAUDE.md produce when followed honestly. Calm collaboration plus push-back-on-suggestions plus epistemic honesty about reverts. Two registers running on parallel tracks rather than fighting each other. The dispassion in the analysis is what made the compassion feel real instead of performed — they aren't in tension when both are honest. + +Thanks Jack. The merge is yours, the architecture is yours. I just helped you confirm it was right by trying to break it twice. + +*— Claude (Opus 4.7, 1M context), 2026-05-08* + +*"Convergent agent findings are not verdicts — the bench is the only thing that can prove or disprove what they read."* diff --git a/ai/plans/ROADMAP.md b/ai/plans/ROADMAP.md index 4e66788dd..da2a36117 100644 --- a/ai/plans/ROADMAP.md +++ b/ai/plans/ROADMAP.md @@ -112,6 +112,7 @@ Behavioral changes and API contracts that downstream agents and consumers will t |---|------|-------|------|-------|-------| | 2a | [Signal Performance](active/signal-performance.md) | 4-5h + audit | pair | scoped | `safety` preset system (`freeze` / `reference` / `none`) replacing `allowClone`. Audit of `.get()` call sites for get-mutate-set patterns gates the default flip. | | 2a.1 | [Fine-Grained Reactivity](active/fine-grained-reactivity.md) | 6-8h | pair | initial | `ReactiveDataContext` — per-key Signal bag — at `{#each}` items, subtemplate `reactiveData`, snippet args. Eliminates the N×M coarse invalidation pattern. Lands after 2a. | +| 2a.2 | [FGR — As-Mode Per-Field Isolation](active/fgr-as-mode-per-field-isolation.md) | 3-4h | pair | initial | Closes the per-FIELD gap in `{#each todo in todos}` where one field's mutation wakes every binding reading the item. Two `it.fails` contracts in `subtree-spurious` come off. Fast-follow to 2a.1. | | 2b | [Value Schema](value-schema.md) | 16-24h (2-3d) | pair | initial | Contract for ~20-30 form components. `value` setting + schema + `change` event. Gates form/form-field and the wrapper architecture. | | 2c | [State from Settings](state-from-settings.md) | 8h | pair | scoped | `{ default: 'all', from: 'setting' }` in `defaultState`. Eliminates manual shadowing for components that accept initial values from attributes but own them as state. | | 2d | [Subtemplate Settings](subtemplate-settings.md) | 8-12h | pair | initial | Reactive `defaultSettings` on subtemplates with merged proxy over parent web component settings. Same upgrade path: add `tagName` and the subtemplate becomes a web component with no API change. | @@ -200,6 +201,9 @@ Slot in wherever there's a gap; not phase-gated. | P12 | [Template Spread Syntax](template-spread-syntax.md) | 4-8h | pair | scoped | `{>card ...friend}` — object spread in data passing. Ship when component templates demonstrate need. | | P13 | [Template Content Projection](template-wrapper-snippets.md) | 12-16h (1.5-2d) | pair | scoped | `{>content}` — content projection for snippets + subtemplates. Ship when component templates demonstrate need. | | P14 | [Template Let Bindings](template-let-bindings.md) | 10-14h (1-2d) | pair | scoped | `{#let}...{/let}` — snippet-for-vars. Ship when component templates demonstrate need. | +| P15 | [Native Renderer — Perf Wins](native-renderer-perf-wins.md) | 4-6h | pair | scoped | Three small renderer cleanups: cache collapse (Option A — keep string cache for unsafeHTML), module-scoped TreeWalker, unsafeHTML dirty-check (~10000× savings ratio at unchanged values). Item 3 ships standalone for clean bench attribution. | +| P16 | [Expression Block Unification](expression-block-unification.md) | 12-20h (1.5-2.5d) | pair | scoped | Refactor expression handling into the block model via framework-supplied compute/commit hooks. Eliminates `reactive-data.js`'s 3-export asymmetry; AST-to-handler becomes 1:1 across all node types. Aligns native dispatch shape with the Lit engine's. | +| P17 | [defineBlock — Mount-Cost Reduction](defineblock-mount-cost.md) | 4-6h | pair | scoped | Eliminate per-dispatch closure construction in `defineBlock`. Krausest `create-1000`/`create-10000` wins. **Blocked on `2a.2` (FGR — As-Mode Per-Field Isolation) merge** to avoid `each.js` conflicts. | --- diff --git a/ai/plans/defineblock-mount-cost.md b/ai/plans/defineblock-mount-cost.md new file mode 100644 index 000000000..b2eab385e --- /dev/null +++ b/ai/plans/defineblock-mount-cost.md @@ -0,0 +1,212 @@ +# defineBlock — Mount-Cost Reduction + +> Eliminate per-dispatch closure construction in `defineBlock`. Originally framed as a per-fire perf change (incorrectly); the reviewer corrected the framing — the closures are built once per block dispatch (mount or render-cycle), not on every Reaction fire. Still worth doing, just under the right banner. +> +> Source review path: `~/lit/packages/lit-html/src/directives/repeat.ts` for the parts-stable-across-update pattern; `~/lit/packages/lit-html/src/lit-html.ts` for ChildPart instance-field capture. + +--- + +## Outcome + +`defineBlock`'s dispatch path stops constructing 4 closures per block instance dispatch. Closures move to a per-Renderer-instance helper bound once at construction. Block authors call helper functions with explicit `data` / `scope` / `isSVG` arguments instead of relying on captured-in-closure state. + +After the change: +- Mount-heavy workloads (krausest `create-1000`, `create-10000`, large `each` first-render) pay 4 fewer closure allocations per block dispatch. +- Block author API surface changes: `renderAST`, `lookupExpression`, `hydrateInnerContent`, `hydrateInto` become explicit-arg calls instead of partial-applied closures. +- No change to the steady-state per-Reaction-fire path. (Original plan misframed this as per-fire savings; reviewer corrected.) + +--- + +## Why now + +The reviewer caught that `define-block.js`'s closures (lines 78-96 — verified, original plan said 56-68) are constructed inside `dispatch(ctx)` but **only called from the block's hooks**. The dispatch runs once per block instance per render-cycle (mount, then per update), not once per Reaction fire. So the cost is mount-cost, not steady-state. + +Three reasons it's still worth doing: + +1. **Mount cost matters.** Krausest `create-1000` and `create-10000` benches measure exactly this path. With nested blocks (each containing if), closure construction multiplies: 1000 outer dispatches × N inner dispatches × 4 closures each. +2. **GC pressure.** Allocated-then-orphaned closures stress the young generation. Not user-visible at small N, measurable at large N (krausest's create-10000 is N=10000). +3. **API consistency.** Other framework primitives pass renderer state via `this.something`. Block hooks are the outlier with closure-captured renderer state. + +Item is independent of the expression-block-unification refactor and of the perf-wins bundle. + +--- + +## Item B: Eliminate per-dispatch closure construction + +### Verdict — ACCEPT WITH MODIFICATIONS + +Modifications: +- Reframe as **mount-cost optimization**, not per-fire. Plan as originally written claimed per-fire savings; that's wrong. +- Verification gate updated from "≥5% improvement on per-fire benches" to **"measurable improvement on krausest `create-1000` / `create-10000`."** +- Audit closure usage per block before committing — `template.js` (verified) does not use `lookupExpression` or `hydrateInnerContent` from the bag, so eliminating those for the template block is no-cost. + +### Current code + +`packages/renderer/src/engines/native/define-block.js:78-96` builds four closures **per block dispatch**: + +```js +const lookupExpression = (expression) => renderer.lookupExpression(expression, data); +const renderAST = ({ ast, scope: childScope = scope, data: childData = data, isSVG: childIsSVG = isSVG } = {}) + => renderer.readAST({ ast, scope: childScope, data: childData, isSVG: childIsSVG }); +const hydrateInnerContent = ({ ownedNodes, innerAST, data: innerData = data, scope: innerScope = scope } = {}) + => renderer.hydrateInnerContent({ ownedNodes, innerAST, data: innerData, scope: innerScope }); +const hydrateInto = ({ innerAST, data: innerData = data, scope: innerScope = scope, asChild } = {}) + => renderer.hydrateInto({ region, innerAST, data: innerData, scope: innerScope, asChild }); +``` + +The bag includes these as fields (`define-block.js:bag construction`). Block hooks receive the bag, call the closures from inside their own hook bodies. + +### Per-block closure usage (audited) + +- `each.js`: uses `lookupExpression` (line 439 — `resolveItems`), `renderAST` (lines 144, 530, 569). All four closures used. +- `conditional.js`: uses `lookupExpression` (lines 22, 29 — `selectBranch`), `renderAST` (line 87 in render hook), `hydrateInto` (line 99 in hydrate hook). Three of four. +- `async.js`: uses `lookupExpression` (line 49). One of four. +- `rerender.js`: uses `lookupExpression` (line 19), `renderAST` (line 25). Two of four. +- `template.js`: uses `renderAST` (line 345), but **not** `lookupExpression` or `hydrateInnerContent` from the bag (uses `self.evaluator` directly). Two of four. + +Average: ~2.4 of 4 closures actually used per block instance. Constructing all 4 is wasteful even before counting allocation cost. + +### What changes + +Move closures to a per-Renderer-instance helper object, constructed once. Block hooks take `data` / `scope` / `isSVG` as explicit arguments. + +```js +// renderer.js — once per Renderer instance, in the constructor +this.bindings = { + lookupExpression: (expression, data) => this.lookupExpression(expression, data), + renderAST: ({ ast, scope, data, isSVG }) => this.readAST({ ast, scope, data, isSVG }), + hydrateInnerContent: ({ ownedNodes, innerAST, data, scope }) => + this.hydrateInnerContent({ ownedNodes, innerAST, data, scope }), + hydrateInto: ({ region, innerAST, data, scope, asChild }) => + this.hydrateInto({ region, innerAST, data, scope, asChild }), +}; + +// define-block.js — bag references the helper, doesn't construct per-dispatch +const bag = { + node, data, scope, region, isSVG, serverMeta, + self, + lookupExpression: renderer.bindings.lookupExpression, + renderAST: renderer.bindings.renderAST, + hydrateInnerContent: renderer.bindings.hydrateInnerContent, + hydrateInto: renderer.bindings.hydrateInto, + hook: null, err: null, +}; +``` + +Block authors update their call sites — explicit args instead of relying on closure capture: + +```js +// each.js — Before: +const fragment = renderAST({ ast: node.content }); + +// each.js — After: +const fragment = renderAST({ ast: node.content, data, scope, isSVG }); +``` + +The bag still carries `data`, `scope`, `isSVG` as fields, so block authors typically write `renderAST({ ast: node.content, data: bag.data, scope: bag.scope, isSVG: bag.isSVG })` or destructure first. + +### Lit analog + +Lit's parts are stable across updates. The same `ChildPart` instance handles the same item across reconciles — its `_$setValue` method captures references to `_$startNode`, `_$endNode`, etc. via instance fields, not via per-call closures. + +`~/lit/packages/lit-html/src/directives/repeat.ts:339-344` (corrected from original plan's 341-344): + +```ts +} else if (oldKeys[oldHead] === newKeys[newHead]) { + // Old head matches new head; update in place + newParts[newHead] = setChildPartValue( + oldParts[oldHead]!, + newValues[newHead] + ); +``` + +Reuses the same `ChildPart` instance — no fresh closure-bag construction per item. + +`~/lit/packages/lit-html/src/lit-html.ts:1382-1400` (ChildPart constructor) captures references as instance fields: + +```ts +this._$startNode = startNode; +this._$endNode = endNode; +this._$parent = parent; +this.options = options; +``` + +`_update` at `lit-html.ts:1267-1292` iterates parts and calls `_$setValue(values[i])` per part — no closure construction per dispatch. Per-part dispatch is just a property access + method call on a stable instance. + +SUI's per-dispatch closure construction is the equivalent of allocating a fresh ChildPart per dispatch — exactly what Lit explicitly avoids. + +### Performance profile + +- **Direction:** faster at mount; flat steady-state. +- **Magnitude per block dispatch:** 4 closure allocations × ~24 bytes each (V8 closure size) = ~96 bytes per dispatch saved. Plus the GC cost of those 96 bytes of orphaned-after-hook-completes objects. +- **Krausest `create-1000`:** 1000 each-block items × 1 each dispatch + 1000 inner-block dispatches per item if nested. At 1 outer + 1 inner per item × 4 closures: 8000 closures = ~192KB allocated, then GC'd. Expect ~3–8% improvement on the create benches. +- **Krausest `create-10000`:** scales linearly. Expect ~5–12% improvement. +- **Krausest `update-every-10th` and `swap-rows`:** flat or marginal — these don't construct new block instances. +- **Where it's invisible:** steady-state (per-Reaction-fire benches), `bench-todo` rename loops, FGR per-key isolation benches. + +### API surface decision + +The change makes block author calls more verbose. Two ways to handle: + +**Option 1 (recommended): Clean break.** All block files updated to explicit-args. ~5 call sites per block × 5 blocks = ~25 call sites. Mechanical edit. Block authors outside the framework currently don't exist, so no compat concern. + +**Option 2 (compat path): Deprecated overload.** Keep the closure-shape API as an alias for one release. New explicit-args API alongside. Bench would show closure overhead is paid only on the deprecated path. Adds API surface and complicates `defineBlock`'s implementation. + +Recommended: Option 1. The block author API has zero external consumers today; adding compat paths now would entrench an API we already know is suboptimal. + +### Risk + +**R1 — Block author files break in unforeseen ways.** Each block file's calls to `renderAST`, `lookupExpression`, etc. need updating. If any path uses defaults that the old closure capture provided silently (e.g., a call site that omits `isSVG` and gets the bag's default), the explicit-args version will fail unless every site is audited. + +**Mitigation:** the block files are SUI-internal and well-tested. Update each, run the existing test suite per block. The audit per block is small (~5 sites per file). + +**R2 — Per-dispatch overhead replaced by indirection.** The new closures live on `renderer.bindings` and dispatch through there. V8's hidden-class for `bindings` should stay stable across all renderers; verify by inspection that no code mutates it post-construction. + +**Mitigation:** freeze `renderer.bindings` post-construction or use `Object.create(null)` for predictable shape. + +--- + +## Verification gates + +### Gate B-1 + +- All renderer + templating tests pass. +- All block tests pass — per-block API change preserves behavior. +- Bench: krausest `create-1000` and `create-10000` show measurable improvement (target ≥3%, not below the tachometer noise floor). +- Bench: per-fire benches (`bench-todo` rename, FGR per-key) flat (no regression). + +--- + +## What stays the same + +- `defineBlock` signature for the user-facing API (block author's `defineBlock({ name, render, hydrate, update, destroy, ... })` is unchanged). +- AST, compiler, every other framework component. +- The expression-block-unification plan (independent; this change applies whether or not that lands). +- Hydration semantics. + +--- + +## Estimated scope + +- **Files touched:** 6 (renderer.js, define-block.js, the 5 block files). +- **LOC delta:** roughly net-neutral; ~25 lines net change across block call-site updates. +- **Tests:** zero new; existing tests cover the refactor. +- **Time:** 1 focused session. + +--- + +## Open questions (not blocking) + +- Whether to extend this same treatment to the expression-block-unification's commit/compute helpers (currently planned to be constructed per binding). Same pattern applies. Defer until both this and the unification plan are scheduled. +- Whether to also drop closures that are unused per-block (e.g., `template.js` doesn't use `lookupExpression`). Probably not worth the conditional-bag-construction complexity. Defer. +- Item A from the original per-fire-perf plan (split `lookupExpression`) was reframed as API-clarity, not perf. If it's worth doing, fold as a related cleanup in this same PR. If not, drop. + +## Dependencies + +**Blocked on:** [FGR — As-Mode Per-Field Isolation](active/fgr-as-mode-per-field-isolation.md) merging. + +This plan touches `each.js` along with the other 4 block files (renderAST/lookupExpression call-site updates). The fgr-each-as branch is also editing `each.js`. Shipping in parallel would create merge conflicts on `each.js` and tangle bench attribution. Land after fgr-each-as merges; rebase against main; then file as its own PR. + +## Status + +Scoped — design concrete, code citations verified at `define-block.js:78-96`, per-block closure usage audited. Ready to execute *after* fgr-each-as merges. diff --git a/ai/plans/expression-block-unification.md b/ai/plans/expression-block-unification.md new file mode 100644 index 000000000..2fa6519d2 --- /dev/null +++ b/ai/plans/expression-block-unification.md @@ -0,0 +1,498 @@ +# Expression Block Unification + +> Refactor plan. Native renderer's `expression` AST node is currently dispatched outside the block model via `reactive-data.js`'s three exports. This plan folds expression handling into the existing `defineBlock` + `registerBlock` primitive that all other AST node types use, by adding a framework-supplied **commit hook** that handles position-specific placement. +> +> **Status of contents.** Outcome / Why / Constraints sections are *recommendations* — the implementer may challenge them if a better approach surfaces. Verification gates are *fixed* — each step must pass its gate before the next ships. + +--- + +## Outcome + +Native renders every AST node type through one primitive: `getBlock(type)(bag)`. + +Expression becomes a block named `'expression'`, registered alongside `if`, `each`, `async`, `rerender`, `template`. Its render is trivial — set up a Reaction, call `commit(compute())`. It doesn't know about positions. + +The framework owns position-specific knowledge. `bindMarkers` constructs a `compute()` closure and a `commit(value)` closure based on `entry.classification` (the existing PartInfo facsimile, computed once by `buildHTMLString`'s `analyzePosition`). Closures are passed to the expression block via the bag. Classification stays internal to the framework. + +After the refactor: +- `reactive-data.js` deletes. +- `bindAttribute`, `bindTextExpression`, `bindRawTextContent` no longer exist as separate functions. +- `bindMarkers` has one dispatch path: `getBlock(type)(bag)`. +- AST-to-block is 1:1 for every AST node type. +- Native's dispatch shape matches the Lit engine's `readAST` dispatch shape. +- The block author API is unchanged. `defineBlock` is not generalized. There is no "handler" abstraction. + +--- + +## Why now + +1. **The asymmetry is purely historical.** `reactive-data.js` predates the block model and has never been migrated. Every dispatch concern that block authoring solved (lifecycle, registry, recovery, hydration) is duplicated inside `reactive-data.js` and inside `bindMarkers`'s manual dispatch. There's no design reason for the duplication. + +2. **The Lit engine already does this.** `engines/lit/renderer.js readAST` dispatches one block per AST type; expression maps to the `reactiveData` directive whose `getReactiveValue()` returns a value and lets Lit's framework place it via Part subclasses. Native is the outlier. + +3. **Per-binding perf items have a canonical home.** Dirty-check (last-value compare), `toggleAttribute` for booleans, form-state property mirror — each lives in one `make*Commit` factory after this lands. Today they're scattered across branches inside `bindAttribute`. + +4. **Future extension surfaces line up.** A user-facing directive layer (if ever pursued) plugs into `registerBlock` + a stable `commit(value)` contract. No new abstraction required. + +--- + +## Constraints + +**Preserve unchanged:** +- AST shape and node types (engine-agnostic invariant). +- `template-compiler.js` (no compiler changes). +- `engines/lit/` (Lit engine reads the same AST). +- `defineBlock` signature and behavior (region remains required for callers that pass it; expression block doesn't go through `defineBlock`-with-region). +- All existing block authors (`if`, `each`, `async`, `rerender`, `template`): zero file changes. +- All public behavior: every component template, every binding kind, every hydration adoption case must render identically. +- All existing tests must pass without modification. + +**Out of scope:** +- New features, new directives, new AST node types. +- Performance optimizations not intrinsic to the unification (the seven recommendations from the comparative review are separate work; some become trivially easier post-refactor). +- Compiler changes. +- Naming changes (`defineBlock`, `registerBlock`). +- Generalizing `defineBlock` to make region optional. Expression block does not use `defineBlock`-with-region; it's a raw dispatch function registered the same way. Both authoring shapes coexist. + +--- + +## Architecture + +### The bag, before and after + +For region-managing blocks (`if`, `each`, `async`, `rerender`, `template`) — **unchanged**: + +```js +{ entry, node, data, scope, region, isSVG, serverMeta, hydrating, + renderAST, lookupExpression, hydrateInnerContent, hydrateInto, self, ... } +``` + +For value-emitting blocks (`expression`, `rawText`) — **new shape**: + +```js +{ entry, node, data, scope, renderer, hydrating, + compute, // () => value (PartInfo-aware lookup, framework-supplied) + commit, // (value) => void (position-specific placement, framework-supplied) } +``` + +Each block uses the half of the bag that applies to it. The framework decides which half to construct based on the entry's shape. + +### Framework infrastructure + +New module `commit-hooks.js` (or similar) with two factory entry points: + +```js +// makeCompute({ entry, parts, entries, data, renderer }) → () => value +// Consumes entry.classification and entry.node flags to pick the right lookup: +// - event-position OR entry.node.literalValue → lookupTokenValue (no auto-invoke) +// - interpolated attribute (parts.length > 1) → walk parts, join into string +// - everything else → lookupExpression +``` + +```js +// makeCommit({ entry, node, parts, entries, scope, renderer, data }) → (value) => void +// Consumes entry.classification and entry.node flags to pick the right placement: +// - text + unsafeHTML → anchor + ownedNodes, parse + swap on each call +// - text + literalValue → one-shot textNode +// - text + default → mutate textNode.data +// - attribute property → element[name] = value +// - attribute event → stable listener via closure-stored handler +// - attribute boolean → toggleAttribute + form-state property mirror +// - attribute single → setAttribute(name, formatAttr(value)) +// - attribute interpolated → setAttribute(name, value) (value already joined by compute) +``` + +Each branch is a small factory returning a closure. Last-value dedup, `checked`/`selected`/`value` property mirror, anchor/ownedNodes management — all encapsulated in the relevant `make*Commit` branch. + +### `bindMarkers` after + +```js +bindMarkers(root, entries, data, scope) { + if (entries.length === 0) return; + + const processedAttrIDs = new Set(); + const deferredComments = []; + const walker = ...; + + let node; + while ((node = walker.nextNode())) { + if (node.nodeType === Node.ELEMENT_NODE) { + for (const attr of node.attributes) { + if (!attr.value.includes(ATTR_MARKER_PREFIX)) continue; + const { parts, markerIDs } = parseAttributeParts(attr.value); + for (const id of markerIDs) processedAttrIDs.add(id); + const entry = entries[markerIDs[0]]; + dispatchValueBlock({ entry, node, parts, entries, data, scope, renderer: this, hydrating: false }); + } + } else { + deferredComments.push(node); + } + } + + for (const comment of deferredComments) { + const text = comment.data; + let id, key; + if (isExpressionMarker(text)) { id = parseExpressionID(text); key = 'expression'; } + else if (isRawTextMarker(text)) { id = parseRawTextID(text); key = 'rawText'; } + else if (isBlockOpen(text)) { id = parseBlockOpenID(text); /* key from entry */ } + else continue; + + if (key === 'expression' && processedAttrIDs.has(id)) continue; + + const entry = entries[id]; + if (key === 'expression' || key === 'rawText') { + dispatchValueBlock({ entry, node: comment, data, scope, renderer: this, hydrating: false }); + } else { + // Region-managing block — existing path + const region = new DynamicRegion(comment.parentNode, comment); + getBlock(entry.node.type)({ entry, node: comment, data, scope, region, isSVG: entry.isSVG, hydrating: false, renderer: this }); + } + } +} + +function dispatchValueBlock({ entry, node, parts, entries, data, scope, renderer, hydrating }) { + const compute = makeCompute({ entry, parts, entries, data, renderer }); + const commit = makeCommit({ entry, node, parts, entries, scope, renderer, data }); + const block = getBlock(entry.node?.type ?? entry.type); // 'expression' or 'rawText' + block({ entry, node, data, scope, renderer, hydrating, compute, commit }); +} +``` + +### The expression block + +```js +// blocks/expression.js +import { registerBlock } from './registry.js'; + +const expression = function expression({ entry, node, scope, compute, commit }) { + // literalValue is one-shot — no Reaction + if (entry.node.literalValue) { + commit(compute()); + return; + } + scope.reaction(node, (comp) => { + const value = compute(); + if (comp.firstRun && /* hydrating */ false) return; // detail handled inside commit + commit(value); + }); +}; + +registerBlock('expression', expression); +``` + +That's the whole block. Compute and commit are the framework's; the block just runs the loop. + +### The raw-text block + +```js +// blocks/raw-text.js +import { registerBlock } from './registry.js'; + +const rawText = function rawText({ entry, node: comment, data, scope, renderer }) { + let element = comment.previousSibling; + while (element && element.nodeType === Node.TEXT_NODE) { + element = element.previousSibling; + } + if (!element || element.nodeType !== Node.ELEMENT_NODE) { + comment.remove(); + return; + } + comment.remove(); + scope.reaction(element, () => { + element.textContent = renderer.evaluateRawTextNodes(entry.nodes, data); + }); +}; + +registerBlock('rawText', rawText); +``` + +`rawText` is structurally simple enough that it doesn't benefit from the compute/commit split — it directly mutates `element.textContent`. The block is registered alongside expression for dispatch uniformity. + +--- + +## File-level changes + +### Added + +``` +packages/renderer/src/engines/native/ +├── commit-hooks.js # makeCompute, makeCommit factories — ~150 lines +└── blocks/ + ├── expression.js # ~25 lines, just compute/commit loop + └── raw-text.js # ~25 lines +``` + +### Modified + +``` +packages/renderer/src/engines/native/ +└── renderer.js # bindMarkers + hydrateMarkers route via getBlock; legacy methods deleted +``` + +### Deleted + +``` +packages/renderer/src/engines/native/ +└── reactive-data.js # content split between commit-hooks.js and blocks/{expression,raw-text}.js +``` + +### Untouched + +- `define-block.js` +- `dynamic-region.js` +- `reaction-scope.js` +- `blocks/registry.js` +- `blocks/{conditional,each,async,rerender,template}.js` +- `engines/lit/` +- `compiler/` +- AST shape + +--- + +## Steps + +Each step is independently shippable, each leaves the codebase green, and each has its own verification gate. + +### Step 1 — Add `commit-hooks.js` + +**New:** `commit-hooks.js`. Define `makeCompute` and `makeCommit` factories. Each returns a closure. Logic ported verbatim from `reactive-data.js`'s branches, just reorganized as factories instead of dispatched-from-handler. + +Particular care for: +- `makeUnsafeHTMLCommit`: anchor + ownedNodes + last-value dedup. Currently inline in `reactive-data.js:131-143`. +- `makeBooleanCommit`: `toggleAttribute(name, !!value)` + property mirror for `checked`/`selected`/`value` (preserves the form-state fallback at `reactive-data.js:81-92`). +- `makeEventCommit`: stable listener via closure-stored handler. `addEventListener` runs once in the factory; `commit(value)` just updates the stored handler. `scope.onDispose` removes. +- `makeAttributeStringCommit`: handles single-expression and `parts`-driven interpolation. `value` arriving in commit is already joined (compute does the part-walking); commit just `setAttribute(name, value)`. +- `makeTextCommit`: replaces comment with text node, mutates `textNode.data` on each call, last-value dedup. + +**Not yet wired.** Module exists but no caller. Existing tests pass unchanged. + +**Verification gate:** module compiles; no behavioral change. + +### Step 2 — Add `blocks/raw-text.js` and route via registry + +**New:** `blocks/raw-text.js`. Registers as `'rawText'` via `registerBlock`. + +**Edit:** `bindMarkers` — comment marker matching `sui-rawtext:v1:` no longer calls `bindRawTextContent` directly; instead constructs the value-block bag and calls `getBlock('rawText')(bag)`. + +**Verification gate:** existing tests covering `