fix(store): reconcile() notifies symbol-keyed properties (closes #2851)

.changeset/reconcile-symbol-keyed-nodes.md

@@ -0,0 +1,5 @@
+---
+"@solidjs/signals": patch
+---
+
+Fix `reconcile()` updates for symbol-keyed store properties so tracked reads and `in` checks are notified like string-keyed properties.

packages/solid-signals/src/store/reconcile.ts

@@ -1,9 +1,9 @@
 import { setSignal } from "../core/index.js";
 import {
+  $DELETED,
   $PROXY,
   $TARGET,
   $TRACK,
-  getKeys,
   isWrappable,
   STORE_HAS,
   STORE_LOOKUP,
@@ -13,22 +13,56 @@ import {
   STORE_VALUE,
   notifySelf,
   storeLookup,
+  symbolKeyedRecords,
   wrap
 } from "./store.js";
 
+function nodeKeys(nodes: Record<PropertyKey, any>): PropertyKey[] {
+  const keys: PropertyKey[] = Object.keys(nodes);
+  // Keep the common string-key path cheap; only symbol-tracked records pay for
+  // symbol enumeration. `$TRACK` is handled separately by callers.
+  if (symbolKeyedRecords.has(nodes)) {
+    const syms = Object.getOwnPropertySymbols(nodes);
+    for (let i = 0, len = syms.length; i < len; i++) {
+      if (syms[i] !== $TRACK) keys.push(syms[i]);
+    }
+  }
+  return keys;
+}
+
 function unwrap(value: any) {
   return value?.[$TARGET]?.[STORE_VALUE] ?? value;
 }
 
-function getOverrideValue(value: any, override: any, key: string, optOverride?: any) {
+function getOverrideValue(value: any, override: any, key: PropertyKey, optOverride?: any) {
   if (optOverride && key in optOverride) return optOverride[key];
   return override && key in override ? override[key] : value[key];
 }
 
+// Append a value's *enumerable* own symbol keys. Symbol-free objects (the
+// common case) pay only an empty `getOwnPropertySymbols` call — no per-key
+// predicate — so the string fast path stays on `Object.keys`.
+function addEnumSymbols(o: any, keys: Set<PropertyKey>) {
+  const syms = Object.getOwnPropertySymbols(o);
+  for (let i = 0, len = syms.length; i < len; i++) {
+    if (Object.prototype.propertyIsEnumerable.call(o, syms[i])) keys.add(syms[i]);
+  }
+}
+
 function getAllKeys(value, override, next) {
-  const keys = getKeys(value, override) as string[];
+  // Reconcile must diff enumerable symbol keys the same way it diffs strings,
+  // but keep the string keys on the `Object.keys` fast path — symbols are
+  // appended only when the object actually has them.
+  const keys = new Set<PropertyKey>(Object.keys(value));
+  addEnumSymbols(value, keys);
+  if (override) {
+    for (const key of Reflect.ownKeys(override))
+      override[key] === $DELETED ? keys.delete(key) : keys.add(key);
+  }
   const nextKeys = Object.keys(next);
-  return Array.from(new Set([...keys, ...nextKeys]));
+  for (let i = 0, len = nextKeys.length; i < len; i++) keys.add(nextKeys[i]);
+  addEnumSymbols(next, keys);
+  return Array.from(keys);
 }
 
 // Array entries can be `null`/`undefined`/primitives, not just keyed objects.
@@ -54,14 +88,14 @@ function keyedMatch(a: any, b: any, keyFn: (item: NonNullable<any>) => any) {
 function syncArrayNodeMembership(target: any, next: any) {
   let nodes = target[STORE_NODE];
   if (nodes) {
-    const keys = Object.keys(nodes);
+    const keys = nodeKeys(nodes);
     for (let i = 0, len = keys.length; i < len; i++) {
       const key = keys[i];
       key in next || setSignal(nodes[key], undefined);
     }
   }
   if ((nodes = target[STORE_HAS])) {
-    const keys = Object.keys(nodes);
+    const keys = nodeKeys(nodes);
     for (let i = 0, len = keys.length; i < len; i++) {
       const key = keys[i];
       setSignal(nodes[key], key in next);
@@ -212,7 +246,7 @@ function applyStateFast(next: any, target: any, keyFn: (item: NonNullable<any>)
   let nodes = target[STORE_NODE];
   if (nodes) {
     const tracked = nodes[$TRACK];
-    const keys = tracked ? getAllKeys(previous, undefined, next) : Object.keys(nodes);
+    const keys = tracked ? getAllKeys(previous, undefined, next) : nodeKeys(nodes);
     for (let i = 0, len = keys.length; i < len; i++) {
       const key = keys[i];
       const node = nodes[key];
@@ -234,7 +268,7 @@ function applyStateFast(next: any, target: any, keyFn: (item: NonNullable<any>)
 
   // has
   if ((nodes = target[STORE_HAS])) {
-    const keys = Object.keys(nodes);
+    const keys = nodeKeys(nodes);
     for (let i = 0, len = keys.length; i < len; i++) {
       const key = keys[i];
       setSignal(nodes[key], key in next);
@@ -364,7 +398,7 @@ function applyStateSlow(next: any, target: any, keyFn: (item: NonNullable<any>)
   // values
   if (nodes) {
     const tracked = nodes[$TRACK];
-    const keys = tracked ? getAllKeys(previous, override, next) : Object.keys(nodes);
+    const keys = tracked ? getAllKeys(previous, override, next) : nodeKeys(nodes);
     for (let i = 0, len = keys.length; i < len; i++) {
       const key = keys[i];
       const node = nodes[key];
@@ -386,7 +420,7 @@ function applyStateSlow(next: any, target: any, keyFn: (item: NonNullable<any>)
 
   // has
   if ((nodes = target[STORE_HAS])) {
-    const keys = Object.keys(nodes);
+    const keys = nodeKeys(nodes);
     for (let i = 0, len = keys.length; i < len; i++) {
       const key = keys[i];
       setSignal(nodes[key], key in next);

packages/solid-signals/src/store/store.ts

@@ -138,6 +138,8 @@ export function createStoreProxy<T extends object>(
 }
 
 export const storeLookup = new WeakMap();
+// Node records that hold at least one user symbol-keyed node.
+export const symbolKeyedRecords = new WeakSet<object>();
 export function wrap<T extends Record<PropertyKey, any>>(value: T, target?: StoreNode): T {
   if (target?.[STORE_WRAP]) return target[STORE_WRAP](value, target);
   let p = value[$PROXY] || storeLookup.get(value);
@@ -242,7 +244,26 @@ function getNode<T>(
     {
       equals: equals,
       unobserved() {
-        if (nodes[property] === s) delete nodes[property];
+        if (nodes[property] === s) {
+          delete nodes[property];
+          // Drop the symbol-record mark once the last user symbol node is
+          // gone, so reconcile's fast path stops probing a now string-only
+          // record. Runs only on symbol-node cleanup (cold), never on reconcile.
+          if (
+            typeof property === "symbol" &&
+            property !== $TRACK &&
+            symbolKeyedRecords.has(nodes)
+          ) {
+            const syms = Object.getOwnPropertySymbols(nodes);
+            let hasUserSymbol = false;
+            for (let i = 0, len = syms.length; i < len; i++)
+              if (syms[i] !== $TRACK) {
+                hasUserSymbol = true;
+                break;
+              }
+            if (!hasUserSymbol) symbolKeyedRecords.delete(nodes);
+          }
+        }
       }
     },
     firewall
@@ -255,6 +276,8 @@ function getNode<T>(
     s._snapshotValue = sv === undefined ? NO_SNAPSHOT : sv;
     snapshotSources?.add(s);
   }
+  // Lets reconcile enumerate symbols only for records that need it.
+  if (typeof property === "symbol" && property !== $TRACK) symbolKeyedRecords.add(nodes);
   return (nodes[property] = s);
 }
 

packages/solid-signals/tests/store/reconcile.test.ts

@@ -537,6 +537,160 @@ describe("setState with reconcile", () => {
     expect(selectedName).toBe("b");
   });
 });
+
+describe("reconcile with symbol-keyed properties", () => {
+  const META = Symbol("meta");
+
+  test("notifies an effect tracking a symbol-keyed property", () => {
+    const [state, setState] = createStore<Record<PropertyKey, any>>({ id: 1, [META]: "old" });
+    let seen: any;
+    createRoot(() => {
+      createEffect(
+        () => state[META],
+        v => {
+          seen = v;
+        }
+      );
+    });
+    flush();
+    expect(seen).toBe("old");
+
+    setState(reconcile({ id: 1, [META]: "new" }, "id"));
+    flush();
+    expect(state[META]).toBe("new"); // value reachable, not shadowed by a stale node
+    expect(seen).toBe("new"); // subscriber notified
+  });
+
+  test("string control updates through the identical path", () => {
+    const [state, setState] = createStore<Record<PropertyKey, any>>({ id: 1, meta: "old" });
+    let seen: any;
+    createRoot(() => {
+      createEffect(
+        () => state.meta,
+        v => {
+          seen = v;
+        }
+      );
+    });
+    flush();
+
+    setState(reconcile({ id: 1, meta: "new" }, "id"));
+    flush();
+    expect(state.meta).toBe("new");
+    expect(seen).toBe("new");
+  });
+
+  test("string and symbol keys on the same store both update", () => {
+    const [state, setState] = createStore<Record<PropertyKey, any>>({
+      id: 1,
+      label: "old",
+      [META]: "old"
+    });
+    let sawLabel: any, sawMeta: any;
+    createRoot(() => {
+      createEffect(
+        () => state.label,
+        v => {
+          sawLabel = v;
+        }
+      );
+      createEffect(
+        () => state[META],
+        v => {
+          sawMeta = v;
+        }
+      );
+    });
+    flush();
+
+    setState(reconcile({ id: 1, label: "new", [META]: "new" }, "id"));
+    flush();
+    expect(sawLabel).toBe("new");
+    expect(sawMeta).toBe("new");
+  });
+
+  test("a symbol key removed by reconcile notifies as undefined", () => {
+    const [state, setState] = createStore<Record<PropertyKey, any>>({ id: 1, [META]: "old" });
+    let seen: any = "unset";
+    createRoot(() => {
+      createEffect(
+        () => state[META],
+        v => {
+          seen = v;
+        }
+      );
+    });
+    flush();
+
+    setState(reconcile({ id: 1 }, "id"));
+    flush();
+    expect(state[META]).toBeUndefined();
+    expect(seen).toBeUndefined();
+  });
+
+  test("a symbol `in` check updates when reconcile adds the key", () => {
+    const [state, setState] = createStore<Record<PropertyKey, any>>({ id: 1 });
+    let has: boolean | undefined;
+    createRoot(() => {
+      createEffect(
+        () => META in state,
+        v => {
+          has = v;
+        }
+      );
+    });
+    flush();
+    expect(has).toBe(false);
+
+    setState(reconcile({ id: 1, [META]: "added" }, "id"));
+    flush();
+    expect(has).toBe(true);
+  });
+
+  test("perf invariant: symbol-record mark is set while tracked and cleared once unobserved", async () => {
+    // Guards the fast-path optimization: only records that currently hold a
+    // user symbol node are enumerated for symbols on reconcile. Asserts the
+    // internal mark rather than behavior (the mark is invisible to behavior).
+    const { symbolKeyedRecords, $TARGET, STORE_NODE } = await import("../../src/store/store.js");
+    const [store] = createStore<Record<PropertyKey, any>>({ id: 1, [META]: "x" });
+    let dispose!: () => void;
+    createRoot(d => {
+      dispose = d;
+      createEffect(
+        () => store[META],
+        () => {}
+      );
+    });
+    flush();
+    const nodes = (store as any)[$TARGET][STORE_NODE];
+    expect(symbolKeyedRecords.has(nodes)).toBe(true);
+    dispose();
+    flush();
+    expect(symbolKeyedRecords.has(nodes)).toBe(false); // no monotonic leak
+  });
+
+  test("nested symbol-keyed value reconciles", () => {
+    const [state, setState] = createStore<Record<PropertyKey, any>>({
+      id: 1,
+      inner: { [META]: "old" }
+    });
+    let seen: any;
+    createRoot(() => {
+      createEffect(
+        () => state.inner[META],
+        v => {
+          seen = v;
+        }
+      );
+    });
+    flush();
+
+    setState(reconcile({ id: 1, inner: { [META]: "new" } }, "id"));
+    flush();
+    expect(state.inner[META]).toBe("new");
+    expect(seen).toBe("new");
+  });
+});
 // type tests
 
 // reconcile