From 2fe20f23dd4a2aa94122286aa21da462c48ec5b8 Mon Sep 17 00:00:00 2001 From: Jay Meistrich Date: Thu, 20 Jun 2024 13:39:24 -0700 Subject: [PATCH] change: simplify mergeIntoObservable by only allowing it for observables, use deepMerge for regular objects --- index.ts | 3 +- src/helpers.ts | 89 ++++++++++++++++++-------------------- src/sync/syncObservable.ts | 39 +++++------------ tests/helpers.test.ts | 48 +++++++++----------- 4 files changed, 77 insertions(+), 102 deletions(-) diff --git a/index.ts b/index.ts index 0444a727..0d8bfcc3 100644 --- a/index.ts +++ b/index.ts @@ -71,13 +71,14 @@ import { symbolDelete, symbolLinked, } from './src/globals'; -import { getValueAtPath, initializePathType, setAtPath } from './src/helpers'; +import { deepMerge, getValueAtPath, initializePathType, setAtPath } from './src/helpers'; import { runWithRetry } from './src/retry'; import { tracking } from './src/tracking'; export const internal = { createPreviousHandler, clone, + deepMerge, ensureNodeValue, findIDKey, get, diff --git a/src/helpers.ts b/src/helpers.ts index 92b8b769..f422906c 100644 --- a/src/helpers.ts +++ b/src/helpers.ts @@ -1,6 +1,6 @@ import { beginBatch, endBatch } from './batching'; import { getNode, isObservable, setNodeValue, symbolDelete, symbolOpaque } from './globals'; -import { isArray, isEmpty, isFunction, isMap, isNumber, isObject, isSet } from './is'; +import { hasOwnProperty, isArray, isEmpty, isFunction, isMap, isNumber, isObject, isSet } from './is'; import type { Change, ObserveEvent, OpaqueObject, Selector, TypeAtPath } from './observableInterfaces'; import type { Observable, ObservableParam } from './observableTypes'; @@ -77,13 +77,13 @@ export function setAtPath( // when setting undefined on an object without this key if (p === undefined) { if (mode === 'merge') { - obj = _mergeIntoObservable(obj, value, false, 0); + obj = deepMerge(obj, value, false, 0); } else { obj = value; } } else { if (mode === 'merge') { - o[p] = _mergeIntoObservable(o[p], value, false, 0); + o[p] = deepMerge(o[p], value, false, 0); } else if (isMap(o)) { o.set(p, value); } else { @@ -100,7 +100,7 @@ export function setInObservableAtPath( value: any, mode: 'assign' | 'set' | 'merge', ) { - let o: Record = value$; + let o: any = value$; let v = value; for (let i = 0; i < path.length; i++) { const p = path[i]; @@ -123,18 +123,20 @@ export function setInObservableAtPath( o.set(v); } } -export function mergeIntoObservable> | object>( - target: T, - ...sources: any[] -): T { +export function mergeIntoObservable>(target: T, ...sources: any[]): T { + if (process.env.NODE_ENV === 'development' || process.env.NODE_ENV === 'test') { + if (!isObservable(target)) { + console.error('[legend-state] should only use mergeIntoObservable with observables'); + } + } beginBatch(); for (let i = 0; i < sources.length; i++) { - target = _mergeIntoObservable(target, sources[i], /*assign*/ i < sources.length - 1, 0); + _mergeIntoObservable(target, sources[i], /*assign*/ i < sources.length - 1, 0); } endBatch(); return target; } -function _mergeIntoObservable> | object>( +function _mergeIntoObservable>>( target: T, source: any, assign: boolean, @@ -143,8 +145,7 @@ function _mergeIntoObservable> | o if (isObservable(source)) { source = source.peek(); } - const needsSet = isObservable(target); - const targetValue = needsSet ? target.peek() : target; + const targetValue = target.peek(); const isTargetArr = isArray(targetValue); const isTargetObj = !isTargetArr && isObject(targetValue); @@ -152,7 +153,7 @@ function _mergeIntoObservable> | o const isSourceMap = isMap(source); const isSourceSet = isSet(source); - if (needsSet && isSourceSet && isSet(targetValue)) { + if (isSourceSet && isSet(targetValue)) { target.set(new Set([...source, ...targetValue])); } else if ((isTargetObj && isObject(source) && !isEmpty(targetValue)) || (isTargetArr && targetValue.length > 0)) { const keys: string[] = isSourceMap || isSourceSet ? Array.from(source.keys()) : Object.keys(source); @@ -165,51 +166,24 @@ function _mergeIntoObservable> | o ? (source as Map).get(key) : (source as Record)[key]; if (sourceValue === symbolDelete) { - needsSet && (target as any)[key]?.delete - ? (target as any)[key].delete() - : delete (target as Record)[key]; + (target as any)[key].delete(); } else { const isObj = isObject(sourceValue); const isArr = !isObj && isArray(sourceValue); const targetChild = (target as Record)[key]; - if ((isObj || isArr) && targetChild && (needsSet || !isEmpty(targetChild))) { - if (!needsSet && (!targetChild || (isObj ? !isObject(targetChild) : !isArray(targetChild)))) { - (target as Record)[key] = assign - ? isArr - ? [...sourceValue] - : { ...sourceValue } - : sourceValue; - } else { - if (levelsDeep > 0 && isEmpty(sourceValue)) { - if (needsSet) { - targetChild.set(sourceValue); - } else { - ((target as Record)[key] as any) = sourceValue; - } - } - _mergeIntoObservable(targetChild, sourceValue, false, levelsDeep + 1); - } - } else { - if (needsSet) { + if ((isObj || isArr) && targetChild) { + if (levelsDeep > 0 && isEmpty(sourceValue)) { targetChild.set(sourceValue); - } else { - const toSet = isObservable(sourceValue) - ? sourceValue - : isObject(sourceValue) - ? { ...sourceValue } - : isArray(sourceValue) - ? [...sourceValue] - : sourceValue; - ((target as Record)[key] as any) = toSet; } + _mergeIntoObservable(targetChild, sourceValue, false, levelsDeep + 1); + } else { + targetChild.set(sourceValue); } } } } else if (source !== undefined) { - needsSet - ? target.set(source) - : ((target as any) = assign ? (isArray(source) ? [...source] : { ...source }) : source); + target.set(source); } return target; @@ -269,3 +243,24 @@ export function applyChanges(value: T, changes: Change[], appl } return value; } +export function deepMerge(target: T, ...sources: any[]): T { + const result: T = { ...target } as T; + + for (let i = 0; i < sources.length; i++) { + const obj2 = sources[i]; + for (const key in obj2) { + if (hasOwnProperty.call(obj2, key)) { + if (obj2[key] instanceof Object && !isObservable(obj2[key]) && Object.keys(obj2[key]).length > 0) { + (result as any)[key] = deepMerge( + (result as any)[key] || (isArray((obj2 as any)[key]) ? [] : {}), + (obj2 as any)[key], + ); + } else { + (result as any)[key] = obj2[key]; + } + } + } + } + + return result; +} diff --git a/src/sync/syncObservable.ts b/src/sync/syncObservable.ts index 5d121c43..445a4b84 100644 --- a/src/sync/syncObservable.ts +++ b/src/sync/syncObservable.ts @@ -15,14 +15,12 @@ import { beginBatch, constructObjectWithPath, endBatch, - hasOwnProperty, internal, isArray, isEmpty, isFunction, isNullOrUndefined, isObject, - isObservable, isPromise, isString, mergeIntoObservable, @@ -48,8 +46,17 @@ import type { SyncedSetParams, } from './syncTypes'; -const { clone, getNode, getNodeValue, getValueAtPath, globalState, runWithRetry, symbolLinked, createPreviousHandler } = - internal; +const { + clone, + deepMerge, + getNode, + getNodeValue, + getValueAtPath, + globalState, + runWithRetry, + symbolLinked, + createPreviousHandler, +} = internal; export const mapSyncPlugins: WeakMap< ClassConstructor, @@ -658,7 +665,7 @@ async function doChangeRemote(changeInfo: PreppedChangeRemote | undefined) { const { value, lastSync, mode } = params; updateResult = { lastSync: Math.max(updateResult.lastSync || 0, lastSync || 0), - value: mergeIntoObservable(updateResult.value, value), + value: deepMerge(updateResult.value, value), mode: mode, }; } else { @@ -879,28 +886,6 @@ async function loadLocal( syncState$.isPersistLoaded.set(!(syncStateValue.numPendingLocalLoads! > 0)); } -function deepMerge(target: T, ...sources: any[]): T { - const result: T = { ...target } as T; - - for (let i = 0; i < sources.length; i++) { - const obj2 = sources[i]; - for (const key in obj2) { - if (hasOwnProperty.call(obj2, key)) { - if (obj2[key] instanceof Object && !isObservable(obj2[key]) && Object.keys(obj2[key]).length > 0) { - (result as any)[key] = deepMerge( - (result as any)[key] || (isArray((obj2 as any)[key]) ? [] : {}), - (obj2 as any)[key], - ); - } else { - (result as any)[key] = obj2[key]; - } - } - } - } - - return result; -} - export function syncObservable( obs$: ObservableParam, syncOptions: SyncedOptions, diff --git a/tests/helpers.test.ts b/tests/helpers.test.ts index 62dafcbf..a3bf834f 100644 --- a/tests/helpers.test.ts +++ b/tests/helpers.test.ts @@ -5,27 +5,26 @@ import { observable } from '../src/observable'; describe('mergeIntoObservable', () => { test('merge onto empty object', () => { - const target = {}; + const target = observable({}); const source = { a: { b: { c: { d: 5 } } } }; const merged = mergeIntoObservable(target, source); - expect(merged).toEqual({ a: { b: { c: { d: 5 } } } }); - expect(isObservable(merged)).toBe(false); + expect(merged.peek()).toEqual({ a: { b: { c: { d: 5 } } } }); }); test('merge undefined should do nothing', () => { - const target = { a: { b: { c: { d: 5 } } } }; + const target = observable({ a: { b: { c: { d: 5 } } } }); const merged = mergeIntoObservable(target, undefined); - expect(merged).toEqual(target); + expect(merged.peek()).toEqual(target.peek()); }); test('merge null should delete', () => { - const target = { a: { b: { c: { d: 5 } } } }; + const target = observable({ a: { b: { c: { d: 5 } } } }); const merged = mergeIntoObservable(target, null); - expect(merged).toEqual(null); + expect(merged.peek()).toEqual(null); }); test('merge null should delete (2)', () => { - const target = { a: { b: { c: { d: 5 } } } }; + const target = observable({ a: { b: { c: { d: 5 } } } }); const source = { a: { b: { c: { d: null } } } }; const merged = mergeIntoObservable(target, source); - expect(merged).toEqual(source); + expect(merged.peek()).toEqual(source); }); test('merge onto empty observable', () => { const target = observable(); @@ -43,35 +42,31 @@ describe('mergeIntoObservable', () => { expect(isObservable(merged)).toBe(true); }); test('should merge two plain objects', () => { - const target = { a: 1, b: 2 }; + const target = observable({ a: 1, b: 2 }); const source = { b: 3, c: 4 }; const merged = mergeIntoObservable(target, source); - expect(merged).toEqual({ a: 1, b: 3, c: 4 }); - expect(isObservable(merged)).toBe(false); + expect(merged.peek()).toEqual({ a: 1, b: 3, c: 4 }); }); test('should merge two observable objects', () => { const target = observable({ a: 1, b: 2 }); const source = observable({ b: 3, c: 4 }); const merged = mergeIntoObservable(target, source); expect(merged.get()).toEqual({ a: 1, b: 3, c: 4 }); - expect(isObservable(merged)).toBe(true); }); test('should merge a plain object and an observable object', () => { const target = observable({ a: 1, b: 2 }); const source = { b: 3, c: 4 }; const merged = mergeIntoObservable(target, source); expect(merged.get()).toEqual({ a: 1, b: 3, c: 4 }); - expect(isObservable(merged)).toBe(true); }); test('should delete a key marked with symbolDelete', () => { - const target = { a: 1, b: 2 }; + const target = observable({ a: 1, b: 2 }); const source = { b: symbolDelete }; const merged = mergeIntoObservable(target, source); - expect(merged).toEqual({ a: 1 }); - expect(isObservable(merged)).toBe(false); + expect(merged.peek()).toEqual({ a: 1 }); }); test('should not merge undefined with sparse array', () => { - const target = { + const target = observable({ id: { panes: [ { @@ -85,7 +80,7 @@ describe('mergeIntoObservable', () => { }, ], }, - }; + }); const panes = []; panes[1] = { item: 'B', @@ -97,7 +92,7 @@ describe('mergeIntoObservable', () => { }; mergeIntoObservable(target, source); - expect(target).toEqual({ + expect(target.peek()).toEqual({ id: { panes: [ { @@ -114,11 +109,10 @@ describe('mergeIntoObservable', () => { }); }); test('merge indexed object into array', () => { - const target = [{ key: '0' }]; + const target = observable([{ key: '0' }]); const source = { 1: { key: '1' } }; const merged = mergeIntoObservable(target, source); - expect(merged).toEqual([{ key: '0' }, { key: '1' }]); - expect(isObservable(merged)).toBe(false); + expect(merged.peek()).toEqual([{ key: '0' }, { key: '1' }]); }); test('Can merge if parent null', () => { interface Data { @@ -136,7 +130,7 @@ describe('mergeIntoObservable', () => { expect(obs.test.test2.test3.get()).toEqual('hi'); }); test('merge multiple should not override intermediate', () => { - const target = { syncMode: 'get' }; + const target = observable({ syncMode: 'get' }); const source1 = { persist: { indexedDB: { @@ -179,7 +173,7 @@ describe('mergeIntoObservable', () => { const merged = mergeIntoObservable(target, source1, source2); expect(JSON.stringify(source1) === source1Str); expect(JSON.stringify(source2) === source2Str); - expect(merged).toEqual({ + expect(merged.peek()).toEqual({ debounceSet: 1000, persist: { indexedDB: { @@ -212,10 +206,10 @@ describe('mergeIntoObservable', () => { synced: true, }); - const merged2 = mergeIntoObservable({ syncMode: 'get' }, source1, source2); + const merged2 = mergeIntoObservable(observable({ syncMode: 'get' }), source1, source2); expect(JSON.stringify(source1) === source1Str); expect(JSON.stringify(source2) === source2Str); - expect(merged2).toEqual({ + expect(merged2.peek()).toEqual({ debounceSet: 1000, persist: { indexedDB: {