feat(react): route element-template background tree via multi-slot#2651
feat(react): route element-template background tree via multi-slot#2651upupming wants to merge 12 commits into
Conversation
🦋 Changeset detectedLatest commit: 3fb1c6b The changes in this PR will be included in the next version bump. This PR includes changesets to release 0 packagesWhen changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR removes the legacy ChangesMulti-Slot Element Template Migration
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly Related PRs
Suggested Reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
Merging this PR will improve performance by 7.42%
|
| Benchmark | BASE |
HEAD |
Efficiency | |
|---|---|---|---|---|
| ⚡ | 004-various-update__main-thread-setAttribute__MT_Ref |
139.3 µs | 129.7 µs | +7.42% |
Tip
Curious why this is faster? Comment @codspeedbot explain why this is faster on this PR, or directly use the CodSpeed MCP with your agent.
Comparing et-multi-slots (3fb1c6b) with main (2061597)
Footnotes
-
26 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports. ↩
React Example with Element Template#683 Bundle Size — 199.79KiB (-0.14%).3fb1c6b(current) vs 2061597 main#681(baseline) Bundle metrics
|
| Current #683 |
Baseline #681 |
|
|---|---|---|
 Initial JS |
0B |
0B |
| Initial CSS |
0B |
0B |
 Cache Invalidation |
27.15% |
0% |
| Chunks |
0 |
0 |
| Assets |
4 |
4 |
| Modules |
89(-2.2%) |
91 |
| Duplicate Modules |
27 |
27 |
| Duplicate Code |
39.77%(-0.03%) |
39.78% |
| Packages |
2 |
2 |
| Duplicate Packages |
0 |
0 |
Bundle size by type 1 change 
1 improvement
| Current #683 |
Baseline #681 |
|
|---|---|---|
| IMG |
145.76KiB |
145.76KiB |
| Other |
54.03KiB (-0.53%) |
54.32KiB |
Bundle analysis report Branch et-multi-slots Project dashboard
Generated by RelativeCI Documentation Report issue
React MTF Example#1547 Bundle Size — 208.12KiB (0%).3fb1c6b(current) vs 2061597 main#1545(baseline) Bundle metrics
|
| Current #1547 |
Baseline #1545 |
|
|---|---|---|
| Initial JS |
0B |
0B |
| Initial CSS |
0B |
0B |
| Cache Invalidation |
0% |
0% |
| Chunks |
0 |
0 |
| Assets |
3 |
3 |
| Modules |
193 |
193 |
| Duplicate Modules |
77 |
77 |
| Duplicate Code |
44.24% |
44.24% |
| Packages |
2 |
2 |
| Duplicate Packages |
0 |
0 |
Bundle size by type no changes
| Current #1547 |
Baseline #1545 |
|
|---|---|---|
| IMG |
111.23KiB |
111.23KiB |
| Other |
96.89KiB |
96.89KiB |
Bundle analysis report Branch et-multi-slots Project dashboard
Generated by RelativeCI Documentation Report issue
React External#1529 Bundle Size — 695.4KiB (0%).3fb1c6b(current) vs 2061597 main#1527(baseline) Bundle metrics
|
| Current #1529 |
Baseline #1527 |
|
|---|---|---|
| Initial JS |
0B |
0B |
| Initial CSS |
0B |
0B |
| Cache Invalidation |
0% |
0% |
| Chunks |
0 |
0 |
| Assets |
3 |
3 |
| Modules |
17 |
17 |
| Duplicate Modules |
5 |
5 |
| Duplicate Code |
8.59% |
8.59% |
| Packages |
0 |
0 |
| Duplicate Packages |
0 |
0 |
Bundle analysis report Branch et-multi-slots Project dashboard
Generated by RelativeCI Documentation Report issue
Web Explorer#9989 Bundle Size — 901.94KiB (0%).3fb1c6b(current) vs 2061597 main#9986(baseline) Bundle metrics
|
| Current #9989 |
Baseline #9986 |
|
|---|---|---|
| Initial JS |
45.06KiB |
45.06KiB |
| Initial CSS |
2.22KiB |
2.22KiB |
| Cache Invalidation |
0% |
0% |
| Chunks |
9 |
9 |
| Assets |
11 |
11 |
| Modules |
229 |
229 |
| Duplicate Modules |
11 |
11 |
| Duplicate Code |
27.19% |
27.19% |
| Packages |
10 |
10 |
| Duplicate Packages |
0 |
0 |
Bundle size by type no changes
| Current #9989 |
Baseline #9986 |
|
|---|---|---|
| JS |
497.56KiB |
497.56KiB |
| Other |
402.16KiB |
402.16KiB |
| CSS |
2.22KiB |
2.22KiB |
Bundle analysis report Branch et-multi-slots Project dashboard
Generated by RelativeCI Documentation Report issue
React Example#8414 Bundle Size — 237.17KiB (0%).3fb1c6b(current) vs 2061597 main#8412(baseline) Bundle metrics
|
| Current #8414 |
Baseline #8412 |
|
|---|---|---|
| Initial JS |
0B |
0B |
| Initial CSS |
0B |
0B |
| Cache Invalidation |
0% |
0% |
| Chunks |
0 |
0 |
| Assets |
4 |
4 |
| Modules |
198 |
198 |
| Duplicate Modules |
80 |
80 |
| Duplicate Code |
44.73% |
44.73% |
| Packages |
2 |
2 |
| Duplicate Packages |
0 |
0 |
Bundle size by type no changes
| Current #8414 |
Baseline #8412 |
|
|---|---|---|
| IMG |
145.76KiB |
145.76KiB |
| Other |
91.42KiB |
91.42KiB |
Bundle analysis report Branch et-multi-slots Project dashboard
Generated by RelativeCI Documentation Report issue
Element-template's background runtime now consumes Preact's __slotIndex directly instead of going through a BackgroundElementTemplateSlot wrapper. The SWC swc_plugin_element_template lowering emits $0, $1, ... named-slot JSX props in place of __etSlot(N, ...) wrapper-call children, so the main-thread renderToOpcodes and the background insertBefore/removeChild both key insert and remove ops off the child's own slot index. The legacy BackgroundElementTemplateSlot is kept for bundles compiled before this change.
Regenerated via `UPDATE=1 pnpm vitest` after the multi-slot lowering: - `render/*/index.js.txt` switch `children: [...]` to `$0: ...`/`$N: ...` JSX props. - `background/render/.../output.txt` drops the `<slot id=N>` wrappers from the background tree serialization. - `patch-compiled/*/ops.txt` and `hydrate/.../output.txt` reflect the new instance-id ordering now that slot wrappers are no longer registered.
…rtion - mockNativePapi/templateTree.test.ts used a `kind: 'attribute' | 'spread', binding: ...` schema that doesn't match what the SWC transform emits (`kind: 'static' | 'slot' | 'spread'`), so the spread-vs-slot ordering case asserted the wrong descriptor was being treated as `spread`. Align with the production schema. - webpack/react-webpack-plugin's background-loader test required an `@lynx-js/react/element-template[/internal]` import to confirm ET was active, which only existed because the old lowering emitted an `__etSlot(N, ...)` runtime call. With multi-slot lowering the import is no longer needed; assert on the `_et_<hash>_` template-id prefix instead.
…anches
- BackgroundElementTemplateInstance.insertBefore/removeChild no longer fall
through to a trailing `if (silent) return` after the multi-slot branch
short-circuits — the implicit return at end-of-function is equivalent.
- Mark the `slotId < 0` guard in syncMultiSlotChildren and the non-numeric
`$N` slot id branch in render-to-opcodes.ts with `v8 ignore` since the
transform never emits them.
- Add adapter tests for `createElementNS('slot')` and a legacy-slot
`insertBefore` test with a non-null anchor so the slot wrapper code paths
stay covered at 100%.
The slot wrapper class only existed to back the old `__etSlot(N, ...)` runtime call emitted by the pre-multi-slot lowering. The new lowering does not emit it, no consumer creates `<slot>` JSX, and `experimental_useElementTemplate` has no published bundles relying on the old path — so remove the class outright. - `BackgroundElementTemplateInstance.insertBefore`/`removeChild` collapse to the single multi-slot path keyed off `child.__slotIndex`. - `document.ts` no longer special-cases `type === 'slot'`. - `__etSlot()` runtime component and its `internal.ts` re-export are deleted. - Tests/fixtures that built trees via slot wrappers were rewritten to attach children directly with `__slotIndex`; obsolete describe blocks asserting slot-wrapper internals were dropped. - Four hydrate fixture `output.txt`s were regenerated because removed slot wrappers no longer consume instance ids.
`children.iterates-existing-slots` and `children.missing-attrs-element` attached then silently removed dummy children only to materialize empty slot indexes on `instance.elementSlots`. The hydrate loop iterates over `Math.max(serialized.length, instance.length)` and treats an empty serialized `[]` slot as truthy, so background-side materialization isn't required — the loop fires either way and the test outcomes are identical without the dance.
upupming
requested review from
HuJean,
Yradex,
colinaaa,
gaoachao,
hzy and
luhc228
as code owners
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 99a30b465f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.changeset/et-multi-slots.md:
- Around line 1-5: The changeset file currently contains only the YAML
delimiters `---` with no package entries between them; update
`.changeset/et-multi-slots.md` to follow project conventions by either removing
the changeset entirely if no package needs a version bump, or add at least one
package entry and version (e.g., add an entry like `"`@lynx-js/`<package-name>":
patch`) between the `---` delimiters so the changeset tool recognizes it; ensure
the top/bottom `---` remain and the package key is a valid package name from the
repo.
In `@packages/react/runtime/src/element-template/background/instance.ts`:
- Around line 196-200: The code sets beforeId = 0 for cross-slot anchors (when
beforeChild.__slotIndex !== slotId) but still inserts locally using beforeChild,
causing elementSlots[slotId] to diverge; update the insertion logic in the same
routine (where beforeChild, beforeId, elementSlots, slotId, and instanceId are
used) so that when beforeChild.__slotIndex !== slotId you treat the anchor as
null for both emitted op and local mutation — i.e., use the same fallback
(append at tail) for elementSlots[slotId] insertion as you do for beforeId to
keep local and main-thread order consistent.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 35d39757-cd1f-4d72-ad69-b9dc38927c22
⛔ Files ignored due to path filters (9)
packages/react/transform/crates/swc_plugin_element_template/tests/__combined_snapshots__/should_handle_deeply_nested_user_components.snapis excluded by!**/*.snappackages/react/transform/crates/swc_plugin_element_template/tests/__combined_snapshots__/should_handle_interpolated_text_with_siblings_js.snapis excluded by!**/*.snappackages/react/transform/crates/swc_plugin_element_template/tests/__combined_snapshots__/should_handle_interpolated_text_with_siblings_lepus.snapis excluded by!**/*.snappackages/react/transform/crates/swc_plugin_element_template/tests/__combined_snapshots__/should_handle_mixed_content.snapis excluded by!**/*.snappackages/react/transform/crates/swc_plugin_element_template/tests/__combined_snapshots__/should_handle_nested_structure_and_dynamic_content.snapis excluded by!**/*.snappackages/react/transform/crates/swc_plugin_element_template/tests/__combined_snapshots__/should_handle_sibling_user_components.snapis excluded by!**/*.snappackages/react/transform/crates/swc_plugin_element_template/tests/__combined_snapshots__/should_handle_user_component.snapis excluded by!**/*.snappackages/react/transform/crates/swc_plugin_element_template/tests/__combined_snapshots__/should_isolate_arrays_with_element_slot_placeholder.snapis excluded by!**/*.snappackages/react/transform/crates/swc_plugin_element_template/tests/__combined_snapshots__/should_verify_text_attribute_and_child_text_slots.snapis excluded by!**/*.snap
📒 Files selected for processing (42)
.changeset/et-multi-slots.mdpackages/react/runtime/__test__/element-template/fixtures/background/instance/_shared.tspackages/react/runtime/__test__/element-template/fixtures/background/instance/ops/supports-silent-insert-before/case.tspackages/react/runtime/__test__/element-template/fixtures/background/render/supports-slot-component-materiality/output.txtpackages/react/runtime/__test__/element-template/fixtures/hydrate/background-hydrate-compiled/children.creates-and-inserts-new/output.txtpackages/react/runtime/__test__/element-template/fixtures/hydrate/background-hydrate-compiled/children.inserts-before-existing-sibling/output.txtpackages/react/runtime/__test__/element-template/fixtures/hydrate/background-hydrate-compiled/children.mixed-operations/output.txtpackages/react/runtime/__test__/element-template/fixtures/hydrate/background-hydrate-compiled/children.non-string-raw-text-key-on-main/output.txtpackages/react/runtime/__test__/element-template/fixtures/hydrate/background-hydrate-compiled/complex-trees.deeply-nested-dynamic-content/output.txtpackages/react/runtime/__test__/element-template/fixtures/hydrate/background-hydrate/_shared.tsxpackages/react/runtime/__test__/element-template/fixtures/hydrate/background-hydrate/children.creates-missing-nodes-recursively/output.txtpackages/react/runtime/__test__/element-template/fixtures/hydrate/background-hydrate/children.missing-slot-record-on-main/output.txtpackages/react/runtime/__test__/element-template/fixtures/hydrate/background-hydrate/coverage.move-before-child/output.txtpackages/react/runtime/__test__/element-template/fixtures/hydrate/background-hydrate/coverage.raw-text-key-branches/output.txtpackages/react/runtime/__test__/element-template/fixtures/patch-compiled/applies-insert-before-with-reference/ops.txtpackages/react/runtime/__test__/element-template/fixtures/patch-compiled/apply-hydration-ops/ops.txtpackages/react/runtime/__test__/element-template/fixtures/render/child-siblings/index.js.txtpackages/react/runtime/__test__/element-template/fixtures/render/component-slot-content/index.js.txtpackages/react/runtime/__test__/element-template/fixtures/render/component/index.js.txtpackages/react/runtime/__test__/element-template/fixtures/render/mapped-view-children/index.js.txtpackages/react/runtime/__test__/element-template/fixtures/render/mixed-children/index.js.txtpackages/react/runtime/__test__/element-template/fixtures/render/multiple-text/index.js.txtpackages/react/runtime/__test__/element-template/fixtures/render/nested-templates/index.js.txtpackages/react/runtime/__test__/element-template/fixtures/render/react-example/index.js.txtpackages/react/runtime/__test__/element-template/runtime/background/adapter/background-adapter.et.test.tsxpackages/react/runtime/__test__/element-template/runtime/background/commit-context.test.tspackages/react/runtime/__test__/element-template/runtime/background/hydrate.test.tspackages/react/runtime/__test__/element-template/runtime/background/instance.test.tspackages/react/runtime/__test__/element-template/runtime/components/slot.test.tspackages/react/runtime/__test__/element-template/runtime/hydration/hydration-listener.test.tspackages/react/runtime/__test__/element-template/runtime/render/render-to-opcodes.et.test.jsxpackages/react/runtime/__test__/element-template/test-utils/mock/mockNativePapi/templateTree.test.tspackages/react/runtime/src/element-template/background/document.tspackages/react/runtime/src/element-template/background/instance.tspackages/react/runtime/src/element-template/internal.tspackages/react/runtime/src/element-template/runtime/components/slot.tspackages/react/runtime/src/element-template/runtime/render/render-to-opcodes.tspackages/react/transform/crates/swc_plugin_element_template/lib.rspackages/react/transform/crates/swc_plugin_element_template/lowering.rspackages/react/transform/crates/swc_plugin_element_template/slot.rspackages/react/transform/crates/swc_plugin_element_template/tests/element_template.rspackages/webpack/react-webpack-plugin/test/background-loader.test.ts
💤 Files with no reviewable changes (5)
- packages/react/runtime/test/element-template/runtime/components/slot.test.ts
- packages/react/runtime/src/element-template/runtime/components/slot.ts
- packages/react/runtime/src/element-template/internal.ts
- packages/react/transform/crates/swc_plugin_element_template/slot.rs
- packages/react/transform/crates/swc_plugin_element_template/lib.rs
…-tail When `parent.insertBefore(child, beforeChild)` is called with a cross-slot `beforeChild`, the op emits `beforeId = 0` so the main-thread slot appends `child` to its tail. The local sibling chain previously still inserted `child` literally before `beforeChild`, which made `syncMultiSlotChildren` filter the chain by `__slotIndex` into `[..., child, existingSlotTail]` while main thread saw `[..., existingSlotTail, child]`. Subsequent ops keyed off the local order would then desync from main. Treat a cross-slot `beforeChild` as null for the linked-list mutation too, so both sides see append-tail. Same-slot anchors still honor the insertion position. Reported by CodeRabbit on PR #2651.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b295b6e63b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // it pending and tear it down on the Snapshot-aligned delayed boundary. | ||
| markRemovedSubtreeForCurrentCommit(child); | ||
| } | ||
| const slotId = child.__slotIndex; |
There was a problem hiding this comment.
I don't think syncing only child.__slotIndex is enough here. For a moved existing child, the slot index can already point to the destination slot by the time removeChild() runs, so the previous source slot can keep a stale reference.
Can we either track the slot the child was attached under, or resync both/all affected slot arrays after a move?
There was a problem hiding this comment.
Seems
root.render(<_et_host $2={<_et_child key="moved" />} />)
root.render(<_et_host $0={<_et_child key="moved" />} />)
results in host slots
[ [ '_et_child:3:0' ], <1 empty item>, [ '_et_child:3:0' ] ]
and the node is not removed from elementSlots[2]
There was a problem hiding this comment.
Good catch — confirmed and fixed in 1c9d75c. Preact's insert() mutates child.__slotIndex to the destination before our insertBefore runs, which triggers child.parent.removeChild(child, true) for the rebind. Reading child.__slotIndex inside removeChild only sees the NEW slot, so the source slot stayed stale.
Updated removeChild to gather every elementSlots[i] that still mentions the child, then resync all of them:
const slotsToResync = new Set<number>([child.__slotIndex]);
for (let i = 0; i < this.elementSlots.length; i += 1) {
if (this.elementSlots[i]?.includes(child)) {
slotsToResync.add(i);
}
}
for (const id of slotsToResync) {
syncMultiSlotChildren(this, id);
}This handles both directions (source slot → destination slot, and the in-place no-op when there's no move). Two regression tests added in instance.test.ts (per your other comment) — see reply on instance.test.ts:1163.
There was a problem hiding this comment.
How about
root.render(<_et_host $1={<_et_child key="moved" />} />)
root.render(<_et_host $0={<_et_child key="moved" />} />)
There was a problem hiding this comment.
Tried the root.render(<_et_host $1={...} />) → root.render(<_et_host $0={...} />) shape end-to-end, but Preact's findMatchingIndex short-circuits before the keyed-cross-slot path is taken in this shape. Walking the diff for the JSX
function MovingChild({ slot }) {
const moved = <text key="moved">moved</text>;
return (
<view>
<view id="top">{slot === 0 ? moved : null}</view>
<view id="bottom">{slot === 1 ? moved : null}</view>
</view>
);
}which compiles to _jsx(_et_host, { $0: slot === 0 ? moved : null, $1: slot === 1 ? moved : null }):
- Render 1 (
slot=1) setsnewChildren = [null, movedVNode]. Preact mounts moved DOM at slotIndex 1. - Render 2 (
slot=0) setsnewChildren = [movedVNode2, null]andoldChildren = [null, oldMoved]. diffChildrenwalksi=0:findMatchingIndex(newMoved, oldChildren, skewedIndex=0, …)evaluates the first branch:if (oldVNode === NULL || oldVNode && key == oldVNode.key && …) return skewedIndex;
oldChildren[0]isnull, so it returns0immediately — no keyed search across positions. The new moved diffs againstEMPTY_OBJ(fresh mount at slot 0), and the old moved at index 1 falls to the trailing unmount loop.
Empirically I confirmed the same: after render(slot=0) the previously-rendered _et_text#3 is still at host.elementSlots[1] with __slotIndex=1 — Preact never observes a cross-slot move because null at the new home index acts as a "claim this position freshly". (Slot-1 cleanup happens through the regular insertion in slot 0 + unmount of the old at slot 1, which doesn't exercise the bug.)
For the bug to surface end-to-end I'd need Preact to update an existing DOM's __nextSlotIndex while keeping the same __e. The cleanest place where that does happen is when Preact's diff matches a keyed child whose new old position is a real (non-null) old vnode of a different __slotIndex — which is what compiled-keyed-list / compiled-keyed-subtree-list already cover for in-slot moves.
The two instance.test.ts regressions (clears the source slot when an existing child moves to an adjacent slot / … non-adjacent slot) drive insertBefore → removeChild with child.__slotIndex mutated in advance, which is exactly the post-insert() state Preact lands the runtime in for a cross-slot keyed move — so they cover the bug's mechanism without needing the artificial Preact shape. Happy to look at a different scenario if you have one in mind that would surface here at the root.render level.
There was a problem hiding this comment.
You were right — chased it down. Added an end-to-end test in 3fb1c6b that does drive the rebind through Preact's diff, plus a fuller writeup of what I found:
Two-element swap doesn't surface the fix. With [null, X] → [X, null] (my first attempt) findMatchingIndex short-circuits on oldChildren[0] === NULL and returns skewedIndex immediately; Preact then unmounts the old X at index 1 and fresh-mounts at 0. With [a, b] → [b, a] (the swap you proposed) findMatchingIndex does find the key, but matchingIndex == skewedIndex ± 1 triggers Preact's skew shortcut — __nextSlotIndex is set on the moved DOM but insert() is not called for it (only nodes flagged INSERT_VNODE go through insert() in constructNewChildrenArray → diffChildren). So __slotIndex is never propagated and the runtime never observes a rebind.
Three-element rotate is the smallest shape that hits the fix. [a, b, c] → [c, a, b] makes c's match land at skewedIndex + 2, falls into the else branch in constructNewChildrenArray that sets INSERT_VNODE, so insert() runs the cross-slot branch (__nextSlotIndex != __slotIndex), mutates c.__slotIndex to the destination, and calls parentDom.insertBefore — which our insertBefore handles via removeChild(c, silent=true). The test asserts host.elementSlots[2] is cleared even though only that silent removeChild ran for the rebind, which is exactly what the source-slot resync delivers.
The skew == ±1 shortcut leaving a/b at their old __slotIndex after the rotate is a separate upstream Preact concern (the optimization assumes single-DOM-container semantics and doesn't propagate per-slot indices). Outside this PR's scope.
|
|
||
| expect(parent.elementSlots[0]).toEqual([slot0Tail, incoming]); | ||
| expect(parent.lastChild).toBe(incoming); | ||
| }); |
There was a problem hiding this comment.
Can we add coverage for moving an already-attached child between slots? The important assertions are that the destination slot gets the child and the old source slot no longer contains it. It would be good to cover both adjacent and non-adjacent slot changes.
There was a problem hiding this comment.
Added in 1c9d75c, covering both adjacent (0 → 1) and non-adjacent (0 → 3) cross-slot moves:
it('clears the source slot when an existing child moves to an adjacent slot', () => {
const parent = new BackgroundElementTemplateInstance('view');
const child = new BackgroundElementTemplateInstance('text');
child.__slotIndex = 0;
parent.appendChild(child);
expect(parent.elementSlots[0]).toEqual([child]);
// Preact's `insert()` updates the child's `__slotIndex` to the destination
// before re-attaching it via parent.insertBefore.
child.__slotIndex = 1;
parent.insertBefore(child, null);
expect(parent.elementSlots[0]).toEqual([]);
expect(parent.elementSlots[1]).toEqual([child]);
});
it('clears the source slot when an existing child moves to a non-adjacent slot', () => {
// ... 0 → 3, with assertions on each intervening slot
expect(parent.elementSlots[0]).toEqual([]);
expect(parent.elementSlots[1]).toBeUndefined();
expect(parent.elementSlots[2]).toBeUndefined();
expect(parent.elementSlots[3]).toEqual([child]);
});Both fail before the removeChild fix and pass after.
| if (hasNamedSlotChildren) { | ||
| renderEtSlotArray(namedSlotChildren, context, vnode, opcodes); | ||
| } else { | ||
| // Legacy ET host nodes still encode dynamic element slots in `children`, |
There was a problem hiding this comment.
Do we still need the legacy children slot-array fallback here? If ET hosts are now expected to use $N props only, I think this fallback can be removed so the renderer has a single input contract.
There was a problem hiding this comment.
Agreed — removed in 1c9d75c. The renderer now only consumes $N props; the body simplified to:
let namedSlotChildren;
for (const name in props) {
if (typeof name !== 'string' || name.charCodeAt(0) !== 36 /* '$' */) continue;
const slotId = +name.slice(1);
if (!(slotId >= 0)) continue;
(namedSlotChildren ??= [])[slotId] = props[name];
}
if (namedSlotChildren !== undefined) {
renderEtSlotArray(namedSlotChildren, context, vnode, opcodes);
}If a future call hands the renderer a host node without $N props (and without children), it simply emits a __OpBegin/__OpEnd pair — same as before. Single input contract going forward.
| ]); | ||
| }); | ||
|
|
||
| it('prefers $N named props over legacy children when both are present', () => { |
There was a problem hiding this comment.
If we remove the legacy children fallback, this coverage should go away too. The renderer tests should focus on the $N input shape instead of keeping the old slot-array contract alive.
There was a problem hiding this comment.
Done in 1c9d75c. Dropped both legacy-shape tests (should emit slot opcodes for ET host slot arrays, should skip empty slot entries when rendering ET host slot arrays) and the prefers $N over legacy children precedence test — none of those input shapes are reachable now. The remaining renderer tests focus on the $N contract:
emits slot opcodes for ET host nodes using $N named propsskips empty $N slots in ET host nodes(the sparse-slot case, e.g. only$1and$4set)
| slot0.setAttribute('id', 0); | ||
| rootInstance.appendChild(slot0); | ||
| const slot1 = new BackgroundElementTemplateSlot(); | ||
| slot1.setAttribute('id', 1); |
There was a problem hiding this comment.
This fixture name looks stale now. The setup no longer creates any existing background slot entry, so it is not really testing “existing slots” anymore. Can we either rename it or materialize an actual empty/sparse background slot?
There was a problem hiding this comment.
Renamed in 1c9d75c to children.no-op-when-both-sides-have-empty-slots. The setup matches the new name — rootInstance.elementSlots starts empty, serialized side ships [[], []], and the assertion verifies the hydrate loop iterates both empty arrays without emitting anything. Fixture directory and runCaseByName(...) reference were updated together.
There was a problem hiding this comment.
Following up — after the rename @upupming noticed offline that the renamed case was now functionally identical to children.missing-attrs-element (both ended up as trivial no-ops once the slot-wrapper materialization was removed). Reverted the rename in ebdef8f and instead made the test actually exercise the Math.max(serializedLen, instanceLen) branch in hydrate.ts:
const rootInstance = new BackgroundElementTemplateInstance('root');
const slot1Child = new BackgroundElementTemplateInstance('child');
slot1Child.__slotIndex = 1;
rootInstance.appendChild(slot1Child);
const before = createHydrationTemplate(-1, 'root', { elementSlots: [[]] });Background side has a child at slot 1, serialized only ships slot 0. The hydrate loop now visits slot 1 (background-only) and emits an insertion for the background-only child:
stream: [
1, 2, "child", null, [], [], // createTemplate
3, -1, 1, 2, 0, // insertNode(parent=-1, slot=1, child=2, beforeId=0)
]
Original name preserved, semantic now matches it.
| // Multi-slot lowering no longer needs an `__etSlot` runtime call, so the | ||
| // ET path is now identified by the `_et_` template id prefix rather than | ||
| // by an `@lynx-js/react/element-template[/internal]` import. | ||
| expect(result.code).toMatch(/_et_[a-z0-9]+_/); |
There was a problem hiding this comment.
This test name does not quite match the assertion anymore. It no longer checks for an ET runtime import; it checks that ET transform happened via the generated template id. Maybe rename it, and add a negative assertion for __etSlot / element-template internal imports.
There was a problem hiding this comment.
Done in 1c9d75c. Renamed the test to routes ET background output through Preact JSX and the multi-slot lowering when ET is enabled and reorganized the assertions:
- Positive: stays on
@lynx-js/react/jsx-runtime, produces_et_<hash>_template ids. - Negative: no
__etSlot, no@lynx-js/react/internal, no@lynx-js/react/element-template/internal, no@lynx-js/react/element-template. None of these should ever appear in multi-slot ET output.
…hten contract Round of follow-ups for Yradex's review on PR #2651: - `BackgroundElementTemplateInstance.removeChild` now resyncs every elementSlots entry that still references the child, not just the slot the child currently believes it's in. Preact's `insert()` mutates `child.__slotIndex` to the destination before calling our `insertBefore`, so the rebind path's `removeChild(child, true)` would previously leave a stale reference in the source slot. Added two regression tests covering adjacent (0→1) and non-adjacent (0→3) cross-slot moves. - Drop the legacy `children` slot-array fallback in `render-to-opcodes.ts` and its accompanying tests. The transform only emits `$N` named props now, so the renderer has a single input contract. - Rename `children.iterates-existing-slots` hydrate fixture to `children.no-op-when-both-sides-have-empty-slots` to reflect that the background side no longer materializes empty slot entries. - Rename the webpack `background-loader` test and add explicit negative assertions for `__etSlot` and the legacy ET internal/external imports.
…ningful The renamed `children.no-op-when-both-sides-have-empty-slots` case was functionally identical to `children.missing-attrs-element` — both ran with an empty background instance and a serialized payload that shipped an empty slot array. Restore the original name and give it the setup it was always supposed to cover: background side carries a child at slot 1 while serialized side only reports slot 0, exercising the `slotCount = Math.max(serializedLen, instanceLen)` branch in `hydrate.ts` so the background-only slot still emits its child as an insertion to the main thread.
…hot path
Match the shape used in `packages/react/runtime/src/snapshot/renderToOpcodes/index.ts`:
```ts
for (const name in props) {
if (name.startsWith('$')) {
(namedSlotChildren ??= [])[+name.slice(1)] = props[name];
}
}
```
The transform only emits `$<int>` props, so the defensive `slotId >= 0`
guard and the `charCodeAt` micro-optimization were never reachable in
practice. Same behavior, less code.
…ld fix Per Yradex's suggestion: cover the source-slot cleanup end-to-end through Preact's diff rather than only at the `insertBefore`/`removeChild` unit level. A simple two-slot swap doesn't surface the fix because Preact's `findMatchingIndex` returns the skewedIndex immediately when the old slot at that position is null, and the `skew==±1` optimization sets the new `__nextSlotIndex` but skips `insert()` — leaving `__slotIndex` (and our slot ops) stale. Both are upstream Preact concerns separate from this PR. A three-element rotate (`[a, b, c] → [c, a, b]`) shifts `c` by 2 positions, which lands on the `INSERT_VNODE` path and reaches our runtime through `parentDom.insertBefore`. The new test confirms that: - `c.__slotIndex` is updated to its new home (slot 0). - `host.elementSlots[2]` is empty even though Preact only fires the rebind via `removeChild(c, silent=true)` — exactly the path the source-slot resync was added for. `a` and `b` stay at their old slot indices because Preact takes the `skew==±1` shortcut for them; that's outside this fix's scope.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3fb1c6b28d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| beforeId, | ||
| ); | ||
| } | ||
| syncMultiSlotChildren(this, slotId); |
There was a problem hiding this comment.
When keyed children swap between two $N slots on the same ET host, internal Preact can take its adjacent-move shortcut without calling this insertBefore path (the new Preact test even notes that only the non-adjacent child trips INSERT_VNODE). In that scenario __slotIndex and elementSlots are never resynced, so a render like slot0=a, slot1=b followed by slot0=b, slot1=a leaves the background slot map in the old order and subsequent hydrate/update ops patch the wrong slot contents. The slot resync needs to cover the adjacent keyed path as well, not only insertions that reach insertBefore.
Useful? React with 👍 / 👎.
2 participants
Summary
Applies the multi-slot pattern from #1764 to element-template mode so the background tree no longer needs
BackgroundElementTemplateSlotwrappers between a template instance and its dynamic children.BackgroundElementTemplateInstancenow carries__slotIndex(set by Preact via$0/$1props);insertBefore/removeChildroute(parent, slotId, child)ops off that field directly — no<slot>wrapper required.renderToOpcodesrecognizes$Nprops and emits__OpSlot Nopcodes; legacychildrenarray stays as fallback for older bundles.swc_plugin_element_templatelowering emits$N={...}JSX attrs instead of__etSlot(N, ...)wrapper calls.slot.rsis deleted;__etSlotruntime export is retained for legacy bundles.Test plan
cargo test -p swc_plugin_element_template— 33 + 16 passing (added two new transform tests verifying$Nlowering for JS and LEPUS targets)CI=true pnpm vitest --config __test__/element-template/vitest.config.ts --run __test__/element-template/runtime/background/instance.test.ts— 76 passing (added 4 multi-slot insert/remove + cross-slot anchor tests)CI=true pnpm vitest --config __test__/element-template/vitest.config.ts --run __test__/element-template/runtime/render/render-to-opcodes.et.test.jsx— 17 passing (added 2 named-slot prop tests)CI=true pnpm vitest --config __test__/element-template/vitest.config.ts --run __test__/element-template/runtime/background/hydrate.test.ts— 31 passingCI=true pnpm vitest --runinpackages/react/runtime— 612 passing (standard snapshot suite unaffected)INSTA_UPDATE=alwaysregenerated 9__combined_snapshots__/*.snapfixtures — verified outputs are<_et_xxx $0={...} $1={...} />with no__etSlotimportThe ET suite still has 69 pre-existing failures (compiled-fixtures, hydration fixtures, mock template tree) that reproduce identically on
origin/mainwithout this change; they are environment /ReactLynx.snapshotCreatorMapsetup issues outside the scope of this PR.Summary by CodeRabbit