diff --git a/lib/Onyx.js b/lib/Onyx.js index 4a4196da..f50a645c 100644 --- a/lib/Onyx.js +++ b/lib/Onyx.js @@ -1050,6 +1050,10 @@ function clear(keysToPreserve = []) { 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 = _.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 // status, or activeClients need to remain in Onyx even when signed out) @@ -1102,7 +1106,11 @@ function clear(keysToPreserve = []) { notifyCollectionSubscribersOnNextTick(key, value); }); - return Storage.multiSet(keyValuesToReset); + // 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, ...keyValuesToPreserve])); }); } diff --git a/lib/SyncQueue.js b/lib/SyncQueue.js index 5775911e..baf5e569 100644 --- a/lib/SyncQueue.js +++ b/lib/SyncQueue.js @@ -21,6 +21,14 @@ export default class SyncQueue { this.run = run; } + /** + * Stop the queue from being processed and clear out any existing tasks + */ + abort() { + this.queue = []; + this.isProcessing = false; + } + process() { if (this.isProcessing || this.queue.length === 0) { return; diff --git a/lib/storage/WebStorage.js b/lib/storage/WebStorage.js index 6dde10db..c027cb49 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'; @@ -27,10 +32,23 @@ 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 = () => Storage.getAllKeys() - .then(keys => _.map(keys, key => this.removeItem(key))) - .then(tasks => Promise.all(tasks)); + // 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 = () => { + 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 which is a more atomic action + // for other browser tabs + _.each(allKeys, raiseStorageSyncEvent); + }); + }; // 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 2398916c..62c253c8 100644 --- a/lib/storage/providers/AsyncStorage.js +++ b/lib/storage/providers/AsyncStorage.js @@ -74,7 +74,7 @@ const provider = { removeItem: AsyncStorage.removeItem, /** - * Clear absolutely everything from storage + * Clear everything from storage * @returns {Promise} */ clear: AsyncStorage.clear, diff --git a/lib/storage/providers/LocalForage.js b/lib/storage/providers/LocalForage.js index ea85a915..e42c505f 100644 --- a/lib/storage/providers/LocalForage.js +++ b/lib/storage/providers/LocalForage.js @@ -75,10 +75,13 @@ const provider = { }, /** - * Clear absolutely everything from storage + * Clear everything from storage and also stops the SyncQueue from adding anything more to storage * @returns {Promise} */ - clear: localforage.clear, + clear() { + this.setItemQueue.abort(); + return localforage.clear(); + }, /** * Returns all keys available in storage diff --git a/tests/unit/onyxClearNativeStorageTest.js b/tests/unit/onyxClearNativeStorageTest.js index 4af1cdeb..133a5eb1 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); @@ -120,8 +98,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)); }); diff --git a/tests/unit/onyxClearWebStorageTest.js b/tests/unit/onyxClearWebStorageTest.js index 92755c53..4499e24e 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); diff --git a/tests/unit/onyxMultiMergeWebStorageTest.js b/tests/unit/onyxMultiMergeWebStorageTest.js index f60e24d8..6eb4b53e 100644 --- a/tests/unit/onyxMultiMergeWebStorageTest.js +++ b/tests/unit/onyxMultiMergeWebStorageTest.js @@ -141,12 +141,8 @@ 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, - }); + // 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 diff --git a/tests/unit/storage/providers/LocalForageProviderTest.js b/tests/unit/storage/providers/LocalForageProviderTest.js index e2c3b57c..a79ca84d 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 = [ @@ -104,4 +106,35 @@ describe('storage/providers/LocalForage', () => { }); }); }); + + it('clear', () => { + // We're creating a Promise which we programatically control when to resolve. + const task = createDeferredTask(); + + // 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 + + // 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}`); + } + + // 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(); + + // 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); + }); + }); });