From d7cfb1262433dcf02cb5a58a7c31e6c032cc9abe Mon Sep 17 00:00:00 2001 From: Tim Golen Date: Wed, 28 Dec 2022 12:36:40 -0700 Subject: [PATCH 01/22] Add multiRemove to both storage providers --- lib/storage/providers/AsyncStorage.js | 1 + lib/storage/providers/LocalForage.js | 10 ++++++++++ lib/storage/providers/SQLiteStorage.js | 10 ++++++++++ 3 files changed, 21 insertions(+) diff --git a/lib/storage/providers/AsyncStorage.js b/lib/storage/providers/AsyncStorage.js index 2398916c0..6ced2ee4f 100644 --- a/lib/storage/providers/AsyncStorage.js +++ b/lib/storage/providers/AsyncStorage.js @@ -78,6 +78,7 @@ const provider = { * @returns {Promise} */ clear: AsyncStorage.clear, + multiRemove: AsyncStorage.multiRemove, }; export default provider; diff --git a/lib/storage/providers/LocalForage.js b/lib/storage/providers/LocalForage.js index ea85a9153..9cd9c49b6 100644 --- a/lib/storage/providers/LocalForage.js +++ b/lib/storage/providers/LocalForage.js @@ -74,6 +74,16 @@ const provider = { return Promise.all(tasks).then(() => Promise.resolve()); }, + /** + * Removes all the keys specified in the array + * @param {Array} keys + * @return {Promise} + */ + multiRemove(keys) { + const tasks = _.map(keys, key => this.removeItem(key)); + return Promise.all(tasks).then(() => Promise.resolve()); + }, + /** * Clear absolutely everything from storage * @returns {Promise} diff --git a/lib/storage/providers/SQLiteStorage.js b/lib/storage/providers/SQLiteStorage.js index 5a1c3b485..7a6fc46de 100644 --- a/lib/storage/providers/SQLiteStorage.js +++ b/lib/storage/providers/SQLiteStorage.js @@ -108,6 +108,16 @@ const provider = { */ removeItem: key => db.executeAsync('DELETE FROM keyvaluepairs WHERE record_key = ?;', [key]), + /** + * Removes all of the given keys + * @param {Array} keys + * @returns {Promise} + */ + multiRemove: (keys) => { + const placeholders = _.map(keys, () => '?').join(','); + return db.executeAsync(`DELETE FROM keyvaluepairs WHERE record_key IN (${placeholders});`, keys); + }, + /** * Clears absolutely everything from storage * @returns {Promise} From 02a600422c6f7b22d78457cc0dcf577ff5ae8038 Mon Sep 17 00:00:00 2001 From: Tim Golen Date: Wed, 28 Dec 2022 12:47:22 -0700 Subject: [PATCH 02/22] Add some basic unit tests --- .../storage/providers/AsyncStorageProviderTest.js | 14 ++++++++++++++ .../storage/providers/LocalForageProviderTest.js | 13 +++++++++++++ 2 files changed, 27 insertions(+) diff --git a/tests/unit/storage/providers/AsyncStorageProviderTest.js b/tests/unit/storage/providers/AsyncStorageProviderTest.js index 3c45ef1ad..4ae8f173e 100644 --- a/tests/unit/storage/providers/AsyncStorageProviderTest.js +++ b/tests/unit/storage/providers/AsyncStorageProviderTest.js @@ -108,4 +108,18 @@ describe('storage/providers/AsyncStorage', () => { .then(values => expect(values).toEqual(items)); }); }); + + describe('multiRemove', () => { + it('Call multiRemove() with the list of keys', () => { + // Given sample content of all basic types + const pairs = SAMPLE_ITEMS.slice(); + const keys = _.map(pairs, pair => pair[0]); + + // When multiple keys are removed + StorageProvider.multiRemove(keys); + + // multiRemove should be called + expect(AsyncStorageMock.multiRemove).toHaveBeenCalledWith(keys); + }); + }); }); diff --git a/tests/unit/storage/providers/LocalForageProviderTest.js b/tests/unit/storage/providers/LocalForageProviderTest.js index e2c3b57c4..004eb20f0 100644 --- a/tests/unit/storage/providers/LocalForageProviderTest.js +++ b/tests/unit/storage/providers/LocalForageProviderTest.js @@ -104,4 +104,17 @@ describe('storage/providers/LocalForage', () => { }); }); }); + + it('multiRemove', () => { + // Given multiple pairs to be saved in storage + const pairs = SAMPLE_ITEMS.slice(); + const keys = _.map(pairs, pair => pair[0]); + + // When they are saved + return StorageProvider.multiRemove(keys) + .then(() => { + // We expect a call to localForage.removeItem for each pair + _.each(keys, key => expect(localforage.removeItem).toHaveBeenCalledWith(key)); + }); + }); }); From 842be4292a774a8f33806cd381d19cb35340b204 Mon Sep 17 00:00:00 2001 From: Tim Golen Date: Wed, 28 Dec 2022 12:47:42 -0700 Subject: [PATCH 03/22] Implement the multiRemove method --- lib/Onyx.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/Onyx.js b/lib/Onyx.js index 4a4196da1..3906f8b66 100644 --- a/lib/Onyx.js +++ b/lib/Onyx.js @@ -1102,7 +1102,7 @@ function clear(keysToPreserve = []) { notifyCollectionSubscribersOnNextTick(key, value); }); - return Storage.multiSet(keyValuesToReset); + return Storage.multiRemove(keyValuesToReset); }); } From b98d3c589b5b332200c562eb3ce40c7424dffe51 Mon Sep 17 00:00:00 2001 From: Tim Golen Date: Thu, 29 Dec 2022 09:11:42 -0700 Subject: [PATCH 04/22] Fix the keys passed to multiRemove --- lib/Onyx.js | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/lib/Onyx.js b/lib/Onyx.js index 3906f8b66..c401bbf8b 100644 --- a/lib/Onyx.js +++ b/lib/Onyx.js @@ -1102,7 +1102,9 @@ function clear(keysToPreserve = []) { notifyCollectionSubscribersOnNextTick(key, value); }); - return Storage.multiRemove(keyValuesToReset); + // Extract all the keys and clear them out + const keysToReset = _.map(keyValuesToReset, keyValueToReset => keyValueToReset[0]); + return Storage.multiRemove(keysToReset); }); } From c57082390894bfef323e619816c49fad7c95d52d Mon Sep 17 00:00:00 2001 From: Tim Golen Date: Thu, 29 Dec 2022 11:13:38 -0700 Subject: [PATCH 05/22] Fix all the tests --- lib/Onyx.js | 6 +++--- tests/unit/onyxClearNativeStorageTest.js | 5 ++++- tests/unit/onyxMultiMergeWebStorageTest.js | 6 +----- 3 files changed, 8 insertions(+), 9 deletions(-) diff --git a/lib/Onyx.js b/lib/Onyx.js index c401bbf8b..282d5ea2e 100644 --- a/lib/Onyx.js +++ b/lib/Onyx.js @@ -1102,9 +1102,9 @@ function clear(keysToPreserve = []) { notifyCollectionSubscribersOnNextTick(key, value); }); - // Extract all the keys and clear them out - const keysToReset = _.map(keyValuesToReset, keyValueToReset => keyValueToReset[0]); - return Storage.multiRemove(keysToReset); + return Storage.multiSet(defaultKeyValuePairs).then(() => { + return Storage.multiRemove(keysToClear); + }); }); } diff --git a/tests/unit/onyxClearNativeStorageTest.js b/tests/unit/onyxClearNativeStorageTest.js index 4af1cdeb9..2704ee334 100644 --- a/tests/unit/onyxClearNativeStorageTest.js +++ b/tests/unit/onyxClearNativeStorageTest.js @@ -79,7 +79,10 @@ describe('Set data while storage is clearing', () => { const cachedValue = cache.getValue(ONYX_KEYS.DEFAULT_KEY); expect(cachedValue).toBe(DEFAULT_VALUE); return Storage.getItem(ONYX_KEYS.DEFAULT_KEY) - .then(storedValue => expect(parseInt(storedValue, 10)).toBe(DEFAULT_VALUE)); + .then(storedValue => { + console.log(storedValue) + expect(parseInt(storedValue, 10)).toBe(DEFAULT_VALUE) + }); }); }); diff --git a/tests/unit/onyxMultiMergeWebStorageTest.js b/tests/unit/onyxMultiMergeWebStorageTest.js index f60e24d81..cb9a0e1af 100644 --- a/tests/unit/onyxMultiMergeWebStorageTest.js +++ b/tests/unit/onyxMultiMergeWebStorageTest.js @@ -142,11 +142,7 @@ describe('Onyx.mergeCollection() amd WebStorage', () => { it('setItem() and multiMerge()', () => { // When Onyx is cleared, since it uses multiSet() to clear out all the values, the keys will remain with null for all the values - expect(localforageMock.storageMap).toEqual({ - test_1: null, - test_2: null, - test_3: null, - }); + expect(localforageMock.storageMap).toEqual({}); // Given no previous data and several calls to setItem and call to mergeCollection to update a given key From 88a165ce10e8026f86b33b5670608e0defa699c4 Mon Sep 17 00:00:00 2001 From: Tim Golen Date: Thu, 29 Dec 2022 11:32:07 -0700 Subject: [PATCH 06/22] Fix lint --- lib/Onyx.js | 5 ++--- tests/unit/onyxClearNativeStorageTest.js | 5 +---- 2 files changed, 3 insertions(+), 7 deletions(-) diff --git a/lib/Onyx.js b/lib/Onyx.js index 282d5ea2e..70c421dee 100644 --- a/lib/Onyx.js +++ b/lib/Onyx.js @@ -1102,9 +1102,8 @@ function clear(keysToPreserve = []) { notifyCollectionSubscribersOnNextTick(key, value); }); - return Storage.multiSet(defaultKeyValuePairs).then(() => { - return Storage.multiRemove(keysToClear); - }); + return Storage.multiSet(defaultKeyValuePairs) + .then(() => Storage.multiRemove(keysToClear)); }); } diff --git a/tests/unit/onyxClearNativeStorageTest.js b/tests/unit/onyxClearNativeStorageTest.js index 2704ee334..4af1cdeb9 100644 --- a/tests/unit/onyxClearNativeStorageTest.js +++ b/tests/unit/onyxClearNativeStorageTest.js @@ -79,10 +79,7 @@ describe('Set data while storage is clearing', () => { const cachedValue = cache.getValue(ONYX_KEYS.DEFAULT_KEY); expect(cachedValue).toBe(DEFAULT_VALUE); return Storage.getItem(ONYX_KEYS.DEFAULT_KEY) - .then(storedValue => { - console.log(storedValue) - expect(parseInt(storedValue, 10)).toBe(DEFAULT_VALUE) - }); + .then(storedValue => expect(parseInt(storedValue, 10)).toBe(DEFAULT_VALUE)); }); }); From fb3ab20dd2c57033be6db06ca66c170b76dd8790 Mon Sep 17 00:00:00 2001 From: Tim Golen Date: Mon, 2 Jan 2023 16:15:21 -0800 Subject: [PATCH 07/22] Use clear and then multiSet --- lib/Onyx.js | 7 +++++-- lib/storage/WebStorage.js | 4 +--- 2 files changed, 6 insertions(+), 5 deletions(-) diff --git a/lib/Onyx.js b/lib/Onyx.js index 70c421dee..daa2928ec 100644 --- a/lib/Onyx.js +++ b/lib/Onyx.js @@ -1049,6 +1049,10 @@ function clear(keysToPreserve = []) { .then((keys) => { const keyValuesToReset = []; const defaultKeys = _.keys(defaultKeyStates); + const keyValuesToPreserve = _.reduce(keysToPreserve, (finalArray, keyToPreserve) => { + finalArray.push([keyToPreserve, cache.getValue(keyToPreserve)]) + return finalArray; + }, []); // The only keys that should not be cleared are: // 1. Anything specifically passed in keysToPreserve (because some keys like language preferences, offline @@ -1102,8 +1106,7 @@ function clear(keysToPreserve = []) { notifyCollectionSubscribersOnNextTick(key, value); }); - return Storage.multiSet(defaultKeyValuePairs) - .then(() => Storage.multiRemove(keysToClear)); + return Storage.clear(defaultKeyValuePairs.concat(keyValuesToPreserve)); }); } diff --git a/lib/storage/WebStorage.js b/lib/storage/WebStorage.js index 6dde10db2..30489f709 100644 --- a/lib/storage/WebStorage.js +++ b/lib/storage/WebStorage.js @@ -28,9 +28,7 @@ const webStorage = { // If we just call Storage.clear other tabs will have no idea which keys were available previously // so that they can call keysChanged for them. That's why we iterate and remove keys one by one - this.clear = () => Storage.getAllKeys() - .then(keys => _.map(keys, key => this.removeItem(key))) - .then(tasks => Promise.all(tasks)); + this.clear = keysToPreserve => Storage.clear().then(() => Storage.multiSet(keysToPreserve)); // This listener will only be triggered by events coming from other tabs global.addEventListener('storage', (event) => { From 4a2faf6e06de85d5bb54206f83aa058abf8a35b0 Mon Sep 17 00:00:00 2001 From: Tim Golen Date: Tue, 3 Jan 2023 11:44:17 -0800 Subject: [PATCH 08/22] WIP --- lib/storage/WebStorage.js | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-) diff --git a/lib/storage/WebStorage.js b/lib/storage/WebStorage.js index 30489f709..0f9ff8fcd 100644 --- a/lib/storage/WebStorage.js +++ b/lib/storage/WebStorage.js @@ -27,8 +27,18 @@ const webStorage = { .then(() => raiseStorageSyncEvent(key)); // If we just call Storage.clear other tabs will have no idea which keys were available previously - // so that they can call keysChanged for them. That's why we iterate and remove keys one by one - this.clear = keysToPreserve => Storage.clear().then(() => Storage.multiSet(keysToPreserve)); + // so that they can call keysChanged for them. That's why we iterate over every key that doesn't need to be + // preserved and raise a storage sync event for them + this.clear = keyValuesToPreserve => Storage.getAllKeys() + .then((allKeys) => { + const keysToPreserve = _.map(keyValuesToPreserve, keyValueToPreserve => keyValueToPreserve[0]); + return _.filter(allKeys, key => !_.contains(keysToPreserve, key)); + }) + .then(keysToRemove => _.map(keysToRemove, key => raiseStorageSyncEvent(key))) + + // Clear out everything from storage and then set + .then(() => Storage.clear()) + .then(() => Storage.multiSet(keyValuesToPreserve)); // This listener will only be triggered by events coming from other tabs global.addEventListener('storage', (event) => { From c58061ca81470ecac7e87ab96520dc30056889ff Mon Sep 17 00:00:00 2001 From: Tim Golen Date: Tue, 3 Jan 2023 17:43:06 -0800 Subject: [PATCH 09/22] Remove multi-remove --- lib/storage/providers/AsyncStorage.js | 1 - lib/storage/providers/LocalForage.js | 10 ---------- lib/storage/providers/SQLiteStorage.js | 10 ---------- .../storage/providers/AsyncStorageProviderTest.js | 14 -------------- .../storage/providers/LocalForageProviderTest.js | 13 ------------- 5 files changed, 48 deletions(-) diff --git a/lib/storage/providers/AsyncStorage.js b/lib/storage/providers/AsyncStorage.js index 6ced2ee4f..2398916c0 100644 --- a/lib/storage/providers/AsyncStorage.js +++ b/lib/storage/providers/AsyncStorage.js @@ -78,7 +78,6 @@ const provider = { * @returns {Promise} */ clear: AsyncStorage.clear, - multiRemove: AsyncStorage.multiRemove, }; export default provider; diff --git a/lib/storage/providers/LocalForage.js b/lib/storage/providers/LocalForage.js index 9cd9c49b6..ea85a9153 100644 --- a/lib/storage/providers/LocalForage.js +++ b/lib/storage/providers/LocalForage.js @@ -74,16 +74,6 @@ const provider = { return Promise.all(tasks).then(() => Promise.resolve()); }, - /** - * Removes all the keys specified in the array - * @param {Array} keys - * @return {Promise} - */ - multiRemove(keys) { - const tasks = _.map(keys, key => this.removeItem(key)); - return Promise.all(tasks).then(() => Promise.resolve()); - }, - /** * Clear absolutely everything from storage * @returns {Promise} diff --git a/lib/storage/providers/SQLiteStorage.js b/lib/storage/providers/SQLiteStorage.js index 7a6fc46de..5a1c3b485 100644 --- a/lib/storage/providers/SQLiteStorage.js +++ b/lib/storage/providers/SQLiteStorage.js @@ -108,16 +108,6 @@ const provider = { */ removeItem: key => db.executeAsync('DELETE FROM keyvaluepairs WHERE record_key = ?;', [key]), - /** - * Removes all of the given keys - * @param {Array} keys - * @returns {Promise} - */ - multiRemove: (keys) => { - const placeholders = _.map(keys, () => '?').join(','); - return db.executeAsync(`DELETE FROM keyvaluepairs WHERE record_key IN (${placeholders});`, keys); - }, - /** * Clears absolutely everything from storage * @returns {Promise} diff --git a/tests/unit/storage/providers/AsyncStorageProviderTest.js b/tests/unit/storage/providers/AsyncStorageProviderTest.js index 4ae8f173e..3c45ef1ad 100644 --- a/tests/unit/storage/providers/AsyncStorageProviderTest.js +++ b/tests/unit/storage/providers/AsyncStorageProviderTest.js @@ -108,18 +108,4 @@ describe('storage/providers/AsyncStorage', () => { .then(values => expect(values).toEqual(items)); }); }); - - describe('multiRemove', () => { - it('Call multiRemove() with the list of keys', () => { - // Given sample content of all basic types - const pairs = SAMPLE_ITEMS.slice(); - const keys = _.map(pairs, pair => pair[0]); - - // When multiple keys are removed - StorageProvider.multiRemove(keys); - - // multiRemove should be called - expect(AsyncStorageMock.multiRemove).toHaveBeenCalledWith(keys); - }); - }); }); diff --git a/tests/unit/storage/providers/LocalForageProviderTest.js b/tests/unit/storage/providers/LocalForageProviderTest.js index 004eb20f0..e2c3b57c4 100644 --- a/tests/unit/storage/providers/LocalForageProviderTest.js +++ b/tests/unit/storage/providers/LocalForageProviderTest.js @@ -104,17 +104,4 @@ describe('storage/providers/LocalForage', () => { }); }); }); - - it('multiRemove', () => { - // Given multiple pairs to be saved in storage - const pairs = SAMPLE_ITEMS.slice(); - const keys = _.map(pairs, pair => pair[0]); - - // When they are saved - return StorageProvider.multiRemove(keys) - .then(() => { - // We expect a call to localForage.removeItem for each pair - _.each(keys, key => expect(localforage.removeItem).toHaveBeenCalledWith(key)); - }); - }); }); From 264be3cfdb722258a48b75e50adaca32f2bd0104 Mon Sep 17 00:00:00 2001 From: Tim Golen Date: Wed, 4 Jan 2023 15:19:17 -0800 Subject: [PATCH 10/22] Move some of the clear() logic into LocalForage --- lib/storage/WebStorage.js | 10 ++++++---- lib/storage/providers/LocalForage.js | 10 ++++++++-- 2 files changed, 14 insertions(+), 6 deletions(-) diff --git a/lib/storage/WebStorage.js b/lib/storage/WebStorage.js index 0f9ff8fcd..093d2b1f2 100644 --- a/lib/storage/WebStorage.js +++ b/lib/storage/WebStorage.js @@ -1,3 +1,8 @@ +/** + * This file is here to wrap LocalForage with a layer that provides data-changed events like the ones that exist + * when using LocalStorage APIs in the browser. These events are great because multiple tabs can listen for when + * data changes and then stay up-to-date with everything happening in Onyx. + */ import _ from 'underscore'; import Storage from './providers/LocalForage'; @@ -35,10 +40,7 @@ const webStorage = { return _.filter(allKeys, key => !_.contains(keysToPreserve, key)); }) .then(keysToRemove => _.map(keysToRemove, key => raiseStorageSyncEvent(key))) - - // Clear out everything from storage and then set - .then(() => Storage.clear()) - .then(() => Storage.multiSet(keyValuesToPreserve)); + .then(() => Storage.clear(keyValuesToPreserve)); // This listener will only be triggered by events coming from other tabs global.addEventListener('storage', (event) => { diff --git a/lib/storage/providers/LocalForage.js b/lib/storage/providers/LocalForage.js index ea85a9153..9454fa140 100644 --- a/lib/storage/providers/LocalForage.js +++ b/lib/storage/providers/LocalForage.js @@ -75,10 +75,16 @@ const provider = { }, /** - * Clear absolutely everything from storage + * Clear most things from storage + * @param {Array} keyValuesToPreserve a set of key value pairs to keep in Onyx after doing a full clear. This argument + * is here to provide a performance boost when deleting a majority of keys, but wanting to keep only a few. + * Example: It's faster than doing a big multiRemove() which is just a loop that calls removeItem(). * @returns {Promise} */ - clear: localforage.clear, + clear(keyValuesToPreserve) { + return localforage.clear() + .then(() => Storage.multiSet(keyValuesToPreserve)); + }, /** * Returns all keys available in storage From a64df8f1fb24dc745e8247d34efa223fdc153da9 Mon Sep 17 00:00:00 2001 From: Tim Golen Date: Wed, 4 Jan 2023 17:32:20 -0800 Subject: [PATCH 11/22] Abort the sync queue when clearing Onyx --- lib/SyncQueue.js | 11 +++++++++++ lib/storage/providers/LocalForage.js | 1 + 2 files changed, 12 insertions(+) diff --git a/lib/SyncQueue.js b/lib/SyncQueue.js index 5775911e9..418ef62a2 100644 --- a/lib/SyncQueue.js +++ b/lib/SyncQueue.js @@ -21,6 +21,17 @@ export default class SyncQueue { this.run = run; } + /** + * Stop the queue from being processed and clear out any existing tasks + */ + abort() { + if (!this.isProcessing || this.queue.length === 0) { + return; + } + this.queue = []; + this.isProcessing = false; + } + process() { if (this.isProcessing || this.queue.length === 0) { return; diff --git a/lib/storage/providers/LocalForage.js b/lib/storage/providers/LocalForage.js index 9454fa140..25b32bd39 100644 --- a/lib/storage/providers/LocalForage.js +++ b/lib/storage/providers/LocalForage.js @@ -82,6 +82,7 @@ const provider = { * @returns {Promise} */ clear(keyValuesToPreserve) { + SyncQueue.abort(); return localforage.clear() .then(() => Storage.multiSet(keyValuesToPreserve)); }, From b1016ea836f0d71d8fd0d86344fa5978a6a10a56 Mon Sep 17 00:00:00 2001 From: Yuwen Memon Date: Thu, 5 Jan 2023 06:56:01 -0800 Subject: [PATCH 12/22] Add test for clear for LocalForage --- lib/storage/providers/LocalForage.js | 2 +- .../providers/LocalForageProviderTest.js | 22 +++++++++++++++++++ 2 files changed, 23 insertions(+), 1 deletion(-) diff --git a/lib/storage/providers/LocalForage.js b/lib/storage/providers/LocalForage.js index 25b32bd39..9c8f3fe6d 100644 --- a/lib/storage/providers/LocalForage.js +++ b/lib/storage/providers/LocalForage.js @@ -82,7 +82,7 @@ const provider = { * @returns {Promise} */ clear(keyValuesToPreserve) { - SyncQueue.abort(); + this.setItemQueue.abort(); return localforage.clear() .then(() => Storage.multiSet(keyValuesToPreserve)); }, diff --git a/tests/unit/storage/providers/LocalForageProviderTest.js b/tests/unit/storage/providers/LocalForageProviderTest.js index e2c3b57c4..93f57b3d8 100644 --- a/tests/unit/storage/providers/LocalForageProviderTest.js +++ b/tests/unit/storage/providers/LocalForageProviderTest.js @@ -104,4 +104,26 @@ describe('storage/providers/LocalForage', () => { }); }); }); + + it('clear', () => { + // Use fake timers, so we can manipulate time at our will for this test. + jest.useFakeTimers(); + + // Given an implementation of setItem that resolves after 1000ms + const setItemTimedOut = setTimeout(() => { + Promise.resolve(); + }, 1000); + localforage.setItem = jest.fn(setItemTimedOut); + + // When we call setItem 5 times, but then call clear after only 1000ms + for (let i = 0; i < 5; i++) { + StorageProvider.setItem(`key${i}`, `value${i}`); + } + jest.advanceTimersByTime(1000); + StorageProvider.clear(); + jest.advanceTimersByTime(4000); + + // Then setItem should only have been called once since all other calls were aborted when we called clear() + expect(localforage.setItem).toHaveBeenCalledTimes(1); + }); }); From c158e4f4ee08131be5b2b8be2a95c554be467a07 Mon Sep 17 00:00:00 2001 From: Tim Golen Date: Thu, 5 Jan 2023 11:18:58 -0800 Subject: [PATCH 13/22] Don't pass clear argument to AsyncStorage --- lib/storage/providers/AsyncStorage.js | 2 +- lib/storage/providers/LocalForage.js | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/storage/providers/AsyncStorage.js b/lib/storage/providers/AsyncStorage.js index 2398916c0..b49693953 100644 --- a/lib/storage/providers/AsyncStorage.js +++ b/lib/storage/providers/AsyncStorage.js @@ -77,7 +77,7 @@ const provider = { * Clear absolutely everything from storage * @returns {Promise} */ - clear: AsyncStorage.clear, + clear: () => AsyncStorage.clear(), }; export default provider; diff --git a/lib/storage/providers/LocalForage.js b/lib/storage/providers/LocalForage.js index 9c8f3fe6d..9f8ef3596 100644 --- a/lib/storage/providers/LocalForage.js +++ b/lib/storage/providers/LocalForage.js @@ -84,7 +84,7 @@ const provider = { clear(keyValuesToPreserve) { this.setItemQueue.abort(); return localforage.clear() - .then(() => Storage.multiSet(keyValuesToPreserve)); + .then(() => this.multiSet(keyValuesToPreserve)); }, /** From d7ef5d362c5f6b479c8d0e5828ba0449492cd92e Mon Sep 17 00:00:00 2001 From: Tim Golen Date: Thu, 5 Jan 2023 11:33:58 -0800 Subject: [PATCH 14/22] Make sure keys are preserved with AsyncStorage provider --- lib/Onyx.js | 2 +- lib/storage/providers/AsyncStorage.js | 10 ++++++++-- tests/unit/onyxClearNativeStorageTest.js | 3 +-- 3 files changed, 10 insertions(+), 5 deletions(-) diff --git a/lib/Onyx.js b/lib/Onyx.js index daa2928ec..7d584ba99 100644 --- a/lib/Onyx.js +++ b/lib/Onyx.js @@ -1050,7 +1050,7 @@ function clear(keysToPreserve = []) { const keyValuesToReset = []; const defaultKeys = _.keys(defaultKeyStates); const keyValuesToPreserve = _.reduce(keysToPreserve, (finalArray, keyToPreserve) => { - finalArray.push([keyToPreserve, cache.getValue(keyToPreserve)]) + finalArray.push([keyToPreserve, cache.getValue(keyToPreserve)]); return finalArray; }, []); diff --git a/lib/storage/providers/AsyncStorage.js b/lib/storage/providers/AsyncStorage.js index b49693953..d08bd568f 100644 --- a/lib/storage/providers/AsyncStorage.js +++ b/lib/storage/providers/AsyncStorage.js @@ -74,10 +74,16 @@ const provider = { removeItem: AsyncStorage.removeItem, /** - * Clear absolutely everything from storage + * Clear most things from storage + * @param {Array} keyValuesToPreserve a set of key value pairs to keep in Onyx after doing a full clear. This argument + * is here to provide a performance boost when deleting a majority of keys, but wanting to keep only a few. + * Example: It's faster than doing a big multiRemove() which is just a loop that calls removeItem(). * @returns {Promise} */ - clear: () => AsyncStorage.clear(), + clear(keyValuesToPreserve) { + return AsyncStorage.clear() + .then(() => this.multiSet(keyValuesToPreserve)); + }, }; export default provider; diff --git a/tests/unit/onyxClearNativeStorageTest.js b/tests/unit/onyxClearNativeStorageTest.js index 4af1cdeb9..10d6e0502 100644 --- a/tests/unit/onyxClearNativeStorageTest.js +++ b/tests/unit/onyxClearNativeStorageTest.js @@ -120,8 +120,7 @@ describe('Set data while storage is clearing', () => { .then(() => { // Then the value of the preserved key is also still set in both the cache and storage expect(regularKeyOnyxValue).toBe(SET_VALUE); - const regularKeyCachedValue = cache.getValue(ONYX_KEYS.REGULAR_KEY); - expect(regularKeyCachedValue).toBe(SET_VALUE); + expect(cache.getValue(ONYX_KEYS.REGULAR_KEY)).toBe(SET_VALUE); return Storage.getItem(ONYX_KEYS.REGULAR_KEY) .then(storedValue => expect(parseInt(storedValue, 10)).toBe(SET_VALUE)); }); From e4313d1490609a9e35dd53f9f53dbc03d9b81649 Mon Sep 17 00:00:00 2001 From: Tim Golen Date: Thu, 5 Jan 2023 11:50:19 -0800 Subject: [PATCH 15/22] Remove tests which are no longer relevant --- tests/unit/onyxClearNativeStorageTest.js | 22 ---------------------- tests/unit/onyxClearWebStorageTest.js | 20 -------------------- 2 files changed, 42 deletions(-) diff --git a/tests/unit/onyxClearNativeStorageTest.js b/tests/unit/onyxClearNativeStorageTest.js index 10d6e0502..133a5eb1d 100644 --- a/tests/unit/onyxClearNativeStorageTest.js +++ b/tests/unit/onyxClearNativeStorageTest.js @@ -43,28 +43,6 @@ describe('Set data while storage is clearing', () => { return Onyx.clear(); }); - it('should persist the value of Onyx.merge when called between the cache and storage clearing', () => { - expect.assertions(3); - - // Given that Onyx is completely clear - // When Onyx.clear() is called - Onyx.clear(); - - // When merge is called between the cache and storage clearing, on a key with a default key state - Onyx.merge(ONYX_KEYS.DEFAULT_KEY, MERGED_VALUE); - return waitForPromisesToResolve() - .then(() => { - // Then the value in Onyx, the cache, and the storage is the merged value - expect(onyxValue).toBe(MERGED_VALUE); - const cachedValue = cache.getValue(ONYX_KEYS.DEFAULT_KEY); - expect(cachedValue).toBe(MERGED_VALUE); - - // Use parseInt to convert the string value from the storage mock back to an integer - return Storage.getItem(ONYX_KEYS.DEFAULT_KEY) - .then(storedValue => expect(parseInt(storedValue, 10)).toBe(MERGED_VALUE)); - }); - }); - it('should replace the value of Onyx.set with the default key state in the cache', () => { expect.assertions(3); diff --git a/tests/unit/onyxClearWebStorageTest.js b/tests/unit/onyxClearWebStorageTest.js index 92755c53c..4499e24e4 100644 --- a/tests/unit/onyxClearWebStorageTest.js +++ b/tests/unit/onyxClearWebStorageTest.js @@ -50,26 +50,6 @@ describe('Set data while storage is clearing', () => { return Onyx.clear(); }); - it('should persist the value of Onyx.merge when called between the cache and storage clearing', () => { - expect.assertions(3); - - // Given that Onyx is completely clear - // When Onyx.clear() is called - Onyx.clear(); - - // When merge is called between the cache and storage clearing, on a key with a default key state - Onyx.merge(ONYX_KEYS.DEFAULT_KEY, MERGED_VALUE); - return waitForPromisesToResolve() - .then(() => { - // Then the value in Onyx, the cache, and the storage is the merged value - expect(onyxValue).toBe(MERGED_VALUE); - const cachedValue = cache.getValue(ONYX_KEYS.DEFAULT_KEY); - expect(cachedValue).toBe(MERGED_VALUE); - const storedValue = Storage.getItem(ONYX_KEYS.DEFAULT_KEY); - return expect(storedValue).resolves.toBe(MERGED_VALUE); - }); - }); - it('should replace the value of Onyx.set with the default key state in the cache', () => { expect.assertions(3); From 5cb00d2cc7d2232752b6b2f34b0437a6ec16bcd3 Mon Sep 17 00:00:00 2001 From: Yuwen Memon Date: Thu, 5 Jan 2023 13:13:17 -0800 Subject: [PATCH 16/22] Update unit test --- tests/unit/storage/providers/LocalForageProviderTest.js | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/tests/unit/storage/providers/LocalForageProviderTest.js b/tests/unit/storage/providers/LocalForageProviderTest.js index 93f57b3d8..2b1443d34 100644 --- a/tests/unit/storage/providers/LocalForageProviderTest.js +++ b/tests/unit/storage/providers/LocalForageProviderTest.js @@ -110,10 +110,7 @@ describe('storage/providers/LocalForage', () => { jest.useFakeTimers(); // Given an implementation of setItem that resolves after 1000ms - const setItemTimedOut = setTimeout(() => { - Promise.resolve(); - }, 1000); - localforage.setItem = jest.fn(setItemTimedOut); + localforage.setItem = jest.fn(() => new Promise(resolve => setTimeout(resolve, 1000))); // When we call setItem 5 times, but then call clear after only 1000ms for (let i = 0; i < 5; i++) { From 93bb6ee9e84d1980041088c5f5d17bf70abbb148 Mon Sep 17 00:00:00 2001 From: Tim Golen Date: Thu, 5 Jan 2023 13:52:59 -0800 Subject: [PATCH 17/22] Improve some comments and clean up the clear logic a bit --- lib/Onyx.js | 9 ++++++++- lib/SyncQueue.js | 3 --- lib/storage/WebStorage.js | 14 +++++--------- lib/storage/providers/AsyncStorage.js | 10 ++-------- lib/storage/providers/LocalForage.js | 10 +++------- tests/unit/onyxMultiMergeWebStorageTest.js | 2 +- 6 files changed, 19 insertions(+), 29 deletions(-) diff --git a/lib/Onyx.js b/lib/Onyx.js index 7d584ba99..9c4196b04 100644 --- a/lib/Onyx.js +++ b/lib/Onyx.js @@ -1049,6 +1049,9 @@ function clear(keysToPreserve = []) { .then((keys) => { const keyValuesToReset = []; const defaultKeys = _.keys(defaultKeyStates); + + // Get all the values for the keys that need to be preserved. These key/value pairs will be set + // in Onyx after the database is cleared(). const keyValuesToPreserve = _.reduce(keysToPreserve, (finalArray, keyToPreserve) => { finalArray.push([keyToPreserve, cache.getValue(keyToPreserve)]); return finalArray; @@ -1106,7 +1109,11 @@ function clear(keysToPreserve = []) { notifyCollectionSubscribersOnNextTick(key, value); }); - return Storage.clear(defaultKeyValuePairs.concat(keyValuesToPreserve)); + // Call clear() and make sure that the default key/values and the key/values from the parameter + // are preserved in storage. This makes sure to always leave storage in a state that contains + // all the default values and any additional values that we want to remain after the database is cleared. + return Storage.clear() + .then(() => Storage.multiSet(defaultKeyValuePairs.concat(keyValuesToPreserve))); }); } diff --git a/lib/SyncQueue.js b/lib/SyncQueue.js index 418ef62a2..baf5e5691 100644 --- a/lib/SyncQueue.js +++ b/lib/SyncQueue.js @@ -25,9 +25,6 @@ export default class SyncQueue { * Stop the queue from being processed and clear out any existing tasks */ abort() { - if (!this.isProcessing || this.queue.length === 0) { - return; - } this.queue = []; this.isProcessing = false; } diff --git a/lib/storage/WebStorage.js b/lib/storage/WebStorage.js index 093d2b1f2..3343a76b0 100644 --- a/lib/storage/WebStorage.js +++ b/lib/storage/WebStorage.js @@ -32,15 +32,11 @@ const webStorage = { .then(() => raiseStorageSyncEvent(key)); // If we just call Storage.clear other tabs will have no idea which keys were available previously - // so that they can call keysChanged for them. That's why we iterate over every key that doesn't need to be - // preserved and raise a storage sync event for them - this.clear = keyValuesToPreserve => Storage.getAllKeys() - .then((allKeys) => { - const keysToPreserve = _.map(keyValuesToPreserve, keyValueToPreserve => keyValueToPreserve[0]); - return _.filter(allKeys, key => !_.contains(keysToPreserve, key)); - }) - .then(keysToRemove => _.map(keysToRemove, key => raiseStorageSyncEvent(key))) - .then(() => Storage.clear(keyValuesToPreserve)); + // so that they can call keysChanged for them. That's why we iterate over every key and raise a storage sync + // event for each one + this.clear = () => Storage.getAllKeys() + .then(allKeys => _.each(allKeys, raiseStorageSyncEvent)) + .then(() => Storage.clear()); // This listener will only be triggered by events coming from other tabs global.addEventListener('storage', (event) => { diff --git a/lib/storage/providers/AsyncStorage.js b/lib/storage/providers/AsyncStorage.js index d08bd568f..62c253c8a 100644 --- a/lib/storage/providers/AsyncStorage.js +++ b/lib/storage/providers/AsyncStorage.js @@ -74,16 +74,10 @@ const provider = { removeItem: AsyncStorage.removeItem, /** - * Clear most things from storage - * @param {Array} keyValuesToPreserve a set of key value pairs to keep in Onyx after doing a full clear. This argument - * is here to provide a performance boost when deleting a majority of keys, but wanting to keep only a few. - * Example: It's faster than doing a big multiRemove() which is just a loop that calls removeItem(). + * Clear everything from storage * @returns {Promise} */ - clear(keyValuesToPreserve) { - return AsyncStorage.clear() - .then(() => this.multiSet(keyValuesToPreserve)); - }, + clear: AsyncStorage.clear, }; export default provider; diff --git a/lib/storage/providers/LocalForage.js b/lib/storage/providers/LocalForage.js index 9f8ef3596..e42c505fa 100644 --- a/lib/storage/providers/LocalForage.js +++ b/lib/storage/providers/LocalForage.js @@ -75,16 +75,12 @@ const provider = { }, /** - * Clear most things from storage - * @param {Array} keyValuesToPreserve a set of key value pairs to keep in Onyx after doing a full clear. This argument - * is here to provide a performance boost when deleting a majority of keys, but wanting to keep only a few. - * Example: It's faster than doing a big multiRemove() which is just a loop that calls removeItem(). + * Clear everything from storage and also stops the SyncQueue from adding anything more to storage * @returns {Promise} */ - clear(keyValuesToPreserve) { + clear() { this.setItemQueue.abort(); - return localforage.clear() - .then(() => this.multiSet(keyValuesToPreserve)); + return localforage.clear(); }, /** diff --git a/tests/unit/onyxMultiMergeWebStorageTest.js b/tests/unit/onyxMultiMergeWebStorageTest.js index cb9a0e1af..6eb4b53ed 100644 --- a/tests/unit/onyxMultiMergeWebStorageTest.js +++ b/tests/unit/onyxMultiMergeWebStorageTest.js @@ -141,7 +141,7 @@ describe('Onyx.mergeCollection() amd WebStorage', () => { }); it('setItem() and multiMerge()', () => { - // When Onyx is cleared, since it uses multiSet() to clear out all the values, the keys will remain with null for all the values + // Onyx should be empty after clear() is called expect(localforageMock.storageMap).toEqual({}); // Given no previous data and several calls to setItem and call to mergeCollection to update a given key From f6312e843d3369b2e241ca232ddb984e7e2d40f5 Mon Sep 17 00:00:00 2001 From: Tim Golen Date: Mon, 9 Jan 2023 11:05:32 -0800 Subject: [PATCH 18/22] Switch the order in which the sync event happens --- lib/storage/WebStorage.js | 18 +++++++++++++++--- 1 file changed, 15 insertions(+), 3 deletions(-) diff --git a/lib/storage/WebStorage.js b/lib/storage/WebStorage.js index 3343a76b0..5025a58f3 100644 --- a/lib/storage/WebStorage.js +++ b/lib/storage/WebStorage.js @@ -34,9 +34,21 @@ const webStorage = { // If we just call Storage.clear other tabs will have no idea which keys were available previously // so that they can call keysChanged for them. That's why we iterate over every key and raise a storage sync // event for each one - this.clear = () => Storage.getAllKeys() - .then(allKeys => _.each(allKeys, raiseStorageSyncEvent)) - .then(() => Storage.clear()); + this.clear = () => { + let allKeys; + + // They keys must be retreived before storage is cleared or else the list of keys would be empty + return Storage.getAllKeys() + .then((keys) => { + allKeys = keys; + }) + .then(() => Storage.clear()) + .then(() => { + // Now that storage is cleared, the storage sync event can happen so that it is more of an atomic + // action + _.each(allKeys, raiseStorageSyncEvent); + }); + }; // This listener will only be triggered by events coming from other tabs global.addEventListener('storage', (event) => { From a5a35c9369703d37668383dc12cfb4f719fb2673 Mon Sep 17 00:00:00 2001 From: Yuwen Memon Date: Mon, 9 Jan 2023 14:16:33 -0800 Subject: [PATCH 19/22] Update tests not to use fakeTime --- .../providers/LocalForageProviderTest.js | 32 +++++++++++++------ 1 file changed, 23 insertions(+), 9 deletions(-) diff --git a/tests/unit/storage/providers/LocalForageProviderTest.js b/tests/unit/storage/providers/LocalForageProviderTest.js index 2b1443d34..a79ca84d2 100644 --- a/tests/unit/storage/providers/LocalForageProviderTest.js +++ b/tests/unit/storage/providers/LocalForageProviderTest.js @@ -2,6 +2,8 @@ import localforage from 'localforage'; import _ from 'underscore'; import StorageProvider from '../../../../lib/storage/providers/LocalForage'; +import createDeferredTask from '../../../../lib/createDeferredTask'; +import waitForPromisesToResolve from '../../../utils/waitForPromisesToResolve'; describe('storage/providers/LocalForage', () => { const SAMPLE_ITEMS = [ @@ -106,21 +108,33 @@ describe('storage/providers/LocalForage', () => { }); it('clear', () => { - // Use fake timers, so we can manipulate time at our will for this test. - jest.useFakeTimers(); + // We're creating a Promise which we programatically control when to resolve. + const task = createDeferredTask(); - // Given an implementation of setItem that resolves after 1000ms - localforage.setItem = jest.fn(() => new Promise(resolve => setTimeout(resolve, 1000))); + // We configure localforage.setItem to return this promise the first time it's called and to otherwise return resolved promises + localforage.setItem = jest.fn() + . mockReturnValue(Promise.resolve()) // Default behavior + . mockReturnValueOnce(task.promise); // First call behavior - // When we call setItem 5 times, but then call clear after only 1000ms + // Make 5 StorageProvider.setItem calls - this adds 5 items to the queue and starts executing the first localForage.setItem for (let i = 0; i < 5; i++) { StorageProvider.setItem(`key${i}`, `value${i}`); } - jest.advanceTimersByTime(1000); + + // At this point,`localForage.setItem` should have been called once, but we control when it resolves, and we'll keep it unresolved. + // This simulates the 1st localForage.setItem taking a random time. + // We then call StorageProvider.clear() while the first localForage.setItem isn't completed yet. StorageProvider.clear(); - jest.advanceTimersByTime(4000); - // Then setItem should only have been called once since all other calls were aborted when we called clear() - expect(localforage.setItem).toHaveBeenCalledTimes(1); + // Any calls that follow this would have been queued - so we don't expect more than 1 `localForage.setItem` call after the + // first one resolves. + task.resolve(); + + // waitForPromisesToResolve() makes jest wait for any promises (even promises returned as the result of a promise) to resolve. + // If StorageProvider.clear() does not abort the queue, more localForage.setItem calls would be executed because they would + // be sitting in the setItemQueue + return waitForPromisesToResolve().then(() => { + expect(localforage.setItem).toHaveBeenCalledTimes(1); + }); }); }); From 0219ea9ea9da3613ed2a5db6d2b7b274ce071cfe Mon Sep 17 00:00:00 2001 From: Tim Golen Date: Tue, 10 Jan 2023 07:58:37 -0800 Subject: [PATCH 20/22] Use map() instead of reduce() --- lib/Onyx.js | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/lib/Onyx.js b/lib/Onyx.js index 9c4196b04..e5ff5553f 100644 --- a/lib/Onyx.js +++ b/lib/Onyx.js @@ -1052,10 +1052,7 @@ function clear(keysToPreserve = []) { // Get all the values for the keys that need to be preserved. These key/value pairs will be set // in Onyx after the database is cleared(). - const keyValuesToPreserve = _.reduce(keysToPreserve, (finalArray, keyToPreserve) => { - finalArray.push([keyToPreserve, cache.getValue(keyToPreserve)]); - return finalArray; - }, []); + const keyValuesToPreserve = _.map(keysToPreserve, key => [key, cache.getValue(key)]); // The only keys that should not be cleared are: // 1. Anything specifically passed in keysToPreserve (because some keys like language preferences, offline From 17cb9cd6660e0c0209cf2d6d1de683295d795d03 Mon Sep 17 00:00:00 2001 From: Tim Golen Date: Tue, 10 Jan 2023 07:59:24 -0800 Subject: [PATCH 21/22] Improve comment --- lib/storage/WebStorage.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/storage/WebStorage.js b/lib/storage/WebStorage.js index 5025a58f3..c027cb493 100644 --- a/lib/storage/WebStorage.js +++ b/lib/storage/WebStorage.js @@ -44,8 +44,8 @@ const webStorage = { }) .then(() => Storage.clear()) .then(() => { - // Now that storage is cleared, the storage sync event can happen so that it is more of an atomic - // action + // Now that storage is cleared, the storage sync event can happen which is a more atomic action + // for other browser tabs _.each(allKeys, raiseStorageSyncEvent); }); }; From 212f27e07ce4c4366efb9b43e10c488422fd6344 Mon Sep 17 00:00:00 2001 From: Tim Golen Date: Tue, 10 Jan 2023 08:02:30 -0800 Subject: [PATCH 22/22] Replace concat with spread operator --- lib/Onyx.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/Onyx.js b/lib/Onyx.js index e5ff5553f..f50a645ce 100644 --- a/lib/Onyx.js +++ b/lib/Onyx.js @@ -1110,7 +1110,7 @@ function clear(keysToPreserve = []) { // are preserved in storage. This makes sure to always leave storage in a state that contains // all the default values and any additional values that we want to remain after the database is cleared. return Storage.clear() - .then(() => Storage.multiSet(defaultKeyValuePairs.concat(keyValuesToPreserve))); + .then(() => Storage.multiSet([...defaultKeyValuePairs, ...keyValuesToPreserve])); }); }