diff --git a/__mocks__/lib/storage/index.js b/__mocks__/lib/storage/index.js deleted file mode 100644 index 97de83c8e..000000000 --- a/__mocks__/lib/storage/index.js +++ /dev/null @@ -1,3 +0,0 @@ -import WebStorage from '../../../lib/storage/WebStorage'; - -export default WebStorage; diff --git a/__mocks__/localforage.js b/__mocks__/localforage.js index c552895f3..3e5fb940a 100644 --- a/__mocks__/localforage.js +++ b/__mocks__/localforage.js @@ -1,7 +1,37 @@ +import _ from 'underscore'; + +let storageMap = {}; + const localforageMock = { + get storageMap() { + return storageMap; + }, + keys() { + return new Promise((resolve) => { + resolve(_.keys(storageMap)); + }); + }, + setInitialMockData: (data) => { + storageMap = data; + }, config: jest.fn(), - getItem: jest.fn(), - setItem: jest.fn(() => Promise.resolve()), + getItem: jest.fn(key => new Promise((resolve) => { + resolve(storageMap[key]); + })), + setItem: jest.fn((key, value) => new Promise((resolve) => { + storageMap[key] = value; + resolve(); + })), + removeItem: jest.fn(key => new Promise((resolve) => { + delete storageMap[key]; + resolve(); + })), + clear() { + return new Promise((resolve) => { + storageMap = {}; + resolve(); + }); + }, }; export default localforageMock; diff --git a/lib/Onyx.js b/lib/Onyx.js index 38fc32bc3..5d7312134 100644 --- a/lib/Onyx.js +++ b/lib/Onyx.js @@ -768,11 +768,12 @@ function mergeCollection(collectionKey, collection) { promises.push(Storage.multiSet(keyValuePairsForNewCollection)); } - // Merge original data to cache - cache.merge(collection); - - // Optimistically inform collection subscribers - keysChanged(collectionKey, collection); + // Prefill cache if necessary by calling get() on any existing keys and then merge original data to cache + // and update all subscribers + Promise.all(_.map(existingKeys, get)).then(() => { + cache.merge(collection); + keysChanged(collectionKey, collection); + }); return Promise.all(promises) .catch(error => evictStorageAndRetry(error, mergeCollection, collection)); @@ -808,7 +809,7 @@ function init({ keys = {}, initialKeyStates = {}, safeEvictionKeys = [], - maxCachedKeysCount = 55, + maxCachedKeysCount = 1000, captureMetrics = false, shouldSyncMultipleInstances = Boolean(global.localStorage), } = {}) { diff --git a/lib/SyncQueue.js b/lib/SyncQueue.js new file mode 100644 index 000000000..5775911e9 --- /dev/null +++ b/lib/SyncQueue.js @@ -0,0 +1,51 @@ +/** + * Synchronous queue that can be used to ensure promise based tasks are run in sequence. + * Pass to the constructor a function that returns a promise to run the task then add data. + * + * @example + * + * const queue = new SyncQueue(({key, val}) => { + * return someAsyncProcess(key, val); + * }); + * + * queue.push({key: 1, val: '1'}); + * queue.push({key: 2, val: '2'}); + */ +export default class SyncQueue { + /** + * @param {Function} run - must return a promise + */ + constructor(run) { + this.queue = []; + this.isProcessing = false; + this.run = run; + } + + process() { + if (this.isProcessing || this.queue.length === 0) { + return; + } + + this.isProcessing = true; + + const {data, resolve, reject} = this.queue.shift(); + this.run(data) + .then(resolve) + .catch(reject) + .finally(() => { + this.isProcessing = false; + this.process(); + }); + } + + /** + * @param {*} data + * @returns {Promise} + */ + push(data) { + return new Promise((resolve, reject) => { + this.queue.push({resolve, reject, data}); + this.process(); + }); + } +} diff --git a/lib/storage/__mocks__/index.js b/lib/storage/__mocks__/index.js new file mode 100644 index 000000000..c7d082958 --- /dev/null +++ b/lib/storage/__mocks__/index.js @@ -0,0 +1,3 @@ +import WebStorage from '../WebStorage'; + +export default WebStorage; diff --git a/lib/storage/providers/LocalForage.js b/lib/storage/providers/LocalForage.js index 7c254be8d..3ac44b7cd 100644 --- a/lib/storage/providers/LocalForage.js +++ b/lib/storage/providers/LocalForage.js @@ -7,39 +7,32 @@ import localforage from 'localforage'; import _ from 'underscore'; import lodashMerge from 'lodash/merge'; +import SyncQueue from '../../SyncQueue'; localforage.config({ name: 'OnyxDB' }); -const writeQueue = []; -let isQueueProcessing = false; - -/** - * Writing very quickly to IndexedDB causes performance issues and can lock up the page and lead to jank. - * So, we are slowing this process down by waiting until one write is complete before moving on - * to the next. - */ -function processNextWrite() { - if (isQueueProcessing || writeQueue.length === 0) { - return; - } - - isQueueProcessing = true; - - const { - key, value, resolve, reject, - } = writeQueue.shift(); - localforage.setItem(key, value) - .then(resolve) - .catch(reject) - .finally(() => { - isQueueProcessing = false; - processNextWrite(); - }); -} - const provider = { + /** + * Writing very quickly to IndexedDB causes performance issues and can lock up the page and lead to jank. + * So, we are slowing this process down by waiting until one write is complete before moving on + * to the next. + */ + setItemQueue: new SyncQueue(({key, value, shouldMerge}) => { + if (shouldMerge) { + return localforage.getItem(key) + .then((existingValue) => { + const newValue = _.isObject(existingValue) + ? lodashMerge({}, existingValue, value) + : value; + return localforage.setItem(key, newValue); + }); + } + + return localforage.setItem(key, value); + }), + /** * Get multiple key-value pairs for the give array of keys in a batch * @param {String[]} keys @@ -61,14 +54,7 @@ const provider = { * @return {Promise} */ multiMerge(pairs) { - const tasks = _.map(pairs, ([key, partialValue]) => this.getItem(key) - .then((existingValue) => { - const newValue = _.isObject(existingValue) - ? lodashMerge(existingValue, partialValue) - : partialValue; - - return this.setItem(key, newValue); - })); + const tasks = _.map(pairs, ([key, value]) => this.setItemQueue.push({key, value, shouldMerge: true})); // We're returning Promise.resolve, otherwise the array of task results will be returned to the caller return Promise.all(tasks).then(() => Promise.resolve()); @@ -117,14 +103,9 @@ const provider = { * @param {*} value * @return {Promise} */ - setItem: (key, value) => ( - new Promise((resolve, reject) => { - writeQueue.push({ - key, value, resolve, reject, - }); - processNextWrite(); - }) - ), + setItem(key, value) { + return this.setItemQueue.push({key, value}); + }, }; export default provider; diff --git a/tests/unit/onyxMultiMergeWebStorageTest.js b/tests/unit/onyxMultiMergeWebStorageTest.js new file mode 100644 index 000000000..c2d48b837 --- /dev/null +++ b/tests/unit/onyxMultiMergeWebStorageTest.js @@ -0,0 +1,172 @@ +import OnyxCache from '../../lib/OnyxCache'; +import waitForPromisesToResolve from '../utils/waitForPromisesToResolve'; +import localforageMock from '../../__mocks__/localforage'; + +const ONYX_KEYS = { + COLLECTION: { + TEST_KEY: 'test_', + }, +}; + +const initialTestObject = {a: 'a'}; +const initialData = { + test_1: initialTestObject, + test_2: initialTestObject, + test_3: initialTestObject, +}; + +localforageMock.setInitialMockData(initialData); + +describe('Onyx.mergeCollection() amd WebStorage', () => { + let Onyx; + + beforeAll(() => { + // Force using WebStorage provider for these tests + jest.mock('../../lib/storage'); + Onyx = require('../../index').default; + jest.useRealTimers(); + + Onyx.init({ + keys: ONYX_KEYS, + registerStorageEventListener: () => {}, + initialKeyStates: {}, + }); + }); + + afterEach(() => Onyx.clear()); + + it('merges two sets of data consecutively', () => { + // Given initial data in storage + expect(localforageMock.storageMap.test_1).toEqual(initialTestObject); + expect(localforageMock.storageMap.test_2).toEqual(initialTestObject); + expect(localforageMock.storageMap.test_3).toEqual(initialTestObject); + + // And an empty cache values for the collection keys + expect(OnyxCache.getValue('test_1')).not.toBeDefined(); + expect(OnyxCache.getValue('test_2')).not.toBeDefined(); + expect(OnyxCache.getValue('test_3')).not.toBeDefined(); + + // When we merge additional data + const additionalDataOne = {b: 'b', c: 'c'}; + Onyx.mergeCollection(ONYX_KEYS.COLLECTION.TEST_KEY, { + test_1: additionalDataOne, + test_2: additionalDataOne, + test_3: additionalDataOne, + }); + + // And call again consecutively with different data + const additionalDataTwo = {d: 'd'}; + Onyx.mergeCollection(ONYX_KEYS.COLLECTION.TEST_KEY, { + test_1: additionalDataTwo, + test_2: additionalDataTwo, + test_3: additionalDataTwo, + }); + + return waitForPromisesToResolve() + .then(() => { + const finalObject = { + a: 'a', b: 'b', c: 'c', d: 'd', + }; + + // Then our new data should merge with the existing data in the cache + expect(OnyxCache.getValue('test_1')).toEqual(finalObject); + expect(OnyxCache.getValue('test_2')).toEqual(finalObject); + expect(OnyxCache.getValue('test_3')).toEqual(finalObject); + + // And the storage should reflect the same state + expect(localforageMock.storageMap.test_1).toEqual(finalObject); + expect(localforageMock.storageMap.test_2).toEqual(finalObject); + expect(localforageMock.storageMap.test_3).toEqual(finalObject); + }); + }); + + it('cache updates correctly when accessed again if keys are removed or evicted', () => { + // Given empty storage + expect(localforageMock.storageMap.test_1).toBeFalsy(); + expect(localforageMock.storageMap.test_2).toBeFalsy(); + expect(localforageMock.storageMap.test_3).toBeFalsy(); + + // And an empty cache values for the collection keys + expect(OnyxCache.getValue('test_1')).toBeFalsy(); + expect(OnyxCache.getValue('test_2')).toBeFalsy(); + expect(OnyxCache.getValue('test_3')).toBeFalsy(); + + // When we merge additional data and wait for the change + const data = {a: 'a', b: 'b'}; + Onyx.mergeCollection(ONYX_KEYS.COLLECTION.TEST_KEY, { + test_1: data, + test_2: data, + test_3: data, + }); + + return waitForPromisesToResolve() + .then(() => { + // Then the cache and storage should match + expect(OnyxCache.getValue('test_1')).toEqual(data); + expect(OnyxCache.getValue('test_2')).toEqual(data); + expect(OnyxCache.getValue('test_3')).toEqual(data); + expect(localforageMock.storageMap.test_1).toEqual(data); + expect(localforageMock.storageMap.test_2).toEqual(data); + expect(localforageMock.storageMap.test_3).toEqual(data); + + // When we drop all the cache keys (but do not modify the underlying storage) and merge another object + OnyxCache.drop('test_1'); + OnyxCache.drop('test_2'); + OnyxCache.drop('test_3'); + + const additionalData = {c: 'c'}; + Onyx.mergeCollection(ONYX_KEYS.COLLECTION.TEST_KEY, { + test_1: additionalData, + test_2: additionalData, + test_3: additionalData, + }); + + return waitForPromisesToResolve(); + }) + .then(() => { + const finalObject = { + a: 'a', b: 'b', c: 'c', + }; + + // Then our new data should merge with the existing data in the cache + expect(OnyxCache.getValue('test_1')).toEqual(finalObject); + expect(OnyxCache.getValue('test_2')).toEqual(finalObject); + expect(OnyxCache.getValue('test_3')).toEqual(finalObject); + + // And the storage should reflect the same state + expect(localforageMock.storageMap.test_1).toEqual(finalObject); + expect(localforageMock.storageMap.test_2).toEqual(finalObject); + expect(localforageMock.storageMap.test_3).toEqual(finalObject); + }); + }); + + it('setItem() and multiMerge()', () => { + expect(localforageMock.storageMap).toEqual({}); + + // Given no previous data and several calls to setItem and call to mergeCollection to update a given key + + // 1st call + Onyx.set('test_1', {a: 'a'}); + + // These merges will all queue together + Onyx.merge('test_1', {b: 'b'}); + Onyx.merge('test_1', {c: 'c'}); + + // 2nd call + Onyx.mergeCollection(ONYX_KEYS.COLLECTION.TEST_KEY, { + test_1: {d: 'd', e: 'e'}, + }); + + // Last call + Onyx.merge('test_1', {f: 'f'}); + return waitForPromisesToResolve() + .then(() => { + const finalObject = { + a: 'a', b: 'b', c: 'c', f: 'f', d: 'd', e: 'e', + }; + + expect(OnyxCache.getValue('test_1')).toEqual(finalObject); + expect(localforageMock.storageMap.test_1).toEqual(finalObject); + }); + }); +}); diff --git a/tests/unit/storage/providers/LocalForageProviderTest.js b/tests/unit/storage/providers/LocalForageProviderTest.js index 1940a09a1..c776a56d3 100644 --- a/tests/unit/storage/providers/LocalForageProviderTest.js +++ b/tests/unit/storage/providers/LocalForageProviderTest.js @@ -78,32 +78,30 @@ describe('storage/providers/LocalForage', () => { const setItemSpy = localforage.setItem.mockImplementation(() => Promise.resolve()); // When data is merged to storage - StorageProvider.multiMerge([ + return StorageProvider.multiMerge([ ['@USER_1', USER_1_DELTA], ['@USER_2', USER_2_DELTA] ]) .then(() => { // Then each existing item should be set with the merged content - expect(setItemSpy).toHaveBeenCalledWith( + expect(setItemSpy).toHaveBeenNthCalledWith(1, '@USER_1', { name: 'Tom', - age: 30, + age: 31, traits: { hair: 'brown', eyes: 'blue' } - } - ); + }); - expect(setItemSpy).toHaveBeenCalledWith( + expect(setItemSpy).toHaveBeenNthCalledWith(2, '@USER_2', { name: 'Sarah', - age: 31, + age: 26, traits: { hair: 'green', } - } - ); + }); }); }); });