diff --git a/lib/OnyxCache.ts b/lib/OnyxCache.ts index fb6d4f795..2bb21791e 100644 --- a/lib/OnyxCache.ts +++ b/lib/OnyxCache.ts @@ -226,7 +226,6 @@ class OnyxCache { this.storageMap = { ...utils.fastMerge(this.storageMap, data, { - shouldRemoveNestedNulls: true, objectRemovalMode: 'replace', }).result, }; diff --git a/lib/OnyxUtils.ts b/lib/OnyxUtils.ts index 50402ea10..b20f3bfef 100644 --- a/lib/OnyxUtils.ts +++ b/lib/OnyxUtils.ts @@ -1018,7 +1018,6 @@ function hasPendingMergeForKey(key: OnyxKey): boolean { */ function prepareKeyValuePairsForStorage( data: Record>, - shouldRemoveNestedNulls?: boolean, replaceNullPatches?: MultiMergeReplaceNullPatches, isProcessingCollectionUpdate?: boolean, ): StorageKeyValuePair[] { @@ -1030,10 +1029,8 @@ function prepareKeyValuePairsForStorage( continue; } - const valueWithoutNestedNullValues = shouldRemoveNestedNulls ?? true ? utils.removeNestedNullValues(value) : value; - - if (valueWithoutNestedNullValues !== undefined) { - pairs.push([key, valueWithoutNestedNullValues, replaceNullPatches?.[key]]); + if (value !== undefined) { + pairs.push([key, value, replaceNullPatches?.[key]]); } } @@ -1085,7 +1082,7 @@ function mergeInternal | undefined, TChange ex // Object values are then merged one after the other return changes.reduce>( (modifiedData, change) => { - const options: FastMergeOptions = mode === 'merge' ? {shouldRemoveNestedNulls: true, objectRemovalMode: 'replace'} : {objectRemovalMode: 'mark'}; + const options: FastMergeOptions = mode === 'merge' ? {objectRemovalMode: 'replace'} : {objectRemovalMode: 'mark'}; const {result, replaceNullPatches} = utils.fastMerge(modifiedData.result, change, options); // eslint-disable-next-line no-param-reassign @@ -1117,9 +1114,7 @@ function initializeWithDefaultKeyStates(): Promise { return Storage.multiGet(keysToFetch).then((pairs) => { const existingDataAsObject = Object.fromEntries(pairs) as Record; - const merged = utils.fastMerge(existingDataAsObject, defaultKeyStates, { - shouldRemoveNestedNulls: true, - }).result; + const merged = utils.fastMerge(existingDataAsObject, defaultKeyStates).result; cache.merge(merged ?? {}); for (const [key, value] of Object.entries(merged ?? {})) keyChanged(key, value); @@ -1393,13 +1388,12 @@ function setWithRetry({key, value, options}: SetParams; - const hasChanged = options?.skipCacheCheck ? true : cache.hasValueChanged(key, valueWithoutNestedNullValues); + const hasChanged = options?.skipCacheCheck ? true : cache.hasValueChanged(key, value); OnyxUtils.logKeyChanged(OnyxUtils.METHOD.SET, key, value, hasChanged); // This approach prioritizes fast UI changes without waiting for data to be stored in device storage. - const updatePromise = OnyxUtils.broadcastUpdate(key, valueWithoutNestedNullValues, hasChanged); + const updatePromise = OnyxUtils.broadcastUpdate(key, value, hasChanged); // If the value has not changed and this isn't a retry attempt, calling Storage.setItem() would be redundant and a waste of performance, so return early instead. if (!hasChanged && !retryAttempt) { @@ -1408,14 +1402,14 @@ function setWithRetry({key, value, options}: SetParams OnyxUtils.retryOperation(error, setWithRetry, {key, value: valueWithoutNestedNullValues, options}, retryAttempt)) + return Storage.setItem(key, value) + .catch((error) => OnyxUtils.retryOperation(error, setWithRetry, {key, value, options}, retryAttempt)) .then(() => { - OnyxUtils.sendActionToDevTools(OnyxUtils.METHOD.SET, key, valueWithoutNestedNullValues); + OnyxUtils.sendActionToDevTools(OnyxUtils.METHOD.SET, key, value); return updatePromise; }); } @@ -1448,7 +1442,7 @@ function multiSetWithRetry(data: OnyxMultiSetInput, retryAttempt?: number): Prom }, {}); } - const keyValuePairsToSet = OnyxUtils.prepareKeyValuePairsForStorage(newData, true); + const keyValuePairsToSet = OnyxUtils.prepareKeyValuePairsForStorage(newData); const updatePromises = keyValuePairsToSet.map(([key, value]) => { // When we use multiSet to set a key we want to clear the current delta changes from Onyx.merge that were queued @@ -1530,7 +1524,7 @@ function setCollectionWithRetry({collectionKey, mutableCollection[key] = null; } - const keyValuePairs = OnyxUtils.prepareKeyValuePairsForStorage(mutableCollection, true, undefined, true); + const keyValuePairs = OnyxUtils.prepareKeyValuePairsForStorage(mutableCollection, undefined, true); const previousCollection = OnyxUtils.getCachedCollection(collectionKey); for (const [key, value] of keyValuePairs) cache.set(key, value); @@ -1636,14 +1630,8 @@ function mergeCollectionWithPatches( newCollection[key] = resultCollection[key]; } - // 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 = 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. - const keyValuePairsForNewCollection = prepareKeyValuePairsForStorage(newCollection, true); + const keyValuePairsForExistingCollection = prepareKeyValuePairsForStorage(existingKeyCollection, mergeReplaceNullPatches); + const keyValuePairsForNewCollection = prepareKeyValuePairsForStorage(newCollection); const promises = []; @@ -1732,7 +1720,7 @@ function partialSetCollection({collectionKey, co const mutableCollection: OnyxInputKeyValueMapping = {...resultCollection}; const existingKeys = resultCollectionKeys.filter((key) => persistedKeys.has(key)); const previousCollection = getCachedCollection(collectionKey, existingKeys); - const keyValuePairs = prepareKeyValuePairsForStorage(mutableCollection, true, undefined, true); + const keyValuePairs = prepareKeyValuePairsForStorage(mutableCollection, undefined, true); for (const [key, value] of keyValuePairs) cache.set(key, value); diff --git a/lib/storage/providers/IDBKeyValProvider/index.ts b/lib/storage/providers/IDBKeyValProvider/index.ts index c140ed763..e5118f43a 100644 --- a/lib/storage/providers/IDBKeyValProvider/index.ts +++ b/lib/storage/providers/IDBKeyValProvider/index.ts @@ -62,7 +62,6 @@ const provider: StorageProvider = { store.delete(key); } else { const newValue = utils.fastMerge(values[index] as Record, value as Record, { - shouldRemoveNestedNulls: true, objectRemovalMode: 'replace', }).result; diff --git a/lib/storage/providers/MemoryOnlyProvider.ts b/lib/storage/providers/MemoryOnlyProvider.ts index 15e9c264d..ed258ee1f 100644 --- a/lib/storage/providers/MemoryOnlyProvider.ts +++ b/lib/storage/providers/MemoryOnlyProvider.ts @@ -82,14 +82,12 @@ const provider: StorageProvider = { /** * Multiple merging of existing and new values in a batch - * This function also removes all nested null values from an object. */ multiMerge(pairs) { for (const [key, value] of pairs) { const existingValue = provider.store[key] as Record; const newValue = utils.fastMerge(existingValue, value as Record, { - shouldRemoveNestedNulls: true, objectRemovalMode: 'replace', }).result; diff --git a/lib/utils.ts b/lib/utils.ts index d7d296531..b82064b15 100644 --- a/lib/utils.ts +++ b/lib/utils.ts @@ -1,4 +1,4 @@ -import type {OnyxInput, OnyxKey} from './types'; +import type {OnyxKey} from './types'; type EmptyObject = Record; type EmptyValue = EmptyObject | null | undefined; @@ -13,9 +13,6 @@ type EmptyValue = EmptyObject | null | undefined; type FastMergeReplaceNullPatch = [string[], unknown]; type FastMergeOptions = { - /** If true, null object values will be removed. */ - shouldRemoveNestedNulls?: 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 losing information about the object removal. @@ -40,9 +37,7 @@ type FastMergeResult = { const ONYX_INTERNALS__REPLACE_OBJECT_MARK = 'ONYX_INTERNALS__REPLACE_OBJECT_MARK'; /** - * 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. + * Merges two objects. */ function fastMerge(target: TValue, source: TValue, options?: FastMergeOptions, metadata?: FastMergeMetadata, basePath: string[] = []): FastMergeResult { if (!metadata) { @@ -60,7 +55,6 @@ function fastMerge(target: TValue, source: TValue, options?: FastMergeOp } const optionsWithDefaults: FastMergeOptions = { - shouldRemoveNestedNulls: options?.shouldRemoveNestedNulls ?? false, objectRemovalMode: options?.objectRemovalMode ?? 'none', }; @@ -91,18 +85,11 @@ function mergeObject>( // First we want to copy over all keys from the target into the destination object, // in case "target" is a mergable object. - // 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) { for (const key of Object.keys(targetObject)) { const targetProperty = targetObject?.[key]; - 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 shouldOmitNullishProperty = options.shouldRemoveNestedNulls && (targetProperty === null || sourceProperty === null); - if (targetProperty === undefined || shouldOmitNullishProperty) { + if (targetProperty === undefined) { continue; } @@ -115,11 +102,7 @@ function mergeObject>( let targetProperty = targetObject?.[key]; const sourceProperty = source?.[key] as Record; - // If "shouldRemoveNestedNulls" is true, we want to remove (nested) null values from the merged object. - // If 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) { + if (sourceProperty === undefined) { continue; } @@ -170,37 +153,6 @@ function isMergeableObject>(value: unkno return isNonNullObject && !(value instanceof RegExp) && !(value instanceof Date) && !Array.isArray(value); } -/** Deep removes the nested null values from the given value. */ -function removeNestedNullValues | null>(value: TValue): TValue { - if (value === null || value === undefined || typeof value !== 'object') { - return value; - } - - if (Array.isArray(value)) { - return [...value] as TValue; - } - - const result: Record = {}; - - // eslint-disable-next-line no-restricted-syntax, guard-for-in - for (const key in value) { - const propertyValue = value[key]; - - if (propertyValue === null || propertyValue === undefined) { - continue; - } - - if (typeof propertyValue === 'object' && !Array.isArray(propertyValue)) { - const valueWithoutNestedNulls = removeNestedNullValues(propertyValue); - result[key] = valueWithoutNestedNulls; - } else { - result[key] = propertyValue; - } - } - - return result as TValue; -} - /** Formats the action name by uppercasing and adding the key if provided. */ function formatActionName(method: string, key?: OnyxKey): string { return key ? `${method.toUpperCase()}/${key}` : method.toUpperCase(); @@ -284,7 +236,6 @@ export default { fastMerge, isEmptyObject, formatActionName, - removeNestedNullValues, checkCompatibilityWithExistingValue, pick, omit, diff --git a/tests/perf-test/utils.perf-test.ts b/tests/perf-test/utils.perf-test.ts index fe9df4b32..0ec3af249 100644 --- a/tests/perf-test/utils.perf-test.ts +++ b/tests/perf-test/utils.perf-test.ts @@ -23,11 +23,7 @@ describe('utils', () => { test('one call', async () => { const target = getRandomReportActions(collectionKey, 1000); const source = getRandomReportActions(collectionKey, 500); - await measureFunction(() => - utils.fastMerge(target, source, { - shouldRemoveNestedNulls: true, - }), - ); + await measureFunction(() => utils.fastMerge(target, source)); }); }); @@ -37,19 +33,6 @@ describe('utils', () => { }); }); - describe('removeNestedNullValues', () => { - test('one call', async () => { - const reportAction = createRandomReportAction(0); - reportAction.actorAccountID = null; - reportAction.person.push(null); - reportAction.message[0].style = null; - reportAction.originalMessage.whisperedTo = null; - reportAction.lastModified = null; - - await measureFunction(() => utils.removeNestedNullValues(reportAction)); - }); - }); - describe('checkCompatibilityWithExistingValue', () => { test('one call', async () => { const value = {}; diff --git a/tests/unit/fastMergeTest.ts b/tests/unit/fastMergeTest.ts index edcbdfab1..f6b33851e 100644 --- a/tests/unit/fastMergeTest.ts +++ b/tests/unit/fastMergeTest.ts @@ -27,15 +27,6 @@ const testObjectWithNullishValues: DeepObject = { }, }; -const testObjectWithNullValuesRemoved: DeepObject = { - b: { - c: { - h: 'h', - }, - d: {}, - }, -}; - const testMergeChanges: DeepObject[] = [ { b: { @@ -78,24 +69,7 @@ describe('fastMerge', () => { }); describe('objects', () => { - it('should merge an object with another object and remove nested null values', () => { - const result = utils.fastMerge(testObject, testObjectWithNullishValues, {shouldRemoveNestedNulls: true}); - - expect(result.result).toEqual({ - a: 'a', - b: { - c: { - h: 'h', - }, - d: { - f: 'f', - }, - g: 'g', - }, - }); - }); - - it('should merge an object with another object and not remove nested null values', () => { + it('should merge an object with another object and keep nested null values', () => { const result = utils.fastMerge(testObject, testObjectWithNullishValues); expect(result.result).toEqual({ @@ -113,20 +87,6 @@ describe('fastMerge', () => { }); }); - it('should merge an object with an empty object and remove deeply nested null values', () => { - const result = utils.fastMerge({}, testObjectWithNullishValues, { - shouldRemoveNestedNulls: true, - }); - - expect(result.result).toEqual(testObjectWithNullValuesRemoved); - }); - - it('should remove null values by merging two identical objects with fastMerge', () => { - const result = utils.removeNestedNullValues(testObjectWithNullishValues); - - expect(result).toEqual(testObjectWithNullValuesRemoved); - }); - it('should replace Date objects', () => { const oldDate = new Date('2024-01-01'); const newDate = new Date('2025-01-01'); @@ -143,7 +103,6 @@ describe('fastMerge', () => { 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, objectRemovalMode: 'mark', }); @@ -172,7 +131,6 @@ describe('fastMerge', () => { }, }, { - shouldRemoveNestedNulls: true, objectRemovalMode: 'replace', }, ); diff --git a/tests/unit/onyxCacheTest.tsx b/tests/unit/onyxCacheTest.tsx index 5f2154287..77a1df1b3 100644 --- a/tests/unit/onyxCacheTest.tsx +++ b/tests/unit/onyxCacheTest.tsx @@ -352,13 +352,13 @@ describe('Onyx', () => { expect(() => cache.merge({})).not.toThrow(); }); - it('Should remove `null` values when merging', () => { + it('Should keep `null` values when merging', () => { cache.set('mockKey', {ID: 5}); cache.set('mockNullKey', null); cache.merge({mockKey: null}); - expect(cache.get('mockKey')).toEqual(undefined); + expect(cache.get('mockKey')).toEqual(null); expect(cache.get('mockNullKey')).toEqual(undefined); }); }); diff --git a/tests/unit/onyxTest.ts b/tests/unit/onyxTest.ts index 0ed0751a9..2eaa4d7d5 100644 --- a/tests/unit/onyxTest.ts +++ b/tests/unit/onyxTest.ts @@ -327,7 +327,7 @@ describe('Onyx', () => { }); }); - it('should remove keys that are set to null when merging', () => { + it('should preserve null values when merging', () => { let testKeyValue: unknown; connection = Onyx.connect({ @@ -368,7 +368,9 @@ describe('Onyx', () => { expect(testKeyValue).toEqual({ test1: { test2: 'test2', - test3: {}, + test3: { + test4: null, + }, }, }); @@ -379,12 +381,12 @@ describe('Onyx', () => { }); }) .then(() => { - expect(testKeyValue).toEqual({test1: {test2: 'test2'}}); + expect(testKeyValue).toEqual({test1: {test2: 'test2', test3: null}}); return Onyx.merge(ONYX_KEYS.TEST_KEY, {test1: null}); }) .then(() => { - expect(testKeyValue).toEqual({}); + expect(testKeyValue).toEqual({test1: null}); }); }); @@ -1343,7 +1345,7 @@ describe('Onyx', () => { }); }); - it("should not set null values in Onyx.merge, when the key doesn't exist yet", () => { + it("should preserve null values in Onyx.merge, when the key doesn't exist yet", () => { let testKeyValue: unknown; connection = Onyx.connect({ @@ -1365,6 +1367,7 @@ describe('Onyx', () => { waypoints: { 1: 'Home', 2: 'Work', + 3: null, }, }); }); @@ -1402,7 +1405,7 @@ describe('Onyx', () => { }); }); - it('mergeCollection should omit nested null values', () => { + it('mergeCollection should preserve nested null values', () => { let result: OnyxCollection; const routineRoute = `${ONYX_KEYS.COLLECTION.TEST_KEY}routine`; @@ -1443,6 +1446,7 @@ describe('Onyx', () => { waypoints: { 1: 'Home', 2: 'Beach', + 3: null, }, }, }); @@ -2616,7 +2620,7 @@ describe('Onyx', () => { }); }); - it('should remove a deeply nested null when merging an existing key', () => { + it('should preserve a deeply nested null when merging an existing key', () => { let result: unknown; connection = Onyx.connect({ @@ -2650,6 +2654,7 @@ describe('Onyx', () => { waypoints: { 1: 'Home', 2: 'Work', + 3: null, }, }); });