Refactor: Consolidate Native Hydrate Path#179
Merged
Conversation
Five small wins bundled — drift hazards, dead state, unnamed magic.
- Drop dead `inheritsData` field from native Renderer + ServerRenderer (lit-only contract)
- Import shared `MARKER_VERSION` in component base instead of redeclaring
- Drop `oldKeys` allocation in each.reconcile — read `r.key` inline
- Name `MAIN_BRANCH_INDEX` constant for the {#if}-main-body sentinel
- Refresh stale bench-hydrate comment referencing removed `hasHydrated` flag
`/sui-block:` was hand-typed across 5 sites. parseServerMeta lived in renderer.js but the matching write was inline at 5 sites in server.js, keeping the format coordinated by comment. - Export BLOCK_CLOSE_PREFIX, parseServerMeta, formatBlockClose from build-html-string.js - formatBlockClose owns the wire format including reserved branchIndex meta - Replace `/sui-block:` literals with the constant (renderer.js, each.js, reactive-data.js) - server.js write sites use formatBlockClose
`hydrateInnerContent` and `hydrateMarkers` took positional arguments
that callers immediately reshaped. Standardize on options.
- Renderer.hydrateMarkers({ root, entries, data, scope })
- Renderer.hydrateInnerContent({ ownedNodes, innerAST, data, scope })
- Update callers in component/base.js, blocks/template.js, define-block.js wrapper
`getItemID`, `getEachData`, item-marker prefix, and key encode/decode were duplicated between `server.js` and `blocks/each.js` with a keep-in-sync comment as the only enforcement. The wire format that bridges SSR and client hydration is now sourced from one module. - New `engines/native/shared/each.js` exporting SUI_ITEM_MARKER, getItemID, getEachData, encodeItemKey, decodeItemKey - `server.js` and `blocks/each.js` import from shared - `blocks/each.js` also imports BLOCK_MARKER + BLOCK_CLOSE_PREFIX for its server-group walker (was inlining literals)
`bindAttribute` ran `parts.find(p => p.markerID !== undefined)` per binding setup to locate the entry it should read classification from. The first dynamic-part marker is already known at parse time — populateAttributeBindings now stores it on the binding entry. - attributeBinding gains `firstMarkerID` field - bindAttribute reads `entries[firstMarkerID]` directly (no closure) - bindAttributeExpression / bindMarkers / hydrateAttributes thread firstMarkerID through
Five walkers across renderer.js, each.js, reactive-data.js declared the same prefix checks for block-open / block-close / expression / raw-text markers inline. Promote the predicates and ID parsers next to the constants in build-html-string.js. - Export isBlockOpen / isBlockClose / isExpressionMarker / isRawTextMarker - Export parseBlockOpenID / parseExpressionID / parseRawTextID - Walkers consume the helpers; literal startsWith calls drop - BLOCK_MARKER / BLOCK_CLOSE_PREFIX / COMMENT_MARKER / RAW_TEXT_MARKER imports trim off renderer.js (now reach via the helpers)
Most elements in a typical template carry zero `__sui` attributes. The empty-array-then-iterate pattern allocated per element regardless. Lazy init via `??=` skips the alloc on the common case while preserving the defensive collect-then-iterate semantics — bindAttributeExpression calls element.removeAttribute for property/event bindings, so live iteration over NamedNodeMap is not safe.
Five blocks (conditional, rerender, sample, template-snippet,
each-elseContent) plus template-subtemplate all hand-rolled the same
post-hydrate fragment-build + region.anchor.after dance. Subtemplate's
copy was 22 lines reaching into a different renderer's internals.
Renderer.hydrateInto unifies the contract — creates a child scope
(opt-out via asChild: false for snippet/subtemplate flat-scope cases),
hydrates region.ownedNodes, reattaches via fragment, and sets
region.endAnchor so attachToRenderRoot's last-node lookup is consistent
across render and hydrate paths.
- Renderer.hydrateInto({ region, innerAST, data, scope, asChild })
- Bag exposes hydrateInto wrapper alongside hydrateInnerContent
- Replace 5 hydrate-and-attach sites in conditional / rerender / each /
template (snippet + subtemplate)
- Update sample.js documentary to show hydrateInto as the canonical pattern
`renderEach` allocated a fresh `new ExpressionEvaluator(...)` per item plus a save/swap dance to scope the data field. Same in `renderTemplate` snippet branch and `renderSubtemplate`'s plain-AST path. The evaluator's `data` field is dead in server-side use — every lookup goes through `lookupExpressionValue(expr, data)` with explicit data, and `evaluateJavascript` resets `jsContext` per call from the passed data. Helpers are reference-shared between any per-call evaluator and the parent so reusing the parent is identical. For a 1000-item SSR list this drops 1000 Proxy + handler allocations plus the swap dance per item. ~1ms saved per render at that scale.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
1 Skipped Deployment
|
⚪ No Meaningful Change for
|
| metric | Change |
|---|---|
add-20 |
0.0% – +0.1% |
bulk-add-500 |
-0.4% – +1.0% |
clear-completed-250 |
-0.6% – +1.3% |
create-10k |
-0.1% – +0.7% |
create-1k |
-1.6% – -0.3% |
edit-cycle-5 |
-0.4% – +0.1% |
edit-start-10 |
-1.2% – +1.1% |
filter-cycle-20 |
-0.3% – +1.2% |
hydrate-each-100 |
-0.5% – +0.7% |
hydrate-each-100-mount |
-0.2% – +1.0% |
hydrate-helper-100-mount |
-0.4% – +0.9% |
remove-10-middle |
-0.8% – +1.0% |
remove-first-10 |
-0.8% – +1.3% |
remove-last-10 |
-0.3% – +0.1% |
remove-middle-10 |
-0.1% – +0.6% |
remove-row-front-20 |
-0.6% – +0.6% |
remove-row-middle-20 |
+0.1% – +1.6% |
replace-1k |
-1.2% – -0.1% |
select-40 |
+0.1% – +1.4% |
swap-rows-20 |
+0.0% – +1.0% |
toggle-10 |
-0.2% – +0.5% |
toggle-all-20 |
-0.0% – +0.2% |
toggle-first-10 |
-0.3% – +0.1% |
toggle-last-10 |
-0.7% – +0.0% |
toggle-middle-10 |
-0.3% – +0.3% |
update-10th-10 |
-1.1% – +0.8% |
🔍 Unsure (6)
Inconclusive (4)
The measured difference is small, and our sampling couldn't confidently place it above or below zero. Running more samples in a future run might settle these metrics.
| metric | Change | Expected Noise |
|---|---|---|
append-1k |
-4.1% – +0.5% | ±2% |
clear-10k |
-3.1% – +3.3% | ±1% |
remove-5-back |
+1.4% – +6.0% | ±2% |
remove-row-back-10 |
-2.2% – +0.1% | ±1% |
Too Fast to Measure Precisely (2)
On benches this short, system jitter (scheduling, GC, JIT) masks sub-4% changes; larger deltas still resolve cleanly.
| metric | Change | Test Time | Expected Noise |
|---|---|---|---|
hydrate-helper-100-state-change |
-12.8% – +8.7% | ~5ms | ±29% |
remove-5-front |
-1.1% – +2.3% | ~88ms | ±2% |
Sample size: 50 · Resolution floor: ±2% · Timeout: 3min · Wall-clock: 16m05s
…leanup - Drop redundant `firstMarkerID` field on `attributeBinding` and the threaded parameter through `bindAttribute` / `bindAttributeExpression`. parts.find runs once per binding setup (not in the reactive loop), so the precompute was unjustified scaffolding. - Refresh stale "first data change" comment in server.js renderEach — adoption is eager since f013d4f. - Reorder build-html-string.js so `RAW_TEXT_MARKER` declares above its predicate. Group all marker constants up top. - Trim restated/over-explained comments across build-html-string.js, shared/each.js, blocks/each.js, blocks/template.js, renderer.js, bench-hydrate.js. Bar: would the comment ship in Vite/Svelte source?
…omment trim
- attributeBinding shape narrows to { parts } — rawAttrName had only one
reader (reconstructed inline at the consumer from the data-sui-bind
payload itself), markerIDs had no readers post-firstMarkerID revert.
- Marker ID parsers use `+str` instead of `parseInt(str)`. Compiler-emitted
inputs are digit-only after a fixed prefix, so semantics match. ~2-3×
faster, drops the no-radix linter concern.
- Comment trims at four clear sites (formatBlockClose, hydrateInto trailing
default sentence, server.js per-item key marker, server.js plain-AST
branch). Bench-hydrate file header refreshed to describe the eager-mount
shape (was still describing pre-#175 lazy adoption).
- author-pull-requests skill: don't leak iteration history into commit
subjects — branch commits feed code-review subagents and round-numbering
biases their read.
…tr sweep
- populateAttributeBindings doc was describing the pre-narrowed
`{ rawAttrName, parts, markerIDs }` shape; refresh to match the
actual `{ parts }` it now stores. Drop the unused first capture
group from ATTR_WITH_MARKER_RE since rawAttrName has no readers.
- Convert remaining parseInt calls on marker ID parsing paths to
unary plus (renderer.js data-sui-bind entryId, parseAttributeParts
marker capture). Same digit-only inputs, same downstream isNaN
gates — consistent with the earlier sweep.
- Drop comment that restated ATTR_MARKER_PREFIX symbol name.
- Trim bench-hydrate inline narration and hydrateInto caller list.
- DocumentFragment.append accepts variadic nodes — three manual loops collapse to one-liners. hydrateInnerContent's container build, the hydrateInto re-attach, and adoptServerItems' per-item assembly all use the spread form now. - DynamicRegion.placeEndAnchor consolidates the boundary-sentinel logic that setContent and hydrateInto were both running. setContent calls it after attaching its fragment; hydrateInto calls it after the post-hydrate re-attach. Future changes to the sentinel contract land in one place. - Drop the misplaced asChild paragraph from sample.js's hydrate doc — the sample body doesn't pass asChild, so the explanation belongs only at hydrateInto's docblock where the param actually exists. Reword stale BLOCK_CLOSE_PREFIX comment that contradicted formatBlockClose's actual output. Trim past-tense narration on each.js elseScope and renderer.js ownedNodes-rebuild comments.
Surgical revert of the oldKeys-drop change from 6435278. The bench flagged a possible regression on remove-5-back across recent iterations and reconcile is the only remove-path code touched on this branch. Reading r.key directly per iteration (instead of from a cached string array) was theorized to cost a property dereference per access; this restores the cached array shape so the bench can speak. Reverts only the oldKeys portion — keeps the four other items from that commit (inheritsData drop, MARKER_VERSION import, MAIN_BRANCH_INDEX, bench comment refresh).
…reuse
Three CR-driven cleanups:
- parseServerMeta tightens the `bN` matcher to a strict regex so future
reserved comment-data prefixes (e.g. `block`, `bypass`) can't silently
clobber branchIndex via the over-broad `startsWith('b')` test. Allows
the `-1` sentinel emitted when no branch matches.
- The `attributeBinding` wrapper carried only `parts` after the round-2
field drops. Storing parts directly on the entry as `attributeParts`
removes one allocation per dynamic attribute at template-prepare time
and one property hop at hydrate.
- DynamicRegion.clear() detaches endAnchor without nulling it. The Text
node lives for the region's lifetime; placeEndAnchor reuses it on the
next fill instead of allocating a fresh one per setContent cycle.
- build-html-string.js doc still pointed at the old `attributeBinding` field name post-rename to `attributeParts` — refresh the path. - Drop block-depth narration in hydrateMarkers; the surrounding isBlockOpen/isBlockClose checks plus blockDepth++/-- are self-evident. - Drop "Plain AST object — render directly" preamble in renderSubtemplate AST branch; the conditional and identifier carry it. - Phrase the dynamic-region clear() endAnchor note as a forward-looking invariant rather than a contrast against prior behavior.
- renderer.js hydrateInto docblock read as JSDoc for asChild's default;
param name + ternary body carry the same meaning. Score 75 to drop.
- template.js subtemplate-hydrate comment restated the call site
(`self.currentInstance.renderer.{data,scope}`). Score 50.
The other in-flight comment-trim suggestions either kept non-obvious
WHY (template.js snippet line, renderer.js hydrateMarkers strip note),
or were already correctly trimmed in earlier commits. Manual
appendChild loops in extractRangeToFragment / createRecord were
correctly NOT converted (live-walk safety, single-endpoint inserts).
…oc updates
Cold-scorer pass produced 8 actionable findings; this commit lands all
of them.
Source:
- shared/each.js SUI_ITEM_MARKER now interpolates MARKER_VERSION instead
of hardcoding 'v1' (drift hazard the file's own header warns about).
- dynamic-region.js placeEndAnchor doc trimmed; first sentence restated
the method name. Kept the load-bearing strict-between-comparison WHY.
- each.js adoptServerItems comment finishes the WHY (sibling block
markers can interleave between item boundaries).
- template.js snippet hydrate comment carries the structural reason for
asChild=false: child scope would dispose with the next region.clear()
and break args reactivity.
- bench-hydrate.js: drop stale "O(1) on main" / "lazy vs eager" inline
comments that referenced the removed strategy classifier.
Skill docs:
- native-renderer.md: replace literal `1000` with `MAIN_BRANCH_INDEX`;
refresh `attributeBinding: { parts, classification }` description to
the current `attributeParts` shape.
- ssr-hydration.md: same field-name refresh.
- sample.js bag-count docstring said "9 keys" plus a list that omitted hydrateInto. Bag has 11 keys after this PR adds hydrateInto. - DATA_SUI_BIND comment listed `?attr` boolean prefix; server never emits `?` — boolean attributes go through tagBindings as the bare attribute name and are dispatched via classification.type at hydrate.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR is designed to simplify hydration by abstracting helpers used during hydration, adding constants where they were necessary, and creating a specialized helper
hydrateIntothat blocks can use to avoid repeated code.This also modifies code comments to be more precise, and consolidates logic for coordinating
eachblocks on server and client where logic was reproduced, as well as various minor fixes to improve legibility, coherence and simplicity for hydration.Risk
5/10 - renderer is the hot path for everything, but the logic is preserved generally just refactored.
How to Test
Tests handle a lot of coverage but the real confirmation is visual tests across code components in docs, src, examples.