From 01e9ff4236c3c25ab533823ea3d7b21241329ca9 Mon Sep 17 00:00:00 2001 From: Christoph Pader Date: Thu, 15 May 2025 16:22:50 +0200 Subject: [PATCH 01/20] fix: improve fastMerge code --- lib/utils.ts | 231 +++++++++++++++++++++++++++++---------------------- 1 file changed, 132 insertions(+), 99 deletions(-) diff --git a/lib/utils.ts b/lib/utils.ts index 88091f6cd..fbb6c3ab2 100644 --- a/lib/utils.ts +++ b/lib/utils.ts @@ -7,51 +7,76 @@ type EmptyValue = EmptyObject | null | undefined; type FastMergeReplaceNullPatch = [string[], unknown]; +type FastMergeOptions = { + /** If true, null object values will be removed. */ + shouldRemoveNestedNulls?: boolean; + /** If true, it means that we are batching merge changes before applying them to the Onyx value, so we must use a special logic to handle these changes. */ + isBatchingMergeChanges?: boolean; + /** If true, any nested objects that contains the internal "ONYX_INTERNALS__REPLACE_OBJECT_MARK" flag will be completely replaced instead of merged. */ + shouldReplaceMarkedObjects?: boolean; +}; + type FastMergeMetadata = { + /** The path to the object that contains the internal "ONYX_INTERNALS__REPLACE_OBJECT_MARK" flag. */ replaceNullPatches: FastMergeReplaceNullPatch[]; }; type FastMergeResult = { + /** The result of the merge. */ result: TValue; + /** The path to the object that contains the internal "ONYX_INTERNALS__REPLACE_OBJECT_MARK" flag. */ replaceNullPatches: FastMergeReplaceNullPatch[]; }; const ONYX_INTERNALS__REPLACE_OBJECT_MARK = 'ONYX_INTERNALS__REPLACE_OBJECT_MARK'; -/** Checks whether the given object is an object and not null/undefined. */ -function isEmptyObject(obj: T | EmptyValue): obj is EmptyValue { - return typeof obj === 'object' && Object.keys(obj || {}).length === 0; -} - -// Mostly copied from https://medium.com/@lubaka.a/how-to-remove-lodash-performance-improvement-b306669ad0e1 - /** - * Checks whether the given value can be merged. It has to be an object, but not an array, RegExp or Date. + * Merges two objects and removes null values if "shouldRemoveNestedNulls" is set to true + * + * We generally want to remove null values from objects written to disk and cache, because it decreases the amount of data stored in memory and on disk. */ -function isMergeableObject(value: unknown): value is Record { - const isNonNullObject = value != null ? typeof value === 'object' : false; - return isNonNullObject && !(value instanceof RegExp) && !(value instanceof Date) && !Array.isArray(value); +function fastMerge(target: TValue, source: TValue, options?: FastMergeOptions, metadata?: FastMergeMetadata, basePath: string[] = []): FastMergeResult { + if (!metadata) { + // eslint-disable-next-line no-param-reassign + metadata = { + replaceNullPatches: [], + }; + } + + // We have to ignore arrays and nullish values here, + // otherwise "mergeObject" will throw an error, + // because it expects an object as "source" + if (Array.isArray(source) || source === null || source === undefined) { + return {result: source, replaceNullPatches: metadata.replaceNullPatches}; + } + + const optionsWithDefaults = { + shouldRemoveNestedNulls: false, + isBatchingMergeChanges: false, + shouldReplaceMarkedObjects: false, + ...options, + }; + + const mergedValue = mergeObject(target, source as Record, optionsWithDefaults, metadata, basePath) as TValue; + + return {result: mergedValue, replaceNullPatches: metadata.replaceNullPatches}; } /** * Merges the source object into the target object. * @param target - The target object. * @param source - The source object. - * @param shouldRemoveNestedNulls - If true, null object values will be removed. - * @param isBatchingMergeChanges - If true, it means that we are batching merge changes before applying - * them to the Onyx value, so we must use a special logic to handle these changes. - * @param shouldReplaceMarkedObjects - If true, any nested objects that contains the internal "ONYX_INTERNALS__REPLACE_OBJECT_MARK" - * flag will be completely replaced instead of merged. + * @param options - The options for the merge. + * @param metadata - The metadata for the merge. + * @param basePath - The base path for the merge. * @returns - The merged object. */ function mergeObject>( target: TObject | unknown | null | undefined, source: TObject, - shouldRemoveNestedNulls: boolean, - isBatchingMergeChanges: boolean, - shouldReplaceMarkedObjects: boolean, + options: FastMergeOptions, metadata: FastMergeMetadata, - basePath: string[] = [], + basePath: string[], ): TObject { const destination: Record = {}; @@ -64,19 +89,20 @@ function mergeObject>( if (targetObject) { // eslint-disable-next-line no-restricted-syntax, guard-for-in for (const key in targetObject) { - const sourceValue = source?.[key]; - const targetValue = targetObject?.[key]; + const targetProperty = targetObject?.[key]; + if (targetProperty === undefined) { + // eslint-disable-next-line no-continue + continue; + } - // If "shouldRemoveNestedNulls" is true, we want to remove null values from the merged object. - // Therefore, if either target or source value is null, we want to prevent the key from being set. - // targetValue should techincally never be "undefined", because it will always be a value from cache or storage - // and we never set "undefined" there. Still, if there targetValue is undefined we don't want to set - // the key explicitly to prevent loose undefined values in objects in cache and storage. - const isSourceOrTargetNull = targetValue === undefined || targetValue === null || sourceValue === null; - const shouldOmitTargetKey = shouldRemoveNestedNulls && isSourceOrTargetNull; + // If "shouldRemoveNestedNulls" is true, we want to remove (nested) null values from the merged object. + // If either the source or target value is null, we want to omit the key from the merged object. + const sourceProperty = source?.[key]; + const isSourceOrTargetNull = targetProperty === null || sourceProperty === null; + const shouldOmitTargetKey = options.shouldRemoveNestedNulls && isSourceOrTargetNull; if (!shouldOmitTargetKey) { - destination[key] = targetValue; + destination[key] = targetProperty; } } } @@ -84,97 +110,104 @@ function mergeObject>( // After copying over all keys from the target object, we want to merge the source object into the destination object. // eslint-disable-next-line no-restricted-syntax, guard-for-in for (const key in source) { - const sourceValue = source?.[key] as Record; - const targetValue = targetObject?.[key]; + const sourceProperty = source?.[key] as Record; + if (sourceProperty === undefined) { + // eslint-disable-next-line no-continue + continue; + } - // If undefined is passed as the source value for a key, we want to generally ignore it. // If "shouldRemoveNestedNulls" is set to true and the source value is null, // we don't want to set/merge the source value into the merged object. - const shouldIgnoreNullSourceValue = shouldRemoveNestedNulls && sourceValue === null; - const shouldOmitSourceKey = sourceValue === undefined || shouldIgnoreNullSourceValue; + const shouldOmitSourceKey = options.shouldRemoveNestedNulls && sourceProperty === null; + if (shouldOmitSourceKey) { + // eslint-disable-next-line no-continue + continue; + } - if (!shouldOmitSourceKey) { - // If the source value is a mergable object, we want to merge it into the target value. - // If "shouldRemoveNestedNulls" is true, "fastMerge" will recursively - // remove nested null values from the merged object. + // If the source value is a mergable object, we want to merge it into the target value. + if (!isMergeableObject(sourceProperty)) { // If source value is any other value we need to set the source value it directly. - if (isMergeableObject(sourceValue)) { - // If the target value is null or undefined, we need to fallback to an empty object, - // so that we can still use "fastMerge" to merge the source value, - // to ensure that nested null values are removed from the merged object. - const targetValueWithFallback = (targetValue ?? {}) as TObject; - - // If we are batching merge changes and the previous merge change (targetValue) is null, - // it means we want to fully replace this object when merging the batched changes with the Onyx value. - // To achieve this, we first mark these nested objects with an internal flag. With the desired objects - // marked, when calling this method again with "shouldReplaceMarkedObjects" set to true we can proceed - // effectively replace them in the next condition. - if (isBatchingMergeChanges && targetValue === null) { - (targetValueWithFallback as Record)[ONYX_INTERNALS__REPLACE_OBJECT_MARK] = true; - metadata.replaceNullPatches.push([[...basePath, key], {...sourceValue}]); - } - - // Then, when merging the batched changes with the Onyx value, if a nested object of the batched changes - // has the internal flag set, we replace the entire destination object with the source one and remove - // the flag. - if (shouldReplaceMarkedObjects && sourceValue[ONYX_INTERNALS__REPLACE_OBJECT_MARK]) { - // We do a spread here in order to have a new object reference and allow us to delete the internal flag - // of the merged object only. - destination[key] = {...sourceValue}; - delete (destination[key] as Record).ONYX_INTERNALS__REPLACE_OBJECT_MARK; - } else { - // For the normal situations we'll just call `fastMerge()` again to merge the nested object. - destination[key] = fastMerge(targetValueWithFallback, sourceValue, shouldRemoveNestedNulls, isBatchingMergeChanges, shouldReplaceMarkedObjects, metadata, [ - ...basePath, - key, - ]).result; - } - } else { - destination[key] = sourceValue; - } + destination[key] = sourceProperty; } + + const targetProperty = targetObject?.[key]; + const targetWithMarks = getTargetPropertyWithRemovalMark(targetProperty, sourceProperty, options, metadata, basePath); + const {finalDestinationProperty, stopTraversing} = replaceMarkedObjects(sourceProperty, options); + + if (stopTraversing) { + destination[key] = finalDestinationProperty; + // eslint-disable-next-line no-continue + continue; + } + + destination[key] = fastMerge(targetWithMarks, sourceProperty, options, metadata, [...basePath, key]).result; } return destination as TObject; } +/** Checks whether the given object is an object and not null/undefined. */ +function isEmptyObject(obj: T | EmptyValue): obj is EmptyValue { + return typeof obj === 'object' && Object.keys(obj || {}).length === 0; +} + +// Mostly copied from https://medium.com/@lubaka.a/how-to-remove-lodash-performance-improvement-b306669ad0e1 + /** - * Merges two objects and removes null values if "shouldRemoveNestedNulls" is set to true - * - * We generally want to remove null values from objects written to disk and cache, because it decreases the amount of data stored in memory and on disk. + * Checks whether the given value can be merged. It has to be an object, but not an array, RegExp or Date. */ -function fastMerge( - target: TValue, - source: TValue, - shouldRemoveNestedNulls: boolean, - isBatchingMergeChanges: boolean, - shouldReplaceMarkedObjects: boolean, - metadata?: FastMergeMetadata, +function isMergeableObject>(value: unknown): value is TObject { + const isNonNullObject = value != null ? typeof value === 'object' : false; + return isNonNullObject && !(value instanceof RegExp) && !(value instanceof Date) && !Array.isArray(value); +} + +function getTargetPropertyWithRemovalMark>( + targetProperty: unknown, + sourceProperty: Record, + options: FastMergeOptions, + metadata: FastMergeMetadata, basePath: string[] = [], -): FastMergeResult { - if (!metadata) { - // eslint-disable-next-line no-param-reassign - metadata = { - replaceNullPatches: [], - }; +): TObject { + const targetPropertyWithMarks = (targetProperty ?? {}) as Record; + + // If we are batching merge changes and the previous merge change (targetValue) is null, + // it means we want to fully replace this object when merging the batched changes with the Onyx value. + // To achieve this, we first mark these nested objects with an internal flag. With the desired objects + // marked, when calling this method again with "shouldReplaceMarkedObjects" set to true we can proceed + // effectively replace them in the next condition. + if (options?.isBatchingMergeChanges && targetProperty === null) { + targetPropertyWithMarks[ONYX_INTERNALS__REPLACE_OBJECT_MARK] = true; + metadata.replaceNullPatches.push([[...basePath], {...sourceProperty}]); } - // We have to ignore arrays and nullish values here, - // otherwise "mergeObject" will throw an error, - // because it expects an object as "source" - if (Array.isArray(source) || source === null || source === undefined) { - return {result: source, replaceNullPatches: metadata.replaceNullPatches}; - } + return targetPropertyWithMarks as TObject; +} - const mergedValue = mergeObject(target, source as Record, shouldRemoveNestedNulls, isBatchingMergeChanges, shouldReplaceMarkedObjects, metadata, basePath) as TValue; +function replaceMarkedObjects>(sourceProperty: TObject, options: FastMergeOptions): {finalDestinationProperty?: TObject; stopTraversing: boolean} { + // Then, when merging the batched changes with the Onyx value, if a nested object of the batched changes + // has the internal flag set, we replace the entire destination object with the source one and remove + // the flag. + if (options.shouldReplaceMarkedObjects && sourceProperty[ONYX_INTERNALS__REPLACE_OBJECT_MARK]) { + // We do a spread here in order to have a new object reference and allow us to delete the internal flag + // of the merged object only. + + const destinationProperty = {...sourceProperty}; + delete destinationProperty.ONYX_INTERNALS__REPLACE_OBJECT_MARK; + return {finalDestinationProperty: destinationProperty, stopTraversing: true}; + } - return {result: mergedValue, replaceNullPatches: metadata.replaceNullPatches}; + // For the normal situations we'll just call `fastMerge()` again to merge the nested object. + return {stopTraversing: false}; } /** Deep removes the nested null values from the given value. */ function removeNestedNullValues | null>(value: TValue): TValue { if (typeof value === 'object' && !Array.isArray(value)) { - return fastMerge(value, value, true, false, true).result; + return fastMerge(value, value, { + shouldRemoveNestedNulls: true, + isBatchingMergeChanges: false, + shouldReplaceMarkedObjects: false, + }).result; } return value; @@ -268,8 +301,8 @@ function hasWithOnyxInstance(mapping: ConnectOptions } export default { - isEmptyObject, fastMerge, + isEmptyObject, formatActionName, removeNestedNullValues, checkCompatibilityWithExistingValue, From a08b5e4f81d881d6f74f9491ba7ef7a2377c1f1d Mon Sep 17 00:00:00 2001 From: Christoph Pader Date: Thu, 15 May 2025 16:24:41 +0200 Subject: [PATCH 02/20] adapt fastMerge usages --- lib/OnyxCache.ts | 7 +++++- lib/OnyxUtils.ts | 17 +++++++++++---- lib/storage/providers/IDBKeyValProvider.ts | 5 ++++- lib/storage/providers/MemoryOnlyProvider.ts | 5 ++++- tests/perf-test/utils.perf-test.ts | 7 +++++- tests/unit/fastMergeTest.ts | 24 +++++++++++++++------ 6 files changed, 50 insertions(+), 15 deletions(-) diff --git a/lib/OnyxCache.ts b/lib/OnyxCache.ts index 691cedfee..d0d4585d4 100644 --- a/lib/OnyxCache.ts +++ b/lib/OnyxCache.ts @@ -164,7 +164,12 @@ class OnyxCache { throw new Error('data passed to cache.merge() must be an Object of onyx key/value pairs'); } - this.storageMap = {...utils.fastMerge(this.storageMap, data, true, false, true).result}; + this.storageMap = { + ...utils.fastMerge(this.storageMap, data, { + shouldRemoveNestedNulls: true, + shouldReplaceMarkedObjects: true, + }).result, + }; Object.entries(data).forEach(([key, value]) => { this.addKey(key); diff --git a/lib/OnyxUtils.ts b/lib/OnyxUtils.ts index 93b6bf651..52452f931 100644 --- a/lib/OnyxUtils.ts +++ b/lib/OnyxUtils.ts @@ -1265,7 +1265,14 @@ function applyMerge | undefined, TChange exten if (changes.some((change) => change && typeof change === 'object')) { // Object values are then merged one after the other - return changes.reduce((modifiedData, change) => utils.fastMerge(modifiedData, change, true, false, true).result, (existingValue || {}) as TChange); + return changes.reduce( + (modifiedData, change) => + utils.fastMerge(modifiedData, change, { + shouldRemoveNestedNulls: true, + shouldReplaceMarkedObjects: true, + }).result, + (existingValue || {}) as TChange, + ); } // If we have anything else we can't merge it so we'll @@ -1284,7 +1291,7 @@ function batchMergeChanges | undefined>(chang // Object values are then merged one after the other return changes.reduce>( (modifiedData, change) => { - const fastMergeResult = utils.fastMerge(modifiedData.result, change, false, true, false); + const fastMergeResult = utils.fastMerge(modifiedData.result, change, {isBatchingMergeChanges: true}); // eslint-disable-next-line no-param-reassign modifiedData.result = fastMergeResult.result; // eslint-disable-next-line no-param-reassign @@ -1310,7 +1317,9 @@ function initializeWithDefaultKeyStates(): Promise { return Storage.multiGet(Object.keys(defaultKeyStates)).then((pairs) => { const existingDataAsObject = Object.fromEntries(pairs); - const merged = utils.fastMerge(existingDataAsObject, defaultKeyStates, true, false, false).result; + const merged = utils.fastMerge(existingDataAsObject, defaultKeyStates, { + shouldRemoveNestedNulls: true, + }).result; cache.merge(merged ?? {}); Object.entries(merged ?? {}).forEach(([key, value]) => keyChanged(key, value, existingDataAsObject)); @@ -1373,7 +1382,7 @@ function subscribeToKey(connectOptions: ConnectOptions { const prev = values[index]; - const newValue = utils.fastMerge(prev as Record, value as Record, true, false, true).result; + const newValue = utils.fastMerge(prev as Record, value as Record, { + shouldRemoveNestedNulls: true, + shouldReplaceMarkedObjects: true, + }).result; return promisifyRequest(store.put(newValue, key)); }); return Promise.all(upsertMany); diff --git a/lib/storage/providers/MemoryOnlyProvider.ts b/lib/storage/providers/MemoryOnlyProvider.ts index 7f1b1c2fb..3ec4d6b56 100644 --- a/lib/storage/providers/MemoryOnlyProvider.ts +++ b/lib/storage/providers/MemoryOnlyProvider.ts @@ -86,7 +86,10 @@ const provider: StorageProvider = { multiMerge(pairs) { _.forEach(pairs, ([key, value]) => { const existingValue = store[key] as Record; - const newValue = utils.fastMerge(existingValue, value as Record, true, false, true).result as OnyxValue; + const newValue = utils.fastMerge(existingValue, value as Record, { + shouldRemoveNestedNulls: true, + shouldReplaceMarkedObjects: true, + }).result as OnyxValue; set(key, newValue); }); diff --git a/tests/perf-test/utils.perf-test.ts b/tests/perf-test/utils.perf-test.ts index b3ab893c7..e0ada9009 100644 --- a/tests/perf-test/utils.perf-test.ts +++ b/tests/perf-test/utils.perf-test.ts @@ -15,6 +15,11 @@ describe('[Utils.js]', () => { const target = getMockedPersonalDetails(1000); const source = getMockedPersonalDetails(500); - await measureFunction(() => utils.fastMerge(target, source, true, false, false)); + await measureFunction(() => + utils.fastMerge(target, source, { + shouldRemoveNestedNulls: true, + shouldReplaceMarkedObjects: true, + }), + ); }); }); diff --git a/tests/unit/fastMergeTest.ts b/tests/unit/fastMergeTest.ts index b70a2d56f..f824d10d7 100644 --- a/tests/unit/fastMergeTest.ts +++ b/tests/unit/fastMergeTest.ts @@ -89,7 +89,9 @@ describe('fastMerge', () => { }); it('should merge an object with an empty object and remove deeply nested null values', () => { - const result = utils.fastMerge({}, testObjectWithNullishValues, true, false, false); + const result = utils.fastMerge({}, testObjectWithNullishValues, { + shouldRemoveNestedNulls: true, + }); expect(result.result).toEqual(testObjectWithNullValuesRemoved); }); @@ -102,20 +104,27 @@ describe('fastMerge', () => { it('should replace an object with an array', () => { // eslint-disable-next-line @typescript-eslint/no-explicit-any - const result = utils.fastMerge(testObject, [1, 2, 3] as any, true, false, false); + const result = utils.fastMerge(testObject, [1, 2, 3] as any, { + shouldRemoveNestedNulls: true, + }); expect(result.result).toEqual([1, 2, 3]); }); it('should replace an array with an object', () => { // eslint-disable-next-line @typescript-eslint/no-explicit-any - const result = utils.fastMerge([1, 2, 3] as any, testObject, true, false, false); + const result = utils.fastMerge([1, 2, 3] as any, testObject, { + shouldRemoveNestedNulls: true, + }); expect(result.result).toEqual(testObject); }); it('should add the "ONYX_INTERNALS__REPLACE_OBJECT_MARK" flag to the target object when its source is set to null and "isBatchingMergeChanges" is true', () => { - const result = utils.fastMerge(testMergeChanges[1], testMergeChanges[0], true, true, false); + const result = utils.fastMerge(testMergeChanges[1], testMergeChanges[0], { + shouldRemoveNestedNulls: true, + isBatchingMergeChanges: true, + }); expect(result.result).toEqual({ b: { @@ -141,9 +150,10 @@ describe('fastMerge', () => { h: 'h', }, }, - true, - false, - true, + { + shouldRemoveNestedNulls: true, + shouldReplaceMarkedObjects: true, + }, ); expect(result.result).toEqual({ From d89bdd095abf842ae2eb0d8ab0eb6bf848359a4f Mon Sep 17 00:00:00 2001 From: Christoph Pader Date: Thu, 15 May 2025 17:06:28 +0200 Subject: [PATCH 03/20] combine back batchMergeChanges and applyMerge --- API-INTERNAL.md | 10 +++++----- lib/OnyxUtils.ts | 37 +++++++------------------------------ tests/unit/onyxUtilsTest.ts | 14 ++++++-------- 3 files changed, 18 insertions(+), 43 deletions(-) diff --git a/API-INTERNAL.md b/API-INTERNAL.md index d8665de66..a098586b3 100644 --- a/API-INTERNAL.md +++ b/API-INTERNAL.md @@ -149,8 +149,8 @@ if shouldRemoveNestedNulls is true and returns the object.

This method transforms an object like {'@MyApp_user': myUserValue, '@MyApp_key': myKeyValue} to an array of key-value pairs in the above format and removes key-value pairs that are being set to null

-
applyMerge(changes)
-

Merges an array of changes with an existing value

+
mergeChanges(changes)
+

Merges an array of changes with an existing value or creates a single change

initializeWithDefaultKeyStates()

Merge user provided default key value pairs.

@@ -483,10 +483,10 @@ to an array of key-value pairs in the above format and removes key-value pairs t **Kind**: global function **Returns**: an array of key - value pairs <[key, value]> - + -## applyMerge(changes) -Merges an array of changes with an existing value +## mergeChanges(changes) +Merges an array of changes with an existing value or creates a single change **Kind**: global function diff --git a/lib/OnyxUtils.ts b/lib/OnyxUtils.ts index 52452f931..98017bd33 100644 --- a/lib/OnyxUtils.ts +++ b/lib/OnyxUtils.ts @@ -1252,35 +1252,12 @@ function prepareKeyValuePairsForStorage(data: Record } /** - * Merges an array of changes with an existing value + * Merges an array of changes with an existing value or creates a single change * - * @param changes Array of changes that should be applied to the existing value + * @param changes Array of changes that should be merged + * @param existingValue The existing value that should be merged with the changes */ -function applyMerge | undefined, TChange extends OnyxInput | undefined>(existingValue: TValue, changes: TChange[]): TChange { - const lastChange = changes?.at(-1); - - if (Array.isArray(lastChange)) { - return lastChange; - } - - if (changes.some((change) => change && typeof change === 'object')) { - // Object values are then merged one after the other - return changes.reduce( - (modifiedData, change) => - utils.fastMerge(modifiedData, change, { - shouldRemoveNestedNulls: true, - shouldReplaceMarkedObjects: true, - }).result, - (existingValue || {}) as TChange, - ); - } - - // If we have anything else we can't merge it so we'll - // simply return the last value that was queued - return lastChange as TChange; -} - -function batchMergeChanges | undefined>(changes: TChange[]): FastMergeResult { +function mergeChanges | undefined, TChange extends OnyxInput | undefined>(changes: TChange[], existingValue?: TValue): FastMergeResult { const lastChange = changes?.at(-1); if (Array.isArray(lastChange)) { @@ -1299,7 +1276,7 @@ function batchMergeChanges | undefined>(chang return modifiedData; }, { - result: {} as TChange, + result: (existingValue ?? {}) as TChange, replaceNullPatches: [], }, ); @@ -1319,6 +1296,7 @@ function initializeWithDefaultKeyStates(): Promise { const merged = utils.fastMerge(existingDataAsObject, defaultKeyStates, { shouldRemoveNestedNulls: true, + shouldReplaceMarkedObjects: true, }).result; cache.merge(merged ?? {}); @@ -1509,7 +1487,7 @@ const OnyxUtils = { hasPendingMergeForKey, removeNullValues, prepareKeyValuePairsForStorage, - applyMerge, + mergeChanges, initializeWithDefaultKeyStates, getSnapshotKey, multiGet, @@ -1521,7 +1499,6 @@ const OnyxUtils = { getEvictionBlocklist, getSkippableCollectionMemberIDs, setSkippableCollectionMemberIDs, - batchMergeChanges, }; GlobalSettings.addGlobalSettingsChangeListener(({enablePerformanceMetrics}) => { diff --git a/tests/unit/onyxUtilsTest.ts b/tests/unit/onyxUtilsTest.ts index d16c0f169..c017ef3d8 100644 --- a/tests/unit/onyxUtilsTest.ts +++ b/tests/unit/onyxUtilsTest.ts @@ -151,15 +151,15 @@ describe('OnyxUtils', () => { }); }); - describe('applyMerge', () => { + describe('mergeChanges', () => { it("should return the last change if it's an array", () => { - const result = OnyxUtils.applyMerge(testObject, [...testMergeChanges, [0, 1, 2]]); + const result = OnyxUtils.mergeChanges([...testMergeChanges, [0, 1, 2]], testObject); expect(result).toEqual([0, 1, 2]); }); it("should return the last change if the changes aren't objects", () => { - const result = OnyxUtils.applyMerge(testObject, ['a', 0, 'b', 1]); + const result = OnyxUtils.mergeChanges(['a', 0, 'b', 1], testObject); expect(result).toEqual(1); }); @@ -180,7 +180,7 @@ describe('OnyxUtils', () => { }, }; - const result = OnyxUtils.applyMerge(testObject, [batchedChanges]); + const result = OnyxUtils.mergeChanges([batchedChanges], testObject); expect(result).toEqual({ a: 'a', @@ -197,11 +197,9 @@ describe('OnyxUtils', () => { }, }); }); - }); - describe('batchMergeChanges', () => { it('should apply the replacement markers if the we have properties with objects being removed and added back during the changes', () => { - const result = OnyxUtils.batchMergeChanges(testMergeChanges); + const result = OnyxUtils.mergeChanges(testMergeChanges); expect(result.result).toEqual({ b: { @@ -225,7 +223,7 @@ describe('OnyxUtils', () => { }); it('should 2', () => { - const result = OnyxUtils.batchMergeChanges([ + const result = OnyxUtils.mergeChanges([ { // Removing the "originalMessage" object in this update. // Any subsequent changes to this object should completely replace the existing object in store. From 43e6665daccfa97f9d3ae867733883d614a15d56 Mon Sep 17 00:00:00 2001 From: Christoph Pader Date: Thu, 15 May 2025 17:06:55 +0200 Subject: [PATCH 04/20] fix: simplify Onyx.merge --- lib/Onyx.ts | 41 ++++++++++++----------------------------- 1 file changed, 12 insertions(+), 29 deletions(-) diff --git a/lib/Onyx.ts b/lib/Onyx.ts index c89d24f38..cdb04f59d 100644 --- a/lib/Onyx.ts +++ b/lib/Onyx.ts @@ -41,7 +41,7 @@ function init({ initialKeyStates = {}, safeEvictionKeys = [], maxCachedKeysCount = 1000, - shouldSyncMultipleInstances = Boolean(global.localStorage), + shouldSyncMultipleInstances = !!global.localStorage, debugSetState = false, enablePerformanceMetrics = false, skippableCollectionMemberIDs = [], @@ -319,54 +319,37 @@ function merge(key: TKey, changes: OnyxMergeInput): if (!validChanges.length) { return Promise.resolve(); } - const batchedDeltaChanges = OnyxUtils.batchMergeChanges(validChanges).result; - - // Case (1): When there is no existing value in storage, we want to set the value instead of merge it. - // Case (2): The presence of a top-level `null` in the merge queue instructs us to drop the whole existing value. - // In this case, we can't simply merge the batched changes with the existing value, because then the null in the merge queue would have no effect. - const shouldSetValue = !existingValue || mergeQueue[key].includes(null); // Clean up the write queue, so we don't apply these changes again. delete mergeQueue[key]; delete mergeQueuePromise[key]; - const logMergeCall = (hasChanged = true) => { - // Logging properties only since values could be sensitive things we don't want to log. - Logger.logInfo(`merge called for key: ${key}${_.isObject(batchedDeltaChanges) ? ` properties: ${_.keys(batchedDeltaChanges).join(',')}` : ''} hasChanged: ${hasChanged}`); - }; - - // If the batched changes equal null, we want to remove the key from storage, to reduce storage size. - const {wasRemoved} = OnyxUtils.removeNullValues(key, batchedDeltaChanges); - - // Calling "OnyxUtils.removeNullValues" removes the key from storage and cache and updates the subscriber. + // Calling "OnyxUtils.remove" removes the key from storage and cache and updates the subscriber. // Therefore, we don't need to further broadcast and update the value so we can return early. - if (wasRemoved) { - logMergeCall(); + if (validChanges.at(-1) === null) { + Logger.logInfo(`merge called for key: ${key} was removed`); + OnyxUtils.remove(key); return Promise.resolve(); } - // If "shouldSetValue" is true, it means that we want to completely replace the existing value with the batched changes, - // so we pass `undefined` to OnyxUtils.applyMerge() first parameter to make it use "batchedDeltaChanges" to - // create a new object for us. - // If "shouldSetValue" is false, it means that we want to merge the batched changes into the existing value, - // so we pass "existingValue" to the first parameter. - const resultValue = OnyxUtils.applyMerge(shouldSetValue ? undefined : existingValue, [batchedDeltaChanges]); + const {result: mergedValue} = OnyxUtils.mergeChanges(validChanges, existingValue); // In cache, we don't want to remove the key if it's null to improve performance and speed up the next merge. - const hasChanged = cache.hasValueChanged(key, resultValue); + const hasChanged = cache.hasValueChanged(key, mergedValue); - logMergeCall(hasChanged); + // Logging properties only since values could be sensitive things we don't want to log. + Logger.logInfo(`merge called for key: ${key}${_.isObject(mergedValue) ? ` properties: ${_.keys(mergedValue).join(',')}` : ''} hasChanged: ${hasChanged}`); // This approach prioritizes fast UI changes without waiting for data to be stored in device storage. - const updatePromise = OnyxUtils.broadcastUpdate(key, resultValue as OnyxValue, hasChanged); + const updatePromise = OnyxUtils.broadcastUpdate(key, mergedValue as OnyxValue, hasChanged); // If the value has not changed, calling Storage.setItem() would be redundant and a waste of performance, so return early instead. if (!hasChanged) { return updatePromise; } - return Storage.mergeItem(key, resultValue as OnyxValue).then(() => { - OnyxUtils.sendActionToDevTools(OnyxUtils.METHOD.MERGE, key, changes, resultValue); + return Storage.setItem(key, mergedValue as OnyxValue).then(() => { + OnyxUtils.sendActionToDevTools(OnyxUtils.METHOD.MERGE, key, changes, mergedValue); return updatePromise; }); } catch (error) { From f190fbd46efb2c67faf0b338beeb6db4801d9cf0 Mon Sep 17 00:00:00 2001 From: Christoph Pader Date: Thu, 15 May 2025 17:07:17 +0200 Subject: [PATCH 05/20] fix: improve Onyx.update --- lib/Onyx.ts | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/lib/Onyx.ts b/lib/Onyx.ts index cdb04f59d..81c1da503 100644 --- a/lib/Onyx.ts +++ b/lib/Onyx.ts @@ -757,7 +757,7 @@ function update(data: OnyxUpdate[]): Promise { // Remove the collection-related key from the updateQueue so that it won't be processed individually. delete updateQueue[key]; - const batchedChanges = OnyxUtils.batchMergeChanges(operations); + const batchedChanges = OnyxUtils.mergeChanges(operations); if (operations[0] === null) { // eslint-disable-next-line no-param-reassign queue.set[key] = batchedChanges.result; @@ -789,13 +789,16 @@ function update(data: OnyxUpdate[]): Promise { }); Object.entries(updateQueue).forEach(([key, operations]) => { - const batchedChanges = OnyxUtils.batchMergeChanges(operations).result; - if (operations[0] === null) { + const batchedChanges = OnyxUtils.mergeChanges(operations).result; promises.push(() => set(key, batchedChanges)); - } else { - promises.push(() => merge(key, batchedChanges)); + return; } + + const mergePromises = operations.map((operation) => { + return merge(key, operation); + }); + promises.push(() => mergePromises.at(0) ?? Promise.resolve()); }); const snapshotPromises = updateSnapshots(data); From 2701874d8ecf876cb8b608deda1219a0034166f1 Mon Sep 17 00:00:00 2001 From: Christoph Pader Date: Thu, 15 May 2025 17:07:30 +0200 Subject: [PATCH 06/20] fix: change signature of storage.mergeItem --- lib/storage/index.ts | 4 ++-- lib/storage/providers/IDBKeyValProvider.ts | 4 ++-- lib/storage/providers/MemoryOnlyProvider.ts | 4 ++-- lib/storage/providers/SQLiteProvider.ts | 6 +++--- lib/storage/providers/types.ts | 4 ++-- 5 files changed, 11 insertions(+), 11 deletions(-) diff --git a/lib/storage/index.ts b/lib/storage/index.ts index 33befed1d..f9b9e2d57 100644 --- a/lib/storage/index.ts +++ b/lib/storage/index.ts @@ -116,9 +116,9 @@ const storage: Storage = { /** * Merging an existing value with a new one */ - mergeItem: (key, preMergedValue) => + mergeItem: (key, change) => tryOrDegradePerformance(() => { - const promise = provider.mergeItem(key, preMergedValue); + const promise = provider.mergeItem(key, change); if (shouldKeepInstancesSync) { return promise.then(() => InstanceSync.mergeItem(key)); diff --git a/lib/storage/providers/IDBKeyValProvider.ts b/lib/storage/providers/IDBKeyValProvider.ts index ac40eb2f1..f4062304b 100644 --- a/lib/storage/providers/IDBKeyValProvider.ts +++ b/lib/storage/providers/IDBKeyValProvider.ts @@ -58,9 +58,9 @@ const provider: StorageProvider = { return Promise.all(upsertMany); }); }), - mergeItem(key, preMergedValue) { + mergeItem(key, change) { // Since Onyx already merged the existing value with the changes, we can just set the value directly. - return provider.setItem(key, preMergedValue); + return provider.multiMerge([[key, change]]); }, multiSet: (pairs) => { const pairsWithoutNull = pairs.filter(([key, value]) => { diff --git a/lib/storage/providers/MemoryOnlyProvider.ts b/lib/storage/providers/MemoryOnlyProvider.ts index 3ec4d6b56..de9a3acfd 100644 --- a/lib/storage/providers/MemoryOnlyProvider.ts +++ b/lib/storage/providers/MemoryOnlyProvider.ts @@ -74,9 +74,9 @@ const provider: StorageProvider = { /** * Merging an existing value with a new one */ - mergeItem(key, preMergedValue) { + mergeItem(key, change) { // Since Onyx already merged the existing value with the changes, we can just set the value directly. - return this.setItem(key, preMergedValue); + return this.multiMerge([[key, change]]); }, /** diff --git a/lib/storage/providers/SQLiteProvider.ts b/lib/storage/providers/SQLiteProvider.ts index d3dd7d7de..660a052de 100644 --- a/lib/storage/providers/SQLiteProvider.ts +++ b/lib/storage/providers/SQLiteProvider.ts @@ -3,7 +3,7 @@ * converting the value to a JSON string */ import {getFreeDiskStorage} from 'react-native-device-info'; -import type {BatchQueryResult, QuickSQLiteConnection, SQLBatchTuple} from 'react-native-quick-sqlite'; +import type {QuickSQLiteConnection, SQLBatchTuple} from 'react-native-quick-sqlite'; import {open} from 'react-native-quick-sqlite'; import type {FastMergeReplaceNullPatch} from '../../utils'; import utils from '../../utils'; @@ -114,9 +114,9 @@ const provider: StorageProvider = { return db.executeBatchAsync(commands); }, - mergeItem(key, preMergedValue) { + mergeItem(key, change) { // Since Onyx already merged the existing value with the changes, we can just set the value directly. - return this.setItem(key, preMergedValue) as Promise; + return this.multiMerge([[key, change]]); }, getAllKeys: () => db.executeAsync('SELECT record_key FROM keyvaluepairs;').then(({rows}) => { diff --git a/lib/storage/providers/types.ts b/lib/storage/providers/types.ts index 6304d0866..0a182a60e 100644 --- a/lib/storage/providers/types.ts +++ b/lib/storage/providers/types.ts @@ -43,9 +43,9 @@ type StorageProvider = { /** * Merges an existing value with a new one - * @param preMergedValue - the pre-merged data from `Onyx.applyMerge` + * @param change - the change to merge with the existing value */ - mergeItem: (key: TKey, preMergedValue: OnyxValue) => Promise; + mergeItem: (key: TKey, change: OnyxValue) => Promise; /** * Returns all keys available in storage From defed16fb9b7cc21bc07ff42c3b5a9e2a171c776 Mon Sep 17 00:00:00 2001 From: Christoph Pader Date: Tue, 20 May 2025 13:08:20 +0200 Subject: [PATCH 07/20] refactor: remove return values from storage providers --- lib/storage/providers/IDBKeyValProvider.ts | 2 +- lib/storage/providers/MemoryOnlyProvider.ts | 2 +- lib/storage/providers/NoopProvider.ts | 2 +- lib/storage/providers/SQLiteProvider.ts | 12 +++++------ lib/storage/providers/types.ts | 22 ++++++++++++--------- 5 files changed, 22 insertions(+), 18 deletions(-) diff --git a/lib/storage/providers/IDBKeyValProvider.ts b/lib/storage/providers/IDBKeyValProvider.ts index f4062304b..036592d7b 100644 --- a/lib/storage/providers/IDBKeyValProvider.ts +++ b/lib/storage/providers/IDBKeyValProvider.ts @@ -55,7 +55,7 @@ const provider: StorageProvider = { }).result; return promisifyRequest(store.put(newValue, key)); }); - return Promise.all(upsertMany); + return Promise.all(upsertMany).then(() => undefined); }); }), mergeItem(key, change) { diff --git a/lib/storage/providers/MemoryOnlyProvider.ts b/lib/storage/providers/MemoryOnlyProvider.ts index de9a3acfd..a6016f99c 100644 --- a/lib/storage/providers/MemoryOnlyProvider.ts +++ b/lib/storage/providers/MemoryOnlyProvider.ts @@ -94,7 +94,7 @@ const provider: StorageProvider = { set(key, newValue); }); - return Promise.resolve([]); + return Promise.resolve(); }, /** diff --git a/lib/storage/providers/NoopProvider.ts b/lib/storage/providers/NoopProvider.ts index f99af069c..ccbee65a6 100644 --- a/lib/storage/providers/NoopProvider.ts +++ b/lib/storage/providers/NoopProvider.ts @@ -54,7 +54,7 @@ const provider: StorageProvider = { * This function also removes all nested null values from an object. */ multiMerge() { - return Promise.resolve([]); + return Promise.resolve(); }, /** diff --git a/lib/storage/providers/SQLiteProvider.ts b/lib/storage/providers/SQLiteProvider.ts index 660a052de..fd3363f79 100644 --- a/lib/storage/providers/SQLiteProvider.ts +++ b/lib/storage/providers/SQLiteProvider.ts @@ -65,14 +65,14 @@ const provider: StorageProvider = { }); }, setItem(key, value) { - return db.executeAsync('REPLACE INTO keyvaluepairs (record_key, valueJSON) VALUES (?, ?);', [key, JSON.stringify(value)]); + return db.executeAsync('REPLACE INTO keyvaluepairs (record_key, valueJSON) VALUES (?, ?);', [key, JSON.stringify(value)]).then(() => undefined); }, multiSet(pairs) { const stringifiedPairs = pairs.map((pair) => [pair[0], JSON.stringify(pair[1] === undefined ? null : pair[1])]); if (utils.isEmptyObject(stringifiedPairs)) { return Promise.resolve(); } - return db.executeBatchAsync([['REPLACE INTO keyvaluepairs (record_key, valueJSON) VALUES (?, json(?));', stringifiedPairs]]); + return db.executeBatchAsync([['REPLACE INTO keyvaluepairs (record_key, valueJSON) VALUES (?, json(?));', stringifiedPairs]]).then(() => undefined); }, multiMerge(pairs, mergeReplaceNullPatches) { const commands: SQLBatchTuple[] = []; @@ -112,7 +112,7 @@ const provider: StorageProvider = { commands.push([replaceQuery, replaceQueryArguments]); } - return db.executeBatchAsync(commands); + return db.executeBatchAsync(commands).then(() => undefined); }, mergeItem(key, change) { // Since Onyx already merged the existing value with the changes, we can just set the value directly. @@ -124,13 +124,13 @@ const provider: StorageProvider = { const result = rows?._array.map((row) => row.record_key); return (result ?? []) as KeyList; }), - removeItem: (key) => db.executeAsync('DELETE FROM keyvaluepairs WHERE record_key = ?;', [key]), + removeItem: (key) => db.executeAsync('DELETE FROM keyvaluepairs WHERE record_key = ?;', [key]).then(() => undefined), removeItems: (keys) => { const placeholders = keys.map(() => '?').join(','); const query = `DELETE FROM keyvaluepairs WHERE record_key IN (${placeholders});`; - return db.executeAsync(query, keys); + return db.executeAsync(query, keys).then(() => undefined); }, - clear: () => db.executeAsync('DELETE FROM keyvaluepairs;', []), + clear: () => db.executeAsync('DELETE FROM keyvaluepairs;', []).then(() => undefined), getDatabaseSize() { return Promise.all([db.executeAsync('PRAGMA page_size;'), db.executeAsync('PRAGMA page_count;'), getFreeDiskStorage()]).then(([pageSizeResult, pageCountResult, bytesRemaining]) => { const pageSize: number = pageSizeResult.rows?.item(0).page_size; diff --git a/lib/storage/providers/types.ts b/lib/storage/providers/types.ts index 0a182a60e..fc3b3cade 100644 --- a/lib/storage/providers/types.ts +++ b/lib/storage/providers/types.ts @@ -1,10 +1,14 @@ -import type {BatchQueryResult, QueryResult} from 'react-native-quick-sqlite'; import type {MixedOperationsQueue, OnyxKey, OnyxValue} from '../../types'; type KeyValuePair = [OnyxKey, OnyxValue]; type KeyList = OnyxKey[]; type KeyValuePairList = KeyValuePair[]; +type DatabaseSize = { + bytesUsed: number; + bytesRemaining: number; +}; + type OnStorageKeyChanged = (key: TKey, value: OnyxValue) => void; type StorageProvider = { @@ -29,23 +33,23 @@ type StorageProvider = { /** * Sets the value for a given key. The only requirement is that the value should be serializable to JSON string */ - setItem: (key: TKey, value: OnyxValue) => Promise; + setItem: (key: TKey, value: OnyxValue) => Promise; /** * Stores multiple key-value pairs in a batch */ - multiSet: (pairs: KeyValuePairList) => Promise; + multiSet: (pairs: KeyValuePairList) => Promise; /** * Multiple merging of existing and new values in a batch */ - multiMerge: (pairs: KeyValuePairList, mergeReplaceNullPatches?: MixedOperationsQueue['mergeReplaceNullPatches']) => Promise; + multiMerge: (pairs: KeyValuePairList, mergeReplaceNullPatches?: MixedOperationsQueue['mergeReplaceNullPatches']) => Promise; /** * Merges an existing value with a new one * @param change - the change to merge with the existing value */ - mergeItem: (key: TKey, change: OnyxValue) => Promise; + mergeItem: (key: TKey, change: OnyxValue) => Promise; /** * Returns all keys available in storage @@ -55,22 +59,22 @@ type StorageProvider = { /** * Removes given key and its value from storage */ - removeItem: (key: OnyxKey) => Promise; + removeItem: (key: OnyxKey) => Promise; /** * Removes given keys and their values from storage */ - removeItems: (keys: KeyList) => Promise; + removeItems: (keys: KeyList) => Promise; /** * Clears absolutely everything from storage */ - clear: () => Promise; + clear: () => Promise; /** * Gets the total bytes of the database file */ - getDatabaseSize: () => Promise<{bytesUsed: number; bytesRemaining: number}>; + getDatabaseSize: () => Promise; /** * @param onStorageKeyChanged Storage synchronization mechanism keeping all opened tabs in sync From a6312b16fc3fb362206600cbfce9748dc48a5a51 Mon Sep 17 00:00:00 2001 From: Christoph Pader Date: Tue, 20 May 2025 13:11:31 +0200 Subject: [PATCH 08/20] fix: invalid fastMerge option --- tests/perf-test/utils.perf-test.ts | 1 - 1 file changed, 1 deletion(-) diff --git a/tests/perf-test/utils.perf-test.ts b/tests/perf-test/utils.perf-test.ts index e0ada9009..6df36df08 100644 --- a/tests/perf-test/utils.perf-test.ts +++ b/tests/perf-test/utils.perf-test.ts @@ -18,7 +18,6 @@ describe('[Utils.js]', () => { await measureFunction(() => utils.fastMerge(target, source, { shouldRemoveNestedNulls: true, - shouldReplaceMarkedObjects: true, }), ); }); From 5f69d7a8bdac79c4ca175b19641b075b4960f3b4 Mon Sep 17 00:00:00 2001 From: Christoph Pader Date: Tue, 20 May 2025 13:18:23 +0200 Subject: [PATCH 09/20] fix: TS error --- tests/unit/fastMergeTest.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/unit/fastMergeTest.ts b/tests/unit/fastMergeTest.ts index f824d10d7..4ed2cef6a 100644 --- a/tests/unit/fastMergeTest.ts +++ b/tests/unit/fastMergeTest.ts @@ -54,7 +54,7 @@ const testMergeChanges: DeepObject[] = [ describe('fastMerge', () => { it('should merge an object with another object and remove nested null values', () => { - const result = utils.fastMerge(testObject, testObjectWithNullishValues, true, false, false); + const result = utils.fastMerge(testObject, testObjectWithNullishValues, {shouldRemoveNestedNulls: true}); expect(result.result).toEqual({ a: 'a', @@ -71,7 +71,7 @@ describe('fastMerge', () => { }); it('should merge an object with another object and not remove nested null values', () => { - const result = utils.fastMerge(testObject, testObjectWithNullishValues, false, false, false); + const result = utils.fastMerge(testObject, testObjectWithNullishValues); expect(result.result).toEqual({ a: 'a', From 173802a9ef320f2e2ba0b8ac35081362aedfea1b Mon Sep 17 00:00:00 2001 From: Christoph Pader Date: Tue, 20 May 2025 13:18:29 +0200 Subject: [PATCH 10/20] add empty lines --- lib/utils.ts | 3 +++ 1 file changed, 3 insertions(+) diff --git a/lib/utils.ts b/lib/utils.ts index fbb6c3ab2..69bc63846 100644 --- a/lib/utils.ts +++ b/lib/utils.ts @@ -10,8 +10,10 @@ type FastMergeReplaceNullPatch = [string[], unknown]; type FastMergeOptions = { /** If true, null object values will be removed. */ shouldRemoveNestedNulls?: boolean; + /** If true, it means that we are batching merge changes before applying them to the Onyx value, so we must use a special logic to handle these changes. */ isBatchingMergeChanges?: boolean; + /** If true, any nested objects that contains the internal "ONYX_INTERNALS__REPLACE_OBJECT_MARK" flag will be completely replaced instead of merged. */ shouldReplaceMarkedObjects?: boolean; }; @@ -24,6 +26,7 @@ type FastMergeMetadata = { type FastMergeResult = { /** The result of the merge. */ result: TValue; + /** The path to the object that contains the internal "ONYX_INTERNALS__REPLACE_OBJECT_MARK" flag. */ replaceNullPatches: FastMergeReplaceNullPatch[]; }; From 351cc720cbd7c4b44edb6018533b464d2cbae688 Mon Sep 17 00:00:00 2001 From: Christoph Pader Date: Tue, 20 May 2025 13:27:15 +0200 Subject: [PATCH 11/20] add missing continue --- lib/utils.ts | 2 ++ 1 file changed, 2 insertions(+) diff --git a/lib/utils.ts b/lib/utils.ts index 69bc63846..289170cc3 100644 --- a/lib/utils.ts +++ b/lib/utils.ts @@ -131,6 +131,8 @@ function mergeObject>( if (!isMergeableObject(sourceProperty)) { // If source value is any other value we need to set the source value it directly. destination[key] = sourceProperty; + // eslint-disable-next-line no-continue + continue; } const targetProperty = targetObject?.[key]; From b7c84fbab543d29971af994adcd0d34d2d3ddf60 Mon Sep 17 00:00:00 2001 From: Christoph Pader Date: Tue, 20 May 2025 16:02:54 +0200 Subject: [PATCH 12/20] fix: simplify fastMerge --- lib/OnyxUtils.ts | 2 +- lib/utils.ts | 70 +++++++++++++------------------------ tests/unit/fastMergeTest.ts | 2 +- 3 files changed, 26 insertions(+), 48 deletions(-) diff --git a/lib/OnyxUtils.ts b/lib/OnyxUtils.ts index 98017bd33..c4c741873 100644 --- a/lib/OnyxUtils.ts +++ b/lib/OnyxUtils.ts @@ -1268,7 +1268,7 @@ function mergeChanges | undefined, TChange ext // Object values are then merged one after the other return changes.reduce>( (modifiedData, change) => { - const fastMergeResult = utils.fastMerge(modifiedData.result, change, {isBatchingMergeChanges: true}); + const fastMergeResult = utils.fastMerge(modifiedData.result, change, {shouldMarkRemovedObjects: true}); // eslint-disable-next-line no-param-reassign modifiedData.result = fastMergeResult.result; // eslint-disable-next-line no-param-reassign diff --git a/lib/utils.ts b/lib/utils.ts index 289170cc3..90dab2eb6 100644 --- a/lib/utils.ts +++ b/lib/utils.ts @@ -12,7 +12,7 @@ type FastMergeOptions = { shouldRemoveNestedNulls?: boolean; /** If true, it means that we are batching merge changes before applying them to the Onyx value, so we must use a special logic to handle these changes. */ - isBatchingMergeChanges?: boolean; + shouldMarkRemovedObjects?: boolean; /** If true, any nested objects that contains the internal "ONYX_INTERNALS__REPLACE_OBJECT_MARK" flag will be completely replaced instead of merged. */ shouldReplaceMarkedObjects?: boolean; @@ -136,16 +136,33 @@ function mergeObject>( } const targetProperty = targetObject?.[key]; - const targetWithMarks = getTargetPropertyWithRemovalMark(targetProperty, sourceProperty, options, metadata, basePath); - const {finalDestinationProperty, stopTraversing} = replaceMarkedObjects(sourceProperty, options); + const targetPropertyWithMarks = (targetProperty ?? {}) as Record; + + // If we are batching merge changes and the previous merge change (targetValue) is null, + // it means we want to fully replace this object when merging the batched changes with the Onyx value. + // To achieve this, we first mark these nested objects with an internal flag. With the desired objects + // marked, when calling this method again with "shouldReplaceMarkedObjects" set to true we can proceed + // effectively replace them in the next condition. + if (options?.shouldMarkRemovedObjects && targetProperty === null) { + targetPropertyWithMarks[ONYX_INTERNALS__REPLACE_OBJECT_MARK] = true; + metadata.replaceNullPatches.push([[...basePath], {...sourceProperty}]); + } + + // Later, when merging the batched changes with the Onyx value, if a nested object of the batched changes + // has the internal flag set, we replace the entire destination object with the source one and remove + // the flag. + if (options.shouldReplaceMarkedObjects && sourceProperty[ONYX_INTERNALS__REPLACE_OBJECT_MARK]) { + // We do a spread here in order to have a new object reference and allow us to delete the internal flag + // of the merged object only. + // eslint-disable-next-line @typescript-eslint/no-unused-vars + const {ONYX_INTERNALS__REPLACE_OBJECT_MARK: _mark, ...sourcePropertyWithoutMark} = sourceProperty; - if (stopTraversing) { - destination[key] = finalDestinationProperty; + destination[key] = sourcePropertyWithoutMark; // eslint-disable-next-line no-continue continue; } - destination[key] = fastMerge(targetWithMarks, sourceProperty, options, metadata, [...basePath, key]).result; + destination[key] = fastMerge(targetPropertyWithMarks, sourceProperty, options, metadata, [...basePath, key]).result; } return destination as TObject; @@ -166,51 +183,12 @@ function isMergeableObject>(value: unkno return isNonNullObject && !(value instanceof RegExp) && !(value instanceof Date) && !Array.isArray(value); } -function getTargetPropertyWithRemovalMark>( - targetProperty: unknown, - sourceProperty: Record, - options: FastMergeOptions, - metadata: FastMergeMetadata, - basePath: string[] = [], -): TObject { - const targetPropertyWithMarks = (targetProperty ?? {}) as Record; - - // If we are batching merge changes and the previous merge change (targetValue) is null, - // it means we want to fully replace this object when merging the batched changes with the Onyx value. - // To achieve this, we first mark these nested objects with an internal flag. With the desired objects - // marked, when calling this method again with "shouldReplaceMarkedObjects" set to true we can proceed - // effectively replace them in the next condition. - if (options?.isBatchingMergeChanges && targetProperty === null) { - targetPropertyWithMarks[ONYX_INTERNALS__REPLACE_OBJECT_MARK] = true; - metadata.replaceNullPatches.push([[...basePath], {...sourceProperty}]); - } - - return targetPropertyWithMarks as TObject; -} - -function replaceMarkedObjects>(sourceProperty: TObject, options: FastMergeOptions): {finalDestinationProperty?: TObject; stopTraversing: boolean} { - // Then, when merging the batched changes with the Onyx value, if a nested object of the batched changes - // has the internal flag set, we replace the entire destination object with the source one and remove - // the flag. - if (options.shouldReplaceMarkedObjects && sourceProperty[ONYX_INTERNALS__REPLACE_OBJECT_MARK]) { - // We do a spread here in order to have a new object reference and allow us to delete the internal flag - // of the merged object only. - - const destinationProperty = {...sourceProperty}; - delete destinationProperty.ONYX_INTERNALS__REPLACE_OBJECT_MARK; - return {finalDestinationProperty: destinationProperty, stopTraversing: true}; - } - - // For the normal situations we'll just call `fastMerge()` again to merge the nested object. - return {stopTraversing: false}; -} - /** Deep removes the nested null values from the given value. */ function removeNestedNullValues | null>(value: TValue): TValue { if (typeof value === 'object' && !Array.isArray(value)) { return fastMerge(value, value, { shouldRemoveNestedNulls: true, - isBatchingMergeChanges: false, + shouldMarkRemovedObjects: false, shouldReplaceMarkedObjects: false, }).result; } diff --git a/tests/unit/fastMergeTest.ts b/tests/unit/fastMergeTest.ts index 4ed2cef6a..24016f62c 100644 --- a/tests/unit/fastMergeTest.ts +++ b/tests/unit/fastMergeTest.ts @@ -123,7 +123,7 @@ describe('fastMerge', () => { it('should add the "ONYX_INTERNALS__REPLACE_OBJECT_MARK" flag to the target object when its source is set to null and "isBatchingMergeChanges" is true', () => { const result = utils.fastMerge(testMergeChanges[1], testMergeChanges[0], { shouldRemoveNestedNulls: true, - isBatchingMergeChanges: true, + shouldMarkRemovedObjects: true, }); expect(result.result).toEqual({ From ee5d461f6feaaecb2e8d49ab726b7c2a1608e91b Mon Sep 17 00:00:00 2001 From: Christoph Pader Date: Wed, 21 May 2025 12:58:03 +0200 Subject: [PATCH 13/20] fix: further improve fastMerge code --- lib/Onyx.ts | 8 +- lib/OnyxCache.ts | 6 +- lib/OnyxUtils.ts | 30 +++++-- lib/storage/providers/IDBKeyValProvider.ts | 2 +- lib/storage/providers/MemoryOnlyProvider.ts | 2 +- lib/utils.ts | 96 +++++++++------------ tests/unit/fastMergeTest.ts | 6 +- tests/unit/onyxUtilsTest.ts | 6 +- 8 files changed, 82 insertions(+), 74 deletions(-) diff --git a/lib/Onyx.ts b/lib/Onyx.ts index 81c1da503..6fd5c0f93 100644 --- a/lib/Onyx.ts +++ b/lib/Onyx.ts @@ -307,7 +307,7 @@ function merge(key: TKey, changes: OnyxMergeInput): } try { - // We first only merge the changes, so we use OnyxUtils.batchMergeChanges() to combine all the changes into just one. + // We first only merge the changes, so we use OnyxUtils.mergeChanges() to combine all the changes into just one. const validChanges = mergeQueue[key].filter((change) => { const {isCompatible, existingValueType, newValueType} = utils.checkCompatibilityWithExistingValue(change, existingValue); if (!isCompatible) { @@ -332,7 +332,7 @@ function merge(key: TKey, changes: OnyxMergeInput): return Promise.resolve(); } - const {result: mergedValue} = OnyxUtils.mergeChanges(validChanges, existingValue); + const {result: mergedValue} = OnyxUtils.mergeAndMarkChanges(validChanges, existingValue); // In cache, we don't want to remove the key if it's null to improve performance and speed up the next merge. const hasChanged = cache.hasValueChanged(key, mergedValue); @@ -757,7 +757,7 @@ function update(data: OnyxUpdate[]): Promise { // Remove the collection-related key from the updateQueue so that it won't be processed individually. delete updateQueue[key]; - const batchedChanges = OnyxUtils.mergeChanges(operations); + const batchedChanges = OnyxUtils.mergeAndMarkChanges(operations); if (operations[0] === null) { // eslint-disable-next-line no-param-reassign queue.set[key] = batchedChanges.result; @@ -790,7 +790,7 @@ function update(data: OnyxUpdate[]): Promise { Object.entries(updateQueue).forEach(([key, operations]) => { if (operations[0] === null) { - const batchedChanges = OnyxUtils.mergeChanges(operations).result; + const batchedChanges = OnyxUtils.mergeAndMarkChanges(operations).result; promises.push(() => set(key, batchedChanges)); return; } diff --git a/lib/OnyxCache.ts b/lib/OnyxCache.ts index d0d4585d4..adc52778b 100644 --- a/lib/OnyxCache.ts +++ b/lib/OnyxCache.ts @@ -167,7 +167,7 @@ class OnyxCache { this.storageMap = { ...utils.fastMerge(this.storageMap, data, { shouldRemoveNestedNulls: true, - shouldReplaceMarkedObjects: true, + objectRemovalMode: 'replace', }).result, }; @@ -232,6 +232,10 @@ class OnyxCache { const temp = []; while (numKeysToRemove > 0) { const value = iterator.next().value; + if (value === undefined) { + break; + } + temp.push(value); numKeysToRemove--; } diff --git a/lib/OnyxUtils.ts b/lib/OnyxUtils.ts index c4c741873..1003d3ae1 100644 --- a/lib/OnyxUtils.ts +++ b/lib/OnyxUtils.ts @@ -28,7 +28,7 @@ import type { OnyxValue, Selector, } from './types'; -import type {FastMergeResult} from './utils'; +import type {FastMergeOptions, FastMergeResult} from './utils'; import utils from './utils'; import type {WithOnyxState} from './withOnyx/types'; import type {DeferredTask} from './createDeferredTask'; @@ -1251,13 +1251,28 @@ function prepareKeyValuePairsForStorage(data: Record }, []); } +function mergeChanges | undefined, TChange extends OnyxInput | undefined>(changes: TChange[], existingValue?: TValue): FastMergeResult { + return applyMerge('merge', changes, existingValue); +} + +function mergeAndMarkChanges | undefined, TChange extends OnyxInput | undefined>( + changes: TChange[], + existingValue?: TValue, +): FastMergeResult { + return applyMerge('mark', changes, existingValue); +} + /** * Merges an array of changes with an existing value or creates a single change * * @param changes Array of changes that should be merged * @param existingValue The existing value that should be merged with the changes */ -function mergeChanges | undefined, TChange extends OnyxInput | undefined>(changes: TChange[], existingValue?: TValue): FastMergeResult { +function applyMerge | undefined, TChange extends OnyxInput | undefined>( + mode: 'merge' | 'mark', + changes: TChange[], + existingValue?: TValue, +): FastMergeResult { const lastChange = changes?.at(-1); if (Array.isArray(lastChange)) { @@ -1268,11 +1283,14 @@ function mergeChanges | undefined, TChange ext // Object values are then merged one after the other return changes.reduce>( (modifiedData, change) => { - const fastMergeResult = utils.fastMerge(modifiedData.result, change, {shouldMarkRemovedObjects: true}); + const options: FastMergeOptions = mode === 'merge' ? {shouldRemoveNestedNulls: true, objectRemovalMode: 'replace'} : {objectRemovalMode: 'mark'}; + const {result, replaceNullPatches} = utils.fastMerge(modifiedData.result, change, options); + // eslint-disable-next-line no-param-reassign - modifiedData.result = fastMergeResult.result; + modifiedData.result = result; // eslint-disable-next-line no-param-reassign - modifiedData.replaceNullPatches = [...modifiedData.replaceNullPatches, ...fastMergeResult.replaceNullPatches]; + modifiedData.replaceNullPatches = [...modifiedData.replaceNullPatches, ...replaceNullPatches]; + return modifiedData; }, { @@ -1296,7 +1314,6 @@ function initializeWithDefaultKeyStates(): Promise { const merged = utils.fastMerge(existingDataAsObject, defaultKeyStates, { shouldRemoveNestedNulls: true, - shouldReplaceMarkedObjects: true, }).result; cache.merge(merged ?? {}); @@ -1488,6 +1505,7 @@ const OnyxUtils = { removeNullValues, prepareKeyValuePairsForStorage, mergeChanges, + mergeAndMarkChanges, initializeWithDefaultKeyStates, getSnapshotKey, multiGet, diff --git a/lib/storage/providers/IDBKeyValProvider.ts b/lib/storage/providers/IDBKeyValProvider.ts index 036592d7b..c61fc851e 100644 --- a/lib/storage/providers/IDBKeyValProvider.ts +++ b/lib/storage/providers/IDBKeyValProvider.ts @@ -51,7 +51,7 @@ const provider: StorageProvider = { const prev = values[index]; const newValue = utils.fastMerge(prev as Record, value as Record, { shouldRemoveNestedNulls: true, - shouldReplaceMarkedObjects: true, + objectRemovalMode: 'replace', }).result; return promisifyRequest(store.put(newValue, key)); }); diff --git a/lib/storage/providers/MemoryOnlyProvider.ts b/lib/storage/providers/MemoryOnlyProvider.ts index a6016f99c..1af95a6e3 100644 --- a/lib/storage/providers/MemoryOnlyProvider.ts +++ b/lib/storage/providers/MemoryOnlyProvider.ts @@ -88,7 +88,7 @@ const provider: StorageProvider = { const existingValue = store[key] as Record; const newValue = utils.fastMerge(existingValue, value as Record, { shouldRemoveNestedNulls: true, - shouldReplaceMarkedObjects: true, + objectRemovalMode: 'replace', }).result as OnyxValue; set(key, newValue); diff --git a/lib/utils.ts b/lib/utils.ts index 90dab2eb6..16b83a2eb 100644 --- a/lib/utils.ts +++ b/lib/utils.ts @@ -11,8 +11,12 @@ type FastMergeOptions = { /** If true, null object values will be removed. */ shouldRemoveNestedNulls?: boolean; - /** If true, it means that we are batching merge changes before applying them to the Onyx value, so we must use a special logic to handle these changes. */ - shouldMarkRemovedObjects?: boolean; + /** + * If set to "mark", we will mark objects that are set to null instead of simply removing them, + * so that we can batch changes together, without loosing information about the object removal. + * If set to "replace", we will completely replace the marked objects with the new value instead of merging them. + * */ + objectRemovalMode?: 'mark' | 'replace' | 'none'; /** If true, any nested objects that contains the internal "ONYX_INTERNALS__REPLACE_OBJECT_MARK" flag will be completely replaced instead of merged. */ shouldReplaceMarkedObjects?: boolean; @@ -53,11 +57,9 @@ function fastMerge(target: TValue, source: TValue, options?: FastMergeOp return {result: source, replaceNullPatches: metadata.replaceNullPatches}; } - const optionsWithDefaults = { - shouldRemoveNestedNulls: false, - isBatchingMergeChanges: false, - shouldReplaceMarkedObjects: false, - ...options, + const optionsWithDefaults: FastMergeOptions = { + shouldRemoveNestedNulls: options?.shouldRemoveNestedNulls ?? false, + objectRemovalMode: options?.objectRemovalMode ?? 'none', }; const mergedValue = mergeObject(target, source as Record, optionsWithDefaults, metadata, basePath) as TValue; @@ -90,80 +92,65 @@ function mergeObject>( // If "shouldRemoveNestedNulls" is true, we want to remove null values from the merged object // and therefore we need to omit keys where either the source or target value is null. if (targetObject) { - // eslint-disable-next-line no-restricted-syntax, guard-for-in - for (const key in targetObject) { + Object.keys(targetObject).forEach((key) => { const targetProperty = targetObject?.[key]; - if (targetProperty === undefined) { - // eslint-disable-next-line no-continue - continue; - } + const sourceProperty = source?.[key]; // If "shouldRemoveNestedNulls" is true, we want to remove (nested) null values from the merged object. // If either the source or target value is null, we want to omit the key from the merged object. - const sourceProperty = source?.[key]; - const isSourceOrTargetNull = targetProperty === null || sourceProperty === null; - const shouldOmitTargetKey = options.shouldRemoveNestedNulls && isSourceOrTargetNull; + const shouldOmitNullishProperty = options.shouldRemoveNestedNulls && (targetProperty === null || sourceProperty === null); - if (!shouldOmitTargetKey) { - destination[key] = targetProperty; + if (targetProperty === undefined || shouldOmitNullishProperty) { + return; } - } + + destination[key] = targetProperty; + }); } // After copying over all keys from the target object, we want to merge the source object into the destination object. - // eslint-disable-next-line no-restricted-syntax, guard-for-in - for (const key in source) { + Object.keys(source).forEach((key) => { + let targetProperty = targetObject?.[key]; const sourceProperty = source?.[key] as Record; - if (sourceProperty === undefined) { - // eslint-disable-next-line no-continue - continue; - } - // If "shouldRemoveNestedNulls" is set to true and the source value is null, - // we don't want to set/merge the source value into the merged object. - const shouldOmitSourceKey = options.shouldRemoveNestedNulls && sourceProperty === null; - if (shouldOmitSourceKey) { - // eslint-disable-next-line no-continue - continue; + // If "shouldRemoveNestedNulls" is true, we want to remove (nested) null values from the merged object. + // If either the source value is null, we want to omit the key from the merged object. + const shouldOmitNullishProperty = options.shouldRemoveNestedNulls && sourceProperty === null; + + if (sourceProperty === undefined || shouldOmitNullishProperty) { + return; } - // If the source value is a mergable object, we want to merge it into the target value. + // If source value is not a mergable object, we need to set the source value it directly. if (!isMergeableObject(sourceProperty)) { - // If source value is any other value we need to set the source value it directly. destination[key] = sourceProperty; - // eslint-disable-next-line no-continue - continue; + return; } - const targetProperty = targetObject?.[key]; - const targetPropertyWithMarks = (targetProperty ?? {}) as Record; - - // If we are batching merge changes and the previous merge change (targetValue) is null, + // If "shouldMarkRemovedObjects" is enabled and the previous merge change (targetProperty) is null, // it means we want to fully replace this object when merging the batched changes with the Onyx value. - // To achieve this, we first mark these nested objects with an internal flag. With the desired objects - // marked, when calling this method again with "shouldReplaceMarkedObjects" set to true we can proceed - // effectively replace them in the next condition. - if (options?.shouldMarkRemovedObjects && targetProperty === null) { - targetPropertyWithMarks[ONYX_INTERNALS__REPLACE_OBJECT_MARK] = true; + // To achieve this, we first mark these nested objects with an internal flag. + // When calling fastMerge again with "shouldReplaceMarkedObjects" enabled, the marked objects will be removed. + if (options.objectRemovalMode === 'mark' && targetProperty === null) { + targetProperty = {[ONYX_INTERNALS__REPLACE_OBJECT_MARK]: true}; metadata.replaceNullPatches.push([[...basePath], {...sourceProperty}]); } // Later, when merging the batched changes with the Onyx value, if a nested object of the batched changes // has the internal flag set, we replace the entire destination object with the source one and remove // the flag. - if (options.shouldReplaceMarkedObjects && sourceProperty[ONYX_INTERNALS__REPLACE_OBJECT_MARK]) { + if (options.objectRemovalMode === 'replace' && sourceProperty[ONYX_INTERNALS__REPLACE_OBJECT_MARK]) { // We do a spread here in order to have a new object reference and allow us to delete the internal flag // of the merged object only. // eslint-disable-next-line @typescript-eslint/no-unused-vars - const {ONYX_INTERNALS__REPLACE_OBJECT_MARK: _mark, ...sourcePropertyWithoutMark} = sourceProperty; - - destination[key] = sourcePropertyWithoutMark; - // eslint-disable-next-line no-continue - continue; + delete sourceProperty[ONYX_INTERNALS__REPLACE_OBJECT_MARK]; + // const {ONYX_INTERNALS__REPLACE_OBJECT_MARK: _mark, ...sourcePropertyWithoutMark} = sourceProperty; + destination[key] = sourceProperty; + return; } - destination[key] = fastMerge(targetPropertyWithMarks, sourceProperty, options, metadata, [...basePath, key]).result; - } + destination[key] = fastMerge(targetProperty, sourceProperty, options, metadata, [...basePath, key]).result; + }); return destination as TObject; } @@ -188,8 +175,7 @@ function removeNestedNullValues | null>(value: if (typeof value === 'object' && !Array.isArray(value)) { return fastMerge(value, value, { shouldRemoveNestedNulls: true, - shouldMarkRemovedObjects: false, - shouldReplaceMarkedObjects: false, + objectRemovalMode: 'replace', }).result; } @@ -294,4 +280,4 @@ export default { hasWithOnyxInstance, ONYX_INTERNALS__REPLACE_OBJECT_MARK, }; -export type {FastMergeResult, FastMergeReplaceNullPatch}; +export type {FastMergeResult, FastMergeReplaceNullPatch, FastMergeOptions}; diff --git a/tests/unit/fastMergeTest.ts b/tests/unit/fastMergeTest.ts index 24016f62c..8821bdb1c 100644 --- a/tests/unit/fastMergeTest.ts +++ b/tests/unit/fastMergeTest.ts @@ -120,10 +120,10 @@ describe('fastMerge', () => { expect(result.result).toEqual(testObject); }); - it('should add the "ONYX_INTERNALS__REPLACE_OBJECT_MARK" flag to the target object when its source is set to null and "isBatchingMergeChanges" is true', () => { + it('should add the "ONYX_INTERNALS__REPLACE_OBJECT_MARK" flag to the merged object when the change is set to null and "objectRemovalMode" is set to "mark"', () => { const result = utils.fastMerge(testMergeChanges[1], testMergeChanges[0], { shouldRemoveNestedNulls: true, - shouldMarkRemovedObjects: true, + objectRemovalMode: 'mark', }); expect(result.result).toEqual({ @@ -152,7 +152,7 @@ describe('fastMerge', () => { }, { shouldRemoveNestedNulls: true, - shouldReplaceMarkedObjects: true, + objectRemovalMode: 'replace', }, ); diff --git a/tests/unit/onyxUtilsTest.ts b/tests/unit/onyxUtilsTest.ts index c017ef3d8..330637df8 100644 --- a/tests/unit/onyxUtilsTest.ts +++ b/tests/unit/onyxUtilsTest.ts @@ -151,7 +151,7 @@ describe('OnyxUtils', () => { }); }); - describe('mergeChanges', () => { + describe('mergeAndMarkChanges', () => { it("should return the last change if it's an array", () => { const result = OnyxUtils.mergeChanges([...testMergeChanges, [0, 1, 2]], testObject); @@ -199,7 +199,7 @@ describe('OnyxUtils', () => { }); it('should apply the replacement markers if the we have properties with objects being removed and added back during the changes', () => { - const result = OnyxUtils.mergeChanges(testMergeChanges); + const result = OnyxUtils.mergeAndMarkChanges(testMergeChanges); expect(result.result).toEqual({ b: { @@ -223,7 +223,7 @@ describe('OnyxUtils', () => { }); it('should 2', () => { - const result = OnyxUtils.mergeChanges([ + const result = OnyxUtils.mergeAndMarkChanges([ { // Removing the "originalMessage" object in this update. // Any subsequent changes to this object should completely replace the existing object in store. From 6d6fbb460bb4b9c71d099d3a94d2e95066824820 Mon Sep 17 00:00:00 2001 From: Christoph Pader Date: Wed, 21 May 2025 15:16:20 +0200 Subject: [PATCH 14/20] remove old comment --- lib/Onyx.ts | 1 - 1 file changed, 1 deletion(-) diff --git a/lib/Onyx.ts b/lib/Onyx.ts index 6fd5c0f93..419898c89 100644 --- a/lib/Onyx.ts +++ b/lib/Onyx.ts @@ -307,7 +307,6 @@ function merge(key: TKey, changes: OnyxMergeInput): } try { - // We first only merge the changes, so we use OnyxUtils.mergeChanges() to combine all the changes into just one. const validChanges = mergeQueue[key].filter((change) => { const {isCompatible, existingValueType, newValueType} = utils.checkCompatibilityWithExistingValue(change, existingValue); if (!isCompatible) { From 24b226962289e59ee7ea4b6ba699483a5a536c50 Mon Sep 17 00:00:00 2001 From: Christoph Pader Date: Wed, 21 May 2025 16:15:06 +0200 Subject: [PATCH 15/20] fix: missing key in path --- lib/utils.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/utils.ts b/lib/utils.ts index 16b83a2eb..9a282cf12 100644 --- a/lib/utils.ts +++ b/lib/utils.ts @@ -133,7 +133,7 @@ function mergeObject>( // When calling fastMerge again with "shouldReplaceMarkedObjects" enabled, the marked objects will be removed. if (options.objectRemovalMode === 'mark' && targetProperty === null) { targetProperty = {[ONYX_INTERNALS__REPLACE_OBJECT_MARK]: true}; - metadata.replaceNullPatches.push([[...basePath], {...sourceProperty}]); + metadata.replaceNullPatches.push([[...basePath, key], {...sourceProperty}]); } // Later, when merging the batched changes with the Onyx value, if a nested object of the batched changes From 4c23dba4ea446fb0fc13acaece2c93d02b73f78b Mon Sep 17 00:00:00 2001 From: Christoph Pader Date: Wed, 21 May 2025 16:15:22 +0200 Subject: [PATCH 16/20] fix: onyxUtilsTest --- tests/unit/onyxUtilsTest.ts | 23 +++++++++++++---------- 1 file changed, 13 insertions(+), 10 deletions(-) diff --git a/tests/unit/onyxUtilsTest.ts b/tests/unit/onyxUtilsTest.ts index 330637df8..2e065fe71 100644 --- a/tests/unit/onyxUtilsTest.ts +++ b/tests/unit/onyxUtilsTest.ts @@ -151,15 +151,15 @@ describe('OnyxUtils', () => { }); }); - describe('mergeAndMarkChanges', () => { + describe('mergeChanges', () => { it("should return the last change if it's an array", () => { - const result = OnyxUtils.mergeChanges([...testMergeChanges, [0, 1, 2]], testObject); + const {result} = OnyxUtils.mergeAndMarkChanges([...testMergeChanges, [0, 1, 2]], testObject); expect(result).toEqual([0, 1, 2]); }); it("should return the last change if the changes aren't objects", () => { - const result = OnyxUtils.mergeChanges(['a', 0, 'b', 1], testObject); + const {result} = OnyxUtils.mergeChanges(['a', 0, 'b', 1], testObject); expect(result).toEqual(1); }); @@ -180,7 +180,7 @@ describe('OnyxUtils', () => { }, }; - const result = OnyxUtils.mergeChanges([batchedChanges], testObject); + const {result} = OnyxUtils.mergeChanges([batchedChanges], testObject); expect(result).toEqual({ a: 'a', @@ -197,11 +197,13 @@ describe('OnyxUtils', () => { }, }); }); + }); + describe('mergeAndMarkChanges', () => { it('should apply the replacement markers if the we have properties with objects being removed and added back during the changes', () => { - const result = OnyxUtils.mergeAndMarkChanges(testMergeChanges); + const {result, replaceNullPatches} = OnyxUtils.mergeAndMarkChanges(testMergeChanges); - expect(result.result).toEqual({ + expect(result).toEqual({ b: { d: { i: 'i', @@ -215,7 +217,7 @@ describe('OnyxUtils', () => { }, }, }); - expect(result.replaceNullPatches).toEqual([ + expect(replaceNullPatches).toEqual([ [['b', 'd'], {i: 'i'}], [['b', 'd'], {i: 'i', j: 'j'}], [['b', 'g'], {k: 'k'}], @@ -223,7 +225,7 @@ describe('OnyxUtils', () => { }); it('should 2', () => { - const result = OnyxUtils.mergeAndMarkChanges([ + const {result, replaceNullPatches} = OnyxUtils.mergeAndMarkChanges([ { // Removing the "originalMessage" object in this update. // Any subsequent changes to this object should completely replace the existing object in store. @@ -252,7 +254,7 @@ describe('OnyxUtils', () => { }, ]); - expect(result.result).toEqual({ + expect(result).toEqual({ originalMessage: { errorMessage: 'newErrorMessage', [utils.ONYX_INTERNALS__REPLACE_OBJECT_MARK]: true, @@ -266,7 +268,8 @@ describe('OnyxUtils', () => { }, }, }); - expect(result.replaceNullPatches).toEqual([ + + expect(replaceNullPatches).toEqual([ [['originalMessage'], {errorMessage: 'newErrorMessage'}], [['receipt', 'nestedObject'], {nestedKey2: 'newNestedKey2'}], ]); From 07447249b69ba8cac169a18419fa08fd1a056b58 Mon Sep 17 00:00:00 2001 From: Christoph Pader Date: Wed, 21 May 2025 16:22:23 +0200 Subject: [PATCH 17/20] fix: tests --- lib/Onyx.ts | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/lib/Onyx.ts b/lib/Onyx.ts index 419898c89..51e655ac9 100644 --- a/lib/Onyx.ts +++ b/lib/Onyx.ts @@ -331,7 +331,7 @@ function merge(key: TKey, changes: OnyxMergeInput): return Promise.resolve(); } - const {result: mergedValue} = OnyxUtils.mergeAndMarkChanges(validChanges, existingValue); + const {result: mergedValue} = OnyxUtils.mergeChanges(validChanges, existingValue); // In cache, we don't want to remove the key if it's null to improve performance and speed up the next merge. const hasChanged = cache.hasValueChanged(key, mergedValue); @@ -427,10 +427,12 @@ function mergeCollection( const existingKeyCollection = existingKeys.reduce((obj: OnyxInputKeyValueMapping, key) => { const {isCompatible, existingValueType, newValueType} = utils.checkCompatibilityWithExistingValue(resultCollection[key], cachedCollectionForExistingKeys[key]); + if (!isCompatible) { Logger.logAlert(logMessages.incompatibleUpdateAlert(key, 'mergeCollection', existingValueType, newValueType)); return obj; } + // eslint-disable-next-line no-param-reassign obj[key] = resultCollection[key]; return obj; From ccedbd8bb96d6b484e9211f020756a591f7a26db Mon Sep 17 00:00:00 2001 From: Christoph Pader Date: Wed, 21 May 2025 16:52:26 +0200 Subject: [PATCH 18/20] further simplify code --- API-INTERNAL.md | 13 ----- lib/Onyx.ts | 40 +++++++-------- lib/OnyxUtils.ts | 56 ++++++++------------- lib/storage/InstanceSync/index.web.ts | 8 +-- lib/storage/providers/IDBKeyValProvider.ts | 2 +- lib/storage/providers/MemoryOnlyProvider.ts | 4 +- lib/storage/providers/SQLiteProvider.ts | 23 +++++---- lib/storage/providers/types.ts | 22 ++++---- lib/types.ts | 5 +- 9 files changed, 71 insertions(+), 102 deletions(-) diff --git a/API-INTERNAL.md b/API-INTERNAL.md index a098586b3..ab9dd07cf 100644 --- a/API-INTERNAL.md +++ b/API-INTERNAL.md @@ -139,11 +139,6 @@ whatever it is we attempted to do.

broadcastUpdate()

Notifies subscribers and writes current value to cache

-
removeNullValues() ⇒
-

Removes a key from storage if the value is null. -Otherwise removes all nested null values in objects, -if shouldRemoveNestedNulls is true and returns the object.

-
prepareKeyValuePairsForStorage() ⇒

Storage expects array like: [["@MyApp_user", value_1], ["@MyApp_key", value_2]] This method transforms an object like {'@MyApp_user': myUserValue, '@MyApp_key': myKeyValue} @@ -464,14 +459,6 @@ whatever it is we attempted to do. ## broadcastUpdate() Notifies subscribers and writes current value to cache -**Kind**: global function - - -## removeNullValues() ⇒ -Removes a key from storage if the value is null. -Otherwise removes all nested null values in objects, -if shouldRemoveNestedNulls is true and returns the object. - **Kind**: global function **Returns**: The value without null values and a boolean "wasRemoved", which indicates if the key got removed completely diff --git a/lib/Onyx.ts b/lib/Onyx.ts index 51e655ac9..3039b611a 100644 --- a/lib/Onyx.ts +++ b/lib/Onyx.ts @@ -27,6 +27,7 @@ import type { OnyxValue, OnyxInput, OnyxMethodMap, + MultiMergeReplaceNullPatches, } from './types'; import OnyxUtils from './OnyxUtils'; import logMessages from './logMessages'; @@ -169,38 +170,31 @@ function set(key: TKey, value: OnyxSetInput): Promis return Promise.resolve(); } - // If the value is null, we remove the key from storage - const {value: valueAfterRemoving, wasRemoved} = OnyxUtils.removeNullValues(key, value); - - const logSetCall = (hasChanged = true) => { - // Logging properties only since values could be sensitive things we don't want to log - Logger.logInfo(`set called for key: ${key}${_.isObject(value) ? ` properties: ${_.keys(value).join(',')}` : ''} hasChanged: ${hasChanged}`); - }; - - // Calling "OnyxUtils.removeNullValues" removes the key from storage and cache and updates the subscriber. + // If the change is null, we can just delete the key. // Therefore, we don't need to further broadcast and update the value so we can return early. - if (wasRemoved) { - logSetCall(); + if (value === null) { + OnyxUtils.remove(key); + Logger.logInfo(`set called for key: ${key} => null passed, so key was removed`); return Promise.resolve(); } - const valueWithoutNullValues = valueAfterRemoving as OnyxValue; - const hasChanged = cache.hasValueChanged(key, valueWithoutNullValues); + const valueWithoutNestedNullValues = utils.removeNestedNullValues(value) as OnyxValue; + const hasChanged = cache.hasValueChanged(key, valueWithoutNestedNullValues); - logSetCall(hasChanged); + Logger.logInfo(`set called for key: ${key}${_.isObject(value) ? ` properties: ${_.keys(value).join(',')}` : ''} hasChanged: ${hasChanged}`); // This approach prioritizes fast UI changes without waiting for data to be stored in device storage. - const updatePromise = OnyxUtils.broadcastUpdate(key, valueWithoutNullValues, hasChanged); + const updatePromise = OnyxUtils.broadcastUpdate(key, valueWithoutNestedNullValues, hasChanged); // If the value has not changed or the key got removed, calling Storage.setItem() would be redundant and a waste of performance, so return early instead. if (!hasChanged) { return updatePromise; } - return Storage.setItem(key, valueWithoutNullValues) - .catch((error) => OnyxUtils.evictStorageAndRetry(error, set, key, valueWithoutNullValues)) + return Storage.setItem(key, valueWithoutNestedNullValues) + .catch((error) => OnyxUtils.evictStorageAndRetry(error, set, key, valueWithoutNestedNullValues)) .then(() => { - OnyxUtils.sendActionToDevTools(OnyxUtils.METHOD.SET, key, valueWithoutNullValues); + OnyxUtils.sendActionToDevTools(OnyxUtils.METHOD.SET, key, valueWithoutNestedNullValues); return updatePromise; }); } @@ -323,10 +317,10 @@ function merge(key: TKey, changes: OnyxMergeInput): delete mergeQueue[key]; delete mergeQueuePromise[key]; - // Calling "OnyxUtils.remove" removes the key from storage and cache and updates the subscriber. + // If the last change is null, we can just delete the key. // Therefore, we don't need to further broadcast and update the value so we can return early. if (validChanges.at(-1) === null) { - Logger.logInfo(`merge called for key: ${key} was removed`); + Logger.logInfo(`merge called for key: ${key} => null passed, so key was removed`); OnyxUtils.remove(key); return Promise.resolve(); } @@ -376,7 +370,7 @@ function merge(key: TKey, changes: OnyxMergeInput): function mergeCollection( collectionKey: TKey, collection: OnyxMergeCollectionInput, - mergeReplaceNullPatches?: MixedOperationsQueue['mergeReplaceNullPatches'], + mergeReplaceNullPatches?: MultiMergeReplaceNullPatches, ): Promise { if (!OnyxUtils.isValidNonEmptyCollectionForMerge(collection)) { Logger.logInfo('mergeCollection() called with invalid or empty value. Skipping this update.'); @@ -449,7 +443,7 @@ function mergeCollection( // When (multi-)merging the values with the existing values in storage, // we don't want to remove nested null values from the data that we pass to the storage layer, // because the storage layer uses them to remove nested keys from storage natively. - const keyValuePairsForExistingCollection = OnyxUtils.prepareKeyValuePairsForStorage(existingKeyCollection, false); + const keyValuePairsForExistingCollection = OnyxUtils.prepareKeyValuePairsForStorage(existingKeyCollection, false, mergeReplaceNullPatches); // We can safely remove nested null values when using (multi-)set, // because we will simply overwrite the existing values in storage. @@ -464,7 +458,7 @@ function mergeCollection( // New keys will be added via multiSet while existing keys will be updated using multiMerge // This is because setting a key that doesn't exist yet with multiMerge will throw errors if (keyValuePairsForExistingCollection.length > 0) { - promises.push(Storage.multiMerge(keyValuePairsForExistingCollection, mergeReplaceNullPatches)); + promises.push(Storage.multiMerge(keyValuePairsForExistingCollection)); } if (keyValuePairsForNewCollection.length > 0) { diff --git a/lib/OnyxUtils.ts b/lib/OnyxUtils.ts index 1003d3ae1..e89769404 100644 --- a/lib/OnyxUtils.ts +++ b/lib/OnyxUtils.ts @@ -20,6 +20,7 @@ import type { DefaultConnectOptions, KeyValueMapping, Mapping, + MultiMergeReplaceNullPatches, OnyxCollection, OnyxEntry, OnyxInput, @@ -35,6 +36,7 @@ import type {DeferredTask} from './createDeferredTask'; import createDeferredTask from './createDeferredTask'; import * as GlobalSettings from './GlobalSettings'; import decorateWithMetrics from './metrics'; +import type {StorageKeyValuePair} from './storage/providers/types'; // Method constants const METHOD = { @@ -1204,34 +1206,6 @@ function hasPendingMergeForKey(key: OnyxKey): boolean { return !!mergeQueue[key]; } -type RemoveNullValuesOutput | undefined> = { - value: Value; - wasRemoved: boolean; -}; - -/** - * Removes a key from storage if the value is null. - * Otherwise removes all nested null values in objects, - * if shouldRemoveNestedNulls is true and returns the object. - * - * @returns The value without null values and a boolean "wasRemoved", which indicates if the key got removed completely - */ -function removeNullValues | undefined>(key: OnyxKey, value: Value, shouldRemoveNestedNulls = true): RemoveNullValuesOutput { - if (value === null) { - remove(key); - return {value, wasRemoved: true}; - } - - if (value === undefined) { - return {value, wasRemoved: false}; - } - - // We can remove all null values in an object by merging it with itself - // utils.fastMerge recursively goes through the object and removes all null values - // Passing two identical objects as source and target to fastMerge will not change it, but only remove the null values - return {value: shouldRemoveNestedNulls ? utils.removeNestedNullValues(value) : value, wasRemoved: false}; -} - /** * Storage expects array like: [["@MyApp_user", value_1], ["@MyApp_key", value_2]] * This method transforms an object like {'@MyApp_user': myUserValue, '@MyApp_key': myKeyValue} @@ -1239,16 +1213,27 @@ function removeNullValues | undefined>(key: Ony * @return an array of key - value pairs <[key, value]> */ -function prepareKeyValuePairsForStorage(data: Record>, shouldRemoveNestedNulls: boolean): Array<[OnyxKey, OnyxInput]> { - return Object.entries(data).reduce]>>((pairs, [key, value]) => { - const {value: valueAfterRemoving, wasRemoved} = removeNullValues(key, value, shouldRemoveNestedNulls); +function prepareKeyValuePairsForStorage( + data: Record>, + shouldRemoveNestedNulls?: boolean, + replaceNullPatches?: MultiMergeReplaceNullPatches, +): StorageKeyValuePair[] { + const pairs: StorageKeyValuePair[] = []; + + Object.entries(data).forEach(([key, value]) => { + if (value === null) { + remove(key); + return; + } + + const valueWithoutNestedNullValues = shouldRemoveNestedNulls ?? true ? utils.removeNestedNullValues(value) : value; - if (!wasRemoved && valueAfterRemoving !== undefined) { - pairs.push([key, valueAfterRemoving]); + if (valueWithoutNestedNullValues !== undefined) { + pairs.push([key, valueWithoutNestedNullValues, replaceNullPatches?.[key]]); } + }); - return pairs; - }, []); + return pairs; } function mergeChanges | undefined, TChange extends OnyxInput | undefined>(changes: TChange[], existingValue?: TValue): FastMergeResult { @@ -1502,7 +1487,6 @@ const OnyxUtils = { evictStorageAndRetry, broadcastUpdate, hasPendingMergeForKey, - removeNullValues, prepareKeyValuePairsForStorage, mergeChanges, mergeAndMarkChanges, diff --git a/lib/storage/InstanceSync/index.web.ts b/lib/storage/InstanceSync/index.web.ts index 67b309791..99a7fe325 100644 --- a/lib/storage/InstanceSync/index.web.ts +++ b/lib/storage/InstanceSync/index.web.ts @@ -5,7 +5,7 @@ */ import type {OnyxKey} from '../../types'; import NoopProvider from '../providers/NoopProvider'; -import type {KeyList, OnStorageKeyChanged} from '../providers/types'; +import type {StorageKeyList, OnStorageKeyChanged} from '../providers/types'; import type StorageProvider from '../providers/types'; const SYNC_ONYX = 'SYNC_ONYX'; @@ -19,7 +19,7 @@ function raiseStorageSyncEvent(onyxKey: OnyxKey) { global.localStorage.removeItem(SYNC_ONYX); } -function raiseStorageSyncManyKeysEvent(onyxKeys: KeyList) { +function raiseStorageSyncManyKeysEvent(onyxKeys: StorageKeyList) { onyxKeys.forEach((onyxKey) => { raiseStorageSyncEvent(onyxKey); }); @@ -54,12 +54,12 @@ const InstanceSync = { multiSet: raiseStorageSyncManyKeysEvent, mergeItem: raiseStorageSyncEvent, clear: (clearImplementation: () => void) => { - let allKeys: KeyList; + let allKeys: StorageKeyList; // The keys must be retrieved before storage is cleared or else the list of keys would be empty return storage .getAllKeys() - .then((keys: KeyList) => { + .then((keys: StorageKeyList) => { allKeys = keys; }) .then(() => clearImplementation()) diff --git a/lib/storage/providers/IDBKeyValProvider.ts b/lib/storage/providers/IDBKeyValProvider.ts index c61fc851e..64d419a83 100644 --- a/lib/storage/providers/IDBKeyValProvider.ts +++ b/lib/storage/providers/IDBKeyValProvider.ts @@ -70,7 +70,7 @@ const provider: StorageProvider = { } return true; - }); + }) as Array<[IDBValidKey, unknown]>; return setMany(pairsWithoutNull, idbKeyValStore); }, diff --git a/lib/storage/providers/MemoryOnlyProvider.ts b/lib/storage/providers/MemoryOnlyProvider.ts index 1af95a6e3..1510d20cb 100644 --- a/lib/storage/providers/MemoryOnlyProvider.ts +++ b/lib/storage/providers/MemoryOnlyProvider.ts @@ -1,7 +1,7 @@ import _ from 'underscore'; import utils from '../../utils'; import type StorageProvider from './types'; -import type {KeyValuePair} from './types'; +import type {StorageKeyValuePair} from './types'; import type {OnyxKey, OnyxValue} from '../../types'; type Store = Record>; @@ -49,7 +49,7 @@ const provider: StorageProvider = { new Promise((resolve) => { this.getItem(key).then((value) => resolve([key, value])); }), - ) as Array>; + ) as Array>; return Promise.all(getPromises); }, diff --git a/lib/storage/providers/SQLiteProvider.ts b/lib/storage/providers/SQLiteProvider.ts index fd3363f79..bdcde2b76 100644 --- a/lib/storage/providers/SQLiteProvider.ts +++ b/lib/storage/providers/SQLiteProvider.ts @@ -8,7 +8,7 @@ import {open} from 'react-native-quick-sqlite'; import type {FastMergeReplaceNullPatch} from '../../utils'; import utils from '../../utils'; import type StorageProvider from './types'; -import type {KeyList, KeyValuePairList} from './types'; +import type {StorageKeyList, StorageKeyValuePair} from './types'; const DB_NAME = 'OnyxDB'; let db: QuickSQLiteConnection; @@ -61,7 +61,7 @@ const provider: StorageProvider = { return db.executeAsync(command, keys).then(({rows}) => { // eslint-disable-next-line no-underscore-dangle const result = rows?._array.map((row) => [row.record_key, JSON.parse(row.valueJSON)]); - return (result ?? []) as KeyValuePairList; + return (result ?? []) as StorageKeyValuePair[]; }); }, setItem(key, value) { @@ -74,7 +74,7 @@ const provider: StorageProvider = { } return db.executeBatchAsync([['REPLACE INTO keyvaluepairs (record_key, valueJSON) VALUES (?, json(?));', stringifiedPairs]]).then(() => undefined); }, - multiMerge(pairs, mergeReplaceNullPatches) { + multiMerge(pairs) { const commands: SQLBatchTuple[] = []; const patchQuery = `INSERT INTO keyvaluepairs (record_key, valueJSON) @@ -93,13 +93,14 @@ const provider: StorageProvider = { // eslint-disable-next-line @typescript-eslint/prefer-for-of for (let i = 0; i < nonNullishPairs.length; i++) { - const pair = nonNullishPairs[i]; - const value = JSON.stringify(pair[1], replacer); - patchQueryArguments.push([pair[0], value]); + const [key, value, replaceNullPatches] = nonNullishPairs[i]; - const patches = mergeReplaceNullPatches?.[pair[0]] ?? []; + const valueAfterReplace = JSON.stringify(value, replacer); + patchQueryArguments.push([key, valueAfterReplace]); + + const patches = replaceNullPatches ?? []; if (patches.length > 0) { - const queries = generateJSONReplaceSQLQueries(pair[0], patches); + const queries = generateJSONReplaceSQLQueries(key, patches); if (queries.length > 0) { replaceQueryArguments.push(...queries); @@ -114,15 +115,15 @@ const provider: StorageProvider = { return db.executeBatchAsync(commands).then(() => undefined); }, - mergeItem(key, change) { + mergeItem(key, change, replaceNullPatches) { // Since Onyx already merged the existing value with the changes, we can just set the value directly. - return this.multiMerge([[key, change]]); + return this.multiMerge([[key, change, replaceNullPatches]]); }, getAllKeys: () => db.executeAsync('SELECT record_key FROM keyvaluepairs;').then(({rows}) => { // eslint-disable-next-line no-underscore-dangle const result = rows?._array.map((row) => row.record_key); - return (result ?? []) as KeyList; + return (result ?? []) as StorageKeyList; }), removeItem: (key) => db.executeAsync('DELETE FROM keyvaluepairs WHERE record_key = ?;', [key]).then(() => undefined), removeItems: (keys) => { diff --git a/lib/storage/providers/types.ts b/lib/storage/providers/types.ts index fc3b3cade..db7525aa5 100644 --- a/lib/storage/providers/types.ts +++ b/lib/storage/providers/types.ts @@ -1,8 +1,8 @@ -import type {MixedOperationsQueue, OnyxKey, OnyxValue} from '../../types'; +import type {OnyxKey, OnyxValue} from '../../types'; +import type {FastMergeReplaceNullPatch} from '../../utils'; -type KeyValuePair = [OnyxKey, OnyxValue]; -type KeyList = OnyxKey[]; -type KeyValuePairList = KeyValuePair[]; +type StorageKeyValuePair = [key: OnyxKey, value: OnyxValue, replaceNullPatches?: FastMergeReplaceNullPatch[]]; +type StorageKeyList = OnyxKey[]; type DatabaseSize = { bytesUsed: number; @@ -28,7 +28,7 @@ type StorageProvider = { /** * Get multiple key-value pairs for the given array of keys in a batch */ - multiGet: (keys: KeyList) => Promise; + multiGet: (keys: StorageKeyList) => Promise; /** * Sets the value for a given key. The only requirement is that the value should be serializable to JSON string @@ -38,23 +38,23 @@ type StorageProvider = { /** * Stores multiple key-value pairs in a batch */ - multiSet: (pairs: KeyValuePairList) => Promise; + multiSet: (pairs: StorageKeyValuePair[]) => Promise; /** * Multiple merging of existing and new values in a batch */ - multiMerge: (pairs: KeyValuePairList, mergeReplaceNullPatches?: MixedOperationsQueue['mergeReplaceNullPatches']) => Promise; + multiMerge: (pairs: StorageKeyValuePair[]) => Promise; /** * Merges an existing value with a new one * @param change - the change to merge with the existing value */ - mergeItem: (key: TKey, change: OnyxValue) => Promise; + mergeItem: (key: TKey, change: OnyxValue, replaceNullPatches?: FastMergeReplaceNullPatch[]) => Promise; /** * Returns all keys available in storage */ - getAllKeys: () => Promise; + getAllKeys: () => Promise; /** * Removes given key and its value from storage @@ -64,7 +64,7 @@ type StorageProvider = { /** * Removes given keys and their values from storage */ - removeItems: (keys: KeyList) => Promise; + removeItems: (keys: StorageKeyList) => Promise; /** * Clears absolutely everything from storage @@ -83,4 +83,4 @@ type StorageProvider = { }; export default StorageProvider; -export type {KeyList, KeyValuePair, KeyValuePairList, OnStorageKeyChanged}; +export type {StorageKeyList, StorageKeyValuePair, OnStorageKeyChanged}; diff --git a/lib/types.ts b/lib/types.ts index cd5556b89..7dbdb1ca7 100644 --- a/lib/types.ts +++ b/lib/types.ts @@ -486,12 +486,14 @@ type InitOptions = { // eslint-disable-next-line @typescript-eslint/no-explicit-any type GenericFunction = (...args: any[]) => any; +type MultiMergeReplaceNullPatches = {[TKey in OnyxKey]: FastMergeReplaceNullPatch[]}; + /** * Represents a combination of Merge and Set operations that should be executed in Onyx */ type MixedOperationsQueue = { merge: OnyxInputKeyValueMapping; - mergeReplaceNullPatches: {[TKey in OnyxKey]: FastMergeReplaceNullPatch[]}; + mergeReplaceNullPatches: MultiMergeReplaceNullPatches; set: OnyxInputKeyValueMapping; }; @@ -533,5 +535,6 @@ export type { OnyxValue, Selector, WithOnyxConnectOptions, + MultiMergeReplaceNullPatches, MixedOperationsQueue, }; From 8391cc7628ff629a513af4550bec0a01cc78d55f Mon Sep 17 00:00:00 2001 From: Christoph Pader Date: Wed, 21 May 2025 17:19:24 +0200 Subject: [PATCH 19/20] keep performance merge logic on native --- lib/Onyx.ts | 23 ++-------------------- lib/OnyxMerge.native.ts | 43 +++++++++++++++++++++++++++++++++++++++++ lib/OnyxMerge.ts | 35 +++++++++++++++++++++++++++++++++ lib/OnyxUtils.ts | 19 ++++++------------ 4 files changed, 86 insertions(+), 34 deletions(-) create mode 100644 lib/OnyxMerge.native.ts create mode 100644 lib/OnyxMerge.ts diff --git a/lib/Onyx.ts b/lib/Onyx.ts index 3039b611a..5bee462a1 100644 --- a/lib/Onyx.ts +++ b/lib/Onyx.ts @@ -1,4 +1,3 @@ -/* eslint-disable no-continue */ import _ from 'underscore'; import lodashPick from 'lodash/pick'; import * as Logger from './Logger'; @@ -35,6 +34,7 @@ import type {Connection} from './OnyxConnectionManager'; import connectionManager from './OnyxConnectionManager'; import * as GlobalSettings from './GlobalSettings'; import decorateWithMetrics from './metrics'; +import OnyxMerge from './OnyxMerge.native'; /** Initialize the store with actions and listening for storage events */ function init({ @@ -325,26 +325,7 @@ function merge(key: TKey, changes: OnyxMergeInput): return Promise.resolve(); } - const {result: mergedValue} = OnyxUtils.mergeChanges(validChanges, existingValue); - - // In cache, we don't want to remove the key if it's null to improve performance and speed up the next merge. - const hasChanged = cache.hasValueChanged(key, mergedValue); - - // Logging properties only since values could be sensitive things we don't want to log. - Logger.logInfo(`merge called for key: ${key}${_.isObject(mergedValue) ? ` properties: ${_.keys(mergedValue).join(',')}` : ''} hasChanged: ${hasChanged}`); - - // This approach prioritizes fast UI changes without waiting for data to be stored in device storage. - const updatePromise = OnyxUtils.broadcastUpdate(key, mergedValue as OnyxValue, hasChanged); - - // If the value has not changed, calling Storage.setItem() would be redundant and a waste of performance, so return early instead. - if (!hasChanged) { - return updatePromise; - } - - return Storage.setItem(key, mergedValue as OnyxValue).then(() => { - OnyxUtils.sendActionToDevTools(OnyxUtils.METHOD.MERGE, key, changes, mergedValue); - return updatePromise; - }); + return OnyxMerge.applyMerge(key, existingValue, validChanges); } catch (error) { Logger.logAlert(`An error occurred while applying merge for key: ${key}, Error: ${error}`); return Promise.resolve(); diff --git a/lib/OnyxMerge.native.ts b/lib/OnyxMerge.native.ts new file mode 100644 index 000000000..f18589e77 --- /dev/null +++ b/lib/OnyxMerge.native.ts @@ -0,0 +1,43 @@ +import _ from 'underscore'; +import * as Logger from './Logger'; +import OnyxUtils from './OnyxUtils'; +import type {OnyxKey, OnyxValue} from './types'; +import cache from './OnyxCache'; +import Storage from './storage'; + +function applyMerge(key: TKey, existingValue: OnyxValue, validChanges: unknown[]): Promise { + // If any of the changes is null, we need to discard the existing value. + const baseValue = validChanges.includes(null) ? undefined : existingValue; + + // We first batch the changes into a single change with object removal marks, + // so that SQLite can merge the changes more efficiently. + const {result: batchedChanges, replaceNullPatches} = OnyxUtils.mergeAndMarkChanges(validChanges); + + // We then merge the batched changes with the existing value, because we need to final merged value to broadcast to subscribers. + const {result: mergedValue} = OnyxUtils.mergeChanges([batchedChanges], baseValue); + + // In cache, we don't want to remove the key if it's null to improve performance and speed up the next merge. + const hasChanged = cache.hasValueChanged(key, mergedValue); + + // Logging properties only since values could be sensitive things we don't want to log. + Logger.logInfo(`merge called for key: ${key}${_.isObject(mergedValue) ? ` properties: ${_.keys(mergedValue).join(',')}` : ''} hasChanged: ${hasChanged}`); + + // This approach prioritizes fast UI changes without waiting for data to be stored in device storage. + const updatePromise = OnyxUtils.broadcastUpdate(key, mergedValue as OnyxValue, hasChanged); + + // If the value has not changed, calling Storage.setItem() would be redundant and a waste of performance, so return early instead. + if (!hasChanged) { + return updatePromise; + } + + return Storage.mergeItem(key, batchedChanges as OnyxValue, replaceNullPatches).then(() => { + OnyxUtils.sendActionToDevTools(OnyxUtils.METHOD.MERGE, key, validChanges, mergedValue); + return updatePromise; + }); +} + +const OnyxMerge = { + applyMerge, +}; + +export default OnyxMerge; diff --git a/lib/OnyxMerge.ts b/lib/OnyxMerge.ts new file mode 100644 index 000000000..bbf65515f --- /dev/null +++ b/lib/OnyxMerge.ts @@ -0,0 +1,35 @@ +import _ from 'underscore'; +import * as Logger from './Logger'; +import OnyxUtils from './OnyxUtils'; +import type {OnyxKey, OnyxValue} from './types'; +import cache from './OnyxCache'; +import Storage from './storage'; + +function applyMerge(key: TKey, existingValue: OnyxValue, validChanges: unknown[]): Promise { + const {result: mergedValue} = OnyxUtils.mergeChanges(validChanges, existingValue); + + // In cache, we don't want to remove the key if it's null to improve performance and speed up the next merge. + const hasChanged = cache.hasValueChanged(key, mergedValue); + + // Logging properties only since values could be sensitive things we don't want to log. + Logger.logInfo(`merge called for key: ${key}${_.isObject(mergedValue) ? ` properties: ${_.keys(mergedValue).join(',')}` : ''} hasChanged: ${hasChanged}`); + + // This approach prioritizes fast UI changes without waiting for data to be stored in device storage. + const updatePromise = OnyxUtils.broadcastUpdate(key, mergedValue as OnyxValue, hasChanged); + + // If the value has not changed, calling Storage.setItem() would be redundant and a waste of performance, so return early instead. + if (!hasChanged) { + return updatePromise; + } + + return Storage.setItem(key, mergedValue as OnyxValue).then(() => { + OnyxUtils.sendActionToDevTools(OnyxUtils.METHOD.MERGE, key, validChanges, mergedValue); + return updatePromise; + }); +} + +const OnyxMerge = { + applyMerge, +}; + +export default OnyxMerge; diff --git a/lib/OnyxUtils.ts b/lib/OnyxUtils.ts index e89769404..9ac4e881b 100644 --- a/lib/OnyxUtils.ts +++ b/lib/OnyxUtils.ts @@ -1236,14 +1236,11 @@ function prepareKeyValuePairsForStorage( return pairs; } -function mergeChanges | undefined, TChange extends OnyxInput | undefined>(changes: TChange[], existingValue?: TValue): FastMergeResult { +function mergeChanges | undefined>(changes: TValue[], existingValue?: TValue): FastMergeResult { return applyMerge('merge', changes, existingValue); } -function mergeAndMarkChanges | undefined, TChange extends OnyxInput | undefined>( - changes: TChange[], - existingValue?: TValue, -): FastMergeResult { +function mergeAndMarkChanges | undefined>(changes: TValue[], existingValue?: TValue): FastMergeResult { return applyMerge('mark', changes, existingValue); } @@ -1253,11 +1250,7 @@ function mergeAndMarkChanges | undefined, TCha * @param changes Array of changes that should be merged * @param existingValue The existing value that should be merged with the changes */ -function applyMerge | undefined, TChange extends OnyxInput | undefined>( - mode: 'merge' | 'mark', - changes: TChange[], - existingValue?: TValue, -): FastMergeResult { +function applyMerge | undefined>(mode: 'merge' | 'mark', changes: TValue[], existingValue?: TValue): FastMergeResult { const lastChange = changes?.at(-1); if (Array.isArray(lastChange)) { @@ -1266,7 +1259,7 @@ function applyMerge | undefined, TChange exten if (changes.some((change) => change && typeof change === 'object')) { // Object values are then merged one after the other - return changes.reduce>( + return changes.reduce>( (modifiedData, change) => { const options: FastMergeOptions = mode === 'merge' ? {shouldRemoveNestedNulls: true, objectRemovalMode: 'replace'} : {objectRemovalMode: 'mark'}; const {result, replaceNullPatches} = utils.fastMerge(modifiedData.result, change, options); @@ -1279,7 +1272,7 @@ function applyMerge | undefined, TChange exten return modifiedData; }, { - result: (existingValue ?? {}) as TChange, + result: (existingValue ?? {}) as TValue, replaceNullPatches: [], }, ); @@ -1287,7 +1280,7 @@ function applyMerge | undefined, TChange exten // If we have anything else we can't merge it so we'll // simply return the last value that was queued - return {result: lastChange as TChange, replaceNullPatches: []}; + return {result: lastChange as TValue, replaceNullPatches: []}; } /** From ed66dad9a1a3ecba57216c456e98784c9a942bf2 Mon Sep 17 00:00:00 2001 From: Christoph Pader Date: Wed, 21 May 2025 17:35:31 +0200 Subject: [PATCH 20/20] fix: invalid param for `sendActionToDevTools` --- lib/Onyx.ts | 7 +++-- .../index.native.ts} | 27 ++++++++++--------- lib/{OnyxMerge.ts => OnyxMerge/index.ts} | 27 ++++++++++--------- lib/OnyxMerge/types.ts | 10 +++++++ 4 files changed, 43 insertions(+), 28 deletions(-) rename lib/{OnyxMerge.native.ts => OnyxMerge/index.native.ts} (73%) rename lib/{OnyxMerge.ts => OnyxMerge/index.ts} (66%) create mode 100644 lib/OnyxMerge/types.ts diff --git a/lib/Onyx.ts b/lib/Onyx.ts index 5bee462a1..4e4e3c4dd 100644 --- a/lib/Onyx.ts +++ b/lib/Onyx.ts @@ -34,7 +34,7 @@ import type {Connection} from './OnyxConnectionManager'; import connectionManager from './OnyxConnectionManager'; import * as GlobalSettings from './GlobalSettings'; import decorateWithMetrics from './metrics'; -import OnyxMerge from './OnyxMerge.native'; +import OnyxMerge from './OnyxMerge/index.native'; /** Initialize the store with actions and listening for storage events */ function init({ @@ -325,7 +325,10 @@ function merge(key: TKey, changes: OnyxMergeInput): return Promise.resolve(); } - return OnyxMerge.applyMerge(key, existingValue, validChanges); + return OnyxMerge.applyMerge(key, existingValue, validChanges).then(({mergedValue, updatePromise}) => { + OnyxUtils.sendActionToDevTools(OnyxUtils.METHOD.MERGE, key, changes, mergedValue); + return updatePromise; + }); } catch (error) { Logger.logAlert(`An error occurred while applying merge for key: ${key}, Error: ${error}`); return Promise.resolve(); diff --git a/lib/OnyxMerge.native.ts b/lib/OnyxMerge/index.native.ts similarity index 73% rename from lib/OnyxMerge.native.ts rename to lib/OnyxMerge/index.native.ts index f18589e77..bf323b53d 100644 --- a/lib/OnyxMerge.native.ts +++ b/lib/OnyxMerge/index.native.ts @@ -1,11 +1,12 @@ import _ from 'underscore'; -import * as Logger from './Logger'; -import OnyxUtils from './OnyxUtils'; -import type {OnyxKey, OnyxValue} from './types'; -import cache from './OnyxCache'; -import Storage from './storage'; - -function applyMerge(key: TKey, existingValue: OnyxValue, validChanges: unknown[]): Promise { +import * as Logger from '../Logger'; +import OnyxUtils from '../OnyxUtils'; +import type {OnyxKey, OnyxValue} from '../types'; +import cache from '../OnyxCache'; +import Storage from '../storage'; +import type {ApplyMerge, ApplyMergeResult} from './types'; + +const applyMerge: ApplyMerge = (key: TKey, existingValue: OnyxValue, validChanges: unknown[]): Promise => { // If any of the changes is null, we need to discard the existing value. const baseValue = validChanges.includes(null) ? undefined : existingValue; @@ -27,14 +28,14 @@ function applyMerge(key: TKey, existingValue: OnyxValue, replaceNullPatches).then(() => { - OnyxUtils.sendActionToDevTools(OnyxUtils.METHOD.MERGE, key, validChanges, mergedValue); - return updatePromise; - }); -} + return Storage.mergeItem(key, batchedChanges as OnyxValue, replaceNullPatches).then(() => ({ + mergedValue, + updatePromise, + })); +}; const OnyxMerge = { applyMerge, diff --git a/lib/OnyxMerge.ts b/lib/OnyxMerge/index.ts similarity index 66% rename from lib/OnyxMerge.ts rename to lib/OnyxMerge/index.ts index bbf65515f..ea53203dd 100644 --- a/lib/OnyxMerge.ts +++ b/lib/OnyxMerge/index.ts @@ -1,11 +1,12 @@ import _ from 'underscore'; -import * as Logger from './Logger'; -import OnyxUtils from './OnyxUtils'; -import type {OnyxKey, OnyxValue} from './types'; -import cache from './OnyxCache'; -import Storage from './storage'; - -function applyMerge(key: TKey, existingValue: OnyxValue, validChanges: unknown[]): Promise { +import * as Logger from '../Logger'; +import OnyxUtils from '../OnyxUtils'; +import type {OnyxKey, OnyxValue} from '../types'; +import cache from '../OnyxCache'; +import Storage from '../storage'; +import type {ApplyMerge, ApplyMergeResult} from './types'; + +const applyMerge: ApplyMerge = (key: TKey, existingValue: OnyxValue, validChanges: unknown[]): Promise => { const {result: mergedValue} = OnyxUtils.mergeChanges(validChanges, existingValue); // In cache, we don't want to remove the key if it's null to improve performance and speed up the next merge. @@ -19,14 +20,14 @@ function applyMerge(key: TKey, existingValue: OnyxValue).then(() => { - OnyxUtils.sendActionToDevTools(OnyxUtils.METHOD.MERGE, key, validChanges, mergedValue); - return updatePromise; - }); -} + return Storage.setItem(key, mergedValue as OnyxValue).then(() => ({ + mergedValue, + updatePromise, + })); +}; const OnyxMerge = { applyMerge, diff --git a/lib/OnyxMerge/types.ts b/lib/OnyxMerge/types.ts new file mode 100644 index 000000000..51405c9ac --- /dev/null +++ b/lib/OnyxMerge/types.ts @@ -0,0 +1,10 @@ +import type {OnyxKey, OnyxValue} from '../types'; + +type ApplyMergeResult = { + mergedValue: OnyxValue; + updatePromise: Promise; +}; + +type ApplyMerge = (key: TKey, existingValue: OnyxValue, validChanges: unknown[]) => Promise; + +export type {ApplyMerge, ApplyMergeResult};