Skip to content

Commit

Permalink
change: simplify mergeIntoObservable by only allowing it for observab…
Browse files Browse the repository at this point in the history 
…les, use deepMerge for regular objects
  • Loading branch information
@jmeistrich
jmeistrich committed Jun 20, 2024
1 parent 40315d3 commit 2fe20f2
Show file tree
Hide file tree
Showing 4 changed files with 77 additions and 102 deletions.
3 changes: 2 additions & 1 deletion index.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
89 changes: 42 additions & 47 deletions src/helpers.ts
View file
Original file line number Diff line number Diff line change
@@ -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';

Expand Down Expand Up @@ -77,13 +77,13 @@ export function setAtPath<T extends object>(
// 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 {
Expand All @@ -100,7 +100,7 @@ export function setInObservableAtPath(
value: any,
mode: 'assign' | 'set' | 'merge',
) {
let o: Record<string, any> = value$;
let o: any = value$;
let v = value;
for (let i = 0; i < path.length; i++) {
const p = path[i];
Expand All @@ -123,18 +123,20 @@ export function setInObservableAtPath(
o.set(v);
}
}
export function mergeIntoObservable<T extends ObservableParam<Record<string, any>> | object>(
target: T,
...sources: any[]
): T {
export function mergeIntoObservable<T extends ObservableParam<any>>(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<T extends ObservableParam<Record<string, any>> | object>(
function _mergeIntoObservable<T extends ObservableParam<Record<string, any>>>(
target: T,
source: any,
assign: boolean,
Expand All @@ -143,16 +145,15 @@ function _mergeIntoObservable<T extends ObservableParam<Record<string, any>> | 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);

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);
Expand All @@ -165,51 +166,24 @@ function _mergeIntoObservable<T extends ObservableParam<Record<string, any>> | o
? (source as Map<any, any>).get(key)
: (source as Record<string, any>)[key];
if (sourceValue === symbolDelete) {
needsSet && (target as any)[key]?.delete
? (target as any)[key].delete()
: delete (target as Record<string, any>)[key];
(target as any)[key].delete();
} else {
const isObj = isObject(sourceValue);
const isArr = !isObj && isArray(sourceValue);
const targetChild = (target as Record<string, any>)[key];

if ((isObj || isArr) && targetChild && (needsSet || !isEmpty(targetChild))) {
if (!needsSet && (!targetChild || (isObj ? !isObject(targetChild) : !isArray(targetChild)))) {
(target as Record<string, any>)[key] = assign
? isArr
? [...sourceValue]
: { ...sourceValue }
: sourceValue;
} else {
if (levelsDeep > 0 && isEmpty(sourceValue)) {
if (needsSet) {
targetChild.set(sourceValue);
} else {
((target as Record<string, any>)[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<string, any>)[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;
Expand Down Expand Up @@ -269,3 +243,24 @@ export function applyChanges<T extends object>(value: T, changes: Change[], appl
}
return value;
}
export function deepMerge<T extends object>(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;
}
39 changes: 12 additions & 27 deletions src/sync/syncObservable.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -15,14 +15,12 @@ import {
beginBatch,
constructObjectWithPath,
endBatch,
hasOwnProperty,
internal,
isArray,
isEmpty,
isFunction,
isNullOrUndefined,
isObject,
isObservable,
isPromise,
isString,
mergeIntoObservable,
Expand All @@ -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<ObservablePersistPlugin>,
Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -879,28 +886,6 @@ async function loadLocal<T>(
syncState$.isPersistLoaded.set(!(syncStateValue.numPendingLocalLoads! > 0));
}

function deepMerge<T extends object>(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<T>(
obs$: ObservableParam<T>,
syncOptions: SyncedOptions<T>,
Expand Down
48 changes: 21 additions & 27 deletions tests/helpers.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand All @@ -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: [
{
Expand All @@ -85,7 +80,7 @@ describe('mergeIntoObservable', () => {
},
],
},
};
});
const panes = [];
panes[1] = {
item: 'B',
Expand All @@ -97,7 +92,7 @@ describe('mergeIntoObservable', () => {
};

mergeIntoObservable(target, source);
expect(target).toEqual({
expect(target.peek()).toEqual({
id: {
panes: [
{
Expand All @@ -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 {
Expand All @@ -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: {
Expand Down Expand Up @@ -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: {
Expand Down Expand Up @@ -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: {
Expand Down

0 comments on commit 2fe20f2

Please sign in to comment.