From 85be00cef982b10de4d1b3da8ef95c5020c7de0a Mon Sep 17 00:00:00 2001 From: thomaflette Date: Thu, 16 Jul 2026 17:14:40 +0900 Subject: [PATCH] fix(store): snapshot() and store materialization keep symbol-keyed props after writes (closes #2894) --- .changeset/preserve-store-symbol-snapshots.md | 7 + packages/solid-signals/src/store/reconcile.ts | 2 + packages/solid-signals/src/store/store.ts | 98 +++++++++- packages/solid-signals/src/store/utils.ts | 25 ++- .../tests/question-scoped-pending.test.ts | 43 +++++ packages/solid-signals/tests/snapshot.test.ts | 180 ++++++++++++++++++ .../tests/store/utilities.test.ts | 51 +++++ 7 files changed, 395 insertions(+), 11 deletions(-) create mode 100644 .changeset/preserve-store-symbol-snapshots.md diff --git a/.changeset/preserve-store-symbol-snapshots.md b/.changeset/preserve-store-symbol-snapshots.md new file mode 100644 index 000000000..b1f88b666 --- /dev/null +++ b/.changeset/preserve-store-symbol-snapshots.md @@ -0,0 +1,7 @@ +--- +"@solidjs/signals": patch +--- + +Preserve enumerable symbol-keyed properties when snapshotting and deeply +tracking stores, including array metadata and untouched nested values. Make +setter drafts writable when the store type is readonly. diff --git a/packages/solid-signals/src/store/reconcile.ts b/packages/solid-signals/src/store/reconcile.ts index a8162c579..c51e5ab49 100644 --- a/packages/solid-signals/src/store/reconcile.ts +++ b/packages/solid-signals/src/store/reconcile.ts @@ -53,6 +53,8 @@ function addEnumSymbols(o: any, syms: symbol[], keys: Set) { } function getAllKeys(value, override, next) { + // Symbols are merged explicitly below; keep the common string-key path on + // Object.keys() and avoid reflecting the base symbols twice. const keys = getKeys(value, override) as PropertyKey[]; const nextKeys = Object.keys(next); // `value` can be a wrapped store (store-in-store) whose ownKeys trap tracks; diff --git a/packages/solid-signals/src/store/store.ts b/packages/solid-signals/src/store/store.ts index 271295fb8..1af8925c8 100644 --- a/packages/solid-signals/src/store/store.ts +++ b/packages/solid-signals/src/store/store.ts @@ -48,7 +48,7 @@ export type Store = Readonly; * projection form — `createStore(fn, seed, { key })` or `createProjection` — * whose derive function reconciles its return by `options.key`. */ -export type StoreSetter = (fn: (state: T) => T | void) => void; +export type StoreSetter = (fn: (state: Mutable) => Mutable | void) => void; /** Tuple returned by the plain `createStore(initialValue)` form. */ export type StoreReturn = [get: Store, set: StoreSetter]; /** Tuple returned by the derived `createStore(fn, seed, options?)` form. */ @@ -64,6 +64,8 @@ export interface ProjectionOptions extends StoreOptions { key?: string | ((item: NonNullable) => any); } export type NoFn = T extends Function ? never : T; +/** Makes the setter draft's top-level properties writable; {@link Store} keeps the getter readonly. */ +export type Mutable = { -readonly [K in keyof T]: T[K] }; type DataNode = Signal; type DataNodes = Record; @@ -186,7 +188,7 @@ function unwrapStoreValue(value: any, map?: Map, lookup?: WeakMap Object.prototype.propertyIsEnumerable.call(o, k)); } +// Plain-object variant that keeps Object.keys() as the fast path and only pays +// descriptor checks for symbols. Do not use this on store proxies: splitting +// strings/symbols would invoke their ownKeys trap twice. +function ownEnumerableKeysPlain(o: object): (string | symbol)[] { + const keys: (string | symbol)[] = Object.keys(o); + const symbols = Object.getOwnPropertySymbols(o); + for (let i = 0, len = symbols.length; i < len; i++) { + const symbol = symbols[i]; + if (Object.prototype.propertyIsEnumerable.call(o, symbol)) keys.push(symbol); + } + return keys; +} + +function ownEnumerableSymbols(o: object): symbol[] { + const symbols = Object.getOwnPropertySymbols(o); + const result: symbol[] = []; + for (let i = 0, len = symbols.length; i < len; i++) { + const symbol = symbols[i]; + if (Object.prototype.propertyIsEnumerable.call(o, symbol)) result.push(symbol); + } + return result; +} + /** * Single chokepoint for the store's layered value resolution: returns the * override layer (optimistic first, then regular) that shadows `property`, or @@ -378,7 +403,10 @@ function walkAffectsScope( collectRecordNodes(target[STORE_NODE], found); collectRecordNodes(target[STORE_HAS], found); override = mergedOverlay(target); - lookup = target[STORE_LOOKUP] ?? lookup; + // Carry the effective lookup into untouched descendants. Default stores + // use the global lookup just like snapshotImpl; without it, nested raw + // objects fall back to string-only enumeration and symbol branches vanish. + lookup = target[STORE_LOOKUP] ?? lookup ?? storeLookup; } if (Array.isArray(raw)) { const len = override?.length ?? raw.length; @@ -386,8 +414,18 @@ function walkAffectsScope( const v = override && i in override ? override[i] : raw[i]; if (v !== $DELETED) walkAffectsScope(v, entry, found, lookup, visited); } + // Arrays can also carry symbol metadata. Enumerate symbols separately to + // avoid scanning large index lists twice. Outside a store tree, retain the + // existing index-only walk. + const symbols = target || lookup ? getStoreSymbols(raw, override) : []; + for (let i = 0, l = symbols.length; i < l; i++) { + const key = symbols[i]; + const desc = getPropertyDescriptor(raw, override, key); + if (!desc || desc.get) continue; + walkAffectsScope(desc.value, entry, found, lookup, visited); + } } else { - const keys = getKeys(raw, override); + const keys = target || lookup ? getStoreKeys(raw, override) : getKeys(raw, override); for (let i = 0, l = keys.length; i < l; i++) { const desc = getPropertyDescriptor(raw, override, keys[i]); if (!desc || desc.get) continue; @@ -507,17 +545,26 @@ export function mergedOverlay(target: StoreNode): Record | und return override && opt ? { ...override, ...opt } : (opt ?? override); } -export function getKeys( +function getKeysImpl( source: Record, override: Record | undefined, - enumerable: boolean = true + enumerable: boolean, + symbols: boolean ): PropertyKey[] { // Plain objects can't trigger proxy traps — only pay for the untrack // closure when the source is itself a wrapped store (store-in-store). const baseKeys = (source as any)[$TARGET] - ? untrack(() => (enumerable ? Object.keys(source) : Reflect.ownKeys(source))) + ? untrack(() => + enumerable + ? symbols + ? ownEnumerableKeys(source) + : Object.keys(source) + : Reflect.ownKeys(source) + ) : enumerable - ? Object.keys(source) + ? symbols + ? ownEnumerableKeysPlain(source) + : Object.keys(source) : Reflect.ownKeys(source); if (!override) return baseKeys; const keys = new Set(baseKeys); @@ -529,6 +576,39 @@ export function getKeys( return Array.from(keys); } +export function getKeys( + source: Record, + override: Record | undefined, + enumerable: boolean = true +): PropertyKey[] { + return getKeysImpl(source, override, enumerable, false); +} + +export function getStoreKeys( + source: Record, + override: Record | undefined +): PropertyKey[] { + return getKeysImpl(source, override, true, true); +} + +export function getStoreSymbols( + source: Record, + override: Record | undefined +): symbol[] { + const symbols = (source as any)[$TARGET] + ? untrack(() => ownEnumerableSymbols(source)) + : ownEnumerableSymbols(source); + if (!override) return symbols; + const result = new Set(symbols); + const overrideSymbols = Object.getOwnPropertySymbols(override); + for (let i = 0, len = overrideSymbols.length; i < len; i++) { + const symbol = overrideSymbols[i]; + if (override[symbol] !== $DELETED) result.add(symbol); + else result.delete(symbol); + } + return Array.from(result); +} + export function getPropertyDescriptor( source: Record, override: Record | undefined, @@ -1048,7 +1128,7 @@ export function storeSetter(store: Store, fn: (draft: T) => * * @returns `[store: Store, setStore: StoreSetter]` */ -export function createStore(store: NoFn | Store>): StoreReturn; +export function createStore(store: NoFn): StoreReturn; export function createStore( fn: (store: T) => void | T | Promise | AsyncIterable, store: Partial | Store>, diff --git a/packages/solid-signals/src/store/utils.ts b/packages/solid-signals/src/store/utils.ts index 250627e52..a3ff1a14a 100644 --- a/packages/solid-signals/src/store/utils.ts +++ b/packages/solid-signals/src/store/utils.ts @@ -7,6 +7,8 @@ import { $TARGET, $TRACK, getKeys, + getStoreKeys, + getStoreSymbols, getPropertyDescriptor, isWrappable, mergedOverlay, @@ -62,6 +64,21 @@ function snapshotImpl( result[i] = unwrapped; } } + // Enumerate array symbols separately to avoid scanning indices twice. + // Spread copies omit symbols, so assign them after the numeric walk. + const symbols = lookup ? getStoreSymbols(item, override) : []; + for (let i = 0, l = symbols.length; i < l; i++) { + const prop = symbols[i]; + const desc = getPropertyDescriptor(item, override, prop); + if (!desc || desc.get) continue; + v = override && prop in override ? override[prop] : item[prop]; + if (track && isWrappable(v)) wrap(v, target); + unwrapped = snapshotImpl(v, track, map, lookup); + if (unwrapped !== v || result) { + if (!result) map.set(item, (result = Object.assign([...item], item))); + result[prop] = unwrapped; + } + } // Deleted trailing slots are skipped above, so restore length to preserve // holes instead of truncating the copy (#2846) — mirrors unwrapStoreValue. if (result) result.length = len; @@ -69,7 +86,9 @@ function snapshotImpl( // Specialized walk for the common no-overlay case (from #2756): the own // descriptor gives the value directly, so each property is read once with // no overlay membership checks. - const keys = getKeys(item, undefined); + // A lookup means this object belongs to an immutable store backing tree, + // even if that nested value has not needed its own proxy yet. + const keys = lookup ? getStoreKeys(item, undefined) : getKeys(item, undefined); for (let i = 0, l = keys.length; i < l; i++) { const prop = keys[i]; const desc = Object.getOwnPropertyDescriptor(item, prop)!; @@ -85,7 +104,9 @@ function snapshotImpl( } } } else { - const keys = getKeys(item, override); + // An override only exists on a store record, and the target branch above + // always set `lookup` alongside it — so this branch is always store-keyed. + const keys = getStoreKeys(item, override); for (let i = 0, l = keys.length; i < l; i++) { let prop = keys[i]; const desc = getPropertyDescriptor(item, override, prop)!; diff --git a/packages/solid-signals/tests/question-scoped-pending.test.ts b/packages/solid-signals/tests/question-scoped-pending.test.ts index 5170000d7..73f10cbd0 100644 --- a/packages/solid-signals/tests/question-scoped-pending.test.ts +++ b/packages/solid-signals/tests/question-scoped-pending.test.ts @@ -612,6 +612,49 @@ describe("affects — captured proxies (#2882)", () => { expect(isPending(() => tags.primary)).toBe(false); }); + it("affects(plain store) covers an untouched symbol-keyed grandchild", async () => { + const meta = Symbol("meta"); + const [state] = createStore({ outer: { [meta]: { value: 1 } } }); + + let resolveIt!: () => void; + const act = action(function* () { + affects(state); + yield new Promise(r => (resolveIt = r)); + }); + + const done = act(); + flush(); + const child = state.outer[meta]; + expect(isPending(() => child.value)).toBe(true); + + resolveIt(); + await done; + flush(); + expect(isPending(() => child.value)).toBe(false); + }); + + it("affects(array store) covers symbol-keyed metadata", async () => { + const meta = Symbol("meta"); + const initial = Object.assign([{ value: 0 }], { [meta]: { value: 1 } }); + const [state] = createStore(initial); + + let resolveIt!: () => void; + const act = action(function* () { + affects(state); + yield new Promise(r => (resolveIt = r)); + }); + + const done = act(); + flush(); + const metadata = state[meta]; + expect(isPending(() => metadata.value)).toBe(true); + + resolveIt(); + await done; + flush(); + expect(isPending(() => metadata.value)).toBe(false); + }); + it("keyless mark on a nested record covers its captured subtree, not siblings", async () => { const [state] = createStore<{ rows: Row[] }>({ rows: seedRows() }); const tags0 = state.rows[0].tags; diff --git a/packages/solid-signals/tests/snapshot.test.ts b/packages/solid-signals/tests/snapshot.test.ts index b631be81c..933c6dd4e 100644 --- a/packages/solid-signals/tests/snapshot.test.ts +++ b/packages/solid-signals/tests/snapshot.test.ts @@ -478,6 +478,186 @@ describe("store snapshot support", () => { expect(snapshot(store)).toEqual([]); }); + + it("preserves symbol-keyed properties after a write", () => { + const meta = Symbol("meta"); + const [store, setStore] = createStore({ value: 1, [meta]: "keep" }); + + setStore(s => { + s.value = 2; + }); + flush(); + + expect(snapshot(store)[meta]).toBe("keep"); + }); + + it("captures a write inside a symbol-keyed subtree", () => { + const meta = Symbol("meta"); + const [store, setStore] = createStore({ [meta]: { value: 1 } }); + + setStore(s => { + s[meta].value = 2; + }); + flush(); + + expect(snapshot(store)[meta].value).toBe(2); + }); + + it("unwraps an unread symbol-keyed store-in-store value", () => { + const meta = Symbol("meta"); + const [child] = createStore({ value: 1 }); + const [parent] = createStore({ [meta]: child }); + + const plain = snapshot(parent); + + expect(plain[meta]).not.toBe(child); + expect(plain[meta]).toEqual({ value: 1 }); + }); + + it("preserves symbol-keyed array metadata after an index write", () => { + const meta = Symbol("meta"); + const initial = Object.assign([1], { [meta]: "keep" }); + const [store, setStore] = createStore(initial); + + setStore(s => { + s[0] = 2; + }); + flush(); + + expect(snapshot(store)[meta]).toBe("keep"); + }); + + it("preserves a symbol-keyed property added after a snapshot", () => { + const meta = Symbol("meta"); + const [store, setStore] = createStore<{ value: number; [meta]?: string }>({ value: 1 }); + + snapshot(store); + setStore(s => { + s[meta] = "added"; + }); + flush(); + + expect(snapshot(store)[meta]).toBe("added"); + }); + + it("preserves symbols on an untouched nested store value", () => { + const meta = Symbol("meta"); + const [store] = createStore({ child: { value: 1, [meta]: "keep" } }); + + expect(snapshot(store).child[meta]).toBe("keep"); + expect(snapshot(store).child[meta]).toBe("keep"); + }); + + it("drops a deleted symbol-keyed property", () => { + const meta = Symbol("meta"); + const [store, setStore] = createStore<{ value: number; [meta]?: string }>({ + value: 1, + [meta]: "gone" + }); + + setStore(s => { + delete s[meta]; + }); + flush(); + + expect(meta in snapshot(store)).toBe(false); + }); + + it("drops a deleted symbol-keyed array metadata property", () => { + const meta = Symbol("meta"); + const initial = Object.assign([1], { [meta]: "gone" }); + const [store, setStore] = createStore(initial); + + setStore(s => { + delete s[meta]; + }); + flush(); + + expect(meta in snapshot(store)).toBe(false); + }); + + it("excludes non-enumerable symbol-keyed properties from written copies", () => { + const meta = Symbol("meta"); + const raw: { value: number; [meta]?: string } = { value: 1 }; + Object.defineProperty(raw, meta, { value: "hidden", enumerable: false }); + const [store, setStore] = createStore(raw); + + setStore(s => { + s.value = 2; + }); + flush(); + + expect(meta in snapshot(store)).toBe(false); + }); + + it("preserves a cycle reached through a symbol key", () => { + const meta = Symbol("meta"); + const [store, setStore] = createStore<{ node: { name: string; [meta]?: unknown } }>({ + node: { name: "n" } + }); + + setStore(s => { + s.node[meta] = s.node; + }); + flush(); + + const snap = snapshot(store); + expect(snap.node[meta]).toBe(snap.node); + }); + + it("keeps shared references between symbol and string keys", () => { + const meta = Symbol("meta"); + const shared = { value: 1 }; + const [store, setStore] = createStore<{ named: typeof shared; [meta]?: typeof shared }>({ + named: shared, + [meta]: shared + }); + + setStore(s => { + s.named.value = 2; + }); + flush(); + + const snap = snapshot(store); + expect(snap[meta]).toBe(snap.named); + expect(snap.named.value).toBe(2); + }); + + it("unwraps a symbol-keyed store-in-store value under a written parent", () => { + const meta = Symbol("meta"); + const [child] = createStore({ value: 1 }); + const [parent, setParent] = createStore<{ label: string; [meta]?: typeof child }>({ + label: "a", + [meta]: child + }); + + setParent(s => { + s.label = "b"; + }); + flush(); + + const plain = snapshot(parent); + expect(plain[meta]).not.toBe(child); + expect(plain[meta]).toEqual({ value: 1 }); + }); + + it("preserves symbol-keyed properties when materializing a written subtree", () => { + const meta = Symbol("meta"); + const [source, setSource] = createStore({ child: { value: 1, [meta]: "keep" } }); + + setSource(s => { + s.child.value = 2; + }); + flush(); + + const [target, setTarget] = createStore<{ child?: typeof source.child }>({}); + setTarget(s => { + s.child = source.child; + }); + flush(); + + expect(target.child?.[meta]).toBe("keep"); + }); }); describe("pending projection skips snapshot capture", () => { diff --git a/packages/solid-signals/tests/store/utilities.test.ts b/packages/solid-signals/tests/store/utilities.test.ts index 3265155c2..689dd805e 100644 --- a/packages/solid-signals/tests/store/utilities.test.ts +++ b/packages/solid-signals/tests/store/utilities.test.ts @@ -556,6 +556,57 @@ describe("deep", () => { expect(effect).toHaveBeenCalledTimes(4); expect(effect.mock.calls[3][0]).toEqual({ list: [{ d: 4 }] }); }); + + test("tracks updates in a symbol-keyed subtree", () => { + const meta = Symbol("meta"); + const [state, setState] = createStore({ [meta]: { value: 1 } }); + const values: number[] = []; + + createRoot(() => { + createEffect( + () => deep(state), + value => { + values.push(value[meta].value); + } + ); + }); + flush(); + + setState(s => { + s[meta].value = 2; + }); + flush(); + + expect(values).toEqual([1, 2]); + }); + + test("notifies on symbol key addition and deletion", () => { + const meta = Symbol("meta"); + const [state, setState] = createStore<{ [meta]?: number }>({}); + const seen: (number | undefined)[] = []; + + createRoot(() => { + createEffect( + () => deep(state), + value => { + seen.push(value[meta]); + } + ); + }); + flush(); + + setState(s => { + s[meta] = 1; + }); + flush(); + + setState(s => { + delete s[meta]; + }); + flush(); + + expect(seen).toEqual([undefined, 1, undefined]); + }); test("handles shared references", () => { const sharedReference = { a: 1,