From b19d23ab407e4ca5ca5900c1d5d47cc296924e86 Mon Sep 17 00:00:00 2001 From: Brenley Dueck Date: Mon, 27 Jul 2026 22:11:09 -0500 Subject: [PATCH] fix(signals): guard reconcile's array paths against array/object shape changes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The object diff refuses to recurse into a pair whose container kinds disagree — `Array.isArray(previous) !== Array.isArray(next)` replaces the slot instead of merging. The array paths reach the same recursion through `keyedMatch` and positional pairing and never applied that rule: two keyless wrappables "match" because `keyFn` returns undefined for both, whatever kind they are. The slot's proxy then stays the wrong kind forever, since `Array.isArray` inspects the proxy target fixed at wrap time. An object slot receiving an array enumerates as `{"0":..,"1":..}`; an array slot receiving an object reads `length` off that object and presents an empty array, dropping the data outright. Route all five sites — the keyed prefix loop and the positional loop in both applyStateFast and applyStateSlow, plus applyArrayItem — through a shared `recursablePair()` that folds the existing markRaw check together with the kind check, matching the object diff and `descendInto`. Co-Authored-By: Claude Opus 5 --- .../reconcile-array-object-shape-guard.md | 18 +++++ packages/solid-signals/src/store/reconcile.ts | 40 ++++++----- .../tests/store/reconcile.test.ts | 66 +++++++++++++++++++ 3 files changed, 106 insertions(+), 18 deletions(-) create mode 100644 .changeset/reconcile-array-object-shape-guard.md diff --git a/.changeset/reconcile-array-object-shape-guard.md b/.changeset/reconcile-array-object-shape-guard.md new file mode 100644 index 000000000..4d4e7786e --- /dev/null +++ b/.changeset/reconcile-array-object-shape-guard.md @@ -0,0 +1,18 @@ +--- +"@solidjs/signals": patch +--- + +Stop `reconcile` from merging an array into an object slot (and vice versa) inside arrays + +The object diff has always refused to recurse into a pair whose kinds disagree — `Array.isArray(previous) !== Array.isArray(next)` replaces the slot instead of merging. The array paths reach the same recursion through `keyedMatch` and positional pairing, and neither applied that rule: two keyless wrappables "match" because `keyFn` returns `undefined` for both, regardless of whether they are arrays or objects. + +The result was a slot whose proxy is permanently the wrong kind. `Array.isArray` on a store proxy inspects the proxy's target, which is fixed at wrap time, so after + +```js +const [state, setState] = createStore({ list: [{ x: 1 }] }); +setState(reconcile({ list: [[10, 20]] }, "id")); +``` + +`state.list[0]` reports `Array.isArray === false` and enumerates as `{ "0": 10, "1": 20 }` — spread, `.map`, `` and `JSON.stringify` all see an object. The reverse direction (array slot receiving an object) is worse: the array-shaped target reads `length` off the incoming object and the store presents an empty array, silently dropping the data. + +Four call sites shared the gap — the keyed prefix loop and the positional loop in both `applyStateFast` and `applyStateSlow`, plus `applyArrayItem` (which covers the keyed diff's trailing and moved slots). They now share a `recursablePair()` helper that folds the existing raw-value (`markRaw`) check together with the container-kind check, so a kind change replaces the slot by reference — exactly what the object diff and `descendInto` already do. diff --git a/packages/solid-signals/src/store/reconcile.ts b/packages/solid-signals/src/store/reconcile.ts index 32b3bc98e..4836b0940 100644 --- a/packages/solid-signals/src/store/reconcile.ts +++ b/packages/solid-signals/src/store/reconcile.ts @@ -118,6 +118,22 @@ function keyedMatch(a: any, b: any, keyFn: (item: NonNullable) => any) { return a === b || (isWrappable(a) && isWrappable(b) && keyFn(a) === keyFn(b)); } +// A pair of array slots may only be merged into when both sides are real store +// children of the SAME container kind. An array and an object are different +// shapes, and merging one into the other leaves the slot's proxy permanently +// mismatched with its value (an array target holding an object, so +// `Array.isArray`/spread/`map` lie). The object diff has always applied this +// rule; the array paths reach the same recursion through `keyedMatch` / +// positional pairing, where two keyless wrappables "match" regardless of kind. +function recursablePair(previous: any, next: any): boolean { + return ( + isWrappable(previous) && + isWrappable(next) && + !(rawValuesUsed && (isRawValue(previous) || isRawValue(next))) && + Array.isArray(previous) === Array.isArray(next) + ); +} + // Array reconciliation updates the slots it visits, then swaps STORE_VALUE. // Previously tracked keys that are absent from `next` still need invalidating, // and `in` dependencies should follow the new value's membership. Use @@ -195,11 +211,7 @@ function applyArrayItem( node: any, keyFn: (item: NonNullable) => any ) { - if ( - isWrappable(next) && - isWrappable(previous) && - !(rawValuesUsed && (isRawValue(previous) || isRawValue(next))) - ) { + if (recursablePair(previous, next)) { const wrapped = wrap(previous, target); node && setSignal(node, wrapped); applyState(next, wrapped, keyFn); @@ -421,7 +433,7 @@ function applyStateFast(next: any, target: any, keyFn: (item: NonNullable) // the recursion dispatch entirely. Raw-marked values are leaves: // replace the slot node instead of recursing. if (item !== next[start]) { - if (rawValuesUsed && (isRawValue(item) || isRawValue(next[start]))) { + if (!recursablePair(item, next[start])) { arrayNodes?.[start] && setSignal(arrayNodes[start], wrapValue(next[start], target)); } else applyStateChild(next[start], item, target, keyFn); } @@ -494,11 +506,7 @@ function applyStateFast(next: any, target: any, keyFn: (item: NonNullable) } else if (next.length) { for (let i = 0, len = next.length; i < len; i++) { const item = previous[i]; - if ( - isWrappable(item) && - isWrappable(next[i]) && - !(rawValuesUsed && (isRawValue(item) || isRawValue(next[i]))) - ) { + if (recursablePair(item, next[i])) { if (item !== next[i]) applyStateChild(next[i], item, target, keyFn); } else { if (item !== next[i]) changed = true; @@ -615,8 +623,8 @@ function applyStateSlow(next: any, target: any, keyFn: (item: NonNullable) ); start++ ) { - if (isWrappable(item) && isWrappable(next[start]) && item !== next[start]) { - if (rawValuesUsed && (isRawValue(item) || isRawValue(next[start]))) { + if (item !== next[start] && isWrappable(item) && isWrappable(next[start])) { + if (!recursablePair(item, next[start])) { nodes?.[start] && setSignal(nodes[start], wrapValue(next[start], target)); } else applyState(next[start], wrap(item, target), keyFn); } @@ -688,11 +696,7 @@ function applyStateSlow(next: any, target: any, keyFn: (item: NonNullable) } else if (next.length) { for (let i = 0, len = next.length; i < len; i++) { const item = getOverrideValue(previous, override, i as any, optOverride); - if ( - isWrappable(item) && - isWrappable(next[i]) && - !(rawValuesUsed && (isRawValue(item) || isRawValue(next[i]))) - ) { + if (recursablePair(item, next[i])) { if (item !== next[i]) applyState(next[i], wrap(item, target), keyFn); } else { if (item !== next[i]) changed = true; diff --git a/packages/solid-signals/tests/store/reconcile.test.ts b/packages/solid-signals/tests/store/reconcile.test.ts index 4443cef2f..88f074b45 100644 --- a/packages/solid-signals/tests/store/reconcile.test.ts +++ b/packages/solid-signals/tests/store/reconcile.test.ts @@ -761,6 +761,72 @@ describe("reconcile without a key (positional merge)", () => { expect(state.items[0].v).toBe(9); }); + describe("array/object shape changes at an array slot", () => { + // Two keyless wrappables "match" for the array diff (both report no key), + // so an array and an object used to be merged into each other, leaving the + // slot's proxy permanently the wrong kind. + test("object -> array at an unkeyed position", () => { + const [state, setState] = createStore({ list: [{ x: 1 }] }); + createRoot(() => { + createEffect( + () => state.list[0].x, + () => {} + ); + }); + flush(); + setState(reconcile({ list: [[10, 20]] }, "id")); + flush(); + expect(Array.isArray(state.list[0])).toBe(true); + expect(snapshot(state.list[0])).toEqual([10, 20]); + }); + + test("array -> object at an unkeyed position", () => { + const [state, setState] = createStore({ list: [[10, 20]] }); + createRoot(() => { + createEffect( + () => state.list[0][0], + () => {} + ); + }); + flush(); + setState(reconcile({ list: [{ x: 1 }] }, "id")); + flush(); + expect(Array.isArray(state.list[0])).toBe(false); + expect(snapshot(state.list[0])).toEqual({ x: 1 }); + }); + + test("keyless slot trailing a keyed array", () => { + const [state, setState] = createStore({ list: [{ id: "a" }, { x: 1 }] }); + createRoot(() => { + createEffect( + () => state.list[1].x, + () => {} + ); + }); + flush(); + setState(reconcile({ list: [{ id: "a" }, [10, 20]] }, "id")); + flush(); + expect(Array.isArray(state.list[1])).toBe(true); + expect(snapshot(state.list[1])).toEqual([10, 20]); + }); + + test("positional (key: null) merge keeps the incoming kind", () => { + const [state, setState] = createStore({ list: [{ x: 1 }, [1]] }); + createRoot(() => { + createEffect( + () => [state.list[0].x, state.list[1][0]], + () => {} + ); + }); + flush(); + setState(reconcile({ list: [[7], { y: 2 }] }, null)); + flush(); + expect(Array.isArray(state.list[0])).toBe(true); + expect(Array.isArray(state.list[1])).toBe(false); + expect(snapshot(state.list)).toEqual([[7], { y: 2 }]); + }); + }); + test("does not enforce root identity", () => { const [state, setState] = createStore({ id: 1, v: 2 }); // keyed (including the "id" default) this throws "different identity"; key: null must merge