diff --git a/lib/Onyx.js b/lib/Onyx.js index 2de92b240..da4131bd2 100644 --- a/lib/Onyx.js +++ b/lib/Onyx.js @@ -848,19 +848,33 @@ function evictStorageAndRetry(error, onyxMethod, ...args) { * * @param {String} key * @param {*} value + * @param {Boolean} hasChanged * @param {String} method */ -function broadcastUpdate(key, value, method) { +function broadcastUpdate(key, value, hasChanged, method) { // Logging properties only since values could be sensitive things we don't want to log Logger.logInfo(`${method}() called for key: ${key}${_.isObject(value) ? ` properties: ${_.keys(value).join(',')}` : ''}`); // Update subscribers if the cached value has changed, or when the subscriber specifically requires // all updates regardless of value changes (indicated by initWithStoredValues set to false). - const hasChanged = cache.hasValueChanged(key, value); - cache.set(key, value); + if (hasChanged) { + cache.set(key, value); + } else { + cache.addToAccessedKeys(key); + } + notifySubscribersOnNextTick(key, value, subscriber => hasChanged || subscriber.initWithStoredValues === false); } +/** + * @private + * @param {String} key + * @returns {Boolean} + */ +function hasPendingMergeForKey(key) { + return Boolean(mergeQueue[key]); +} + /** * Write a value to our store with the given key * @@ -874,8 +888,19 @@ function set(key, value) { return remove(key); } + if (hasPendingMergeForKey(key)) { + Logger.logAlert(`Onyx.set() called after Onyx.merge() for key: ${key}. It is recommended to use set() or merge() not both.`); + } + + const hasChanged = cache.hasValueChanged(key, value); + // This approach prioritizes fast UI changes without waiting for data to be stored in device storage. - broadcastUpdate(key, value, 'set'); + broadcastUpdate(key, value, hasChanged, 'set'); + + // If the value has not changed, calling Storage.setItem() would be redundant and a waste of performance, so return early instead. + if (!hasChanged) { + return Promise.resolve(); + } return Storage.setItem(key, value) .catch(error => evictStorageAndRetry(error, set, key, value)); @@ -918,11 +943,11 @@ function multiSet(data) { * Merges an array of changes with an existing value * * @private - * @param {Array<*>} changes Array of changes that should be applied to the existing value * @param {*} existingValue + * @param {Array<*>} changes Array of changes that should be applied to the existing value * @returns {*} */ -function applyMerge(changes, existingValue) { +function applyMerge(existingValue, changes) { const lastChange = _.last(changes); if (_.isArray(existingValue) || _.isArray(lastChange)) { @@ -931,14 +956,10 @@ function applyMerge(changes, existingValue) { if (_.isObject(existingValue) || _.every(changes, _.isObject)) { // Object values are merged one after the other - return _.reduce(changes, (modifiedData, change) => { - // lodash adds a small overhead so we don't use it here - // eslint-disable-next-line prefer-object-spread, rulesdir/prefer-underscore-method - const newData = Object.assign({}, fastMerge(modifiedData, change)); - - // Remove all first level keys that are explicitly set to null. - return _.omit(newData, value => _.isNull(value)); - }, existingValue || {}); + // 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), + existingValue || {}); } // If we have anything else we can't merge it so we'll @@ -979,17 +1000,30 @@ function merge(key, changes) { .then((existingValue) => { try { // 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) - const batchedChanges = applyMerge(mergeQueue[key]); + const batchedChanges = applyMerge(undefined, mergeQueue[key]); // 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 = applyMerge([batchedChanges], existingValue); + let modifiedData = applyMerge(existingValue, [batchedChanges]); + + // For objects, the key for null values needs to be removed from the object to ensure the value will get removed from storage completely. + // On native, SQLite will remove top-level keys that are null. To be consistent, we remove them on web too. + if (!_.isArray(modifiedData) && _.isObject(modifiedData)) { + modifiedData = _.omit(modifiedData, value => _.isNull(value)); + } + + const hasChanged = cache.hasValueChanged(key, modifiedData); // This approach prioritizes fast UI changes without waiting for data to be stored in device storage. - broadcastUpdate(key, modifiedData, 'merge'); + broadcastUpdate(key, modifiedData, hasChanged, 'merge'); + + // If the value has not changed, calling Storage.setItem() would be redundant and a waste of performance, so return early instead. + if (!hasChanged) { + return Promise.resolve(); + } return Storage.mergeItem(key, batchedChanges, modifiedData); } catch (error) { @@ -1000,15 +1034,6 @@ function merge(key, changes) { }); } -/** - * @private - * @param {String} key - * @returns {Boolean} - */ -function hasPendingMergeForKey(key) { - return Boolean(mergeQueue[key]); -} - /** * Merge user provided default key value pairs. * @private diff --git a/lib/storage/__mocks__/index.js b/lib/storage/__mocks__/index.js index 36362e148..fed1cb238 100644 --- a/lib/storage/__mocks__/index.js +++ b/lib/storage/__mocks__/index.js @@ -4,9 +4,8 @@ import fastMerge from '../../fastMerge'; let storageMapInternal = {}; const set = jest.fn((key, value) => { - // eslint-disable-next-line no-param-reassign storageMapInternal[key] = value; - return Promise.resolve(storageMapInternal[key]); + return Promise.resolve(value); }); const localForageMock = {