diff --git a/lib/Onyx.js b/lib/Onyx.js index d4d329e0f..e5ff25171 100644 --- a/lib/Onyx.js +++ b/lib/Onyx.js @@ -1082,14 +1082,12 @@ function multiSet(data) { function applyMerge(existingValue, changes) { const lastChange = _.last(changes); - if (_.isArray(existingValue) || _.isArray(lastChange)) { + if (_.isArray(lastChange)) { return lastChange; } - if (_.isObject(existingValue) || _.every(changes, _.isObject)) { - // 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 + if (_.some(changes, _.isObject)) { + // Object values are then merged one after the other return _.reduce(changes, (modifiedData, change) => utils.fastMerge(modifiedData, change), existingValue || {}); } @@ -1120,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]) { @@ -1134,12 +1138,21 @@ 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 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 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(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. diff --git a/lib/fastMerge.js b/lib/fastMerge.js deleted file mode 100644 index 2ddc5ad62..000000000 --- a/lib/fastMerge.js +++ /dev/null @@ -1,65 +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) { - 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 140057125..bdc2378a5 100644 --- a/lib/utils.js +++ b/lib/utils.js @@ -65,8 +65,9 @@ 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 + // 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; } diff --git a/tests/unit/onyxTest.js b/tests/unit/onyxTest.js index 7b6f5eb2b..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'}); }); }); @@ -848,4 +848,59 @@ 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(() => { + expect(testKeyValue).toEqual({ + test1: 'test1', test2: 'test2', test3: 'test3', test4: 'test4', + }); + }); + }); + + 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'}); + Onyx.merge(ONYX_KEYS.TEST_KEY, {test3: 'test3'}); + return waitForPromisesToResolve(); + }) + .then(() => { + expect(testKeyValue).toEqual({ + test2: 'test2', + test3: 'test3', + }); + }); + }); });