From 5ec158c4e1631afa202f0cd5cca6481e43d315d2 Mon Sep 17 00:00:00 2001 From: Christoph Pader Date: Tue, 12 Sep 2023 17:55:34 +0200 Subject: [PATCH 01/33] fix: improve merging with batched objects (nullish values ignored previously) --- lib/Onyx.js | 30 +++++++++++++++++++++++++++--- lib/utils.js | 13 +++++++++++++ 2 files changed, 40 insertions(+), 3 deletions(-) create mode 100644 lib/utils.js diff --git a/lib/Onyx.js b/lib/Onyx.js index c8ffcf821..173ad1c6e 100644 --- a/lib/Onyx.js +++ b/lib/Onyx.js @@ -8,6 +8,7 @@ import createDeferredTask from './createDeferredTask'; import fastMerge from './fastMerge'; import * as PerformanceUtils from './metrics/PerformanceUtils'; import Storage from './storage'; +import Utils from './utils'; // Method constants const METHOD = { @@ -1025,11 +1026,21 @@ function applyMerge(existingValue, changes) { return lastChange; } - if (_.isObject(existingValue) || _.every(changes, _.isObject)) { + if (_.isObject(existingValue) || _.isObject(lastChange)) { + // We want to merge multiple object deltas together + // If all of the changes are objects, we can simply merge all of them together + // If the batched changes contain nullish values or arrays, we should only merge the very last objects in the batch + let numberOfObjectsAtTheEnd; + if (_.every(changes, _.isObject)) { + numberOfObjectsAtTheEnd = changes.length; + } else { + numberOfObjectsAtTheEnd = Utils.countObjectsAtEndOfArray(changes); + } + // Object values are merged one after the other // lodash adds a small overhead so we don't use it here // eslint-disable-next-line prefer-object-spread, rulesdir/prefer-underscore-method - return _.reduce(changes, (modifiedData, change) => fastMerge(modifiedData, change), + return _.reduce(changes.slice(-numberOfObjectsAtTheEnd), (modifiedData, change) => fastMerge(modifiedData, change), existingValue || {}); } @@ -1073,12 +1084,25 @@ function merge(key, changes) { // We first only merge the changes, so we can provide these to the native implementation (SQLite uses only delta changes in "JSON_PATCH" to merge) let batchedChanges = applyMerge(undefined, mergeQueue[key]); + if (_.isNull(batchedChanges)) { + return remove(key); + } + + // The batchedChanges is the result of all changes in the mergeQueue merged together. + // If the mergeQueue contains nullish values (anywhere) and last value(s), it means that the user + // intends to reset the existing value or remove the key + // If there are other changes queued afterwards, we cannot simply merge the remaining changes + // and merge it with the existingValue. Instead, we have to omit the existingValue and only set + // the batchedChanges after any nullish values. + // This only affects objects, because arrays and nullish values are always replaced by the newest value. + const omitExistingValue = _.some(mergeQueue[key], change => _.isUndefined(change) || _.isNull(change)); + // Clean up the write queue so we // don't apply these changes again delete mergeQueue[key]; // After that we merge the batched changes with the existing value - const modifiedData = removeNullObjectValues(applyMerge(existingValue, [batchedChanges])); + const modifiedData = Utils.removeNullObjectValues(applyMerge(omitExistingValue ? undefined : existingValue, [batchedChanges])); // On native platforms we use SQLite which utilises JSON_PATCH to merge changes. // JSON_PATCH generally removes top-level nullish values from the stored object. diff --git a/lib/utils.js b/lib/utils.js new file mode 100644 index 000000000..3d7618fe1 --- /dev/null +++ b/lib/utils.js @@ -0,0 +1,13 @@ +function countObjectsAtEndOfArray(array) { + let count = 0; + for (let i = array.length - 1; i >= 0; i--) { + if (typeof array[i] === 'object' && array[i] !== null) { + count++; + } else { + break; + } + } + return count; +} + +export default {countObjectsAtEndOfArray}; From 71c528ec4468875697034a16cd7b8d2ef3039681 Mon Sep 17 00:00:00 2001 From: Christoph Pader Date: Tue, 12 Sep 2023 18:16:50 +0200 Subject: [PATCH 02/33] fix: fix fastMerge not being able to merge nullish values --- lib/fastMerge.js | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/lib/fastMerge.js b/lib/fastMerge.js index a79825d44..2ddc5ad62 100644 --- a/lib/fastMerge.js +++ b/lib/fastMerge.js @@ -1,3 +1,5 @@ +import _ from 'underscore'; + // Mostly copied from https://medium.com/@lubaka.a/how-to-remove-lodash-performance-improvement-b306669ad0e1 /** @@ -54,10 +56,7 @@ function mergeObject(target, source) { * @returns {Object|Array} */ function fastMerge(target, source) { - // lodash adds a small overhead so we don't use it here - // eslint-disable-next-line rulesdir/prefer-underscore-method - const array = Array.isArray(source); - if (array) { + if (_.isArray(source) || _.isNull(source) || _.isUndefined(source)) { return source; } return mergeObject(target, source); From bd2927310eb15a9bd5aa2570132207f0efe33eb4 Mon Sep 17 00:00:00 2001 From: Christoph Pader Date: Tue, 12 Sep 2023 18:17:10 +0200 Subject: [PATCH 03/33] fix: improve and simplify batched object merging --- lib/Onyx.js | 30 ++++++++++++------------------ 1 file changed, 12 insertions(+), 18 deletions(-) diff --git a/lib/Onyx.js b/lib/Onyx.js index 173ad1c6e..8967bf384 100644 --- a/lib/Onyx.js +++ b/lib/Onyx.js @@ -1022,25 +1022,19 @@ function multiSet(data) { function applyMerge(existingValue, changes) { const lastChange = _.last(changes); - if (_.isArray(existingValue) || _.isArray(lastChange)) { + if (_.isArray(lastChange)) { return lastChange; } - if (_.isObject(existingValue) || _.isObject(lastChange)) { - // We want to merge multiple object deltas together - // If all of the changes are objects, we can simply merge all of them together - // If the batched changes contain nullish values or arrays, we should only merge the very last objects in the batch - let numberOfObjectsAtTheEnd; - if (_.every(changes, _.isObject)) { - numberOfObjectsAtTheEnd = changes.length; - } else { - numberOfObjectsAtTheEnd = Utils.countObjectsAtEndOfArray(changes); - } - + if (_.isObject(lastChange)) { // Object values are merged one after the other + // If there are non-object values in the changes, they will be overwritten in the end, + // because the last value has to be an object. + // fastMerge will just overwrite the existingValue with non-object values + // ==================================================== // lodash adds a small overhead so we don't use it here // eslint-disable-next-line prefer-object-spread, rulesdir/prefer-underscore-method - return _.reduce(changes.slice(-numberOfObjectsAtTheEnd), (modifiedData, change) => fastMerge(modifiedData, change), + return _.reduce(changes, (modifiedData, change) => fastMerge(modifiedData, change), existingValue || {}); } @@ -1089,11 +1083,11 @@ function merge(key, changes) { } // The batchedChanges is the result of all changes in the mergeQueue merged together. - // If the mergeQueue contains nullish values (anywhere) and last value(s), it means that the user - // intends to reset the existing value or remove the key - // If there are other changes queued afterwards, we cannot simply merge the remaining changes - // and merge it with the existingValue. Instead, we have to omit the existingValue and only set - // the batchedChanges after any nullish values. + // If the mergeQueue contains nullish values, it means that the user + // intends to reset the existing value (undefined) or remove the key (null) + // If there are other changes queued afterwards, we cannot simply batch the remaining changes + // and merge it with the existingValue. Instead, in order for the nullish value to have an effect, + // we have to omit the existingValue and replace it with the batchedChanges after the last nullish value. // This only affects objects, because arrays and nullish values are always replaced by the newest value. const omitExistingValue = _.some(mergeQueue[key], change => _.isUndefined(change) || _.isNull(change)); From a9c0463f5459b49307d3e8349432886a4ef42845 Mon Sep 17 00:00:00 2001 From: Christoph Pader Date: Tue, 12 Sep 2023 18:32:04 +0200 Subject: [PATCH 04/33] fix: remove unused util function --- lib/Onyx.js | 3 +-- lib/utils.js | 13 ------------- 2 files changed, 1 insertion(+), 15 deletions(-) delete mode 100644 lib/utils.js diff --git a/lib/Onyx.js b/lib/Onyx.js index 8967bf384..159a17f64 100644 --- a/lib/Onyx.js +++ b/lib/Onyx.js @@ -8,7 +8,6 @@ import createDeferredTask from './createDeferredTask'; import fastMerge from './fastMerge'; import * as PerformanceUtils from './metrics/PerformanceUtils'; import Storage from './storage'; -import Utils from './utils'; // Method constants const METHOD = { @@ -1096,7 +1095,7 @@ function merge(key, changes) { delete mergeQueue[key]; // After that we merge the batched changes with the existing value - const modifiedData = Utils.removeNullObjectValues(applyMerge(omitExistingValue ? undefined : existingValue, [batchedChanges])); + const modifiedData = removeNullObjectValues(applyMerge(omitExistingValue ? undefined : existingValue, [batchedChanges])); // On native platforms we use SQLite which utilises JSON_PATCH to merge changes. // JSON_PATCH generally removes top-level nullish values from the stored object. diff --git a/lib/utils.js b/lib/utils.js deleted file mode 100644 index 3d7618fe1..000000000 --- a/lib/utils.js +++ /dev/null @@ -1,13 +0,0 @@ -function countObjectsAtEndOfArray(array) { - let count = 0; - for (let i = array.length - 1; i >= 0; i--) { - if (typeof array[i] === 'object' && array[i] !== null) { - count++; - } else { - break; - } - } - return count; -} - -export default {countObjectsAtEndOfArray}; From 39c7784d9f175ea86ebff433027c42fa41ecc7b2 Mon Sep 17 00:00:00 2001 From: Christoph Pader Date: Wed, 13 Sep 2023 11:21:26 +0200 Subject: [PATCH 05/33] feat: add new test which tests merging objects and undefined in a batch --- tests/unit/onyxTest.js | 30 ++++++++++++++++++++++++++++++ 1 file changed, 30 insertions(+) diff --git a/tests/unit/onyxTest.js b/tests/unit/onyxTest.js index 5b10e2589..6918a91ee 100644 --- a/tests/unit/onyxTest.js +++ b/tests/unit/onyxTest.js @@ -819,4 +819,34 @@ describe('Onyx', () => { Onyx.disconnect(connectionIDs); }); }); + + it('should merge an object with a batch of objects and undefined', () => { + let testKeyValue; + + connectionID = Onyx.connect({ + key: ONYX_KEYS.TEST_KEY, + initWithStoredValues: false, + callback: (value) => { + testKeyValue = value; + }, + }); + + return Onyx.set(ONYX_KEYS.TEST_KEY, {test1: 'test1'}) + .then(() => { + expect(testKeyValue).toEqual({test1: 'test1'}); + Onyx.merge(ONYX_KEYS.TEST_KEY, {test2: 'test2'}); + Onyx.merge(ONYX_KEYS.TEST_KEY, {test3: 'test3'}); + Onyx.merge(ONYX_KEYS.TEST_KEY, undefined); + Onyx.merge(ONYX_KEYS.TEST_KEY, {test4: 'test4'}); + Onyx.merge(ONYX_KEYS.TEST_KEY, undefined); + return waitForPromisesToResolve(); + }) + .then(() => { + console.log({testKeyValue}); + + expect(testKeyValue).toEqual({ + test1: 'test1', test2: 'test2', test3: 'test3', test4: 'test4', + }); + }); + }); }); From e5652669da108209d1419f4a47ee76458db9f1e8 Mon Sep 17 00:00:00 2001 From: Christoph Pader Date: Wed, 13 Sep 2023 11:27:42 +0200 Subject: [PATCH 06/33] fix: ignore undefined values in a batch of object changes --- lib/Onyx.js | 27 ++++++++------------------- 1 file changed, 8 insertions(+), 19 deletions(-) diff --git a/lib/Onyx.js b/lib/Onyx.js index 159a17f64..72088fcec 100644 --- a/lib/Onyx.js +++ b/lib/Onyx.js @@ -1025,15 +1025,13 @@ function applyMerge(existingValue, changes) { return lastChange; } - if (_.isObject(lastChange)) { - // Object values are merged one after the other - // If there are non-object values in the changes, they will be overwritten in the end, - // because the last value has to be an object. - // fastMerge will just overwrite the existingValue with non-object values - // ==================================================== - // lodash adds a small overhead so we don't use it here - // eslint-disable-next-line prefer-object-spread, rulesdir/prefer-underscore-method - return _.reduce(changes, (modifiedData, change) => fastMerge(modifiedData, change), + if (_.some(changes, _.isObject)) { + // There might be nullish values in a batch of changes that we want to ignore when merging objects. + // Therefore, we filter out nullish values before merging. + const changesWithoutNullishValues = _.filter(changes, change => !_.isNull(change) && !_.isUndefined(change)); + + // Object values are then merged one after the other + return _.reduce(changesWithoutNullishValues, (modifiedData, change) => fastMerge(modifiedData, change), existingValue || {}); } @@ -1081,21 +1079,12 @@ function merge(key, changes) { return remove(key); } - // The batchedChanges is the result of all changes in the mergeQueue merged together. - // If the mergeQueue contains nullish values, it means that the user - // intends to reset the existing value (undefined) or remove the key (null) - // If there are other changes queued afterwards, we cannot simply batch the remaining changes - // and merge it with the existingValue. Instead, in order for the nullish value to have an effect, - // we have to omit the existingValue and replace it with the batchedChanges after the last nullish value. - // This only affects objects, because arrays and nullish values are always replaced by the newest value. - const omitExistingValue = _.some(mergeQueue[key], change => _.isUndefined(change) || _.isNull(change)); - // Clean up the write queue so we // don't apply these changes again delete mergeQueue[key]; // After that we merge the batched changes with the existing value - const modifiedData = removeNullObjectValues(applyMerge(omitExistingValue ? undefined : existingValue, [batchedChanges])); + const modifiedData = removeNullObjectValues(applyMerge(existingValue, [batchedChanges])); // On native platforms we use SQLite which utilises JSON_PATCH to merge changes. // JSON_PATCH generally removes top-level nullish values from the stored object. From 94607693d672e4702ff2fb0e3262160756a22dbd Mon Sep 17 00:00:00 2001 From: Sibtain Ali Date: Thu, 14 Sep 2023 05:47:16 +0500 Subject: [PATCH 07/33] feat: bump the peer dependency version of react-native-performance --- package-lock.json | 2 +- package.json | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/package-lock.json b/package-lock.json index 9eeff929f..ef692d258 100644 --- a/package-lock.json +++ b/package-lock.json @@ -53,7 +53,7 @@ "idb-keyval": "^6.2.1", "react": ">=18.1.0", "react-native-device-info": "^10.3.0", - "react-native-performance": "^4.0.0", + "react-native-performance": "^5.1.0", "react-native-quick-sqlite": "^8.0.0-beta.2" }, "peerDependenciesMeta": { diff --git a/package.json b/package.json index 8f342b5f8..6a6751d98 100644 --- a/package.json +++ b/package.json @@ -79,7 +79,7 @@ "peerDependencies": { "idb-keyval": "^6.2.1", "react": ">=18.1.0", - "react-native-performance": "^4.0.0", + "react-native-performance": "^5.1.0", "react-native-quick-sqlite": "^8.0.0-beta.2", "react-native-device-info": "^10.3.0" }, From 050178dbda878166cac22ceb34b0bfd05b0d5f41 Mon Sep 17 00:00:00 2001 From: OSBotify Date: Thu, 14 Sep 2023 03:42:12 +0000 Subject: [PATCH 08/33] 1.0.77 --- package-lock.json | 4 ++-- package.json | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/package-lock.json b/package-lock.json index ef692d258..9c39857d8 100644 --- a/package-lock.json +++ b/package-lock.json @@ -1,12 +1,12 @@ { "name": "react-native-onyx", - "version": "1.0.76", + "version": "1.0.77", "lockfileVersion": 2, "requires": true, "packages": { "": { "name": "react-native-onyx", - "version": "1.0.76", + "version": "1.0.77", "license": "MIT", "dependencies": { "ascii-table": "0.0.9", diff --git a/package.json b/package.json index 6a6751d98..b1b1c8e61 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "react-native-onyx", - "version": "1.0.76", + "version": "1.0.77", "author": "Expensify, Inc.", "homepage": "https://expensify.com", "description": "State management for React Native", From 5c04af8a7946edec45b0fee680dafe51028ad3a7 Mon Sep 17 00:00:00 2001 From: Christoph Pader Date: Thu, 14 Sep 2023 10:26:52 +0200 Subject: [PATCH 09/33] fix: improve comment --- lib/Onyx.js | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/lib/Onyx.js b/lib/Onyx.js index 72088fcec..d546cdf25 100644 --- a/lib/Onyx.js +++ b/lib/Onyx.js @@ -1026,7 +1026,8 @@ function applyMerge(existingValue, changes) { } if (_.some(changes, _.isObject)) { - // There might be nullish values in a batch of changes that we want to ignore when merging objects. + // There might be nullish values in a batch of changes that we want to ignore when merging objects, + // because otherwise they would overwrite the previous changes. // Therefore, we filter out nullish values before merging. const changesWithoutNullishValues = _.filter(changes, change => !_.isNull(change) && !_.isUndefined(change)); From d26478997ca0c2608f686409781f05b98793e6ad Mon Sep 17 00:00:00 2001 From: Christoph Pader Date: Thu, 14 Sep 2023 10:28:18 +0200 Subject: [PATCH 10/33] fix: add comment about fastMerge change --- lib/fastMerge.js | 3 +++ 1 file changed, 3 insertions(+) diff --git a/lib/fastMerge.js b/lib/fastMerge.js index 2ddc5ad62..753dd6354 100644 --- a/lib/fastMerge.js +++ b/lib/fastMerge.js @@ -56,6 +56,9 @@ function mergeObject(target, source) { * @returns {Object|Array} */ function fastMerge(target, source) { + // We have to ignore arrays and nullish values here, + // otherwise "mergeObject" will throw an error, + // because it expects an object as "source" if (_.isArray(source) || _.isNull(source) || _.isUndefined(source)) { return source; } From 36e10a2dd8e8f4df1a41eaf5d45ec05aefec97a7 Mon Sep 17 00:00:00 2001 From: Christoph Pader Date: Tue, 19 Sep 2023 13:09:50 +0200 Subject: [PATCH 11/33] fix: remove console.log --- tests/unit/onyxTest.js | 2 -- 1 file changed, 2 deletions(-) diff --git a/tests/unit/onyxTest.js b/tests/unit/onyxTest.js index 12af69edc..138ff038e 100644 --- a/tests/unit/onyxTest.js +++ b/tests/unit/onyxTest.js @@ -871,8 +871,6 @@ describe('Onyx', () => { return waitForPromisesToResolve(); }) .then(() => { - console.log({testKeyValue}); - expect(testKeyValue).toEqual({ test1: 'test1', test2: 'test2', test3: 'test3', test4: 'test4', }); From 7e52d6d8177c51d8bcb57b2ac7fefea9b1fe1b13 Mon Sep 17 00:00:00 2001 From: Christoph Pader Date: Wed, 20 Sep 2023 08:48:15 +0200 Subject: [PATCH 12/33] Update lib/Onyx.js Co-authored-by: Tim Golen --- lib/Onyx.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/Onyx.js b/lib/Onyx.js index 31065415f..ff2a843a5 100644 --- a/lib/Onyx.js +++ b/lib/Onyx.js @@ -1088,7 +1088,7 @@ function applyMerge(existingValue, changes) { if (_.some(changes, _.isObject)) { // There might be nullish values in a batch of changes that we want to ignore when merging objects, - // because otherwise they would overwrite the previous changes. + // because otherwise they overwrite the previous changes in the batch. // Therefore, we filter out nullish values before merging. const changesWithoutNullishValues = _.filter(changes, change => !_.isNull(change) && !_.isUndefined(change)); From 40ee983c6c1670699de018e2d2bb8dcedd397994 Mon Sep 17 00:00:00 2001 From: Tim Golen Date: Thu, 21 Sep 2023 05:30:16 +0800 Subject: [PATCH 13/33] Testing dependent keys --- tests/unit/withOnyxTest.js | 20 ++++++++------------ 1 file changed, 8 insertions(+), 12 deletions(-) diff --git a/tests/unit/withOnyxTest.js b/tests/unit/withOnyxTest.js index 1f36cfe09..d4b00894a 100644 --- a/tests/unit/withOnyxTest.js +++ b/tests/unit/withOnyxTest.js @@ -250,18 +250,14 @@ describe('withOnyxTest', () => { Onyx.mergeCollection(ONYX_KEYS.COLLECTION.RELATED_KEY, {related_1: 'Test'}); return waitForPromisesToResolve() .then(() => { - const TestComponentWithOnyx = compose( - withOnyx({ - testObject: { - key: `${ONYX_KEYS.COLLECTION.TEST_KEY}${collectionItemID}`, - }, - }), - withOnyx({ - testThing: { - key: ({testObject}) => `${ONYX_KEYS.COLLECTION.RELATED_KEY}${testObject.id}`, - }, - }), - )(ViewWithCollections); + const TestComponentWithOnyx = withOnyx({ + testObject: { + key: `${ONYX_KEYS.COLLECTION.TEST_KEY}${collectionItemID}`, + }, + testThing: { + key: ({testObject}) => `${ONYX_KEYS.COLLECTION.RELATED_KEY}${testObject.id}`, + }, + })(ViewWithCollections); render(); }) .then(() => { From f8eec0391f140166dc03bc165a7e84bde8853010 Mon Sep 17 00:00:00 2001 From: Tim Golen Date: Thu, 21 Sep 2023 06:34:14 +0800 Subject: [PATCH 14/33] Pass withOnyx state to key functions --- .gitignore | 1 + lib/withOnyx.js | 8 ++++---- tests/unit/withOnyxTest.js | 3 ++- 3 files changed, 7 insertions(+), 5 deletions(-) diff --git a/.gitignore b/.gitignore index 41f9bece4..4ee70c35e 100644 --- a/.gitignore +++ b/.gitignore @@ -11,6 +11,7 @@ dist/ # IDEs .idea +.vscode # Decrypted private key we do not want to commit .github/OSBotify-private-key.asc diff --git a/lib/withOnyx.js b/lib/withOnyx.js index 976152848..bbd610090 100644 --- a/lib/withOnyx.js +++ b/lib/withOnyx.js @@ -92,12 +92,12 @@ export default function (mapOnyxToState, shouldDelayUpdates = false) { this.checkEvictableKeys(); } - componentDidUpdate(prevProps) { + componentDidUpdate(prevProps, prevState) { // If any of the mappings use data from the props, then when the props change, all the // connections need to be reconnected with the new props _.each(mapOnyxToState, (mapping, propertyName) => { - const previousKey = Str.result(mapping.key, prevProps); - const newKey = Str.result(mapping.key, this.props); + const previousKey = Str.result(mapping.key, {...prevProps, ...prevState}); + const newKey = Str.result(mapping.key, {...this.props, ...this.state}); if (previousKey !== newKey) { Onyx.disconnect(this.activeConnectionIDs[previousKey], previousKey); delete this.activeConnectionIDs[previousKey]; @@ -238,7 +238,7 @@ export default function (mapOnyxToState, shouldDelayUpdates = false) { * component */ connectMappingToOnyx(mapping, statePropertyName) { - const key = Str.result(mapping.key, this.props); + const key = Str.result(mapping.key, {...this.props, ...this.state}); // eslint-disable-next-line rulesdir/prefer-onyx-connect-in-libs this.activeConnectionIDs[key] = Onyx.connect({ diff --git a/tests/unit/withOnyxTest.js b/tests/unit/withOnyxTest.js index d4b00894a..1f510ae1c 100644 --- a/tests/unit/withOnyxTest.js +++ b/tests/unit/withOnyxTest.js @@ -255,10 +255,11 @@ describe('withOnyxTest', () => { key: `${ONYX_KEYS.COLLECTION.TEST_KEY}${collectionItemID}`, }, testThing: { - key: ({testObject}) => `${ONYX_KEYS.COLLECTION.RELATED_KEY}${testObject.id}`, + key: ({testObject}) => `${ONYX_KEYS.COLLECTION.RELATED_KEY}${(testObject && testObject.id) || 0}`, }, })(ViewWithCollections); render(); + return waitForPromisesToResolve(); }) .then(() => { expect(onRender).toHaveBeenLastCalledWith({ From 1f5c9b4fdfae5fc46dff43fd4a8a38a803cf31f7 Mon Sep 17 00:00:00 2001 From: Tim Golen Date: Thu, 21 Sep 2023 06:38:38 +0800 Subject: [PATCH 15/33] Remove unused import --- tests/unit/withOnyxTest.js | 1 - 1 file changed, 1 deletion(-) diff --git a/tests/unit/withOnyxTest.js b/tests/unit/withOnyxTest.js index 1f510ae1c..f3c76cfd0 100644 --- a/tests/unit/withOnyxTest.js +++ b/tests/unit/withOnyxTest.js @@ -5,7 +5,6 @@ import Onyx, {withOnyx} from '../../lib'; import ViewWithText from '../components/ViewWithText'; import ViewWithCollections from '../components/ViewWithCollections'; import waitForPromisesToResolve from '../utils/waitForPromisesToResolve'; -import compose from '../../lib/compose'; import ViewWithObject from '../components/ViewWithObject'; const ONYX_KEYS = { From 1d5b945fe09cd2bb7c46ee891acf0452e3ea3f31 Mon Sep 17 00:00:00 2001 From: Tim Golen Date: Thu, 21 Sep 2023 07:10:21 +0800 Subject: [PATCH 16/33] Filter out unrelated data before passing state --- lib/withOnyx.js | 18 +++++++++++++----- 1 file changed, 13 insertions(+), 5 deletions(-) diff --git a/lib/withOnyx.js b/lib/withOnyx.js index bbd610090..e9ea70977 100644 --- a/lib/withOnyx.js +++ b/lib/withOnyx.js @@ -93,15 +93,21 @@ export default function (mapOnyxToState, shouldDelayUpdates = false) { } componentDidUpdate(prevProps, prevState) { + // When the state is passed to the key functions with Str.result(), omit anything + // from state that was not part of the mapped keys. + const mappingPropNames = _.pluck(mapOnyxToState, 'key'); + const prevStateOnlyWithOnyxData = _.pick(prevState, mappingPropNames); + const stateOnlyWithOnyxData = _.pick(this.state, mappingPropNames); + // If any of the mappings use data from the props, then when the props change, all the // connections need to be reconnected with the new props - _.each(mapOnyxToState, (mapping, propertyName) => { - const previousKey = Str.result(mapping.key, {...prevProps, ...prevState}); - const newKey = Str.result(mapping.key, {...this.props, ...this.state}); + _.each(mapOnyxToState, (mapping, propName) => { + const previousKey = Str.result(mapping.key, {...prevProps, prevStateOnlyWithOnyxData}); + const newKey = Str.result(mapping.key, {...this.props, stateOnlyWithOnyxData}); if (previousKey !== newKey) { Onyx.disconnect(this.activeConnectionIDs[previousKey], previousKey); delete this.activeConnectionIDs[previousKey]; - this.connectMappingToOnyx(mapping, propertyName); + this.connectMappingToOnyx(mapping, propName); } }); this.checkEvictableKeys(); @@ -238,7 +244,9 @@ export default function (mapOnyxToState, shouldDelayUpdates = false) { * component */ connectMappingToOnyx(mapping, statePropertyName) { - const key = Str.result(mapping.key, {...this.props, ...this.state}); + const mappingPropNames = _.pluck(mapOnyxToState, 'key'); + const stateOnlyWithOnyxData = _.pick(this.state, mappingPropNames); + const key = Str.result(mapping.key, {...this.props, stateOnlyWithOnyxData}); // eslint-disable-next-line rulesdir/prefer-onyx-connect-in-libs this.activeConnectionIDs[key] = Onyx.connect({ From ab062880383d09d179f9a9cfff6e5b05e0e0895e Mon Sep 17 00:00:00 2001 From: Tim Golen Date: Thu, 21 Sep 2023 08:23:15 +0800 Subject: [PATCH 17/33] Add tests for nested dependencies and filter the state before passing --- lib/withOnyx.js | 18 +++++++----- tests/unit/withOnyxTest.js | 59 ++++++++++++++++++++++++++++++++------ 2 files changed, 61 insertions(+), 16 deletions(-) diff --git a/lib/withOnyx.js b/lib/withOnyx.js index e9ea70977..01221b7a0 100644 --- a/lib/withOnyx.js +++ b/lib/withOnyx.js @@ -92,18 +92,17 @@ export default function (mapOnyxToState, shouldDelayUpdates = false) { this.checkEvictableKeys(); } - componentDidUpdate(prevProps, prevState) { + componentDidUpdate() { // When the state is passed to the key functions with Str.result(), omit anything // from state that was not part of the mapped keys. - const mappingPropNames = _.pluck(mapOnyxToState, 'key'); - const prevStateOnlyWithOnyxData = _.pick(prevState, mappingPropNames); + const mappingPropNames = _.keys(mapOnyxToState); const stateOnlyWithOnyxData = _.pick(this.state, mappingPropNames); // If any of the mappings use data from the props, then when the props change, all the // connections need to be reconnected with the new props _.each(mapOnyxToState, (mapping, propName) => { - const previousKey = Str.result(mapping.key, {...prevProps, prevStateOnlyWithOnyxData}); - const newKey = Str.result(mapping.key, {...this.props, stateOnlyWithOnyxData}); + const previousKey = mapping.previousKey; + const newKey = Str.result(mapping.key, {...this.props, ...stateOnlyWithOnyxData}); if (previousKey !== newKey) { Onyx.disconnect(this.activeConnectionIDs[previousKey], previousKey); delete this.activeConnectionIDs[previousKey]; @@ -244,9 +243,14 @@ export default function (mapOnyxToState, shouldDelayUpdates = false) { * component */ connectMappingToOnyx(mapping, statePropertyName) { - const mappingPropNames = _.pluck(mapOnyxToState, 'key'); + const mappingPropNames = _.keys(mapOnyxToState); const stateOnlyWithOnyxData = _.pick(this.state, mappingPropNames); - const key = Str.result(mapping.key, {...this.props, stateOnlyWithOnyxData}); + const key = Str.result(mapping.key, {...this.props, ...stateOnlyWithOnyxData}); + + // Remember the previous key so that if it ever changes, the component will reconnect to Onyx + // in componentDidUpdate + // eslint-disable-next-line no-param-reassign + mapOnyxToState[statePropertyName].previousKey = key; // eslint-disable-next-line rulesdir/prefer-onyx-connect-in-libs this.activeConnectionIDs[key] = Onyx.connect({ diff --git a/tests/unit/withOnyxTest.js b/tests/unit/withOnyxTest.js index f3c76cfd0..c6ee21406 100644 --- a/tests/unit/withOnyxTest.js +++ b/tests/unit/withOnyxTest.js @@ -11,7 +11,10 @@ const ONYX_KEYS = { TEST_KEY: 'test', COLLECTION: { TEST_KEY: 'test_', - RELATED_KEY: 'related_', + STATIC: 'static_', + DEPENDS_ON_STATIC: 'dependsOnStatic_', + DEPENDS_ON_DEPENDS_ON_STATIC: 'dependsOnDependsOnStatic_', + DEPENDS_ON_DEPENDS_ON_DEPENDS_ON_STATIC: 'dependsOnDependsOnDependsOnStatic_', }, SIMPLE_KEY: 'simple', SIMPLE_KEY_2: 'simple2', @@ -242,27 +245,65 @@ describe('withOnyxTest', () => { }); it('should pass a prop from one connected component to another', () => { - const collectionItemID = 1; const onRender = jest.fn(); const markReadyForHydration = jest.fn(); - Onyx.mergeCollection(ONYX_KEYS.COLLECTION.TEST_KEY, {test_1: {id: 1}}); - Onyx.mergeCollection(ONYX_KEYS.COLLECTION.RELATED_KEY, {related_1: 'Test'}); + + // Given three collections with multiple items in each + Onyx.mergeCollection(ONYX_KEYS.COLLECTION.STATIC, { + static_1: {name: 'Static 1', id: 1}, + static_2: {name: 'Static 2', id: 2}, + }); + + // And one collection will depends on data being loaded from the static collection + Onyx.mergeCollection(ONYX_KEYS.COLLECTION.DEPENDS_ON_STATIC, { + dependsOnStatic_1: {name: 'dependsOnStatic 1', id: 3}, + dependsOnStatic_2: {name: 'dependsOnStatic 2', id: 4}, + }); + + // And one collection will depend on the data being loaded from the collection that depends on the static collection (multiple nested dependencies) + Onyx.mergeCollection(ONYX_KEYS.COLLECTION.DEPENDS_ON_DEPENDS_ON_STATIC, { + dependsOnDependsOnStatic_3: {name: 'dependsOnDependsOnStatic 1', id: 5}, + dependsOnDependsOnStatic_4: {name: 'dependsOnDependsOnStatic 2', id: 6}, + }); + + // And another collection with one more layer of dependency just to prove it works + Onyx.mergeCollection(ONYX_KEYS.COLLECTION.DEPENDS_ON_DEPENDS_ON_DEPENDS_ON_STATIC, { + dependsOnDependsOnDependsOnStatic_5: {name: 'dependsOnDependsOnDependsOnStatic 1'}, + dependsOnDependsOnDependsOnStatic_6: {name: 'dependsOnDependsOnDependsOnStatic 2'}, + }); + + // When a component is rendered using withOnyx and several nested dependencies on the keys return waitForPromisesToResolve() .then(() => { const TestComponentWithOnyx = withOnyx({ - testObject: { - key: `${ONYX_KEYS.COLLECTION.TEST_KEY}${collectionItemID}`, + staticObject: { + key: `${ONYX_KEYS.COLLECTION.STATIC}1`, }, - testThing: { - key: ({testObject}) => `${ONYX_KEYS.COLLECTION.RELATED_KEY}${(testObject && testObject.id) || 0}`, + dependentObject: { + key: ({staticObject}) => `${ONYX_KEYS.COLLECTION.DEPENDS_ON_STATIC}${(staticObject && staticObject.id) || 0}`, + }, + multiDependentObject: { + key: ({dependentObject}) => `${ONYX_KEYS.COLLECTION.DEPENDS_ON_DEPENDS_ON_STATIC}${(dependentObject && dependentObject.id) || 0}`, + }, + extremeMultiDependentObject: { + key: ({multiDependentObject}) => `${ONYX_KEYS.COLLECTION.DEPENDS_ON_DEPENDS_ON_DEPENDS_ON_STATIC}${(multiDependentObject && multiDependentObject.id) || 0}`, }, })(ViewWithCollections); render(); return waitForPromisesToResolve(); }) + + // Then all of the data gets properly loaded into the component as expected with the nested dependencies resolved .then(() => { expect(onRender).toHaveBeenLastCalledWith({ - collections: {}, markReadyForHydration, onRender, testObject: {id: 1}, testThing: 'Test', + markReadyForHydration, + onRender, + collections: {}, + testObject: {isDefaultProp: true}, + staticObject: {name: 'Static 1', id: 1}, + dependentObject: {name: 'dependsOnStatic 1', id: 3}, + multiDependentObject: {name: 'dependsOnDependsOnStatic 1', id: 5}, + extremeMultiDependentObject: {name: 'dependsOnDependsOnDependsOnStatic 1'}, }); }); }); From 0b2d29b3937c253404ec452ab98a2a8a9be42d94 Mon Sep 17 00:00:00 2001 From: Tim Golen Date: Thu, 21 Sep 2023 08:35:47 +0800 Subject: [PATCH 18/33] DRY up method --- lib/withOnyx.js | 18 ++++++++++++------ 1 file changed, 12 insertions(+), 6 deletions(-) diff --git a/lib/withOnyx.js b/lib/withOnyx.js index 01221b7a0..58e2a87e6 100644 --- a/lib/withOnyx.js +++ b/lib/withOnyx.js @@ -20,6 +20,15 @@ function getDisplayName(component) { return component.displayName || component.name || 'Component'; } +/** + * Removes all the keys from state that are unrelated to the onyx data being mapped to the component. + * + * @param {Object} state of the component + * @param {Object} onyxToStateMapping the object holding all of the mapping configuration for the component + * @returns {Object} + */ +const getOnyxDataFromState = (state, onyxToStateMapping) => _.pick(state, _.keys(onyxToStateMapping)); + export default function (mapOnyxToState, shouldDelayUpdates = false) { // A list of keys that must be present in tempState before we can render the WrappedComponent const requiredKeysForInit = _.chain(mapOnyxToState) @@ -95,14 +104,13 @@ export default function (mapOnyxToState, shouldDelayUpdates = false) { componentDidUpdate() { // When the state is passed to the key functions with Str.result(), omit anything // from state that was not part of the mapped keys. - const mappingPropNames = _.keys(mapOnyxToState); - const stateOnlyWithOnyxData = _.pick(this.state, mappingPropNames); + const onyxDataFromState = getOnyxDataFromState(this.state, mapOnyxToState); // If any of the mappings use data from the props, then when the props change, all the // connections need to be reconnected with the new props _.each(mapOnyxToState, (mapping, propName) => { const previousKey = mapping.previousKey; - const newKey = Str.result(mapping.key, {...this.props, ...stateOnlyWithOnyxData}); + const newKey = Str.result(mapping.key, {...this.props, ...onyxDataFromState}); if (previousKey !== newKey) { Onyx.disconnect(this.activeConnectionIDs[previousKey], previousKey); delete this.activeConnectionIDs[previousKey]; @@ -243,9 +251,7 @@ export default function (mapOnyxToState, shouldDelayUpdates = false) { * component */ connectMappingToOnyx(mapping, statePropertyName) { - const mappingPropNames = _.keys(mapOnyxToState); - const stateOnlyWithOnyxData = _.pick(this.state, mappingPropNames); - const key = Str.result(mapping.key, {...this.props, ...stateOnlyWithOnyxData}); + const key = Str.result(mapping.key, {...this.props, ...getOnyxDataFromState(this.state, mapOnyxToState)}); // Remember the previous key so that if it ever changes, the component will reconnect to Onyx // in componentDidUpdate From fc750636a61865bc13f143b0c6585ef7b5b0c9ea Mon Sep 17 00:00:00 2001 From: OSBotify Date: Thu, 21 Sep 2023 04:18:36 +0000 Subject: [PATCH 19/33] 1.0.85 --- package-lock.json | 4 ++-- package.json | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/package-lock.json b/package-lock.json index 810bbd625..71124035a 100644 --- a/package-lock.json +++ b/package-lock.json @@ -1,12 +1,12 @@ { "name": "react-native-onyx", - "version": "1.0.84", + "version": "1.0.85", "lockfileVersion": 2, "requires": true, "packages": { "": { "name": "react-native-onyx", - "version": "1.0.84", + "version": "1.0.85", "license": "MIT", "dependencies": { "ascii-table": "0.0.9", diff --git a/package.json b/package.json index 456cba28b..bdefd612b 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "react-native-onyx", - "version": "1.0.84", + "version": "1.0.85", "author": "Expensify, Inc.", "homepage": "https://expensify.com", "description": "State management for React Native", From 54ae2058062c66cdc356992966c6ba880a3f7a25 Mon Sep 17 00:00:00 2001 From: Christoph Pader Date: Thu, 21 Sep 2023 08:52:48 +0200 Subject: [PATCH 20/33] fix: don't ignore null values --- lib/Onyx.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/Onyx.js b/lib/Onyx.js index ff2a843a5..afc834c91 100644 --- a/lib/Onyx.js +++ b/lib/Onyx.js @@ -1090,7 +1090,7 @@ function applyMerge(existingValue, changes) { // There might be nullish values in a batch of changes that we want to ignore when merging objects, // because otherwise they overwrite the previous changes in the batch. // Therefore, we filter out nullish values before merging. - const changesWithoutNullishValues = _.filter(changes, change => !_.isNull(change) && !_.isUndefined(change)); + const changesWithoutNullishValues = _.filter(changes, change => !_.isUndefined(change)); // Object values are then merged one after the other return _.reduce(changesWithoutNullishValues, (modifiedData, change) => utils.fastMerge(modifiedData, change), From 7bb465733c869990a62030a6d6d8bd46e093e14f Mon Sep 17 00:00:00 2001 From: Christoph Pader Date: Thu, 21 Sep 2023 08:53:59 +0200 Subject: [PATCH 21/33] fix: remove unneccesary comment --- lib/utils.js | 2 -- 1 file changed, 2 deletions(-) diff --git a/lib/utils.js b/lib/utils.js index 140057125..72204584a 100644 --- a/lib/utils.js +++ b/lib/utils.js @@ -65,8 +65,6 @@ function mergeObject(target, source) { * @returns {Object|Array} */ function fastMerge(target, source) { - // lodash adds a small overhead so we don't use it here - // eslint-disable-next-line rulesdir/prefer-underscore-method if (_.isArray(source) || _.isNull(source) || _.isUndefined(source)) { return source; } From 504634920dedee1a7b338dddeeecd50e8bada2e2 Mon Sep 17 00:00:00 2001 From: Christoph Pader Date: Thu, 21 Sep 2023 08:55:09 +0200 Subject: [PATCH 22/33] fix: update comment --- lib/Onyx.js | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/lib/Onyx.js b/lib/Onyx.js index afc834c91..ae2f283b9 100644 --- a/lib/Onyx.js +++ b/lib/Onyx.js @@ -1087,9 +1087,10 @@ function applyMerge(existingValue, changes) { } if (_.some(changes, _.isObject)) { - // There might be nullish values in a batch of changes that we want to ignore when merging objects, + // There might be undefined values in a batch of changes that we want to ignore when merging objects, // because otherwise they overwrite the previous changes in the batch. - // Therefore, we filter out nullish values before merging. + // Values of null will still overwrite the existing value + // Therefore, we filter out undefined values before merging. const changesWithoutNullishValues = _.filter(changes, change => !_.isUndefined(change)); // Object values are then merged one after the other From d8b0573383560403f0fed8caa95bd54d7c2d1ffa Mon Sep 17 00:00:00 2001 From: Christoph Pader Date: Thu, 21 Sep 2023 09:05:37 +0200 Subject: [PATCH 23/33] fix: remove fastMerge.js and update comment --- lib/fastMerge.js | 68 ------------------------------------------------ lib/utils.js | 3 +++ 2 files changed, 3 insertions(+), 68 deletions(-) delete mode 100644 lib/fastMerge.js diff --git a/lib/fastMerge.js b/lib/fastMerge.js deleted file mode 100644 index 753dd6354..000000000 --- a/lib/fastMerge.js +++ /dev/null @@ -1,68 +0,0 @@ -import _ from 'underscore'; - -// Mostly copied from https://medium.com/@lubaka.a/how-to-remove-lodash-performance-improvement-b306669ad0e1 - -/** - * @param {mixed} val - * @returns {boolean} -*/ -function isMergeableObject(val) { - const nonNullObject = val != null ? typeof val === 'object' : false; - return (nonNullObject - && Object.prototype.toString.call(val) !== '[object RegExp]' - && Object.prototype.toString.call(val) !== '[object Date]'); -} - -/** - * @param {Object} target - * @param {Object} source - * @returns {Object} -*/ -function mergeObject(target, source) { - const destination = {}; - if (isMergeableObject(target)) { - // lodash adds a small overhead so we don't use it here - // eslint-disable-next-line rulesdir/prefer-underscore-method - const targetKeys = Object.keys(target); - for (let i = 0; i < targetKeys.length; ++i) { - const key = targetKeys[i]; - destination[key] = target[key]; - } - } - - // lodash adds a small overhead so we don't use it here - // eslint-disable-next-line rulesdir/prefer-underscore-method - const sourceKeys = Object.keys(source); - for (let i = 0; i < sourceKeys.length; ++i) { - const key = sourceKeys[i]; - if (source[key] === undefined) { - // eslint-disable-next-line no-continue - continue; - } - if (!isMergeableObject(source[key]) || !target[key]) { - destination[key] = source[key]; - } else { - // eslint-disable-next-line no-use-before-define - destination[key] = fastMerge(target[key], source[key]); - } - } - - return destination; -} - -/** - * @param {Object|Array} target - * @param {Object|Array} source - * @returns {Object|Array} -*/ -function fastMerge(target, source) { - // We have to ignore arrays and nullish values here, - // otherwise "mergeObject" will throw an error, - // because it expects an object as "source" - if (_.isArray(source) || _.isNull(source) || _.isUndefined(source)) { - return source; - } - return mergeObject(target, source); -} - -export default fastMerge; diff --git a/lib/utils.js b/lib/utils.js index 72204584a..bdc2378a5 100644 --- a/lib/utils.js +++ b/lib/utils.js @@ -65,6 +65,9 @@ function mergeObject(target, source) { * @returns {Object|Array} */ function fastMerge(target, source) { + // We have to ignore arrays and nullish values here, + // otherwise "mergeObject" will throw an error, + // because it expects an object as "source" if (_.isArray(source) || _.isNull(source) || _.isUndefined(source)) { return source; } From 45ecede92f22df47f94ddee60958248c7ea6cc19 Mon Sep 17 00:00:00 2001 From: Christoph Pader Date: Thu, 21 Sep 2023 09:07:47 +0200 Subject: [PATCH 24/33] fix: update version --- package-lock.json | 4 ++-- package.json | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/package-lock.json b/package-lock.json index 71124035a..810bbd625 100644 --- a/package-lock.json +++ b/package-lock.json @@ -1,12 +1,12 @@ { "name": "react-native-onyx", - "version": "1.0.85", + "version": "1.0.84", "lockfileVersion": 2, "requires": true, "packages": { "": { "name": "react-native-onyx", - "version": "1.0.85", + "version": "1.0.84", "license": "MIT", "dependencies": { "ascii-table": "0.0.9", diff --git a/package.json b/package.json index bdefd612b..456cba28b 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "react-native-onyx", - "version": "1.0.85", + "version": "1.0.84", "author": "Expensify, Inc.", "homepage": "https://expensify.com", "description": "State management for React Native", From 589b182e89f0af9a1e4ef3fda03f0979700f2169 Mon Sep 17 00:00:00 2001 From: Christoph Pader Date: Thu, 21 Sep 2023 09:13:35 +0200 Subject: [PATCH 25/33] feat: add additional test --- tests/unit/onyxTest.js | 25 +++++++++++++++++++++++++ 1 file changed, 25 insertions(+) diff --git a/tests/unit/onyxTest.js b/tests/unit/onyxTest.js index 138ff038e..75dd3d084 100644 --- a/tests/unit/onyxTest.js +++ b/tests/unit/onyxTest.js @@ -876,4 +876,29 @@ describe('Onyx', () => { }); }); }); + + it('should merge an object with null and overwrite the value', () => { + let testKeyValue; + + connectionID = Onyx.connect({ + key: ONYX_KEYS.TEST_KEY, + initWithStoredValues: false, + callback: (value) => { + testKeyValue = value; + }, + }); + + return Onyx.set(ONYX_KEYS.TEST_KEY, {test1: 'test1'}) + .then(() => { + expect(testKeyValue).toEqual({test1: 'test1'}); + Onyx.merge(ONYX_KEYS.TEST_KEY, null); + Onyx.merge(ONYX_KEYS.TEST_KEY, {test2: 'test2'}); + return waitForPromisesToResolve(); + }) + .then(() => { + expect(testKeyValue).toEqual({ + test2: 'test2', + }); + }); + }); }); From d3868eaa3cf93bed8851fc634d04a4be3fbcd686 Mon Sep 17 00:00:00 2001 From: Christoph Pader Date: Thu, 21 Sep 2023 09:16:21 +0200 Subject: [PATCH 26/33] fix: update variable name --- lib/Onyx.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/Onyx.js b/lib/Onyx.js index ae2f283b9..9521af099 100644 --- a/lib/Onyx.js +++ b/lib/Onyx.js @@ -1091,10 +1091,10 @@ function applyMerge(existingValue, changes) { // because otherwise they overwrite the previous changes in the batch. // Values of null will still overwrite the existing value // Therefore, we filter out undefined values before merging. - const changesWithoutNullishValues = _.filter(changes, change => !_.isUndefined(change)); + const changesWithoutUndefined = _.filter(changes, change => !_.isUndefined(change)); // Object values are then merged one after the other - return _.reduce(changesWithoutNullishValues, (modifiedData, change) => utils.fastMerge(modifiedData, change), + return _.reduce(changesWithoutUndefined, (modifiedData, change) => utils.fastMerge(modifiedData, change), existingValue || {}); } From 813dc4142dccfe2064def083a47136a365bcfee2 Mon Sep 17 00:00:00 2001 From: Christoph Pader Date: Thu, 21 Sep 2023 09:24:55 +0200 Subject: [PATCH 27/33] fix: null value not overwriting existing value --- lib/Onyx.js | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/lib/Onyx.js b/lib/Onyx.js index 9521af099..f312dc7e8 100644 --- a/lib/Onyx.js +++ b/lib/Onyx.js @@ -1146,8 +1146,12 @@ function merge(key, changes) { delete mergeQueue[key]; delete mergeQueuePromise[key]; + // If the mergeQueue included a null value, we want to overwrite the 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 overwriteExistingValue = _.includes(mergeQueue[key], null); + // After that we merge the batched changes with the existing value - const modifiedData = utils.removeNullObjectValues(applyMerge(existingValue, [batchedChanges])); + const modifiedData = utils.removeNullObjectValues(applyMerge(overwriteExistingValue ? undefined : existingValue, [batchedChanges])); // On native platforms we use SQLite which utilises JSON_PATCH to merge changes. // JSON_PATCH generally removes top-level nullish values from the stored object. From 0122acfdfbfd5895945d5155c23046426fd5ca83 Mon Sep 17 00:00:00 2001 From: Christoph Pader Date: Thu, 21 Sep 2023 09:28:48 +0200 Subject: [PATCH 28/33] fix: move overwriteExistingValue above merge queue clearing --- lib/Onyx.js | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/lib/Onyx.js b/lib/Onyx.js index f312dc7e8..4fe87d01e 100644 --- a/lib/Onyx.js +++ b/lib/Onyx.js @@ -1142,14 +1142,14 @@ function merge(key, changes) { return remove(key); } - // Clean up the write queue, so we don't apply these changes again - delete mergeQueue[key]; - delete mergeQueuePromise[key]; - // If the mergeQueue included a null value, we want to overwrite the 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 overwriteExistingValue = _.includes(mergeQueue[key], null); + // Clean up the write queue, so we don't apply these changes again + delete mergeQueue[key]; + delete mergeQueuePromise[key]; + // After that we merge the batched changes with the existing value const modifiedData = utils.removeNullObjectValues(applyMerge(overwriteExistingValue ? undefined : existingValue, [batchedChanges])); From d8a4fc696139d5b268327e36576f6cd0843b2c6e Mon Sep 17 00:00:00 2001 From: Christoph Pader Date: Thu, 21 Sep 2023 10:19:03 +0200 Subject: [PATCH 29/33] update test --- tests/unit/onyxTest.js | 2 ++ 1 file changed, 2 insertions(+) diff --git a/tests/unit/onyxTest.js b/tests/unit/onyxTest.js index 75dd3d084..48bee62a9 100644 --- a/tests/unit/onyxTest.js +++ b/tests/unit/onyxTest.js @@ -893,11 +893,13 @@ describe('Onyx', () => { expect(testKeyValue).toEqual({test1: 'test1'}); Onyx.merge(ONYX_KEYS.TEST_KEY, null); Onyx.merge(ONYX_KEYS.TEST_KEY, {test2: 'test2'}); + Onyx.merge(ONYX_KEYS.TEST_KEY, {test3: 'test3'}); return waitForPromisesToResolve(); }) .then(() => { expect(testKeyValue).toEqual({ test2: 'test2', + test3: 'test3', }); }); }); From dcf98623afca57a32cc96c2eb53ec5a622ed3c60 Mon Sep 17 00:00:00 2001 From: Christoph Pader Date: Fri, 22 Sep 2023 08:32:05 +0200 Subject: [PATCH 30/33] fix: improve null batching --- lib/Onyx.js | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/lib/Onyx.js b/lib/Onyx.js index 4fe87d01e..814e71d3d 100644 --- a/lib/Onyx.js +++ b/lib/Onyx.js @@ -1142,16 +1142,17 @@ function merge(key, changes) { return remove(key); } - // If the mergeQueue included a null value, we want to overwrite the existing value + // The presence of a `null` in the merge queue instructs us to drop the 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 overwriteExistingValue = _.includes(mergeQueue[key], null); + const shouldOverwriteExistingValue = _.includes(mergeQueue[key], null); // Clean up the write queue, so we don't apply these changes again delete mergeQueue[key]; delete mergeQueuePromise[key]; // After that we merge the batched changes with the existing value - const modifiedData = utils.removeNullObjectValues(applyMerge(overwriteExistingValue ? undefined : existingValue, [batchedChanges])); + const updatedValue = shouldOverwriteExistingValue ? batchedChanges : applyMerge(existingValue, [batchedChanges]); + const modifiedData = utils.removeNullObjectValues(updatedValue); // On native platforms we use SQLite which utilises JSON_PATCH to merge changes. // JSON_PATCH generally removes top-level nullish values from the stored object. From 0086b913957f31bc91876ebbfa87be88227cdeff Mon Sep 17 00:00:00 2001 From: Christoph Pader Date: Fri, 22 Sep 2023 09:23:27 +0200 Subject: [PATCH 31/33] fix: ignore undefined values in the first place --- lib/Onyx.js | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/lib/Onyx.js b/lib/Onyx.js index 814e71d3d..5596ab57a 100644 --- a/lib/Onyx.js +++ b/lib/Onyx.js @@ -1,6 +1,6 @@ /* eslint-disable no-continue */ import {deepEqual} from 'fast-equals'; -import _ from 'underscore'; +import _, {isUndefined} from 'underscore'; import * as Logger from './Logger'; import cache from './OnyxCache'; import * as Str from './Str'; @@ -1087,14 +1087,8 @@ function applyMerge(existingValue, changes) { } if (_.some(changes, _.isObject)) { - // There might be undefined values in a batch of changes that we want to ignore when merging objects, - // because otherwise they overwrite the previous changes in the batch. - // Values of null will still overwrite the existing value - // Therefore, we filter out undefined values before merging. - const changesWithoutUndefined = _.filter(changes, change => !_.isUndefined(change)); - // Object values are then merged one after the other - return _.reduce(changesWithoutUndefined, (modifiedData, change) => utils.fastMerge(modifiedData, change), + return _.reduce(changes, (modifiedData, change) => utils.fastMerge(modifiedData, change), existingValue || {}); } @@ -1124,6 +1118,12 @@ function applyMerge(existingValue, changes) { * @returns {Promise} */ function merge(key, changes) { + // Top-level undefined values are ignored + // Therefore we need to prevent adding them to the merge queue + if (_.isUndefined(changes)) { + return mergeQueue[key] ? mergeQueuePromise[key] : Promise.resolve(); + } + // Merge attempts are batched together. The delta should be applied after a single call to get() to prevent a race condition. // Using the initial value from storage in subsequent merge attempts will lead to an incorrect final merged value. if (mergeQueue[key]) { From cb6d4cc832ad90130b522e6c2df5e1395507e6a9 Mon Sep 17 00:00:00 2001 From: Christoph Pader Date: Fri, 22 Sep 2023 09:27:46 +0200 Subject: [PATCH 32/33] fix: update onyx test --- tests/unit/onyxTest.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/unit/onyxTest.js b/tests/unit/onyxTest.js index 48bee62a9..ef2b49f94 100644 --- a/tests/unit/onyxTest.js +++ b/tests/unit/onyxTest.js @@ -202,7 +202,7 @@ describe('Onyx', () => { }); }); - it('should merge an object with undefined', () => { + it('should ignore top-level undefined values', () => { let testKeyValue; connectionID = Onyx.connect({ @@ -219,7 +219,7 @@ describe('Onyx', () => { return Onyx.merge(ONYX_KEYS.TEST_KEY, undefined); }) .then(() => { - expect(testKeyValue).toEqual(undefined); + expect(testKeyValue).toEqual({test1: 'test1'}); }); }); From 1d8ee345bbdda2f4945aa3398b2e99990228fa1d Mon Sep 17 00:00:00 2001 From: Christoph Pader Date: Fri, 22 Sep 2023 11:01:50 +0200 Subject: [PATCH 33/33] fix: remove unused import --- lib/Onyx.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/Onyx.js b/lib/Onyx.js index 5596ab57a..e5ff25171 100644 --- a/lib/Onyx.js +++ b/lib/Onyx.js @@ -1,6 +1,6 @@ /* eslint-disable no-continue */ import {deepEqual} from 'fast-equals'; -import _, {isUndefined} from 'underscore'; +import _ from 'underscore'; import * as Logger from './Logger'; import cache from './OnyxCache'; import * as Str from './Str';