From b240d8c597bcc5ab339e7e1637ee20243a922d42 Mon Sep 17 00:00:00 2001 From: Christoph Pader Date: Mon, 24 Jul 2023 10:58:02 +0200 Subject: [PATCH 01/16] only set and merge if the value has changed --- lib/Onyx.js | 18 +++++++++++++++--- 1 file changed, 15 insertions(+), 3 deletions(-) diff --git a/lib/Onyx.js b/lib/Onyx.js index 2de92b240..a5f5238cf 100644 --- a/lib/Onyx.js +++ b/lib/Onyx.js @@ -849,6 +849,7 @@ function evictStorageAndRetry(error, onyxMethod, ...args) { * @param {String} key * @param {*} value * @param {String} method + * @returns {Boolean} */ function broadcastUpdate(key, value, method) { // Logging properties only since values could be sensitive things we don't want to log @@ -857,8 +858,15 @@ function broadcastUpdate(key, value, method) { // 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); + + return hasChanged; } /** @@ -875,7 +883,9 @@ function set(key, value) { } // This approach prioritizes fast UI changes without waiting for data to be stored in device storage. - broadcastUpdate(key, value, 'set'); + const hasChanged = broadcastUpdate(key, value, 'set'); + + if (hasChanged) { return Promise.resolve(); } return Storage.setItem(key, value) .catch(error => evictStorageAndRetry(error, set, key, value)); @@ -989,7 +999,9 @@ function merge(key, changes) { const modifiedData = applyMerge([batchedChanges], existingValue); // This approach prioritizes fast UI changes without waiting for data to be stored in device storage. - broadcastUpdate(key, modifiedData, 'merge'); + const hasChanged = broadcastUpdate(key, modifiedData, 'merge'); + + if (hasChanged) { return Promise.resolve(); } return Storage.mergeItem(key, batchedChanges, modifiedData); } catch (error) { From a4ab77a7a2bf680c96a0ccd9f5f381e364ec0944 Mon Sep 17 00:00:00 2001 From: Christoph Pader Date: Mon, 24 Jul 2023 11:37:07 +0200 Subject: [PATCH 02/16] remove unused eslint rule --- lib/storage/__mocks__/index.js | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) 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 = { From bc5ae6a7783e71b507ebb660e045e9b500669e55 Mon Sep 17 00:00:00 2001 From: Christoph Pader Date: Tue, 25 Jul 2023 12:52:09 +0200 Subject: [PATCH 03/16] fix: wrong condition --- lib/Onyx.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/Onyx.js b/lib/Onyx.js index a5f5238cf..333c7a6b4 100644 --- a/lib/Onyx.js +++ b/lib/Onyx.js @@ -885,7 +885,7 @@ function set(key, value) { // This approach prioritizes fast UI changes without waiting for data to be stored in device storage. const hasChanged = broadcastUpdate(key, value, 'set'); - if (hasChanged) { return Promise.resolve(); } + if (!hasChanged) { return Promise.resolve(); } return Storage.setItem(key, value) .catch(error => evictStorageAndRetry(error, set, key, value)); @@ -1001,7 +1001,7 @@ function merge(key, changes) { // This approach prioritizes fast UI changes without waiting for data to be stored in device storage. const hasChanged = broadcastUpdate(key, modifiedData, 'merge'); - if (hasChanged) { return Promise.resolve(); } + if (!hasChanged) { return Promise.resolve(); } return Storage.mergeItem(key, batchedChanges, modifiedData); } catch (error) { From 7722ed2a1f78c3c127725477f1e1bd3e16ed022d Mon Sep 17 00:00:00 2001 From: Christoph Pader Date: Tue, 25 Jul 2023 13:01:47 +0200 Subject: [PATCH 04/16] add comment for broadcastUpdate return value --- lib/Onyx.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/Onyx.js b/lib/Onyx.js index 333c7a6b4..f14668fe0 100644 --- a/lib/Onyx.js +++ b/lib/Onyx.js @@ -849,7 +849,7 @@ function evictStorageAndRetry(error, onyxMethod, ...args) { * @param {String} key * @param {*} value * @param {String} method - * @returns {Boolean} + * @returns {Boolean} whether the value for the given key has changed */ function broadcastUpdate(key, value, method) { // Logging properties only since values could be sensitive things we don't want to log From 120943e0a6cf64278da7356a7910b755b01fa20a Mon Sep 17 00:00:00 2001 From: Christoph Pader Date: Tue, 25 Jul 2023 13:16:24 +0200 Subject: [PATCH 05/16] add back log in set --- lib/Onyx.js | 22 +++++++++++++--------- 1 file changed, 13 insertions(+), 9 deletions(-) diff --git a/lib/Onyx.js b/lib/Onyx.js index f14668fe0..983005ee2 100644 --- a/lib/Onyx.js +++ b/lib/Onyx.js @@ -869,6 +869,15 @@ function broadcastUpdate(key, value, method) { return hasChanged; } +/** + * @private + * @param {String} key + * @returns {Boolean} + */ +function hasPendingMergeForKey(key) { + return Boolean(mergeQueue[key]); +} + /** * Write a value to our store with the given key * @@ -882,6 +891,10 @@ 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.`); + } + // This approach prioritizes fast UI changes without waiting for data to be stored in device storage. const hasChanged = broadcastUpdate(key, value, 'set'); @@ -1012,15 +1025,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 From 39adecf3c347d5de0d446eeccc74547d478e5a65 Mon Sep 17 00:00:00 2001 From: Christoph Pader Date: Tue, 25 Jul 2023 14:23:24 +0200 Subject: [PATCH 06/16] fix: null values may not be omitted when merging batched changes --- lib/Onyx.js | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-) diff --git a/lib/Onyx.js b/lib/Onyx.js index 983005ee2..f1780efd8 100644 --- a/lib/Onyx.js +++ b/lib/Onyx.js @@ -941,11 +941,12 @@ 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 + * @param {bool} omitNull Whether null top-level null values should be omitted * @returns {*} */ -function applyMerge(changes, existingValue) { +function applyMerge(existingValue, changes, omitNull = true) { const lastChange = _.last(changes); if (_.isArray(existingValue) || _.isArray(lastChange)) { @@ -960,7 +961,7 @@ function applyMerge(changes, existingValue) { const newData = Object.assign({}, fastMerge(modifiedData, change)); // Remove all first level keys that are explicitly set to null. - return _.omit(newData, value => _.isNull(value)); + return omitNull ? _.omit(newData, value => _.isNull(value)) : newData; }, existingValue || {}); } @@ -1002,14 +1003,18 @@ 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], false); // 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); + const modifiedData = applyMerge(existingValue, [batchedChanges]); + + console.log({ + key, existingValue, batchedChanges, modifiedData, + }); // This approach prioritizes fast UI changes without waiting for data to be stored in device storage. const hasChanged = broadcastUpdate(key, modifiedData, 'merge'); From d22b19d4407d7fe824aa3251de2dfd8bcc310b5c Mon Sep 17 00:00:00 2001 From: Christoph Pader Date: Tue, 25 Jul 2023 14:36:18 +0200 Subject: [PATCH 07/16] remove console.log --- lib/Onyx.js | 4 ---- 1 file changed, 4 deletions(-) diff --git a/lib/Onyx.js b/lib/Onyx.js index f1780efd8..986a4d113 100644 --- a/lib/Onyx.js +++ b/lib/Onyx.js @@ -1012,10 +1012,6 @@ function merge(key, changes) { // After that we merge the batched changes with the existing value const modifiedData = applyMerge(existingValue, [batchedChanges]); - console.log({ - key, existingValue, batchedChanges, modifiedData, - }); - // This approach prioritizes fast UI changes without waiting for data to be stored in device storage. const hasChanged = broadcastUpdate(key, modifiedData, 'merge'); From dd13cc1e88d9054b292147180595054e6324df5d Mon Sep 17 00:00:00 2001 From: Christoph Pader Date: Wed, 26 Jul 2023 10:53:22 +0200 Subject: [PATCH 08/16] Update lib/Onyx.js Co-authored-by: Tim Golen --- lib/Onyx.js | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/lib/Onyx.js b/lib/Onyx.js index 986a4d113..ce67da9ac 100644 --- a/lib/Onyx.js +++ b/lib/Onyx.js @@ -898,7 +898,9 @@ function set(key, value) { // This approach prioritizes fast UI changes without waiting for data to be stored in device storage. const hasChanged = broadcastUpdate(key, value, 'set'); - if (!hasChanged) { return Promise.resolve(); } + if (!hasChanged) { + return Promise.resolve(); + } return Storage.setItem(key, value) .catch(error => evictStorageAndRetry(error, set, key, value)); From 3cf17a6d33b021ada5918f7faf8d181083daa43c Mon Sep 17 00:00:00 2001 From: Christoph Pader Date: Wed, 26 Jul 2023 10:55:19 +0200 Subject: [PATCH 09/16] add comments --- lib/Onyx.js | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/lib/Onyx.js b/lib/Onyx.js index ce67da9ac..cd4886168 100644 --- a/lib/Onyx.js +++ b/lib/Onyx.js @@ -898,9 +898,10 @@ function set(key, value) { // This approach prioritizes fast UI changes without waiting for data to be stored in device storage. const hasChanged = broadcastUpdate(key, value, 'set'); - if (!hasChanged) { + // If the value has not changed then we don't need to write it to storage + if (!hasChanged) { return Promise.resolve(); - } + } return Storage.setItem(key, value) .catch(error => evictStorageAndRetry(error, set, key, value)); @@ -1017,7 +1018,10 @@ function merge(key, changes) { // This approach prioritizes fast UI changes without waiting for data to be stored in device storage. const hasChanged = broadcastUpdate(key, modifiedData, 'merge'); - if (!hasChanged) { return Promise.resolve(); } + // If the value has not changed then we don't need to write it to storage + if (!hasChanged) { + return Promise.resolve(); + } return Storage.mergeItem(key, batchedChanges, modifiedData); } catch (error) { From 5dcb829902487f29a4690522875c46a939bf82a0 Mon Sep 17 00:00:00 2001 From: Christoph Pader Date: Wed, 26 Jul 2023 11:00:40 +0200 Subject: [PATCH 10/16] pass hasChanged to broadcastUpdate function --- lib/Onyx.js | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/lib/Onyx.js b/lib/Onyx.js index cd4886168..73bb1fd90 100644 --- a/lib/Onyx.js +++ b/lib/Onyx.js @@ -848,16 +848,16 @@ function evictStorageAndRetry(error, onyxMethod, ...args) { * * @param {String} key * @param {*} value + * @param {Boolean} hasChanged * @param {String} method * @returns {Boolean} whether the value for the given key has changed */ -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); if (hasChanged) { cache.set(key, value); } else { @@ -895,8 +895,10 @@ function set(key, value) { 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. - const hasChanged = broadcastUpdate(key, value, 'set'); + broadcastUpdate(key, value, hasChanged, 'set'); // If the value has not changed then we don't need to write it to storage if (!hasChanged) { @@ -1015,8 +1017,10 @@ function merge(key, changes) { // After that we merge the batched changes with the existing value const modifiedData = applyMerge(existingValue, [batchedChanges]); + const hasChanged = cache.hasValueChanged(key, modifiedData); + // This approach prioritizes fast UI changes without waiting for data to be stored in device storage. - const hasChanged = broadcastUpdate(key, modifiedData, 'merge'); + broadcastUpdate(key, modifiedData, hasChanged, 'merge'); // If the value has not changed then we don't need to write it to storage if (!hasChanged) { From 91cc330fbfa55617e9e4f907c257708194b9b635 Mon Sep 17 00:00:00 2001 From: Christoph Pader Date: Wed, 26 Jul 2023 11:14:03 +0200 Subject: [PATCH 11/16] extract top-level null omiting --- lib/Onyx.js | 22 +++++++++++----------- 1 file changed, 11 insertions(+), 11 deletions(-) diff --git a/lib/Onyx.js b/lib/Onyx.js index 73bb1fd90..35e22afc8 100644 --- a/lib/Onyx.js +++ b/lib/Onyx.js @@ -951,7 +951,7 @@ function multiSet(data) { * @param {bool} omitNull Whether null top-level null values should be omitted * @returns {*} */ -function applyMerge(existingValue, changes, omitNull = true) { +function applyMerge(existingValue, changes) { const lastChange = _.last(changes); if (_.isArray(existingValue) || _.isArray(lastChange)) { @@ -960,14 +960,9 @@ function applyMerge(existingValue, changes, omitNull = true) { 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 omitNull ? _.omit(newData, value => _.isNull(value)) : newData; - }, existingValue || {}); + // lodash adds a small overhead so we don't use it here + return _.reduce(changes, (modifiedData, change) => ({...fastMerge(modifiedData, change)}), + existingValue || {}); } // If we have anything else we can't merge it so we'll @@ -1008,14 +1003,19 @@ 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(undefined, mergeQueue[key], false); + 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(existingValue, [batchedChanges]); + let modifiedData = applyMerge(existingValue, [batchedChanges]); + + // For objects, we want to remove all top level keys that are null + if (!_.isArray(modifiedData) && _.isObject(modifiedData)) { + modifiedData = _.omit(modifiedData, value => _.isNull(value)); + } const hasChanged = cache.hasValueChanged(key, modifiedData); From 3fc85a4e5877e0a0824fd21cf9472607c9a3b1bb Mon Sep 17 00:00:00 2001 From: Christoph Pader Date: Wed, 26 Jul 2023 21:25:09 +0200 Subject: [PATCH 12/16] update comments --- lib/Onyx.js | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/lib/Onyx.js b/lib/Onyx.js index 35e22afc8..323526720 100644 --- a/lib/Onyx.js +++ b/lib/Onyx.js @@ -900,7 +900,7 @@ function set(key, value) { // This approach prioritizes fast UI changes without waiting for data to be stored in device storage. broadcastUpdate(key, value, hasChanged, 'set'); - // If the value has not changed then we don't need to write it to storage + // 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(); } @@ -961,7 +961,8 @@ function applyMerge(existingValue, changes) { 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 - return _.reduce(changes, (modifiedData, change) => ({...fastMerge(modifiedData, change)}), + // eslint-disable-next-line prefer-object-spread, rulesdir/prefer-underscore-method + return _.reduce(changes, (modifiedData, change) => Object.assign({}, fastMerge(modifiedData, change)), existingValue || {}); } @@ -1012,7 +1013,8 @@ function merge(key, changes) { // After that we merge the batched changes with the existing value let modifiedData = applyMerge(existingValue, [batchedChanges]); - // For objects, we want to remove all top level keys that are null + // 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 keys 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)); } @@ -1022,7 +1024,7 @@ function merge(key, changes) { // This approach prioritizes fast UI changes without waiting for data to be stored in device storage. broadcastUpdate(key, modifiedData, hasChanged, 'merge'); - // If the value has not changed then we don't need to write it to storage + // 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(); } From c91227de15f81e6e7207b315028ec1a30e7cb61e Mon Sep 17 00:00:00 2001 From: Christoph Pader Date: Wed, 26 Jul 2023 22:16:37 +0200 Subject: [PATCH 13/16] 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 323526720..2032f49ae 100644 --- a/lib/Onyx.js +++ b/lib/Onyx.js @@ -1014,7 +1014,7 @@ function merge(key, changes) { 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 keys top-level keys that are null. To be consistent, we remove them on web too. + // 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)); } From 4f930ad07d717da1f17a20ce8effd54f18a5e304 Mon Sep 17 00:00:00 2001 From: Christoph Pader Date: Wed, 26 Jul 2023 22:43:27 +0200 Subject: [PATCH 14/16] remove unnecessary Object.assign --- lib/Onyx.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/Onyx.js b/lib/Onyx.js index 2032f49ae..b996f64d6 100644 --- a/lib/Onyx.js +++ b/lib/Onyx.js @@ -962,7 +962,7 @@ function applyMerge(existingValue, 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) => Object.assign({}, fastMerge(modifiedData, change)), + return _.reduce(changes, (modifiedData, change) => fastMerge(modifiedData, change), existingValue || {}); } From 4ee679cfcb4cf6b3f348e8c20c35f8622f7a9ce1 Mon Sep 17 00:00:00 2001 From: Christoph Pader Date: Thu, 27 Jul 2023 10:01:08 +0200 Subject: [PATCH 15/16] removed unused jsdoc --- lib/Onyx.js | 1 - 1 file changed, 1 deletion(-) diff --git a/lib/Onyx.js b/lib/Onyx.js index b996f64d6..baf4f0ace 100644 --- a/lib/Onyx.js +++ b/lib/Onyx.js @@ -948,7 +948,6 @@ function multiSet(data) { * @private * @param {*} existingValue * @param {Array<*>} changes Array of changes that should be applied to the existing value - * @param {bool} omitNull Whether null top-level null values should be omitted * @returns {*} */ function applyMerge(existingValue, changes) { From 6f270f44ca05976a9c0030b16a30df2ec589568a Mon Sep 17 00:00:00 2001 From: Christoph Pader Date: Thu, 27 Jul 2023 10:13:50 +0200 Subject: [PATCH 16/16] fix: remove return value --- lib/Onyx.js | 3 --- 1 file changed, 3 deletions(-) diff --git a/lib/Onyx.js b/lib/Onyx.js index baf4f0ace..da4131bd2 100644 --- a/lib/Onyx.js +++ b/lib/Onyx.js @@ -850,7 +850,6 @@ function evictStorageAndRetry(error, onyxMethod, ...args) { * @param {*} value * @param {Boolean} hasChanged * @param {String} method - * @returns {Boolean} whether the value for the given key has changed */ function broadcastUpdate(key, value, hasChanged, method) { // Logging properties only since values could be sensitive things we don't want to log @@ -865,8 +864,6 @@ function broadcastUpdate(key, value, hasChanged, method) { } notifySubscribersOnNextTick(key, value, subscriber => hasChanged || subscriber.initWithStoredValues === false); - - return hasChanged; } /**