From e0d91c32fa45a85819a36d46f04a0bdfad419785 Mon Sep 17 00:00:00 2001 From: Russell Davis <551404+russelldavis@users.noreply.github.com> Date: Thu, 16 May 2024 12:28:09 -0700 Subject: [PATCH] Don't mutate deepMap values (#305) Previously, calling setKey on a $deepMap would mutate the value in-place. This can cause various problems, for example #250 and #290. Those were both solved with workarounds, however, this solves it in a better way, by not mutating the deepMap in the first place. Some advantages of this approach: * No need for structuredClone * The oldValue passed to listeners is the same object reference as the value that was previously passed. This is how immutable stores usually work, so it will align better with expectations, cause fewer issues, and allow for new use cases, like the upcoming batching PR which needs that. * Copies were already being made at each nesting level of the key being set, they just weren't being used (so maybe this was the original intent?). Either way, that means there's no new work being done, and perf will actually improve since we no longer need to call structuredClone on oldValue. As part of this change, when a key path extends past the end of an array the result will be a sparse array rather than filling it with undefined. It would have required extra code (and slower perf) to keep the old behavior, and I don't see a good reason for it. The new behavior aligns with what JS does natively, and with other stores like SolidJS. But let me know if that's a problem. --- deep-map/index.d.ts | 4 +++- deep-map/index.js | 9 ++------- deep-map/index.test.ts | 32 +++++++++++++++++++++----------- deep-map/path.d.ts | 16 ++++++++++------ deep-map/path.js | 11 +++++------ deep-map/path.test.ts | 28 ++++++++++++++++------------ 6 files changed, 57 insertions(+), 43 deletions(-) diff --git a/deep-map/index.d.ts b/deep-map/index.d.ts index 6214339c..52a036cc 100644 --- a/deep-map/index.d.ts +++ b/deep-map/index.d.ts @@ -32,7 +32,9 @@ export type DeepMapStore = { notify(oldValue?: T, changedKey?: AllPaths): void /** - * Change key in store value. + * Change key in store value. Copies are made at each level of `key` so that + * the old value is not mutated (but it does not do a full deep copy -- + * references to objects will still be shared between the old and new value). * * ```js * $settings.setKey('visuals.theme', 'dark') diff --git a/deep-map/index.js b/deep-map/index.js index 024e3958..6d37e198 100644 --- a/deep-map/index.js +++ b/deep-map/index.js @@ -6,14 +6,9 @@ export { getPath, setByKey, setPath } from './path.js' export function deepMap(initial = {}) { let $deepMap = atom(initial) $deepMap.setKey = (key, value) => { - let oldValue - try { - oldValue = structuredClone($deepMap.value) - } catch { - oldValue = { ...$deepMap.value } - } if (getPath($deepMap.value, key) !== value) { - $deepMap.value = { ...setPath($deepMap.value, key, value) } + let oldValue = $deepMap.value + $deepMap.value = setPath($deepMap.value, key, value) $deepMap.notify(oldValue, key) } } diff --git a/deep-map/index.test.ts b/deep-map/index.test.ts index d8306848..2499193a 100644 --- a/deep-map/index.test.ts +++ b/deep-map/index.test.ts @@ -324,7 +324,7 @@ test('does not run queued listeners after they are unsubscribed', () => { deepStrictEqual(events, ['a1', 'b1', 'a2', 'c2']) }) -test('notifies correct previous value from deep store', t => { +test('notifies correct previous value from deep store', () => { type DeepValue = { a: number; b: { nested: { deep: number } } } let events: DeepValue[] = [] @@ -345,16 +345,6 @@ test('notifies correct previous value from deep store', t => { { a: 1, b: { nested: { deep: 0 } } }, { a: 1, b: { nested: { deep: 1 } } } ]) - - t.mock.method(global, 'structuredClone', () => { - throw new Error('structuredClone is not supported') - }) - - events = [] - $store.setKey('b.nested.deep', 3) - deepStrictEqual(events, [{ a: 1, b: { nested: { deep: 3 } } }]) - - t.mock.reset() unbind() }) @@ -384,6 +374,26 @@ test('passes previous value to subscribers', () => { unbind() }) +test('oldValue references the same object as the previous value', () => { + let events: ({ a: number } | undefined)[] = [] + let $store = deepMap({ a: 0 }) + let unbind = $store.subscribe((value, oldValue) => { + events.push(oldValue) + }) + + let oldValue1 = $store.value + $store.setKey('a', 1) + let oldValue2 = $store.value + $store.setKey('a', 2) + + // Intentionally not using deepStrictEqual here - we're testing object reference equality + equal(events.length, 3) + equal(events[0], undefined) + equal(events[1], oldValue1) + equal(events[2], oldValue2) + unbind() +}) + test('keys starting with numbers do not create unnecessary arrays', () => { let $store = deepMap<{ key?: { '100key': string } }>({}) diff --git a/deep-map/path.d.ts b/deep-map/path.d.ts index 57042954..77da0dad 100644 --- a/deep-map/path.d.ts +++ b/deep-map/path.d.ts @@ -103,14 +103,16 @@ export function getPath>( ): FromPath /** - * Set a deep value by key. Initialized arrays with `undefined` - * if you set arbitrary length. + * Set a deep value by key. Makes a copy at each level of `path` so the input + * object is not mutated (but does not do a full deep copy -- references to + * objects will still be shared between input and output). Sparse arrays will + * be created if you set arbitrary length. * * ``` * import { setPath } from 'nanostores' * * setPath({ a: { b: { c: [] } } }, 'a.b.c[1]', 'hey') - * // Returns `{ a: { b: { c: [undefined, 'hey'] } } }` + * // Returns `{ a: { b: { c: [, 'hey'] } } }` * ``` * * @param obj Any object. @@ -124,14 +126,16 @@ export function setPath>( ): T /** - * Set a deep value by path. Initialized arrays with `undefined` - * if you set arbitrary length. + * Set a deep value by path. Makes a copy at each level of `path` so the input + * object is not mutated (but does not do a full deep copy -- references to + * objects will still be shared between input and output). Sparse arrays will + * be created if you set arbitrary length. * * ``` * import { setByKey } from 'nanostores' * * setByKey({ a: { b: { c: [] } } }, ['a', 'b', 'c', 1], 'hey') - * // Returns `{ a: { b: { c: [undefined, 'hey'] } } }` + * // Returns `{ a: { b: { c: [, 'hey'] } } }` * ``` * * @param obj Any object. diff --git a/deep-map/path.js b/deep-map/path.js index 179d7180..434d8c6c 100644 --- a/deep-map/path.js +++ b/deep-map/path.js @@ -16,11 +16,10 @@ export function setPath(obj, path, value) { export function setByKey(obj, splittedKeys, value) { let key = splittedKeys[0] - ensureKey(obj, key, splittedKeys[1]) let copy = Array.isArray(obj) ? [...obj] : { ...obj } if (splittedKeys.length === 1) { if (value === undefined) { - if (Array.isArray(obj)) { + if (Array.isArray(copy)) { copy.splice(key, 1) } else { delete copy[key] @@ -30,9 +29,9 @@ export function setByKey(obj, splittedKeys, value) { } return copy } - let newVal = setByKey(obj[key], splittedKeys.slice(1), value) - obj[key] = newVal - return obj + ensureKey(copy, key, splittedKeys[1]) + copy[key] = setByKey(copy[key], splittedKeys.slice(1), value) + return copy } const ARRAY_INDEX = /(.*)\[(\d+)\]/ @@ -58,7 +57,7 @@ function ensureKey(obj, key, nextKey) { let isNum = IS_NUMBER.test(nextKey) if (isNum) { - obj[key] = Array(parseInt(nextKey, 10) + 1).fill(undefined) + obj[key] = Array(parseInt(nextKey, 10) + 1) } else { obj[key] = {} } diff --git a/deep-map/path.test.ts b/deep-map/path.test.ts index d2afbadb..a482e041 100644 --- a/deep-map/path.test.ts +++ b/deep-map/path.test.ts @@ -58,7 +58,7 @@ test('creating objects', () => { type TestObj = { a?: { b?: { c?: { d: string } } } } let initial: TestObj = {} - setPath(initial, 'a.b.c.d', 'val') + initial = setPath(initial, 'a.b.c.d', 'val') equal(initial.a?.b?.c?.d, 'val') }) @@ -66,10 +66,13 @@ test('creating arrays', () => { type TestObj = { a?: string[] } let initial: TestObj = {} - setPath(initial, 'a[0]', 'val') + initial = setPath(initial, 'a[0]', 'val') deepStrictEqual(initial, { a: ['val'] }) - setPath(initial, 'a[3]', 'val3') - deepStrictEqual(initial, { a: ['val', undefined, undefined, 'val3'] }) + initial = setPath(initial, 'a[3]', 'val3') + // The expected value is a sparse array + let expectedA = ['val'] + expectedA[3] = 'val3' + deepStrictEqual(initial, { a: expectedA }) }) test('removes arrays', () => { @@ -77,30 +80,30 @@ test('removes arrays', () => { let initial: TestObj = { a: ['a', 'b'] } // @ts-expect-error: incorrect key here - setPath(initial, 'a[1]', undefined) + initial = setPath(initial, 'a[1]', undefined) deepStrictEqual(initial, { a: ['a'] }) // @ts-expect-error: incorrect key here - setPath(initial, 'a[0]', undefined) + initial = setPath(initial, 'a[0]', undefined) deepStrictEqual(initial, { a: [] }) }) -test('changes object reference, when this level key is changed', () => { +test('changes object reference at this level and earlier levels when key is changed', () => { type Obj = { a: { b: { c: number; d: string }; e: number } } let b = { c: 1, d: '1' } let a = { b, e: 1 } let initial: Obj = { a } - setPath(initial, 'a.b.c', 2) - equal(initial.a, a) + initial = setPath(initial, 'a.b.c', 2) + notEqual(initial.a, a) notEqual(initial.a.b, b) - setPath(initial, 'a.e', 2) + initial = setPath(initial, 'a.e', 2) notEqual(initial.a, a) }) -test('array items mutation changes identity on the same level', () => { +test('array items mutation changes identity on the same and earlier levels', () => { let arr1 = { a: 1 } let arr2 = { a: 2 } let d = [arr1, arr2] @@ -108,7 +111,8 @@ test('array items mutation changes identity on the same level', () => { let initial = { a: { b: { c } } } let newInitial = setPath(initial, 'a.b.c.d[1].a', 3) - equal(newInitial.a.b.c.d, d) + notEqual(newInitial, initial) + notEqual(newInitial.a.b.c.d, d) equal(newInitial.a.b.c.d[0], d[0]) notEqual(newInitial.a.b.c.d[1], arr2) deepStrictEqual(newInitial.a.b.c.d[1], { a: 3 })