diff --git a/.gitignore b/.gitignore index a7d7facc..4bf2fed2 100644 --- a/.gitignore +++ b/.gitignore @@ -18,5 +18,9 @@ dist/ # Published package *.tgz +# Yalc +.yalc +yalc.lock + # Perf tests -.reassure \ No newline at end of file +.reassure diff --git a/lib/Onyx.ts b/lib/Onyx.ts index ee039a91..ff9927c1 100644 --- a/lib/Onyx.ts +++ b/lib/Onyx.ts @@ -245,7 +245,7 @@ function set(key: TKey, value: OnyxEntry): Promise { - const keyValuePairs = OnyxUtils.prepareKeyValuePairsForStorage(data); + const keyValuePairs = OnyxUtils.prepareKeyValuePairsForStorage(data, true); const updatePromises = keyValuePairs.map(([key, value]) => { const prevValue = cache.getValue(key, false); @@ -285,7 +285,7 @@ function merge(key: TKey, changes: OnyxEntry(collectionKey: TK obj[key] = mergedCollection[key]; return obj; }, {}); - const keyValuePairsForExistingCollection = OnyxUtils.prepareKeyValuePairsForStorage(existingKeyCollection); - const keyValuePairsForNewCollection = OnyxUtils.prepareKeyValuePairsForStorage(newCollection); + + // 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); + + // We can safely remove nested null values when using (multi-)set, + // because we will simply overwrite the existing values in storage. + const keyValuePairsForNewCollection = OnyxUtils.prepareKeyValuePairsForStorage(newCollection, true); const promises = []; diff --git a/lib/OnyxUtils.ts b/lib/OnyxUtils.ts index 67c654c7..fe1b9b3f 100644 --- a/lib/OnyxUtils.ts +++ b/lib/OnyxUtils.ts @@ -453,6 +453,7 @@ function keysChanged( // We prepare the "cached collection" which is the entire collection + the new partial data that // was merged in via mergeCollection(). const cachedCollection = getCachedCollection(collectionKey); + const cachedCollectionWithoutNestedNulls = utils.removeNestedNullValues(cachedCollection) as Record; // Regular Onyx.connect() subscriber found. if (typeof subscriber.callback === 'function') { @@ -464,7 +465,7 @@ function keysChanged( // send the whole cached collection. if (isSubscribedToCollectionKey) { if (subscriber.waitForCollectionCallback) { - subscriber.callback(cachedCollection); + subscriber.callback(cachedCollectionWithoutNestedNulls); continue; } @@ -473,7 +474,7 @@ function keysChanged( const dataKeys = Object.keys(partialCollection ?? {}); for (let j = 0; j < dataKeys.length; j++) { const dataKey = dataKeys[j]; - subscriber.callback(cachedCollection[dataKey], dataKey); + subscriber.callback(cachedCollectionWithoutNestedNulls[dataKey], dataKey); } continue; } @@ -482,7 +483,7 @@ function keysChanged( // notify them with the cached data for that key only. if (isSubscribedToCollectionMemberKey) { const subscriberCallback = subscriber.callback as DefaultConnectCallback; - subscriberCallback(cachedCollection[subscriber.key], subscriber.key as TKey); + subscriberCallback(cachedCollectionWithoutNestedNulls[subscriber.key], subscriber.key as TKey); continue; } @@ -621,13 +622,16 @@ function keyChanged( } if (isCollectionKey(subscriber.key) && subscriber.waitForCollectionCallback) { const cachedCollection = getCachedCollection(subscriber.key); - cachedCollection[key] = data; - subscriber.callback(cachedCollection); + const cachedCollectionWithoutNestedNulls = utils.removeNestedNullValues(cachedCollection) as Record; + + cachedCollectionWithoutNestedNulls[key] = data; + subscriber.callback(cachedCollectionWithoutNestedNulls); continue; } + const dataWithoutNestedNulls = utils.removeNestedNullValues(data); const subscriberCallback = subscriber.callback as DefaultConnectCallback; - subscriberCallback(data, key); + subscriberCallback(dataWithoutNestedNulls, key); continue; } @@ -752,7 +756,8 @@ function sendDataToConnection(mapping: Mapping, val: return; } - (mapping as DefaultConnectOptions).callback?.(val, matchedKey as TKey); + const valuesWithoutNestedNulls = utils.removeNestedNullValues(val); + (mapping as DefaultConnectOptions).callback?.(valuesWithoutNestedNulls, matchedKey as TKey); } /** @@ -963,11 +968,12 @@ type RemoveNullValuesOutput = { /** * Removes a key from storage if the value is null. - * Otherwise removes all nested null values in objects and returns the object + * 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(key: OnyxKey, value: OnyxValue): RemoveNullValuesOutput { +function removeNullValues(key: OnyxKey, value: OnyxValue, shouldRemoveNestedNulls = true): RemoveNullValuesOutput { if (value === null) { remove(key); return {value, wasRemoved: true}; @@ -976,7 +982,7 @@ function removeNullValues(key: OnyxKey, value: OnyxValue): RemoveNullVa // 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: utils.removeNestedNullValues(value as Record), wasRemoved: false}; + return {value: shouldRemoveNestedNulls ? utils.removeNestedNullValues(value as Record) : (value as Record), wasRemoved: false}; } /** @@ -986,20 +992,16 @@ function removeNullValues(key: OnyxKey, value: OnyxValue): RemoveNullVa * @return an array of key - value pairs <[key, value]> */ -function prepareKeyValuePairsForStorage(data: Record>): Array<[OnyxKey, OnyxValue]> { - const keyValuePairs: Array<[OnyxKey, OnyxValue]> = []; - - Object.entries(data).forEach(([key, value]) => { - const {value: valueAfterRemoving, wasRemoved} = removeNullValues(key, value); +function prepareKeyValuePairsForStorage(data: Record>, shouldRemoveNestedNulls: boolean): Array<[OnyxKey, OnyxValue]> { + return Object.entries(data).reduce]>>((pairs, [key, value]) => { + const {value: valueAfterRemoving, wasRemoved} = removeNullValues(key, value, shouldRemoveNestedNulls); - if (wasRemoved) { - return; + if (!wasRemoved) { + pairs.push([key, valueAfterRemoving]); } - keyValuePairs.push([key, valueAfterRemoving]); - }); - - return keyValuePairs; + return pairs; + }, []); } /** @@ -1007,7 +1009,7 @@ function prepareKeyValuePairsForStorage(data: Record * * @param changes Array of changes that should be applied to the existing value */ -function applyMerge(existingValue: OnyxValue, changes: Array>, shouldRemoveNullObjectValues: boolean): OnyxValue { +function applyMerge(existingValue: OnyxValue, changes: Array>, shouldRemoveNestedNulls: boolean): OnyxValue { const lastChange = changes?.at(-1); if (Array.isArray(lastChange)) { @@ -1017,7 +1019,7 @@ function applyMerge(existingValue: OnyxValue, changes: Array change && typeof change === 'object')) { // Object values are then merged one after the other return changes.reduce( - (modifiedData, change) => utils.fastMerge(modifiedData as Record, change as Record, shouldRemoveNullObjectValues), + (modifiedData, change) => utils.fastMerge(modifiedData as Record, change as Record, shouldRemoveNestedNulls), existingValue || {}, ); } diff --git a/lib/storage/providers/MemoryOnlyProvider.ts b/lib/storage/providers/MemoryOnlyProvider.ts index 0dd0397a..f67cff12 100644 --- a/lib/storage/providers/MemoryOnlyProvider.ts +++ b/lib/storage/providers/MemoryOnlyProvider.ts @@ -68,11 +68,7 @@ const provider: StorageProvider = { multiSet(pairs) { const setPromises = _.map(pairs, ([key, value]) => this.setItem(key, value)); - return new Promise((resolve) => { - Promise.all(setPromises).then(() => { - resolve(undefined); - }); - }); + return Promise.all(setPromises).then(() => undefined); }, /** diff --git a/lib/utils.ts b/lib/utils.ts index 02bbf4e0..e29e472f 100644 --- a/lib/utils.ts +++ b/lib/utils.ts @@ -24,15 +24,15 @@ function isMergeableObject(value: unknown): value is Record { * Merges the source object into the target object. * @param target - The target object. * @param source - The source object. - * @param shouldRemoveNullObjectValues - If true, null object values will be removed. + * @param shouldRemoveNestedNulls - If true, null object values will be removed. * @returns - The merged object. */ -function mergeObject>(target: TObject | null, source: TObject, shouldRemoveNullObjectValues = true): TObject { +function mergeObject>(target: TObject | null, source: TObject, shouldRemoveNestedNulls = true): TObject { const destination: Record = {}; // First we want to copy over all keys from the target into the destination object, // in case "target" is a mergable object. - // If "shouldRemoveNullObjectValues" is true, we want to remove null values from the merged 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 (isMergeableObject(target)) { const targetKeys = Object.keys(target); @@ -41,10 +41,10 @@ function mergeObject>(target: TObject | const sourceValue = source?.[key]; const targetValue = target?.[key]; - // If "shouldRemoveNullObjectValues" is true, we want to remove null values from the merged object. + // 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. const isSourceOrTargetNull = targetValue === null || sourceValue === null; - const shouldOmitTargetKey = shouldRemoveNullObjectValues && isSourceOrTargetNull; + const shouldOmitTargetKey = shouldRemoveNestedNulls && isSourceOrTargetNull; if (!shouldOmitTargetKey) { destination[key] = targetValue; @@ -60,14 +60,14 @@ function mergeObject>(target: TObject | const targetValue = target?.[key]; // If undefined is passed as the source value for a key, we want to generally ignore it. - // If "shouldRemoveNullObjectValues" is set to true and the source value is null, + // 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 = shouldRemoveNullObjectValues && sourceValue === null; + const shouldIgnoreNullSourceValue = shouldRemoveNestedNulls && sourceValue === null; const shouldOmitSourceKey = sourceValue === undefined || shouldIgnoreNullSourceValue; if (!shouldOmitSourceKey) { // If the source value is a mergable object, we want to merge it into the target value. - // If "shouldRemoveNullObjectValues" is true, "fastMerge" will recursively + // If "shouldRemoveNestedNulls" is true, "fastMerge" will recursively // remove nested null values from the merged object. // If source value is any other value we need to set the source value it directly. if (isMergeableObject(sourceValue)) { @@ -75,7 +75,7 @@ function mergeObject>(target: TObject | // 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; - destination[key] = fastMerge(targetValueWithFallback, sourceValue, shouldRemoveNullObjectValues); + destination[key] = fastMerge(targetValueWithFallback, sourceValue, shouldRemoveNestedNulls); } else { destination[key] = sourceValue; } @@ -86,13 +86,13 @@ function mergeObject>(target: TObject | } /** - * Merges two objects and removes null values if "shouldRemoveNullObjectValues" is set to true + * 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. * On native, when merging an existing value with new changes, SQLite will use JSON_PATCH, which removes top-level nullish values. * To be consistent with the behaviour for merge, we'll also want to remove null values for "set" operations. */ -function fastMerge>(target: TObject | null, source: TObject | null, shouldRemoveNullObjectValues = true): TObject | null { +function fastMerge>(target: TObject | null, source: TObject | null, shouldRemoveNestedNulls = true): TObject | null { // We have to ignore arrays and nullish values here, // otherwise "mergeObject" will throw an error, // because it expects an object as "source" @@ -100,16 +100,16 @@ function fastMerge>(target: TObject | nu return source; } - return mergeObject(target, source, shouldRemoveNullObjectValues); + return mergeObject(target, source, shouldRemoveNestedNulls); } /** Deep removes the nested null values from the given value. */ -function removeNestedNullValues(value: unknown[] | Record): Record | unknown[] | null { +function removeNestedNullValues>(value: unknown | unknown[] | TObject): Record | unknown[] | null { if (typeof value === 'object' && !Array.isArray(value)) { - return fastMerge(value, value); + return fastMerge(value as Record | null, value as Record | null); } - return value; + return value as Record | null; } /** Formats the action name by uppercasing and adding the key if provided. */ diff --git a/lib/withOnyx.js b/lib/withOnyx.js index c89838db..2ea6dbb5 100644 --- a/lib/withOnyx.js +++ b/lib/withOnyx.js @@ -321,6 +321,7 @@ export default function (mapOnyxToState, shouldDelayUpdates = false) { // that should not be passed to a wrapped component let stateToPass = _.omit(this.state, 'loading'); stateToPass = _.omit(stateToPass, _.isNull); + const stateToPassWithoutNestedNulls = utils.removeNestedNullValues(stateToPass); // Spreading props and state is necessary in an HOC where the data cannot be predicted return ( @@ -329,7 +330,7 @@ export default function (mapOnyxToState, shouldDelayUpdates = false) { // eslint-disable-next-line react/jsx-props-no-spreading {...propsToPass} // eslint-disable-next-line react/jsx-props-no-spreading - {...stateToPass} + {...stateToPassWithoutNestedNulls} ref={this.props.forwardedRef} /> ); diff --git a/tests/unit/onyxTest.js b/tests/unit/onyxTest.js index a75fd6bb..faeda068 100644 --- a/tests/unit/onyxTest.js +++ b/tests/unit/onyxTest.js @@ -1071,4 +1071,78 @@ describe('Onyx', () => { expect(testKeyValue).toEqual(null); }); }); + + it('should merge a non-existing key with a nested null removed', () => { + let testKeyValue; + + connectionID = Onyx.connect({ + key: ONYX_KEYS.TEST_KEY, + initWithStoredValues: false, + callback: (value) => { + testKeyValue = value; + }, + }); + + return Onyx.merge(ONYX_KEYS.TEST_KEY, { + waypoints: { + 1: 'Home', + 2: 'Work', + 3: null, + }, + }).then(() => { + expect(testKeyValue).toEqual({ + waypoints: { + 1: 'Home', + 2: 'Work', + }, + }); + }); + }); + + it('mergeCollection should omit nested null values', () => { + let result; + + const routineRoute = `${ONYX_KEYS.COLLECTION.TEST_KEY}routine`; + const holidayRoute = `${ONYX_KEYS.COLLECTION.TEST_KEY}holiday`; + + connectionID = Onyx.connect({ + key: ONYX_KEYS.COLLECTION.TEST_KEY, + initWithStoredValues: false, + callback: (value) => (result = value), + waitForCollectionCallback: true, + }); + + return Onyx.mergeCollection(ONYX_KEYS.COLLECTION.TEST_KEY, { + [routineRoute]: { + waypoints: { + 1: 'Home', + 2: 'Work', + 3: 'Gym', + }, + }, + [holidayRoute]: { + waypoints: { + 1: 'Home', + 2: 'Beach', + 3: null, + }, + }, + }).then(() => { + expect(result).toEqual({ + [routineRoute]: { + waypoints: { + 1: 'Home', + 2: 'Work', + 3: 'Gym', + }, + }, + [holidayRoute]: { + waypoints: { + 1: 'Home', + 2: 'Beach', + }, + }, + }); + }); + }); });