diff --git a/.changeset/reconcile-walk-bound.md b/.changeset/reconcile-walk-bound.md new file mode 100644 index 0000000000..443363038e --- /dev/null +++ b/.changeset/reconcile-walk-bound.md @@ -0,0 +1,5 @@ +--- +"@solidjs/web": patch +--- + +The keyed reconciler is back on the slot-ownership design from May: the common-prefix and common-suffix walks match by identity, with no per-node liveness check. #574 — a node dropped after a replace plus an end-swap — is fixed at its cause instead: the suffix walk is bounded to the open range, so it can no longer reach back into entries the pass already consumed. The append branch anchors on a node the slot still owns, so a node user code moved away cannot make an insert throw. In the map branch, an old node that is no longer ours is consumed rather than "replaced" by an insert at the region's end, so the changed window lands in list order around a moved-away node. A node moved away that sits in the unchanged prefix or suffix stays where it was moved, as it did before the #574 patch. A 1000-row swap drops from about 30 to 25 µs; long reorders and memo-wall's one-change update 10–50%. diff --git a/packages/web/src/reconcile.ts b/packages/web/src/reconcile.ts index a70af11004..89f0809ae9 100644 --- a/packages/web/src/reconcile.ts +++ b/packages/web/src/reconcile.ts @@ -11,9 +11,8 @@ export default function reconcileArrays( export default function reconcileArrays(parentNode: any, a: any[], b: any[], marker?: any) { // Ownership: an unclaimed node (no `$$SLOT`) is fair game; a tagged node // belongs only to the slot whose marker matches. `a[]` can name a node that - // replace/swap already took out of this parent (or out of this slot). - // Prefix/suffix must not treat those as still-live common ends — that was - // the 2fe6310f drop (#574). + // user code moved out of this parent (or into another slot of it); every + // anchor read and destructive op below checks before touching one. const isLive = n => { if (!n) return false; const tag = n[$$SLOT]; @@ -34,14 +33,20 @@ export default function reconcileArrays(parentNode: any, a: any[], b: any[], mar anchorTag; while (aStart < aEnd || bStart < bEnd) { - // common prefix - if (a[aStart] === b[bStart] && isLive(a[aStart])) { + // common prefix / suffix. Neither walk checks liveness: inside the open + // range [aStart, aEnd) this pass never detaches a node (replace, remove + // and swap all consume the node they take out), and a node user code + // migrated away is left alone by skipping it here — every destructive + // branch below still gates on ownership. The suffix walk is bounded to + // the open range: unbounded, it could reach back past `aStart` to a node + // a map-branch replace had already taken out while it was still pending + // in `b`, count it as a live common end, and never reinsert it (#574). + if (a[aStart] === b[bStart]) { aStart++; bStart++; continue; } - // common suffix - while (a[aEnd - 1] === b[bEnd - 1] && isLive(a[aEnd - 1])) { + while (aEnd > aStart && bEnd > bStart && a[aEnd - 1] === b[bEnd - 1]) { aEnd--; bEnd--; } @@ -49,10 +54,17 @@ export default function reconcileArrays(parentNode: any, a: any[], b: any[], mar if (aEnd === aStart) { let node; if (bEnd < bLength) { - if (bStart) { - const prev = b[bStart - 1]; - node = isLive(prev) ? prev.nextSibling : after; - } else node = b[bEnd - bStart]; + // The walks no longer check liveness, so the prefix node before the + // insert and the suffix's first node can each be one user code moved + // away: anchoring on it threw (NotFoundError) or, falling back to + // `after`, landed the insert past the live suffix. Anchor on the last + // live prefix node's successor, else the first suffix node still ours. + if (bStart && isLive((node = b[bStart - 1]))) node = node.nextSibling; + else { + let k = bEnd; + while (k < bLength && !isLive(b[k])) k++; + node = k < bLength ? b[k] : after; + } } else node = after; while (bStart < bEnd) { @@ -102,6 +114,14 @@ export default function reconcileArrays(parentNode: any, a: any[], b: any[], mar } // fallback to map } else { + // An old node that is no longer ours holds no position in this region: + // it can neither be replaced nor anchor an insert — both used to put the + // new node at the region's end, out of order. Consume it and let the new + // node land when its own turn comes. + if (!isLive(a[aStart])) { + aStart++; + continue; + } if (!map) { map = new Map(); let i = bStart; @@ -122,28 +142,19 @@ export default function reconcileArrays(parentNode: any, a: any[], b: any[], mar } if (sequence > index - bStart) { - const head = a[aStart]; - const node = isLive(head) ? head : after; + const node = a[aStart]; while (bStart < index) { const n = b[bStart++]; parentNode.insertBefore(n, node); if (marker) n[$$SLOT] = marker; } } else { - const oldNode = a[aStart++]; const newNode = b[bStart++]; - if (isLive(oldNode)) { - parentNode.replaceChild(newNode, oldNode); - } else { - parentNode.insertBefore(newNode, after); - } + parentNode.replaceChild(newNode, a[aStart++]); if (marker) newNode[$$SLOT] = marker; } } else aStart++; - } else { - const n = a[aStart++]; - if (isLive(n)) n.remove(); - } + } else a[aStart++].remove(); } } } diff --git a/packages/web/test/runtime/insert.spec.js b/packages/web/test/runtime/insert.spec.js index 6ee010fe14..36edd4b6db 100644 --- a/packages/web/test/runtime/insert.spec.js +++ b/packages/web/test/runtime/insert.spec.js @@ -767,6 +767,136 @@ describe("r.insert with migrating nodes", () => { expect(parent.contains(C)).toBe(true); }); + // The migration rule from the slot-ownership change (May 2026): nothing + // throws, a node moved out of the slot is never destroyed by it, and every + // destructive op touches only nodes the slot owns. The prefix and suffix + // walks match by identity alone, as they did then: a node moved away that + // sits in the unchanged prefix or suffix stays where it was moved. Nodes in + // the changed window are placed in list order. + it("a moved-away node in the unchanged prefix stays where it was moved", () => { + const parent = document.createElement("div"); + const other = document.createElement("div"); + const mk = t => Object.assign(document.createElement("span"), { textContent: t }); + const [n1, n2, n3, n4] = ["1", "2", "3", "4"].map(mk); + [n1, n2, n3].forEach(n => parent.appendChild(n)); + other.appendChild(n2); + reconcileArrays(parent, [n1, n2, n3], [n1, n2, n3, n4]); + expect([...parent.children].map(n => n.textContent)).toEqual(["1", "3", "4"]); + expect(other.contains(n2)).toBe(true); + }); + + it("a removed node in the unchanged suffix stays removed", () => { + const parent = document.createElement("div"); + const mk = t => Object.assign(document.createElement("span"), { textContent: t }); + const [n0, n1, n2, n3] = ["0", "1", "2", "3"].map(mk); + [n1, n2, n3].forEach(n => parent.appendChild(n)); + n3.remove(); + reconcileArrays(parent, [n1, n2, n3], [n0, n1, n2, n3]); + expect([...parent.children].map(n => n.textContent)).toEqual(["0", "1", "2"]); + expect(n3.parentNode).toBe(null); + }); + + it("a moved-away node the new list drops stays where it was moved", () => { + const parent = document.createElement("div"); + const other = document.createElement("div"); + const mk = t => Object.assign(document.createElement("span"), { textContent: t }); + const [a1, a2, a3, a4] = ["1", "2", "3", "4"].map(mk); + [a1, a2, a3].forEach(n => parent.appendChild(n)); + other.appendChild(a2); + reconcileArrays(parent, [a1, a2, a3], [a3, a4, a1]); + expect([...parent.children].map(n => n.textContent)).toEqual(["3", "4", "1"]); + expect(other.contains(a2)).toBe(true); + }); + + // A dead old node in the map branch used to be "replaced" by inserting the + // new node at the region's end: [a, b] with a moved away, then [d, b, a], + // came out [b, d, a]. The dead node is now consumed and d lands in order. + it("the changed window is placed in list order around a moved-away node", () => { + const parent = document.createElement("div"); + const other = document.createElement("div"); + const mk = t => Object.assign(document.createElement("span"), { textContent: t }); + const [a, b, d] = ["a", "b", "d"].map(mk); + parent.appendChild(a); + parent.appendChild(b); + other.appendChild(a); + reconcileArrays(parent, [a, b], [d, b, a]); + expect([...parent.children].map(n => n.textContent)).toEqual(["d", "b", "a"]); + }); + + // With the walks matching by identity, the node the append branch anchors + // on can be one user code moved away. Anchoring on it threw NotFoundError + // and halted reactivity; falling back to the region end put the insert past + // the live suffix. + it("reconcileArrays does not anchor an insert on a migrated suffix node", () => { + const parent = document.createElement("div"); + const other = document.createElement("div"); + const mk = t => Object.assign(document.createElement("span"), { textContent: t }); + const x = mk("x"), + y = mk("y"); + other.appendChild(x); + expect(() => reconcileArrays(parent, [x], [y, x])).not.toThrow(); + expect(parent.contains(y)).toBe(true); + expect(other.contains(x)).toBe(true); + }); + + it("a prepend over a migrated tail stays in its slot and keeps reacting", () => { + // via insert: render [z, x]; user code moves x into a modal; then [y, x]. + const mk = t => Object.assign(document.createElement("span"), { textContent: t }); + const x = mk("x"), + y = mk("y"), + z = mk("z"), + w = mk("w"); + const parent = document.createElement("div"); + const before = parent.appendChild(mk("before")); + const marker = parent.appendChild(document.createTextNode("")); + const after = parent.appendChild(mk("after")); + const other = document.createElement("div"); + const [list, setList] = createSignal([z, x]); + createRoot(() => r.insert(parent, list, marker)); + flush(); + other.appendChild(x); + setList([y, x]); + expect(() => flush()).not.toThrow(); + const kids = [...parent.childNodes]; + expect(kids.indexOf(before)).toBeLessThan(kids.indexOf(y)); + expect(kids.indexOf(y)).toBeLessThan(kids.indexOf(marker)); + expect(kids.indexOf(marker)).toBeLessThan(kids.indexOf(after)); + expect(parent.contains(z)).toBe(false); + // still reactive after the transition + setList([y, w]); + flush(); + expect(parent.contains(w)).toBe(true); + expect(kids.indexOf(before)).toBeLessThan([...parent.childNodes].indexOf(w)); + }); + + it("an insert after a migrated prefix node lands before the live suffix", () => { + const parent = document.createElement("div"); + const other = document.createElement("div"); + const mk = t => Object.assign(document.createElement("span"), { textContent: t }); + const x = mk("x"), + y = mk("y"), + s = mk("s"); + parent.appendChild(x); + parent.appendChild(s); + other.appendChild(x); + reconcileArrays(parent, [x, s], [x, y, s]); + expect([...parent.children].map(n => n.textContent)).toEqual(["y", "s"]); + }); + + it("an insert before a migrated suffix head anchors on the next live suffix node", () => { + const parent = document.createElement("div"); + const other = document.createElement("div"); + const mk = t => Object.assign(document.createElement("span"), { textContent: t }); + const x = mk("x"), + y = mk("y"), + s = mk("s"); + parent.appendChild(x); + parent.appendChild(s); + other.appendChild(x); + reconcileArrays(parent, [x, s], [y, x, s]); + expect([...parent.children].map(n => n.textContent)).toEqual(["y", "s"]); + }); + it("reconcile keeps a migrated node alive when its array drops it", () => { // Insert [n1, n2, n3]; user code migrates n2 to another parent; then // reconcile from [n1, n2, n3] to [n1, n3]. The remove-branch and