From a343ef1da8b465b9da03f975ce0ef6b0f29db399 Mon Sep 17 00:00:00 2001 From: Peter Velkov Date: Tue, 25 May 2021 11:00:48 +0300 Subject: [PATCH 01/40] test: move metrics decoration tests to separate file --- tests/unit/onyxMetricsDecorationTest.js | 84 +++++++++++++++++++++++++ tests/unit/onyxTest.js | 76 ---------------------- 2 files changed, 84 insertions(+), 76 deletions(-) create mode 100644 tests/unit/onyxMetricsDecorationTest.js diff --git a/tests/unit/onyxMetricsDecorationTest.js b/tests/unit/onyxMetricsDecorationTest.js new file mode 100644 index 000000000..e2811a808 --- /dev/null +++ b/tests/unit/onyxMetricsDecorationTest.js @@ -0,0 +1,84 @@ +import waitForPromisesToResolve from '../utils/waitForPromisesToResolve'; + +describe('Onyx', () => { + describe('Metrics Capturing Decoration', () => { + const ONYX_KEYS = { + TEST_KEY: 'test', + ANOTHER_TEST: 'anotherTest', + }; + + // makes require calls to always return a "fresh" (undecorated) instance + beforeEach(() => jest.resetModules()); + + it('Should expose metrics methods when `captureMetrics` is true', () => { + // Get a fresh Onyx instance + const Onyx = require('../../index').default; + + // WHEN Onyx is initialized with `captureMetrics: true` + Onyx.init({ + keys: ONYX_KEYS, + registerStorageEventListener: jest.fn(), + captureMetrics: true, + }); + + // THEN Onyx should have statistic related methods + expect(Onyx.getMetrics).toEqual(expect.any(Function)); + expect(Onyx.printMetrics).toEqual(expect.any(Function)); + expect(Onyx.resetMetrics).toEqual(expect.any(Function)); + }); + + it('Should not expose metrics methods when `captureMetrics` is false or not set', () => { + // Get a fresh Onyx instance + const IsolatedOnyx = require('../../index').default; + + // WHEN Onyx is initialized without setting `captureMetrics` + IsolatedOnyx.init({ + keys: ONYX_KEYS, + registerStorageEventListener: jest.fn(), + }); + + // THEN Onyx should not have statistic related methods + expect(IsolatedOnyx.getMetrics).not.toBeDefined(); + expect(IsolatedOnyx.printMetrics).not.toBeDefined(); + expect(IsolatedOnyx.resetMetrics).not.toBeDefined(); + + // WHEN Onyx is initialized with `captureMetrics: false` + IsolatedOnyx.init({ + keys: ONYX_KEYS, + registerStorageEventListener: jest.fn(), + captureMetrics: false, + }); + + // THEN Onyx should not have statistic related methods + expect(IsolatedOnyx.getMetrics).not.toBeDefined(); + expect(IsolatedOnyx.printMetrics).not.toBeDefined(); + expect(IsolatedOnyx.resetMetrics).not.toBeDefined(); + }); + + it('Should decorate exposed methods', () => { + // Get a fresh Onyx instance + const IsolatedOnyx = require('../../index').default; + + // GIVEN Onyx is initialized with `captureMetrics: true` + IsolatedOnyx.init({ + keys: ONYX_KEYS, + registerStorageEventListener: jest.fn(), + captureMetrics: true, + }); + + // WHEN calling decorated methods through Onyx[methodName] + const methods = ['set', 'multiSet', 'clear', 'merge', 'mergeCollection']; + methods.forEach(name => IsolatedOnyx[name]('mockKey', {mockKey: {mockValue: 'mockValue'}})); + + return waitForPromisesToResolve() + .then(() => { + // THEN metrics should have captured data for each method + const summaries = IsolatedOnyx.getMetrics().summaries; + + methods.forEach((name) => { + expect(summaries[`Onyx:${name}`].total).toBeGreaterThan(0); + }); + }); + }); + }); +}); diff --git a/tests/unit/onyxTest.js b/tests/unit/onyxTest.js index 4ee1a8149..1740a1747 100644 --- a/tests/unit/onyxTest.js +++ b/tests/unit/onyxTest.js @@ -264,80 +264,4 @@ describe('Onyx', () => { expect(error.message).toEqual(`Provided collection does not have all its data belonging to the same parent. CollectionKey: ${ONYX_KEYS.COLLECTION.TEST_KEY}, DataKey: not_my_test`); } }); - - describe('captureMetrics', () => { - // makes require calls to always return a "fresh" (undecorated) instance - beforeEach(() => jest.resetModules()); - - it('Should expose metrics methods when `captureMetrics` is true', () => { - // Run in isolation so the top import is unaffected - const IsolatedOnyx = require('../../index').default; - - // WHEN Onyx is initialized with `captureMetrics: true` - IsolatedOnyx.init({ - keys: ONYX_KEYS, - registerStorageEventListener: jest.fn(), - captureMetrics: true, - }); - - // THEN Onyx should have statistic related methods - expect(IsolatedOnyx.getMetrics).toEqual(expect.any(Function)); - expect(IsolatedOnyx.printMetrics).toEqual(expect.any(Function)); - expect(IsolatedOnyx.resetMetrics).toEqual(expect.any(Function)); - }); - - it('Should not expose metrics methods when `captureMetrics` is false or not set', () => { - // Run in isolation so the top import is unaffected - const IsolatedOnyx = require('../../index').default; - - // WHEN Onyx is initialized without setting `captureMetrics` - IsolatedOnyx.init({ - keys: ONYX_KEYS, - registerStorageEventListener: jest.fn(), - }); - - // THEN Onyx should not have statistic related methods - expect(IsolatedOnyx.getMetrics).not.toBeDefined(); - expect(IsolatedOnyx.printMetrics).not.toBeDefined(); - expect(IsolatedOnyx.resetMetrics).not.toBeDefined(); - - // WHEN Onyx is initialized with `captureMetrics: false` - IsolatedOnyx.init({ - keys: ONYX_KEYS, - registerStorageEventListener: jest.fn(), - captureMetrics: false, - }); - - // THEN Onyx should not have statistic related methods - expect(IsolatedOnyx.getMetrics).not.toBeDefined(); - expect(IsolatedOnyx.printMetrics).not.toBeDefined(); - expect(IsolatedOnyx.resetMetrics).not.toBeDefined(); - }); - - it('Should decorate exposed methods', () => { - // Run in isolation so the top import is unaffected - const IsolatedOnyx = require('../../index').default; - - // GIVEN Onyx is initialized with `captureMetrics: true` - IsolatedOnyx.init({ - keys: ONYX_KEYS, - registerStorageEventListener: jest.fn(), - captureMetrics: true, - }); - - // WHEN calling decorated methods through Onyx[methodName] - const methods = ['set', 'multiSet', 'clear', 'merge', 'mergeCollection']; - methods.forEach(name => IsolatedOnyx[name]('mockKey', {mockKey: {mockValue: 'mockValue'}})); - - return waitForPromisesToResolve() - .then(() => { - // THEN metrics should have captured data for each method - const summaries = IsolatedOnyx.getMetrics().summaries; - - methods.forEach((name) => { - expect(summaries[`Onyx:${name}`].total).toBeGreaterThan(0); - }); - }); - }); - }); }); From a7ee841f33dfd1007887c6cb3944f68d514adb2b Mon Sep 17 00:00:00 2001 From: Peter Velkov Date: Tue, 25 May 2021 11:10:30 +0300 Subject: [PATCH 02/40] test: move Onyx require call to `beforeEach` --- tests/unit/onyxMetricsDecorationTest.js | 40 +++++++++++-------------- 1 file changed, 18 insertions(+), 22 deletions(-) diff --git a/tests/unit/onyxMetricsDecorationTest.js b/tests/unit/onyxMetricsDecorationTest.js index e2811a808..d7b1ce65e 100644 --- a/tests/unit/onyxMetricsDecorationTest.js +++ b/tests/unit/onyxMetricsDecorationTest.js @@ -2,18 +2,20 @@ import waitForPromisesToResolve from '../utils/waitForPromisesToResolve'; describe('Onyx', () => { describe('Metrics Capturing Decoration', () => { + let Onyx; + const ONYX_KEYS = { TEST_KEY: 'test', ANOTHER_TEST: 'anotherTest', }; - // makes require calls to always return a "fresh" (undecorated) instance - beforeEach(() => jest.resetModules()); + // Always use a "fresh" (and undecorated) instance + beforeEach(() => { + jest.resetModules(); + Onyx = require('../../index').default; + }); it('Should expose metrics methods when `captureMetrics` is true', () => { - // Get a fresh Onyx instance - const Onyx = require('../../index').default; - // WHEN Onyx is initialized with `captureMetrics: true` Onyx.init({ keys: ONYX_KEYS, @@ -28,39 +30,33 @@ describe('Onyx', () => { }); it('Should not expose metrics methods when `captureMetrics` is false or not set', () => { - // Get a fresh Onyx instance - const IsolatedOnyx = require('../../index').default; - // WHEN Onyx is initialized without setting `captureMetrics` - IsolatedOnyx.init({ + Onyx.init({ keys: ONYX_KEYS, registerStorageEventListener: jest.fn(), }); // THEN Onyx should not have statistic related methods - expect(IsolatedOnyx.getMetrics).not.toBeDefined(); - expect(IsolatedOnyx.printMetrics).not.toBeDefined(); - expect(IsolatedOnyx.resetMetrics).not.toBeDefined(); + expect(Onyx.getMetrics).not.toBeDefined(); + expect(Onyx.printMetrics).not.toBeDefined(); + expect(Onyx.resetMetrics).not.toBeDefined(); // WHEN Onyx is initialized with `captureMetrics: false` - IsolatedOnyx.init({ + Onyx.init({ keys: ONYX_KEYS, registerStorageEventListener: jest.fn(), captureMetrics: false, }); // THEN Onyx should not have statistic related methods - expect(IsolatedOnyx.getMetrics).not.toBeDefined(); - expect(IsolatedOnyx.printMetrics).not.toBeDefined(); - expect(IsolatedOnyx.resetMetrics).not.toBeDefined(); + expect(Onyx.getMetrics).not.toBeDefined(); + expect(Onyx.printMetrics).not.toBeDefined(); + expect(Onyx.resetMetrics).not.toBeDefined(); }); it('Should decorate exposed methods', () => { - // Get a fresh Onyx instance - const IsolatedOnyx = require('../../index').default; - // GIVEN Onyx is initialized with `captureMetrics: true` - IsolatedOnyx.init({ + Onyx.init({ keys: ONYX_KEYS, registerStorageEventListener: jest.fn(), captureMetrics: true, @@ -68,12 +64,12 @@ describe('Onyx', () => { // WHEN calling decorated methods through Onyx[methodName] const methods = ['set', 'multiSet', 'clear', 'merge', 'mergeCollection']; - methods.forEach(name => IsolatedOnyx[name]('mockKey', {mockKey: {mockValue: 'mockValue'}})); + methods.forEach(name => Onyx[name]('mockKey', {mockKey: {mockValue: 'mockValue'}})); return waitForPromisesToResolve() .then(() => { // THEN metrics should have captured data for each method - const summaries = IsolatedOnyx.getMetrics().summaries; + const summaries = Onyx.getMetrics().summaries; methods.forEach((name) => { expect(summaries[`Onyx:${name}`].total).toBeGreaterThan(0); From d23b8eba07dfbd05a2830b6682b2821252a403e8 Mon Sep 17 00:00:00 2001 From: Peter Velkov Date: Tue, 25 May 2021 11:24:19 +0300 Subject: [PATCH 03/40] test: fix `withOnyx` test not returning promise --- tests/unit/withOnyxTest.js | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/tests/unit/withOnyxTest.js b/tests/unit/withOnyxTest.js index 440786474..6053bac4b 100644 --- a/tests/unit/withOnyxTest.js +++ b/tests/unit/withOnyxTest.js @@ -25,7 +25,7 @@ describe('withOnyx', () => { it('should render with the test data when using withOnyx', () => { let result; - Onyx.set(ONYX_KEYS.TEST_KEY, 'test1') + return Onyx.set(ONYX_KEYS.TEST_KEY, 'test1') .then(() => { const TestComponentWithOnyx = withOnyx({ text: { @@ -38,8 +38,7 @@ describe('withOnyx', () => { }) .then(() => { const textComponent = result.getByText('test1'); - expect(textComponent).toBeTruthy(); - expect(result).toHaveBeenCalledTimes(999); + expect(textComponent).not.toBeNull(); }); }); From a6df3db5b67132ec9eccbf3996bbe63740aa1651 Mon Sep 17 00:00:00 2001 From: Peter Velkov Date: Tue, 25 May 2021 12:45:06 +0300 Subject: [PATCH 04/40] test: add a test suite for Onyx cache --- tests/unit/onyxCacheTest.js | 114 ++++++++++++++++++++++++++++++++++++ 1 file changed, 114 insertions(+) create mode 100644 tests/unit/onyxCacheTest.js diff --git a/tests/unit/onyxCacheTest.js b/tests/unit/onyxCacheTest.js new file mode 100644 index 000000000..c2f9653ec --- /dev/null +++ b/tests/unit/onyxCacheTest.js @@ -0,0 +1,114 @@ +import React from 'react'; +import {render} from '@testing-library/react-native'; + +import waitForPromisesToResolve from '../utils/waitForPromisesToResolve'; +import ViewWithText from '../components/ViewWithText'; + +describe('Onyx', () => { + describe('Cache', () => { + let Onyx; + let withOnyx; + + const ONYX_KEYS = { + TEST_KEY: 'test', + ANOTHER_TEST: 'anotherTest', + }; + + // Always use a "fresh" (and undecorated) instance + beforeEach(() => { + jest.resetModules(); + const OnyxModule = require('../../index'); + Onyx = OnyxModule.default; + withOnyx = OnyxModule.withOnyx; + + Onyx.init({ + keys: ONYX_KEYS, + registerStorageEventListener: jest.fn(), + }); + }); + + it('Expect a single call to AsyncStorage.getItem when multiple components use the same key', async () => { + const AsyncStorageMock = require('@react-native-community/async-storage/jest/async-storage-mock'); + + // GIVEN a component connected to Onyx + const TestComponentWithOnyx = withOnyx({ + text: { + key: ONYX_KEYS.TEST_KEY, + }, + })(ViewWithText); + + // GIVEN some data about that key exists in Onyx + await Onyx.set(ONYX_KEYS.TEST_KEY, 'mockValue'); + await waitForPromisesToResolve(); + + // WHEN multiple components are rendered + render( + <> + + + + + ); + + // THEN Async storage `getItem` should be called only once + await waitForPromisesToResolve(); + expect(AsyncStorageMock.getItem).toHaveBeenCalledTimes(1); + }); + + it('Expect a single call to AsyncStorage.getAllKeys when multiple components use the same key', async () => { + const AsyncStorageMock = require('@react-native-community/async-storage/jest/async-storage-mock'); + + // GIVEN a component connected to Onyx + const TestComponentWithOnyx = withOnyx({ + text: { + key: ONYX_KEYS.TEST_KEY, + }, + })(ViewWithText); + + // GIVEN some data about that key exists in Onyx + await Onyx.set(ONYX_KEYS.TEST_KEY, 'mockValue'); + await waitForPromisesToResolve(); + + // WHEN multiple components are rendered + render( + <> + + + + + ); + + // THEN Async storage `getItem` should be called only once + await waitForPromisesToResolve(); + expect(AsyncStorageMock.getAllKeys).toHaveBeenCalledTimes(1); + }); + + it('Expect multiple calls to AsyncStorage.getItem when no existing component is using a key', async () => { + const AsyncStorageMock = require('@react-native-community/async-storage/jest/async-storage-mock'); + + // GIVEN a component connected to Onyx + const TestComponentWithOnyx = withOnyx({ + text: { + key: ONYX_KEYS.TEST_KEY, + }, + })(ViewWithText); + + // GIVEN some data about that key exists in Onyx + await Onyx.set(ONYX_KEYS.TEST_KEY, 'mockValue'); + await waitForPromisesToResolve(); + + // WHEN a component is rendered and unmounted and no longer available + const result = render(); + await waitForPromisesToResolve(); + result.unmount(); + await waitForPromisesToResolve(); + + // THEN When another component using the same storage key is rendered + render(); + + // THEN Async storage `getItem` should be called twice + await waitForPromisesToResolve(); + expect(AsyncStorageMock.getItem).toHaveBeenCalledTimes(2); + }); + }); +}); From 231fe00d5cac703275f6121710bca0acdc782aeb Mon Sep 17 00:00:00 2001 From: Peter Velkov Date: Tue, 25 May 2021 16:12:06 +0300 Subject: [PATCH 05/40] feat: Add separate service for cache --- lib/OnyxCache.js | 130 +++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 130 insertions(+) create mode 100644 lib/OnyxCache.js diff --git a/lib/OnyxCache.js b/lib/OnyxCache.js new file mode 100644 index 000000000..abbac4518 --- /dev/null +++ b/lib/OnyxCache.js @@ -0,0 +1,130 @@ +import _ from 'underscore'; + +const isDefined = _.negate(_.isUndefined); + +/** + * Encapsulates Onyx cache related functionality + */ +class OnyxCache { + constructor() { + /** + * @private + * Cache of all the storage keys available in persistent storage + * @type {Set} + */ + this.storageKeys = new Set(); + + /** + * @private + * A map of cached values + * @type {Record} + */ + this.storageMap = {}; + + /** + * @private + * Captured pending tasks for already running storage methods + * @type {Record} + */ + this.pendingPromises = {}; + + // bind all methods to prevent problems with `this` + _.bindAll( + this, + 'getAllKeys', 'getValue', 'hasCacheForKey', 'update', 'remove', 'merge', 'resolveTask', + ); + } + + /** + * Get all the storage keys + * @returns {string[]} + */ + getAllKeys() { + return [...this.storageKeys]; + } + + /** + * Get a cached value from storage + * @param {string} key + * @returns {*} + */ + getValue(key) { + return this.storageMap[key]; + } + + /** + * Check whether cache has data for the given key + * @param {string} key + * @returns {boolean} + */ + hasCacheForKey(key) { + return key in this.storageMap; + } + + /** + * Update or add data to cache + * @param {string} key + * @param {*} [value] + */ + update(key, value) { + // Update all storage keys + this.storageKeys.add(key); + + // When value is provided update general cache as well + if (isDefined(value)) { + this.storageMap[key] = value; + } + } + + /** + * Remove data from cache + * @param {string} key + */ + remove(key) { + this.storageKeys.delete(key); + delete this.storageMap[key]; + } + + /** + * Merge data to cache, any non existing keys will be crated + * @param {Array<[string, Object]>} pairs - array of key value pairs + */ + merge(pairs) { + _.forEach(pairs, ([key, value]) => { + const currentValue = _.result(this.storageMap, key, {}); + const merged = {...currentValue, ...value}; + this.update(key, merged); + }); + } + + /** + * Use this method to prevents additional calls from the same thing + * When a promise for a given call already exists the caller would + * be hooked to it. Otherwise the task is started and captured so + * that other callers don't have to start over + * Particularly useful for data retrieving methods + * @param {string} taskName + * @param {function(): Promise} startTask - Provide a promise returning function that + * will be invoked if there is no pending promise for this task + * @returns {Promise} + */ + resolveTask(taskName, startTask) { + // When a task is already running return it right away + if (isDefined(this.pendingPromises[taskName])) { + return this.pendingPromises[taskName]; + } + + // Otherwise start the task and store a reference + this.pendingPromises[taskName] = startTask() + .finally(() => { + // Cleanup after the task is over + delete this.pendingPromises[taskName]; + }); + + return this.pendingPromises[taskName]; + } +} + +const instance = new OnyxCache(); + +export default instance; From 2f76d80df10e7c6d5be4a86fa60956c74fc77789 Mon Sep 17 00:00:00 2001 From: Peter Velkov Date: Tue, 25 May 2021 17:45:07 +0300 Subject: [PATCH 06/40] test: add OnyxCache tests --- tests/unit/onyxCacheTest.js | 273 +++++++++++++++++++++++++++++++++++- 1 file changed, 272 insertions(+), 1 deletion(-) diff --git a/tests/unit/onyxCacheTest.js b/tests/unit/onyxCacheTest.js index c2f9653ec..6db1f8a5f 100644 --- a/tests/unit/onyxCacheTest.js +++ b/tests/unit/onyxCacheTest.js @@ -5,7 +5,278 @@ import waitForPromisesToResolve from '../utils/waitForPromisesToResolve'; import ViewWithText from '../components/ViewWithText'; describe('Onyx', () => { - describe('Cache', () => { + describe('Cache Service', () => { + /** @type OnyxCache */ + let cache; + + // Always use a "fresh" instance + beforeEach(() => { + jest.resetModules(); + cache = require('../../lib/OnyxCache').default; + }); + + describe('getAllKeys', () => { + it('Should be empty initially', () => { + // GIVEN empty cache + + // WHEN all keys are retrieved + const allKeys = cache.getAllKeys(); + + // THEN the result should be empty + expect(allKeys).toEqual([]); + }); + + it('Should keep storage keys', () => { + // GIVEN cache with some items + cache.update('mockKey', 'mockValue'); + cache.update('mockKey2', 'mockValue'); + cache.update('mockKey3', 'mockValue'); + + // THEN the keys should be stored in cache + const allKeys = cache.getAllKeys(); + expect(allKeys).toEqual(['mockKey', 'mockKey2', 'mockKey3']); + }); + + it('Should keep storage keys even when no values are provided', () => { + // GIVEN cache with some items + cache.update('mockKey'); + cache.update('mockKey2'); + cache.update('mockKey3'); + + // THEN the keys should be stored in cache + const allKeys = cache.getAllKeys(); + expect(allKeys).toEqual(['mockKey', 'mockKey2', 'mockKey3']); + }); + + it('Should not store duplicate keys', () => { + // GIVEN cache with some items + cache.update('mockKey', 'mockValue'); + cache.update('mockKey2', 'mockValue'); + cache.update('mockKey3', 'mockValue'); + + // WHEN an existing keys is later updated + cache.update('mockKey2', 'new mock value'); + + // THEN getAllKeys should not include a duplicate value + const allKeys = cache.getAllKeys(); + expect(allKeys).toEqual(['mockKey', 'mockKey2', 'mockKey3']); + }); + }); + + describe('getValue', () => { + it('Should return undefined when there is no stored value', () => { + // GIVEN empty cache + + // WHEN a value is retrieved + const result = cache.getValue('mockKey'); + + // THEN it should be undefined + expect(result).not.toBeDefined(); + }); + + it('Should return cached value when it exists', () => { + // GIVEN cache with some items + cache.update('mockKey', {items: ['mockValue', 'mockValue2']}); + cache.update('mockKey2', 'mockValue3'); + + // WHEN a value is retrieved + // THEN it should be the correct value + expect(cache.getValue('mockKey')).toEqual({items: ['mockValue', 'mockValue2']}); + expect(cache.getValue('mockKey2')).toEqual('mockValue3'); + }); + }); + + describe('hasCacheForKey', () => { + it('Should return false when there is no stored value', () => { + // GIVEN empty cache + + // WHEN a value does not exist in cache + // THEN it should return false + expect(cache.hasCacheForKey('mockKey')).toBe(false); + }); + + it('Should return cached value when it exists', () => { + // GIVEN cache with some items + cache.update('mockKey', {items: ['mockValue', 'mockValue2']}); + cache.update('mockKey2', 'mockValue3'); + + // WHEN a value exists in cache + // THEN it should return true + expect(cache.hasCacheForKey('mockKey')).toBe(true); + expect(cache.hasCacheForKey('mockKey2')).toBe(true); + }); + }); + + describe('update', () => { + it('Should add data to cache when both key and value are provided', () => { + // GIVEN empty cache + + // WHEN update is called with key and value + cache.update('mockKey', {value: 'mockValue'}); + + // THEN data should be cached + expect(cache.getValue('mockKey')).toEqual({value: 'mockValue'}); + }); + + it('Should only store the key when no value is provided', () => { + // GIVEN empty cache + + // WHEN update is called with key and value + cache.update('mockKey'); + + // THEN there should be no cached value + expect(cache.getValue('mockKey')).not.toBeDefined(); + + // THEN but a key should be available + expect(cache.getAllKeys()).toEqual(expect.arrayContaining(['mockKey'])); + }); + + it('Should overwrite existing cache items for the given key', () => { + // GIVEN cache with some items + cache.update('mockKey', {value: 'mockValue'}); + cache.update('mockKey2', {other: 'otherMockValue'}); + + // WHEN update is called for an existing key + cache.update('mockKey2', {value: []}); + + // THEN the value should be overwritten + expect(cache.getValue('mockKey2')).toEqual({value: []}); + }); + }); + + describe('remove', () => { + it('Should remove the key from all keys', () => { + // GIVEN cache with some items + cache.update('mockKey', 'mockValue'); + cache.update('mockKey2', 'mockValue'); + cache.update('mockKey3', 'mockValue'); + + // WHEN an key is removed + cache.remove('mockKey2'); + + // THEN getAllKeys should not include the removed value + const allKeys = cache.getAllKeys(); + expect(allKeys).toEqual(['mockKey', 'mockKey3']); + }); + + it('Should remove the key from cache', () => { + // GIVEN cache with some items + cache.update('mockKey', {items: ['mockValue', 'mockValue2']}); + cache.update('mockKey2', 'mockValue3'); + + // WHEN a key is removed + cache.remove('mockKey'); + + // THEN a value should not be available in cache + expect(cache.hasCacheForKey('mockKey')).toBe(false); + expect(cache.getValue('mockKey')).not.toBeDefined(); + }); + }); + + describe('merge', () => { + it('Should create the value in cache when it does not exist', () => { + // GIVEN empty cache + + // WHEN merge is called with key value pairs + cache.merge([ + ['mockKey', {value: 'mockValue'}], + ['mockKey2', {value: 'mockValue2'}], + ]); + + // THEN data should be created in cache + expect(cache.getValue('mockKey')).toEqual({value: 'mockValue'}); + expect(cache.getValue('mockKey2')).toEqual({value: 'mockValue2'}); + }); + + it('Should merge data to existing cache value', () => { + // GIVEN cache with some items + cache.update('mockKey', {value: 'mockValue'}); + cache.update('mockKey2', {other: 'otherMockValue', mock: 'mock'}); + + // WHEN merge is called with key value pairs + cache.merge([ + ['mockKey', {mockItems: []}], + ['mockKey2', {items: [1, 2], other: 'overwrittenMockValue'}] + ]); + + // THEN the values should be merged together in cache + expect(cache.getValue('mockKey')).toEqual({ + value: 'mockValue', + mockItems: [], + }); + + expect(cache.getValue('mockKey2')).toEqual({ + other: 'overwrittenMockValue', + items: [1, 2], + mock: 'mock', + }); + }); + }); + + describe('resolveTask', () => { + it('Should start a new task when no pending task exists', () => { + // GIVEN empty cache and a function returning a promise + const task = jest.fn().mockResolvedValue({data: 'mockData'}); + + // WHEN resolve task is called with this task + cache.resolveTask('mockTask', task); + + // THEN the task should be triggered + expect(task).toHaveBeenCalledTimes(1); + }); + + it('Should not start a task again when it is already captured and running', () => { + // GIVEN cache that have captured a promise for a pending task + const task = jest.fn().mockResolvedValue({data: 'mockData'}); + cache.resolveTask('mockTask', task); + task.mockClear(); + + // WHEN a function tries to run the same task + cache.resolveTask('mockTask', task); + cache.resolveTask('mockTask', task); + cache.resolveTask('mockTask', task); + + // THEN the task should not have been called again + expect(task).not.toHaveBeenCalled(); + }); + + it('Should start a new task when a previous task was completed and removed', async () => { + // GIVEN cache that have captured a promise for a pending task + const task = jest.fn().mockResolvedValue({data: 'mockData'}); + cache.resolveTask('mockTask', task); + task.mockClear(); + + // WHEN the task is completed + await waitForPromisesToResolve(); + + // WHEN a function tries to run the same task + cache.resolveTask('mockTask', task); + + // THEN a new task should be started + expect(task).toHaveBeenCalledTimes(1); + }); + + it('Should resolve all tasks with the same result', () => { + // GIVEN empty cache and a function returning a promise + const task = jest.fn().mockResolvedValue({data: 'mockData'}); + + // WHEN multiple tasks are executed at the same time + const promise1 = cache.resolveTask('mockTask', task); + const promise2 = cache.resolveTask('mockTask', task); + const promise3 = cache.resolveTask('mockTask', task); + + // THEN they all should have the same result + Promise.all([promise1, promise2, promise3]) + .then(([result1, result2, result3]) => { + expect(result1).toEqual({data: 'mockData'}); + expect(result2).toEqual({data: 'mockData'}); + expect(result3).toEqual({data: 'mockData'}); + }); + }); + }); + }); + + describe('Onyx with Cache', () => { let Onyx; let withOnyx; From 104678615bfe35e0a32d494229bbe3200e442c64 Mon Sep 17 00:00:00 2001 From: Peter Velkov Date: Tue, 25 May 2021 18:14:09 +0300 Subject: [PATCH 07/40] feat: Use OnyxCache to cache storage operations --- lib/Onyx.js | 75 +++++++++++++++++++++++++++++++++++++++--------- lib/OnyxCache.js | 1 + 2 files changed, 62 insertions(+), 14 deletions(-) diff --git a/lib/Onyx.js b/lib/Onyx.js index 5210a3347..aee8611eb 100644 --- a/lib/Onyx.js +++ b/lib/Onyx.js @@ -3,6 +3,7 @@ import AsyncStorage from '@react-native-community/async-storage'; import Str from 'expensify-common/lib/str'; import lodashMerge from 'lodash/merge'; import {registerLogger, logInfo, logAlert} from './Logger'; +import cache from './OnyxCache'; // Keeps track of the last connectionID that was used so we can keep incrementing it let lastConnectionID = 0; @@ -34,9 +35,21 @@ let defaultKeyStates = {}; * @returns {Promise<*>} */ function get(key) { - return AsyncStorage.getItem(key) - .then(val => JSON.parse(val)) - .catch(err => logInfo(`Unable to get item from persistent storage. Key: ${key} Error: ${err}`)); + // When we already have the value in cache - resolve right away + if (cache.hasCacheForKey(key)) { + return Promise.resolve(cache.getValue(key)); + } + + /* Otherwise setup a task to retrieve it. + This ensures concurrent usages would not start the same task */ + return cache.resolveTask('get', () => AsyncStorage.getItem(key) + .then((val) => { + // Add values to cache and return parsed result + const parsed = JSON.parse(val); + cache.update(key, parsed); + return parsed; + }) + .catch(err => logInfo(`Unable to get item from persistent storage. Key: ${key} Error: ${err}`))); } /** @@ -44,7 +57,20 @@ function get(key) { * @returns {Promise} */ function getAllKeys() { - return AsyncStorage.getAllKeys(); + // When we've already read stored keys, resolve right away + const storedKeys = cache.getAllKeys(); + if (storedKeys.length > 0) { + return Promise.resolve(storedKeys); + } + + /* Otherwise setup a task to retrieve them. + This ensures concurrent usages would not start the same task */ + return cache.resolveTask('getAllKeys', () => AsyncStorage.getAllKeys() + .then((keys) => { + // Add values to cache and return original result + _.each(keys, key => cache.update(key)); + return keys; + })); } /** @@ -377,8 +403,13 @@ function disconnect(connectionID, keyToRemoveFromEvictionBlocklist) { * @return {Promise} */ function remove(key) { - return AsyncStorage.removeItem(key) - .then(() => keyChanged(key, null)); + // Remove from cache + cache.remove(key); + + // Optimistically inform subscribers + keyChanged(key, null); + + return AsyncStorage.removeItem(key); } /** @@ -414,9 +445,14 @@ function evictStorageAndRetry(error, ionMethod, ...args) { * @returns {Promise} */ function set(key, val) { + // Adds the key to cache when it's not available + cache.update(key, val); + + // Optimistically inform subscribers + keyChanged(key, val); + // Write the thing to persistent storage, which will trigger a storage event for any other tabs open on this domain return AsyncStorage.setItem(key, JSON.stringify(val)) - .then(() => keyChanged(key, val)) .catch(error => evictStorageAndRetry(error, set, key, val)); } @@ -428,6 +464,7 @@ function set(key, val) { * @returns {Promise} */ function multiSet(data) { + // Todo: use _.pairs for the same result // AsyncStorage expenses the data in an array like: // [["@MyApp_user", "value_1"], ["@MyApp_key", "value_2"]] // This method will transform the params from a better JSON format like: @@ -437,8 +474,13 @@ function multiSet(data) { [key, JSON.stringify(val)], ]), []); + // Capture any changes to cache + cache.merge(keyValuePairs); + + // Optimistically inform subscribers + _.each(data, (val, key) => keyChanged(key, val)); + return AsyncStorage.multiSet(keyValuePairs) - .then(() => _.each(data, (val, key) => keyChanged(key, val))) .catch(error => evictStorageAndRetry(error, multiSet, data)); } @@ -522,13 +564,11 @@ function initializeWithDefaultKeyStates() { * @returns {Promise} */ function clear() { - let allKeys; return getAllKeys() - .then(keys => allKeys = keys) - .then(() => AsyncStorage.clear()) - .then(() => { - _.each(allKeys, (key) => { + .then((keys) => { + _.each(keys, (key) => { keyChanged(key, null); + cache.remove(key); }); initializeWithDefaultKeyStates(); @@ -563,6 +603,7 @@ function mergeCollection(collectionKey, collection) { } }); + // Todo: use _.pairs for the same result const keyValuePairsForExistingCollection = _.reduce(existingKeyCollection, (finalArray, val, key) => ([ ...finalArray, [key, JSON.stringify(val)], @@ -577,8 +618,14 @@ function mergeCollection(collectionKey, collection) { const existingCollectionPromise = AsyncStorage.multiMerge(keyValuePairsForExistingCollection); const newCollectionPromise = AsyncStorage.multiSet(keyValuePairsForNewCollection); + // Capture any changes to cache + cache.merge(keyValuePairsForExistingCollection); + cache.merge(keyValuePairsForNewCollection); + + // Optimistically inform subscribers + keysChanged(collectionKey, collection); + return Promise.all([existingCollectionPromise, newCollectionPromise]) - .then(() => keysChanged(collectionKey, collection)) .catch(error => evictStorageAndRetry(error, mergeCollection, collection)); }); } diff --git a/lib/OnyxCache.js b/lib/OnyxCache.js index abbac4518..9670ee43d 100644 --- a/lib/OnyxCache.js +++ b/lib/OnyxCache.js @@ -3,6 +3,7 @@ import _ from 'underscore'; const isDefined = _.negate(_.isUndefined); /** + * In memory cache providing data by reference * Encapsulates Onyx cache related functionality */ class OnyxCache { From 273afe1e9d9c0345f5bf706ec545865668b32b78 Mon Sep 17 00:00:00 2001 From: Peter Velkov Date: Tue, 25 May 2021 19:10:37 +0300 Subject: [PATCH 08/40] test: update Onyx with Cache to run correctly --- tests/unit/onyxCacheTest.js | 36 ++++++++++++++++++++++++------------ 1 file changed, 24 insertions(+), 12 deletions(-) diff --git a/tests/unit/onyxCacheTest.js b/tests/unit/onyxCacheTest.js index 6db1f8a5f..cb8fe3e78 100644 --- a/tests/unit/onyxCacheTest.js +++ b/tests/unit/onyxCacheTest.js @@ -285,9 +285,7 @@ describe('Onyx', () => { ANOTHER_TEST: 'anotherTest', }; - // Always use a "fresh" (and undecorated) instance - beforeEach(() => { - jest.resetModules(); + async function initOnyx() { const OnyxModule = require('../../index'); Onyx = OnyxModule.default; withOnyx = OnyxModule.withOnyx; @@ -296,6 +294,17 @@ describe('Onyx', () => { keys: ONYX_KEYS, registerStorageEventListener: jest.fn(), }); + + // Onyx init introduces some side effects e.g. calls the getAllKeys + // We're clearing to have a fresh mock calls history + await waitForPromisesToResolve(); + jest.clearAllMocks(); + } + + // Always use a "fresh" (and undecorated) instance + beforeEach(() => { + jest.resetModules(); + return initOnyx(); }); it('Expect a single call to AsyncStorage.getItem when multiple components use the same key', async () => { @@ -308,9 +317,10 @@ describe('Onyx', () => { }, })(ViewWithText); - // GIVEN some data about that key exists in Onyx - await Onyx.set(ONYX_KEYS.TEST_KEY, 'mockValue'); - await waitForPromisesToResolve(); + // GIVEN some string value for that key exists in storage + AsyncStorageMock.getItem.mockResolvedValue('"mockValue"'); + AsyncStorageMock.getAllKeys.mockResolvedValue([ONYX_KEYS.TEST_KEY]); + await initOnyx(); // WHEN multiple components are rendered render( @@ -336,9 +346,10 @@ describe('Onyx', () => { }, })(ViewWithText); - // GIVEN some data about that key exists in Onyx - await Onyx.set(ONYX_KEYS.TEST_KEY, 'mockValue'); - await waitForPromisesToResolve(); + // GIVEN some string value for that key exists in storage + await initOnyx(); + AsyncStorageMock.getItem.mockResolvedValue('"mockValue"'); + AsyncStorageMock.getAllKeys.mockResolvedValue([ONYX_KEYS.TEST_KEY]); // WHEN multiple components are rendered render( @@ -364,9 +375,10 @@ describe('Onyx', () => { }, })(ViewWithText); - // GIVEN some data about that key exists in Onyx - await Onyx.set(ONYX_KEYS.TEST_KEY, 'mockValue'); - await waitForPromisesToResolve(); + // GIVEN some string value for that key exists in storage + AsyncStorageMock.getItem.mockResolvedValue('"mockValue"'); + AsyncStorageMock.getAllKeys.mockResolvedValue([ONYX_KEYS.TEST_KEY]); + await initOnyx(); // WHEN a component is rendered and unmounted and no longer available const result = render(); From 47739e4927183af5b7ccb57cd5d673530c030ad7 Mon Sep 17 00:00:00 2001 From: Peter Velkov Date: Tue, 25 May 2021 21:21:53 +0300 Subject: [PATCH 09/40] refactor: cache.merge should throw when a value is not an object or array --- lib/OnyxCache.js | 10 ++++++++-- tests/unit/onyxCacheTest.js | 10 ++++++++++ 2 files changed, 18 insertions(+), 2 deletions(-) diff --git a/lib/OnyxCache.js b/lib/OnyxCache.js index 9670ee43d..2643d1b22 100644 --- a/lib/OnyxCache.js +++ b/lib/OnyxCache.js @@ -93,8 +93,14 @@ class OnyxCache { merge(pairs) { _.forEach(pairs, ([key, value]) => { const currentValue = _.result(this.storageMap, key, {}); - const merged = {...currentValue, ...value}; - this.update(key, merged); + + if (_.isObject(value) || _.isArray(value)) { + const merged = {...currentValue, ...value}; + this.update(key, merged); + return; + } + + throw new Error(`The provided merge value is invalid: ${value}`); }); } diff --git a/tests/unit/onyxCacheTest.js b/tests/unit/onyxCacheTest.js index cb8fe3e78..9688e24f6 100644 --- a/tests/unit/onyxCacheTest.js +++ b/tests/unit/onyxCacheTest.js @@ -211,6 +211,16 @@ describe('Onyx', () => { mock: 'mock', }); }); + + it('Should throw when the value parameter is not an object or array', () => { + // GIVEN cache with some items + cache.update('mockKey', 'someStringValue'); + + const badMergeValue = [['mockKey', 'usually we do not want to merge strings right?']]; + + // WHEN merge is not called with array or an object + expect(() => cache.merge(badMergeValue)).toThrow('The provided merge value is invalid'); + }); }); describe('resolveTask', () => { From f4a523fb06658121a5633045ed7e1c08e5b5a69e Mon Sep 17 00:00:00 2001 From: Peter Velkov Date: Tue, 25 May 2021 21:40:36 +0300 Subject: [PATCH 10/40] refactor: extract prepareKeyValuePairsForStorage method Fixed an issue with merge collections to cache --- lib/Onyx.js | 45 ++++++++++++++++++++------------------------- 1 file changed, 20 insertions(+), 25 deletions(-) diff --git a/lib/Onyx.js b/lib/Onyx.js index aee8611eb..b57fcabf9 100644 --- a/lib/Onyx.js +++ b/lib/Onyx.js @@ -456,6 +456,17 @@ function set(key, val) { .catch(error => evictStorageAndRetry(error, set, key, val)); } +/** + * AsyncStorage expects array like: [["@MyApp_user", "value_1"], ["@MyApp_key", "value_2"]] + * This method transforms an object like {'@MyApp_user': 'myUserValue', '@MyApp_key': 'myKeyValue'} + * to an array of key-value pairs in the above format + * @param {Record} data + * @return {Array<[string, string]>} an array of stringified key - value pairs + */ +function prepareKeyValuePairsForStorage(data) { + return _.keys(data).map(key => [key, JSON.stringify(data[key])]); +} + /** * Sets multiple keys and values. Example * Onyx.multiSet({'key1': 'a', 'key2': 'b'}); @@ -464,18 +475,10 @@ function set(key, val) { * @returns {Promise} */ function multiSet(data) { - // Todo: use _.pairs for the same result - // AsyncStorage expenses the data in an array like: - // [["@MyApp_user", "value_1"], ["@MyApp_key", "value_2"]] - // This method will transform the params from a better JSON format like: - // {'@MyApp_user': 'myUserValue', '@MyApp_key': 'myKeyValue'} - const keyValuePairs = _.reduce(data, (finalArray, val, key) => ([ - ...finalArray, - [key, JSON.stringify(val)], - ]), []); - - // Capture any changes to cache - cache.merge(keyValuePairs); + const keyValuePairs = prepareKeyValuePairsForStorage(data); + + // Capture (non-stringified) changes to cache + cache.merge(_.pairs(data)); // Optimistically inform subscribers _.each(data, (val, key) => keyChanged(key, val)); @@ -603,26 +606,18 @@ function mergeCollection(collectionKey, collection) { } }); - // Todo: use _.pairs for the same result - const keyValuePairsForExistingCollection = _.reduce(existingKeyCollection, (finalArray, val, key) => ([ - ...finalArray, - [key, JSON.stringify(val)], - ]), []); - const keyValuePairsForNewCollection = _.reduce(newCollection, (finalArray, val, key) => ([ - ...finalArray, - [key, JSON.stringify(val)], - ]), []); + const keyValuePairsForExistingCollection = prepareKeyValuePairsForStorage(existingKeyCollection); + const keyValuePairsForNewCollection = prepareKeyValuePairsForStorage(newCollection); // New keys will be added via multiSet while existing keys will be updated using multiMerge // This is because setting a key that doesn't exist yet with multiMerge will throw errors const existingCollectionPromise = AsyncStorage.multiMerge(keyValuePairsForExistingCollection); const newCollectionPromise = AsyncStorage.multiSet(keyValuePairsForNewCollection); - // Capture any changes to cache - cache.merge(keyValuePairsForExistingCollection); - cache.merge(keyValuePairsForNewCollection); + // Capture (non-stringified) changes to cache + cache.merge(_.pairs(collection)); - // Optimistically inform subscribers + // Optimistically inform collection subscribers keysChanged(collectionKey, collection); return Promise.all([existingCollectionPromise, newCollectionPromise]) From 6b659d736a41bc589417252e7eba8679deed52a3 Mon Sep 17 00:00:00 2001 From: Peter Velkov Date: Tue, 25 May 2021 22:42:37 +0300 Subject: [PATCH 11/40] refactor: simplify mergeCollection keys partitioning --- lib/Onyx.js | 20 +++++++++----------- 1 file changed, 9 insertions(+), 11 deletions(-) diff --git a/lib/Onyx.js b/lib/Onyx.js index b57fcabf9..659a07607 100644 --- a/lib/Onyx.js +++ b/lib/Onyx.js @@ -594,18 +594,16 @@ function mergeCollection(collectionKey, collection) { } }); - const existingKeyCollection = {}; - const newCollection = {}; return getAllKeys() - .then((keys) => { - _.each(collection, (data, dataKey) => { - if (keys.includes(dataKey)) { - existingKeyCollection[dataKey] = data; - } else { - newCollection[dataKey] = data; - } - }); - + .then((persistedKeys) => { + // Split to keys that exist in storage and keys that don't + const [existingKeys, newKeys] = _.chain(collection) + .keys() + .partition(key => persistedKeys.includes(key)) + .value(); + + const existingKeyCollection = _.pick(collection, existingKeys); + const newCollection = _.pick(collection, newKeys); const keyValuePairsForExistingCollection = prepareKeyValuePairsForStorage(existingKeyCollection); const keyValuePairsForNewCollection = prepareKeyValuePairsForStorage(newCollection); From f7feafc212594e8855ff1daae813b6c79f7189f2 Mon Sep 17 00:00:00 2001 From: Peter Velkov Date: Wed, 26 May 2021 00:10:13 +0300 Subject: [PATCH 12/40] docs: add todo notes on `withOnyx` test not passing expected props to component --- tests/unit/withOnyxTest.js | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/tests/unit/withOnyxTest.js b/tests/unit/withOnyxTest.js index 6053bac4b..79d1351ec 100644 --- a/tests/unit/withOnyxTest.js +++ b/tests/unit/withOnyxTest.js @@ -43,6 +43,7 @@ describe('withOnyx', () => { }); it('should update withOnyx subscriber multiple times when merge is used', () => { + // Todo: ViewWithCollections does not expect a `text` prop const TestComponentWithOnyx = withOnyx({ text: { key: ONYX_KEYS.COLLECTION.TEST_KEY, @@ -61,6 +62,7 @@ describe('withOnyx', () => { }); it('should update withOnyx subscriber just once when mergeCollection is used', () => { + // Todo: ViewWithCollections does not expect a `text` prop const TestComponentWithOnyx = withOnyx({ text: { key: ONYX_KEYS.COLLECTION.TEST_KEY, @@ -77,6 +79,7 @@ describe('withOnyx', () => { }); it('should update withOnyx subscribing to individual key if mergeCollection is used', () => { + // Todo: ViewWithCollections does not expect a `text` prop const collectionItemID = 1; const TestComponentWithOnyx = withOnyx({ text: { @@ -94,6 +97,7 @@ describe('withOnyx', () => { }); it('should update withOnyx subscribing to individual key with merged value if mergeCollection is used', () => { + // Todo: ViewWithCollections does not expect a `text` prop const collectionItemID = 4; const TestComponentWithOnyx = withOnyx({ text: { From 16d42ac061c23d8502280a1c5bb2e3e91a3f1cc5 Mon Sep 17 00:00:00 2001 From: Peter Velkov Date: Wed, 26 May 2021 00:27:23 +0300 Subject: [PATCH 13/40] fix: `get` should capture different tasks for different keys --- lib/Onyx.js | 2 +- tests/unit/onyxCacheTest.js | 43 +++++++++++++++++++++++++++++++++++++ 2 files changed, 44 insertions(+), 1 deletion(-) diff --git a/lib/Onyx.js b/lib/Onyx.js index 659a07607..caa2e9260 100644 --- a/lib/Onyx.js +++ b/lib/Onyx.js @@ -42,7 +42,7 @@ function get(key) { /* Otherwise setup a task to retrieve it. This ensures concurrent usages would not start the same task */ - return cache.resolveTask('get', () => AsyncStorage.getItem(key) + return cache.resolveTask(`get:${key}`, () => AsyncStorage.getItem(key) .then((val) => { // Add values to cache and return parsed result const parsed = JSON.parse(val); diff --git a/tests/unit/onyxCacheTest.js b/tests/unit/onyxCacheTest.js index 9688e24f6..f03fb8ee4 100644 --- a/tests/unit/onyxCacheTest.js +++ b/tests/unit/onyxCacheTest.js @@ -3,6 +3,7 @@ import {render} from '@testing-library/react-native'; import waitForPromisesToResolve from '../utils/waitForPromisesToResolve'; import ViewWithText from '../components/ViewWithText'; +import ViewWithCollections from '../components/ViewWithCollections'; describe('Onyx', () => { describe('Cache Service', () => { @@ -293,6 +294,9 @@ describe('Onyx', () => { const ONYX_KEYS = { TEST_KEY: 'test', ANOTHER_TEST: 'anotherTest', + COLLECTION: { + MOCK_COLLECTION: 'mock_collection_', + }, }; async function initOnyx() { @@ -403,5 +407,44 @@ describe('Onyx', () => { await waitForPromisesToResolve(); expect(AsyncStorageMock.getItem).toHaveBeenCalledTimes(2); }); + + it('Expect multiple calls to AsyncStorage.getItem when multiple keys are used', async () => { + const AsyncStorageMock = require('@react-native-community/async-storage/jest/async-storage-mock'); + + // GIVEN two component + const TestComponentWithOnyx = withOnyx({ + testObject: { + key: ONYX_KEYS.TEST_KEY, + }, + })(ViewWithCollections); + + const AnotherTestComponentWithOnyx = withOnyx({ + text: { + key: ONYX_KEYS.ANOTHER_TEST, + }, + })(ViewWithText); + + // GIVEN some values exist in storage + AsyncStorageMock.setItem(ONYX_KEYS.TEST_KEY, JSON.stringify({ID: 15, data: 'mock object with ID'})); + AsyncStorageMock.setItem(ONYX_KEYS.ANOTHER_TEST, JSON.stringify('mock text')); + AsyncStorageMock.getAllKeys.mockResolvedValue([ONYX_KEYS.TEST_KEY, ONYX_KEYS.ANOTHER_TEST]); + await initOnyx(); + + // WHEN the components are rendered multiple times + render(); + render(); + render(); + render(); + render(); + render(); + + // THEN Async storage `getItem` should be called exactly two times (once for each key) + await waitForPromisesToResolve(); + expect(AsyncStorageMock.getItem).toHaveBeenCalledTimes(2); + expect(AsyncStorageMock.getItem.mock.calls).toEqual([ + [ONYX_KEYS.TEST_KEY], + [ONYX_KEYS.ANOTHER_TEST] + ]); + }); }); }); From 5b11545ab48c506d0fe5fffe13f06ff01fb56b73 Mon Sep 17 00:00:00 2001 From: Peter Velkov Date: Thu, 27 May 2021 16:59:05 +0300 Subject: [PATCH 14/40] fix: clear async storage --- lib/Onyx.js | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/lib/Onyx.js b/lib/Onyx.js index caa2e9260..ea15f3d1a 100644 --- a/lib/Onyx.js +++ b/lib/Onyx.js @@ -573,9 +573,9 @@ function clear() { keyChanged(key, null); cache.remove(key); }); - - initializeWithDefaultKeyStates(); - }); + }) + .then(() => AsyncStorage.clear()) + .then(initializeWithDefaultKeyStates); } /** From 0c51923ef782b54000bc71a9cb8fe56c00711df2 Mon Sep 17 00:00:00 2001 From: Peter Velkov Date: Thu, 27 May 2021 18:42:28 +0300 Subject: [PATCH 15/40] feat: enhance cache.merge to work with primitives as well as complex values --- lib/OnyxCache.js | 31 ++++++++--- tests/unit/onyxCacheTest.js | 103 +++++++++++++++++++++++++++++++++--- 2 files changed, 120 insertions(+), 14 deletions(-) diff --git a/lib/OnyxCache.js b/lib/OnyxCache.js index 2643d1b22..4b9fb25cd 100644 --- a/lib/OnyxCache.js +++ b/lib/OnyxCache.js @@ -88,19 +88,34 @@ class OnyxCache { /** * Merge data to cache, any non existing keys will be crated - * @param {Array<[string, Object]>} pairs - array of key value pairs + * Array items are concatenated + * Object items are merged + * If the existing value in cache and the new value are not the same type + * the existing value is replaced by the new value + * In case the merge value for a key is undefined - the key is removed from cache + * @param {Array<[string, *]>} pairs - array of key value pairs */ merge(pairs) { - _.forEach(pairs, ([key, value]) => { - const currentValue = _.result(this.storageMap, key, {}); - - if (_.isObject(value) || _.isArray(value)) { - const merged = {...currentValue, ...value}; - this.update(key, merged); + _.forEach(pairs, ([key, valueToMerge]) => { + if (_.isUndefined(valueToMerge)) { + this.remove(key); return; } - throw new Error(`The provided merge value is invalid: ${value}`); + const existingValue = this.storageMap[key]; + let newValue = valueToMerge; + + /* Perform merge or concatenation for complex values otherwise just replace the old value */ + + if (_.isObject(valueToMerge) && _.isObject(existingValue)) { + newValue = {...existingValue, ...valueToMerge}; + } + + if (_.isArray(valueToMerge) && _.isArray(existingValue)) { + newValue = [...existingValue, ...valueToMerge]; + } + + this.update(key, newValue); }); } diff --git a/tests/unit/onyxCacheTest.js b/tests/unit/onyxCacheTest.js index f03fb8ee4..fbf18735b 100644 --- a/tests/unit/onyxCacheTest.js +++ b/tests/unit/onyxCacheTest.js @@ -213,14 +213,105 @@ describe('Onyx', () => { }); }); - it('Should throw when the value parameter is not an object or array', () => { - // GIVEN cache with some items - cache.update('mockKey', 'someStringValue'); + it('Should merge objects correctly', () => { + // GIVEN cache with existing object data + cache.update('mockKey', {value: 'mockValue', anotherValue: 'overwrite me'}); + + // WHEN merge is called with an object + cache.merge([ + ['mockKey', {mockItems: [], anotherValue: 'overwritten'}], + ]); + + // THEN the values should be merged together in cache + expect(cache.getValue('mockKey')).toEqual({ + value: 'mockValue', + mockItems: [], + anotherValue: 'overwritten', + }); + }); + + it('Should merge arrays correctly', () => { + // GIVEN cache with existing array data + cache.update('mockKey', [1, 2, 3]); + + // WHEN merge is called with an array + cache.merge([ + ['mockKey', [3, 4, 5]], + ]); + + // THEN the arrays should be concatenated + expect(cache.getValue('mockKey')).toEqual([1, 2, 3, 3, 4, 5]); + }); + + it('Should work with primitive values', () => { + // GIVEN cache with existing data + cache.update('mockKey', {}); + + // WHEN merge is called with bool + cache.merge([['mockKey', false]]); + + // THEN the object should be overwritten with a bool value + expect(cache.getValue('mockKey')).toEqual(false); - const badMergeValue = [['mockKey', 'usually we do not want to merge strings right?']]; + // WHEN merge is called with number + cache.merge([['mockKey', 0]]); + + // THEN the value should be overwritten + expect(cache.getValue('mockKey')).toEqual(0); + + // WHEN merge is called with string + cache.merge([['mockKey', '123']]); + + // THEN the value should be overwritten + expect(cache.getValue('mockKey')).toEqual('123'); + + // WHEN merge is called with string again + cache.merge([['mockKey', '123']]); + + // THEN strings should not have been concatenated + expect(cache.getValue('mockKey')).toEqual('123'); + + // WHEN merge is called with an object + cache.merge([['mockKey', {value: 'myMockObject'}]]); + + // THEN the old primitive value should be overwritten with the object + expect(cache.getValue('mockKey')).toEqual({value: 'myMockObject'}); + }); + + it('Should remove a key if the new value is `undefined`', () => { + // GIVEN cache with existing data + cache.update('mockKey', {}); + cache.update('mockKey1', {}); + + // WHEN merge is called key value pair and the value is undefined + cache.merge([['mockKey', undefined]]); + cache.merge([['mockKey2']]); + + // THEN the key should be removed from cache + expect(cache.hasCacheForKey('mockKey')).toBe(false); + expect(cache.hasCacheForKey('mockKey2')).toBe(false); + }); + + it('Should work with multiple key value pairs', () => { + // GIVEN cache with existing data + cache.update('mockKey1', {ID: 1}); + cache.update('mockKey2', {ID: 2}); + cache.update('mockKey3', {ID: 3}); + + // WHEN merge is called with multiple key value pairs + cache.merge([ + ['mockKey2', {value: 'mockValue2'}], + ['mockKey4', 'Some string data'], + ['mockKey3', {ID: '3'}], + ['mockKey5', {ID: '3'}], + ]); - // WHEN merge is not called with array or an object - expect(() => cache.merge(badMergeValue)).toThrow('The provided merge value is invalid'); + // THEN values new values should be added and existing values updated + expect(cache.getValue('mockKey1')).toEqual({ID: 1}); + expect(cache.getValue('mockKey2')).toEqual({ID: 2, value: 'mockValue2'}); + expect(cache.getValue('mockKey3')).toEqual({ID: '3'}); + expect(cache.getValue('mockKey4')).toEqual('Some string data'); + expect(cache.getValue('mockKey5')).toEqual({ID: '3'}); }); }); From 22ad916433eea13ac8a4bf726acd33002894c31f Mon Sep 17 00:00:00 2001 From: Peter Velkov Date: Thu, 27 May 2021 18:44:11 +0300 Subject: [PATCH 16/40] refactor: update `hasCacheForKey` to use `isDefined` --- lib/OnyxCache.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/OnyxCache.js b/lib/OnyxCache.js index 4b9fb25cd..c79d43ab4 100644 --- a/lib/OnyxCache.js +++ b/lib/OnyxCache.js @@ -59,7 +59,7 @@ class OnyxCache { * @returns {boolean} */ hasCacheForKey(key) { - return key in this.storageMap; + return isDefined(this.storageMap[key]); } /** From d0c62c59085af39ee5c51444cd559ea7de685cde Mon Sep 17 00:00:00 2001 From: Peter Velkov Date: Thu, 27 May 2021 18:51:39 +0300 Subject: [PATCH 17/40] docs: cache.update try to better explain this method --- lib/OnyxCache.js | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/lib/OnyxCache.js b/lib/OnyxCache.js index c79d43ab4..4710dce1c 100644 --- a/lib/OnyxCache.js +++ b/lib/OnyxCache.js @@ -64,14 +64,16 @@ class OnyxCache { /** * Update or add data to cache + * Providing value is optional. Skipping value would just capture the key so that + * it's available in `getAllKeys` * @param {string} key * @param {*} [value] */ update(key, value) { - // Update all storage keys + // Keep storage keys up to date so that `getAllKeys` works correctly this.storageKeys.add(key); - // When value is provided update general cache as well + // When a 2nd parameter (value) is provided - store it under the key if (isDefined(value)) { this.storageMap[key] = value; } From 1b35161c6742f0d130d69d66d2a9d62c8b958c3e Mon Sep 17 00:00:00 2001 From: Peter Velkov Date: Thu, 27 May 2021 19:14:03 +0300 Subject: [PATCH 18/40] feat: clean up cache when the last key for a subscription is disconnected --- lib/Onyx.js | 7 +++++++ tests/unit/onyxCacheTest.js | 34 ++++++++++++++++++++++++++++++++++ 2 files changed, 41 insertions(+) diff --git a/lib/Onyx.js b/lib/Onyx.js index ea15f3d1a..29d5344d3 100644 --- a/lib/Onyx.js +++ b/lib/Onyx.js @@ -393,7 +393,14 @@ function disconnect(connectionID, keyToRemoveFromEvictionBlocklist) { removeFromEvictionBlockList(keyToRemoveFromEvictionBlocklist, connectionID); } + const key = callbackToStateMapping[connectionID].key; delete callbackToStateMapping[connectionID]; + + // When the last subscriber disconnects, drop cache as well + const remainingConnectionsForKey = _.filter(callbackToStateMapping, {key}); + if (remainingConnectionsForKey.length === 0) { + cache.remove(key); + } } /** diff --git a/tests/unit/onyxCacheTest.js b/tests/unit/onyxCacheTest.js index fbf18735b..5bbbde05a 100644 --- a/tests/unit/onyxCacheTest.js +++ b/tests/unit/onyxCacheTest.js @@ -537,5 +537,39 @@ describe('Onyx', () => { [ONYX_KEYS.ANOTHER_TEST] ]); }); + + it('Expect a single call to AsyncStorage.getItem when at least one component is still subscribed to a key', async () => { + const AsyncStorageMock = require('@react-native-community/async-storage/jest/async-storage-mock'); + + // GIVEN a component connected to Onyx + const TestComponentWithOnyx = withOnyx({ + text: { + key: ONYX_KEYS.TEST_KEY, + }, + })(ViewWithText); + + // GIVEN some string value for that key exists in storage + AsyncStorageMock.getItem.mockResolvedValue('"mockValue"'); + AsyncStorageMock.getAllKeys.mockResolvedValue([ONYX_KEYS.TEST_KEY]); + await initOnyx(); + + // WHEN multiple components are rendered + render(); + const result2 = render(); + const result3 = render(); + await waitForPromisesToResolve(); + + // WHEN components unmount but at least one remain mounted + result2.unmount(); + result3.unmount(); + await waitForPromisesToResolve(); + + // THEN When another component using the same storage key is rendered + render(); + + // THEN Async storage `getItem` should be called once + await waitForPromisesToResolve(); + expect(AsyncStorageMock.getItem).toHaveBeenCalledTimes(1); + }); }); }); From b33e6969f070bb257d1cfbb2947924297eae3b2a Mon Sep 17 00:00:00 2001 From: Peter Velkov Date: Thu, 27 May 2021 19:53:20 +0300 Subject: [PATCH 19/40] refactor: encapsulate cache checking and resolving logic for getAllKeys in the cache service --- lib/Onyx.js | 15 +-------- lib/OnyxCache.js | 21 ++++++++++--- tests/unit/onyxCacheTest.js | 62 ++++++++++++++++++++++++++++--------- 3 files changed, 64 insertions(+), 34 deletions(-) diff --git a/lib/Onyx.js b/lib/Onyx.js index 29d5344d3..e157e21e8 100644 --- a/lib/Onyx.js +++ b/lib/Onyx.js @@ -57,20 +57,7 @@ function get(key) { * @returns {Promise} */ function getAllKeys() { - // When we've already read stored keys, resolve right away - const storedKeys = cache.getAllKeys(); - if (storedKeys.length > 0) { - return Promise.resolve(storedKeys); - } - - /* Otherwise setup a task to retrieve them. - This ensures concurrent usages would not start the same task */ - return cache.resolveTask('getAllKeys', () => AsyncStorage.getAllKeys() - .then((keys) => { - // Add values to cache and return original result - _.each(keys, key => cache.update(key)); - return keys; - })); + return cache.getAllKeys(() => AsyncStorage.getAllKeys()); } /** diff --git a/lib/OnyxCache.js b/lib/OnyxCache.js index 4710dce1c..36c26782d 100644 --- a/lib/OnyxCache.js +++ b/lib/OnyxCache.js @@ -38,10 +38,20 @@ class OnyxCache { /** * Get all the storage keys - * @returns {string[]} + * @param {function(): Promise} fallback + * @returns {Promise} */ - getAllKeys() { - return [...this.storageKeys]; + getAllKeys(fallback) { + if (this.storageKeys.size > 0) { + return Promise.resolve([...this.storageKeys]); + } + + const startTask = () => fallback().then((keys) => { + this.storageKeys = new Set(keys); + return keys; + }); + + return this.resolveTask('getAllKeys', startTask); } /** @@ -122,15 +132,16 @@ class OnyxCache { } /** + * @template T * Use this method to prevents additional calls from the same thing * When a promise for a given call already exists the caller would * be hooked to it. Otherwise the task is started and captured so * that other callers don't have to start over * Particularly useful for data retrieving methods * @param {string} taskName - * @param {function(): Promise} startTask - Provide a promise returning function that + * @param {function(): Promise} startTask - Provide a promise returning function that * will be invoked if there is no pending promise for this task - * @returns {Promise} + * @returns {Promise} */ resolveTask(taskName, startTask) { // When a task is already running return it right away diff --git a/tests/unit/onyxCacheTest.js b/tests/unit/onyxCacheTest.js index 5bbbde05a..c43e51ccf 100644 --- a/tests/unit/onyxCacheTest.js +++ b/tests/unit/onyxCacheTest.js @@ -17,39 +17,52 @@ describe('Onyx', () => { }); describe('getAllKeys', () => { - it('Should be empty initially', () => { - // GIVEN empty cache + it('Should be empty and resolve from fallback initially', async () => { + // GIVEN empty cache and a fallback function + const mockFallback = jest.fn().mockResolvedValue(['a', 'b', 'c']); // WHEN all keys are retrieved - const allKeys = cache.getAllKeys(); + const result = await cache.getAllKeys(mockFallback); + + // THEN the result should be provided from the fallback + expect(mockFallback).toHaveBeenCalledTimes(1); + expect(result).toEqual(['a', 'b', 'c']); - // THEN the result should be empty - expect(allKeys).toEqual([]); + // THEN fallback result should be stored to cache and fallback not executed + const cachedResult = await cache.getAllKeys(mockFallback); + expect(cachedResult).toEqual(['a', 'b', 'c']); + expect(mockFallback).toHaveBeenCalledTimes(1); }); - it('Should keep storage keys', () => { + it('Should keep storage keys', async () => { // GIVEN cache with some items cache.update('mockKey', 'mockValue'); cache.update('mockKey2', 'mockValue'); cache.update('mockKey3', 'mockValue'); + // GIVEN a fallback function + const mockFallback = jest.fn().mockResolvedValue(['a', 'b', 'c']); + // THEN the keys should be stored in cache - const allKeys = cache.getAllKeys(); + const allKeys = await cache.getAllKeys(mockFallback); expect(allKeys).toEqual(['mockKey', 'mockKey2', 'mockKey3']); + + // AND the fallback should be executed + expect(mockFallback).not.toHaveBeenCalled(); }); - it('Should keep storage keys even when no values are provided', () => { + it('Should keep storage keys even when no values are provided', async () => { // GIVEN cache with some items cache.update('mockKey'); cache.update('mockKey2'); cache.update('mockKey3'); // THEN the keys should be stored in cache - const allKeys = cache.getAllKeys(); + const allKeys = await cache.getAllKeys(jest.fn()); expect(allKeys).toEqual(['mockKey', 'mockKey2', 'mockKey3']); }); - it('Should not store duplicate keys', () => { + it('Should not store duplicate keys', async () => { // GIVEN cache with some items cache.update('mockKey', 'mockValue'); cache.update('mockKey2', 'mockValue'); @@ -59,9 +72,27 @@ describe('Onyx', () => { cache.update('mockKey2', 'new mock value'); // THEN getAllKeys should not include a duplicate value - const allKeys = cache.getAllKeys(); + const allKeys = await cache.getAllKeys(jest.fn()); expect(allKeys).toEqual(['mockKey', 'mockKey2', 'mockKey3']); }); + + it('Should execute the fallback only once for concurrent calls', async () => { + // GIVEN empty cache and a fallback function + const mockFallback = jest.fn().mockResolvedValue(['a', 'b', 'c']); + + // WHEN all keys are retrieved in parallel + const promise1 = cache.getAllKeys(mockFallback); + const promise2 = cache.getAllKeys(mockFallback); + const promise3 = cache.getAllKeys(mockFallback); + + const [result1, result2, result3] = await Promise.all([promise1, promise2, promise3]); + + // THEN the fallback should be called only once + expect(mockFallback).toHaveBeenCalledTimes(1); + expect(result1).toEqual(['a', 'b', 'c']); + expect(result2).toEqual(['a', 'b', 'c']); + expect(result3).toEqual(['a', 'b', 'c']); + }); }); describe('getValue', () => { @@ -119,7 +150,7 @@ describe('Onyx', () => { expect(cache.getValue('mockKey')).toEqual({value: 'mockValue'}); }); - it('Should only store the key when no value is provided', () => { + it('Should only store the key when no value is provided', async () => { // GIVEN empty cache // WHEN update is called with key and value @@ -129,7 +160,8 @@ describe('Onyx', () => { expect(cache.getValue('mockKey')).not.toBeDefined(); // THEN but a key should be available - expect(cache.getAllKeys()).toEqual(expect.arrayContaining(['mockKey'])); + const allKeys = await cache.getAllKeys(); + expect(allKeys).toEqual(expect.arrayContaining(['mockKey'])); }); it('Should overwrite existing cache items for the given key', () => { @@ -146,7 +178,7 @@ describe('Onyx', () => { }); describe('remove', () => { - it('Should remove the key from all keys', () => { + it('Should remove the key from all keys', async () => { // GIVEN cache with some items cache.update('mockKey', 'mockValue'); cache.update('mockKey2', 'mockValue'); @@ -156,7 +188,7 @@ describe('Onyx', () => { cache.remove('mockKey2'); // THEN getAllKeys should not include the removed value - const allKeys = cache.getAllKeys(); + const allKeys = await cache.getAllKeys(); expect(allKeys).toEqual(['mockKey', 'mockKey3']); }); From 4af3abf7773e035235ad8b1739426e3f372117b2 Mon Sep 17 00:00:00 2001 From: Peter Velkov Date: Thu, 27 May 2021 21:05:57 +0300 Subject: [PATCH 20/40] refactor: encapsulate cache checking and resolving logic for get in the cache service --- lib/Onyx.js | 10 +-- lib/OnyxCache.js | 16 ++++- tests/unit/onyxCacheTest.js | 122 ++++++++++++++++++++++++------------ 3 files changed, 96 insertions(+), 52 deletions(-) diff --git a/lib/Onyx.js b/lib/Onyx.js index e157e21e8..940fdb7aa 100644 --- a/lib/Onyx.js +++ b/lib/Onyx.js @@ -35,16 +35,8 @@ let defaultKeyStates = {}; * @returns {Promise<*>} */ function get(key) { - // When we already have the value in cache - resolve right away - if (cache.hasCacheForKey(key)) { - return Promise.resolve(cache.getValue(key)); - } - - /* Otherwise setup a task to retrieve it. - This ensures concurrent usages would not start the same task */ - return cache.resolveTask(`get:${key}`, () => AsyncStorage.getItem(key) + return cache.getValue(key, () => AsyncStorage.getItem(key) .then((val) => { - // Add values to cache and return parsed result const parsed = JSON.parse(val); cache.update(key, parsed); return parsed; diff --git a/lib/OnyxCache.js b/lib/OnyxCache.js index 36c26782d..8d5417d1e 100644 --- a/lib/OnyxCache.js +++ b/lib/OnyxCache.js @@ -56,11 +56,18 @@ class OnyxCache { /** * Get a cached value from storage + * @template T * @param {string} key - * @returns {*} + * @param {function(key: string): Promise} fallback + * @returns {Promise} */ - getValue(key) { - return this.storageMap[key]; + getValue(key, fallback) { + if (this.hasCacheForKey(key)) { + return Promise.resolve(this.storageMap[key]); + } + + const startTask = () => fallback(key).then(value => this.update(key, value)); + return this.resolveTask(`getValue:${key}`, startTask); } /** @@ -78,6 +85,7 @@ class OnyxCache { * it's available in `getAllKeys` * @param {string} key * @param {*} [value] + * @returns {*} value - returns the cache value */ update(key, value) { // Keep storage keys up to date so that `getAllKeys` works correctly @@ -87,6 +95,8 @@ class OnyxCache { if (isDefined(value)) { this.storageMap[key] = value; } + + return value; } /** diff --git a/tests/unit/onyxCacheTest.js b/tests/unit/onyxCacheTest.js index c43e51ccf..3d7fab8ce 100644 --- a/tests/unit/onyxCacheTest.js +++ b/tests/unit/onyxCacheTest.js @@ -96,25 +96,55 @@ describe('Onyx', () => { }); describe('getValue', () => { - it('Should return undefined when there is no stored value', () => { - // GIVEN empty cache + it('Should use the fallback function when there is no stored value', async () => { + // GIVEN empty cache and a fallback function + const mockFallback = jest.fn().mockResolvedValue('myResult'); // WHEN a value is retrieved - const result = cache.getValue('mockKey'); + const result = await cache.getValue('mockKey', mockFallback); // THEN it should be undefined - expect(result).not.toBeDefined(); + expect(result).toEqual('myResult'); + expect(mockFallback).toHaveBeenCalledTimes(1); }); - it('Should return cached value when it exists', () => { + it('Should return cached value when it exists', async () => { // GIVEN cache with some items cache.update('mockKey', {items: ['mockValue', 'mockValue2']}); - cache.update('mockKey2', 'mockValue3'); + + // GIVEN a fallback function + const mockFallback = jest.fn().mockResolvedValue('myResult'); // WHEN a value is retrieved - // THEN it should be the correct value - expect(cache.getValue('mockKey')).toEqual({items: ['mockValue', 'mockValue2']}); - expect(cache.getValue('mockKey2')).toEqual('mockValue3'); + const value = await cache.getValue('mockKey', mockFallback); + + // THEN it should have the correct content + expect(value).toEqual({items: ['mockValue', 'mockValue2']}); + + // AND the fallback function should be executed + expect(mockFallback).not.toHaveBeenCalled(); + }); + + it('Should execute the fallback only once per key for concurrent calls', async () => { + // GIVEN empty cache and a fallback function + const mockFallback = jest.fn().mockImplementation(key => Promise.resolve(`Result for ${key}`)); + + // WHEN keys are retrieved in parallel + const promise1 = cache.getValue('mockKey1', mockFallback); + const promise2 = cache.getValue('mockKey2', mockFallback); + const promise3 = cache.getValue('mockKey1', mockFallback); + const promise4 = cache.getValue('mockKey1', mockFallback); + const promise5 = cache.getValue('mockKey2', mockFallback); + + const results = await Promise.all([promise1, promise2, promise3, promise4, promise5]); + + // THEN the fallback should be called 2 times - once per key + expect(mockFallback).toHaveBeenCalledTimes(2); + expect(results[0]).toEqual('Result for mockKey1'); + expect(results[1]).toEqual('Result for mockKey2'); + expect(results[2]).toEqual('Result for mockKey1'); + expect(results[3]).toEqual('Result for mockKey1'); + expect(results[4]).toEqual('Result for mockKey2'); }); }); @@ -140,31 +170,32 @@ describe('Onyx', () => { }); describe('update', () => { - it('Should add data to cache when both key and value are provided', () => { + it('Should add data to cache when both key and value are provided', async () => { // GIVEN empty cache // WHEN update is called with key and value cache.update('mockKey', {value: 'mockValue'}); // THEN data should be cached - expect(cache.getValue('mockKey')).toEqual({value: 'mockValue'}); + const data = await cache.getValue('mockKey', jest.fn()); + expect(data).toEqual({value: 'mockValue'}); }); - it('Should only store the key when no value is provided', async () => { + it('Should store only the key when no value is provided', async () => { // GIVEN empty cache // WHEN update is called with key and value cache.update('mockKey'); // THEN there should be no cached value - expect(cache.getValue('mockKey')).not.toBeDefined(); + expect(cache.hasCacheForKey('mockKey')).toBe(false); // THEN but a key should be available - const allKeys = await cache.getAllKeys(); + const allKeys = await cache.getAllKeys(jest.fn()); expect(allKeys).toEqual(expect.arrayContaining(['mockKey'])); }); - it('Should overwrite existing cache items for the given key', () => { + it('Should overwrite existing cache items for the given key', async () => { // GIVEN cache with some items cache.update('mockKey', {value: 'mockValue'}); cache.update('mockKey2', {other: 'otherMockValue'}); @@ -173,7 +204,8 @@ describe('Onyx', () => { cache.update('mockKey2', {value: []}); // THEN the value should be overwritten - expect(cache.getValue('mockKey2')).toEqual({value: []}); + const value = await cache.getValue('mockKey2', jest.fn()); + expect(value).toEqual({value: []}); }); }); @@ -188,7 +220,7 @@ describe('Onyx', () => { cache.remove('mockKey2'); // THEN getAllKeys should not include the removed value - const allKeys = await cache.getAllKeys(); + const allKeys = await cache.getAllKeys(jest.fn()); expect(allKeys).toEqual(['mockKey', 'mockKey3']); }); @@ -202,12 +234,11 @@ describe('Onyx', () => { // THEN a value should not be available in cache expect(cache.hasCacheForKey('mockKey')).toBe(false); - expect(cache.getValue('mockKey')).not.toBeDefined(); }); }); describe('merge', () => { - it('Should create the value in cache when it does not exist', () => { + it('Should create the value in cache when it does not exist', async () => { // GIVEN empty cache // WHEN merge is called with key value pairs @@ -217,11 +248,13 @@ describe('Onyx', () => { ]); // THEN data should be created in cache - expect(cache.getValue('mockKey')).toEqual({value: 'mockValue'}); - expect(cache.getValue('mockKey2')).toEqual({value: 'mockValue2'}); + const value1 = await cache.getValue('mockKey', jest.fn()); + const value2 = await cache.getValue('mockKey2', jest.fn()); + expect(value1).toEqual({value: 'mockValue'}); + expect(value2).toEqual({value: 'mockValue2'}); }); - it('Should merge data to existing cache value', () => { + it('Should merge data to existing cache value', async () => { // GIVEN cache with some items cache.update('mockKey', {value: 'mockValue'}); cache.update('mockKey2', {other: 'otherMockValue', mock: 'mock'}); @@ -233,19 +266,21 @@ describe('Onyx', () => { ]); // THEN the values should be merged together in cache - expect(cache.getValue('mockKey')).toEqual({ + const value1 = await cache.getValue('mockKey', jest.fn()); + expect(value1).toEqual({ value: 'mockValue', mockItems: [], }); - expect(cache.getValue('mockKey2')).toEqual({ + const value2 = await cache.getValue('mockKey2', jest.fn()); + expect(value2).toEqual({ other: 'overwrittenMockValue', items: [1, 2], mock: 'mock', }); }); - it('Should merge objects correctly', () => { + it('Should merge objects correctly', async () => { // GIVEN cache with existing object data cache.update('mockKey', {value: 'mockValue', anotherValue: 'overwrite me'}); @@ -255,14 +290,15 @@ describe('Onyx', () => { ]); // THEN the values should be merged together in cache - expect(cache.getValue('mockKey')).toEqual({ + const value = await cache.getValue('mockKey', jest.fn()); + expect(value).toEqual({ value: 'mockValue', mockItems: [], anotherValue: 'overwritten', }); }); - it('Should merge arrays correctly', () => { + it('Should merge arrays correctly', async () => { // GIVEN cache with existing array data cache.update('mockKey', [1, 2, 3]); @@ -272,10 +308,11 @@ describe('Onyx', () => { ]); // THEN the arrays should be concatenated - expect(cache.getValue('mockKey')).toEqual([1, 2, 3, 3, 4, 5]); + const value = await cache.getValue('mockKey', jest.fn()); + expect(value).toEqual([1, 2, 3, 3, 4, 5]); }); - it('Should work with primitive values', () => { + it('Should work with primitive values', async () => { // GIVEN cache with existing data cache.update('mockKey', {}); @@ -283,31 +320,36 @@ describe('Onyx', () => { cache.merge([['mockKey', false]]); // THEN the object should be overwritten with a bool value - expect(cache.getValue('mockKey')).toEqual(false); + const bool = await cache.getValue('mockKey', jest.fn()); + expect(bool).toBe(false); // WHEN merge is called with number cache.merge([['mockKey', 0]]); // THEN the value should be overwritten - expect(cache.getValue('mockKey')).toEqual(0); + const number = await cache.getValue('mockKey', jest.fn()); + expect(number).toEqual(0); // WHEN merge is called with string cache.merge([['mockKey', '123']]); // THEN the value should be overwritten - expect(cache.getValue('mockKey')).toEqual('123'); + const string = await cache.getValue('mockKey', jest.fn()); + expect(string).toEqual('123'); // WHEN merge is called with string again cache.merge([['mockKey', '123']]); // THEN strings should not have been concatenated - expect(cache.getValue('mockKey')).toEqual('123'); + const string2 = await cache.getValue('mockKey', jest.fn()); + expect(string2).toEqual('123'); // WHEN merge is called with an object cache.merge([['mockKey', {value: 'myMockObject'}]]); // THEN the old primitive value should be overwritten with the object - expect(cache.getValue('mockKey')).toEqual({value: 'myMockObject'}); + const object = await cache.getValue('mockKey', jest.fn()); + expect(object).toEqual({value: 'myMockObject'}); }); it('Should remove a key if the new value is `undefined`', () => { @@ -324,7 +366,7 @@ describe('Onyx', () => { expect(cache.hasCacheForKey('mockKey2')).toBe(false); }); - it('Should work with multiple key value pairs', () => { + it('Should work with multiple key value pairs', async () => { // GIVEN cache with existing data cache.update('mockKey1', {ID: 1}); cache.update('mockKey2', {ID: 2}); @@ -339,11 +381,11 @@ describe('Onyx', () => { ]); // THEN values new values should be added and existing values updated - expect(cache.getValue('mockKey1')).toEqual({ID: 1}); - expect(cache.getValue('mockKey2')).toEqual({ID: 2, value: 'mockValue2'}); - expect(cache.getValue('mockKey3')).toEqual({ID: '3'}); - expect(cache.getValue('mockKey4')).toEqual('Some string data'); - expect(cache.getValue('mockKey5')).toEqual({ID: '3'}); + expect(await cache.getValue('mockKey1', jest.fn())).toEqual({ID: 1}); + expect(await cache.getValue('mockKey2', jest.fn())).toEqual({ID: 2, value: 'mockValue2'}); + expect(await cache.getValue('mockKey3', jest.fn())).toEqual({ID: '3'}); + expect(await cache.getValue('mockKey4', jest.fn())).toEqual('Some string data'); + expect(await cache.getValue('mockKey5', jest.fn())).toEqual({ID: '3'}); }); }); From ceb4acf6b804d36e22ffc06d02cfe1718a415bcc Mon Sep 17 00:00:00 2001 From: Peter Velkov Date: Thu, 27 May 2021 21:08:44 +0300 Subject: [PATCH 21/40] Rename tests/decorateWithMetrics.js to tests/decorateWithMetricsTest.js --- tests/unit/{decorateWithMetrics.js => decorateWithMetricsTest.js} | 0 1 file changed, 0 insertions(+), 0 deletions(-) rename tests/unit/{decorateWithMetrics.js => decorateWithMetricsTest.js} (100%) diff --git a/tests/unit/decorateWithMetrics.js b/tests/unit/decorateWithMetricsTest.js similarity index 100% rename from tests/unit/decorateWithMetrics.js rename to tests/unit/decorateWithMetricsTest.js From 5435063cab9ae16a2a76a0cb4525f50f9542d8a5 Mon Sep 17 00:00:00 2001 From: Peter Velkov Date: Thu, 27 May 2021 21:29:49 +0300 Subject: [PATCH 22/40] test: check what happens with cache when async storage rejects --- lib/Onyx.js | 5 ++--- tests/unit/onyxCacheTest.js | 16 +++++++++++++++- 2 files changed, 17 insertions(+), 4 deletions(-) diff --git a/lib/Onyx.js b/lib/Onyx.js index 940fdb7aa..0aee10383 100644 --- a/lib/Onyx.js +++ b/lib/Onyx.js @@ -38,10 +38,9 @@ function get(key) { return cache.getValue(key, () => AsyncStorage.getItem(key) .then((val) => { const parsed = JSON.parse(val); - cache.update(key, parsed); return parsed; - }) - .catch(err => logInfo(`Unable to get item from persistent storage. Key: ${key} Error: ${err}`))); + })) + .catch(err => logInfo(`Unable to get item from persistent storage. Key: ${key} Error: ${err}`)); } /** diff --git a/tests/unit/onyxCacheTest.js b/tests/unit/onyxCacheTest.js index 3d7fab8ce..aa7dd82d6 100644 --- a/tests/unit/onyxCacheTest.js +++ b/tests/unit/onyxCacheTest.js @@ -103,7 +103,7 @@ describe('Onyx', () => { // WHEN a value is retrieved const result = await cache.getValue('mockKey', mockFallback); - // THEN it should be undefined + // THEN the fallback should be used to retrieve the value expect(result).toEqual('myResult'); expect(mockFallback).toHaveBeenCalledTimes(1); }); @@ -146,6 +146,20 @@ describe('Onyx', () => { expect(results[3]).toEqual('Result for mockKey1'); expect(results[4]).toEqual('Result for mockKey2'); }); + + it('Should not store cache if the fallback method failed', async () => { + // GIVEN empty cache and a fallback function that rejects + const mockFallback = jest.fn().mockRejectedValue( + new Error('Unable to get item from persistent storage') + ); + + // WHEN a value is retrieved + return cache.getValue('mockKey', mockFallback) + .catch(() => { + // THEN no value should be in cache + expect(cache.hasCacheForKey('mockKey')).toBe(false); + }); + }); }); describe('hasCacheForKey', () => { From dc2a54083e60217026dd844f26450956b8434f68 Mon Sep 17 00:00:00 2001 From: Peter Velkov Date: Thu, 27 May 2021 22:18:06 +0300 Subject: [PATCH 23/40] docs: update getAllKeys and getValue documentation --- lib/OnyxCache.js | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-) diff --git a/lib/OnyxCache.js b/lib/OnyxCache.js index 8d5417d1e..d7a566c32 100644 --- a/lib/OnyxCache.js +++ b/lib/OnyxCache.js @@ -38,14 +38,18 @@ class OnyxCache { /** * Get all the storage keys - * @param {function(): Promise} fallback + * Provide a promise returning fallback that will be executed when + * keys aren't available in cache + * @param {function(): Promise} fallback - a callback to retrieve keys from storage * @returns {Promise} */ getAllKeys(fallback) { + // When we have storage keys in cache resolve right away if (this.storageKeys.size > 0) { return Promise.resolve([...this.storageKeys]); } + // Read keys using the fallback. This task optimizes concurrent calls see `resolveTask` for details const startTask = () => fallback().then((keys) => { this.storageKeys = new Set(keys); return keys; @@ -56,16 +60,20 @@ class OnyxCache { /** * Get a cached value from storage + * Provide a promise returning fallback that will be executed when + * the value is not available in cache * @template T * @param {string} key - * @param {function(key: string): Promise} fallback + * @param {function(key: string): Promise} fallback - a fallback to retrieve the item from storage * @returns {Promise} */ getValue(key, fallback) { + // When we have cache resolve right away if (this.hasCacheForKey(key)) { return Promise.resolve(this.storageMap[key]); } + // Get the value using the fallback. This task optimizes concurrent calls see `resolveTask` for details const startTask = () => fallback(key).then(value => this.update(key, value)); return this.resolveTask(`getValue:${key}`, startTask); } @@ -142,12 +150,12 @@ class OnyxCache { } /** - * @template T * Use this method to prevents additional calls from the same thing * When a promise for a given call already exists the caller would * be hooked to it. Otherwise the task is started and captured so * that other callers don't have to start over * Particularly useful for data retrieving methods + * @template T * @param {string} taskName * @param {function(): Promise} startTask - Provide a promise returning function that * will be invoked if there is no pending promise for this task From 34cb2aacd3bcf50e05bd823b859b231a157fa76d Mon Sep 17 00:00:00 2001 From: Peter Velkov Date: Thu, 27 May 2021 22:18:22 +0300 Subject: [PATCH 24/40] test: Shorten test case names --- tests/unit/onyxCacheTest.js | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/tests/unit/onyxCacheTest.js b/tests/unit/onyxCacheTest.js index aa7dd82d6..159a6f48a 100644 --- a/tests/unit/onyxCacheTest.js +++ b/tests/unit/onyxCacheTest.js @@ -500,7 +500,7 @@ describe('Onyx', () => { return initOnyx(); }); - it('Expect a single call to AsyncStorage.getItem when multiple components use the same key', async () => { + it('Expect a single call to getItem when multiple components use the same key', async () => { const AsyncStorageMock = require('@react-native-community/async-storage/jest/async-storage-mock'); // GIVEN a component connected to Onyx @@ -529,7 +529,7 @@ describe('Onyx', () => { expect(AsyncStorageMock.getItem).toHaveBeenCalledTimes(1); }); - it('Expect a single call to AsyncStorage.getAllKeys when multiple components use the same key', async () => { + it('Expect a single call to getAllKeys when multiple components use the same key', async () => { const AsyncStorageMock = require('@react-native-community/async-storage/jest/async-storage-mock'); // GIVEN a component connected to Onyx @@ -558,7 +558,7 @@ describe('Onyx', () => { expect(AsyncStorageMock.getAllKeys).toHaveBeenCalledTimes(1); }); - it('Expect multiple calls to AsyncStorage.getItem when no existing component is using a key', async () => { + it('Expect multiple calls to getItem when no existing component is using a key', async () => { const AsyncStorageMock = require('@react-native-community/async-storage/jest/async-storage-mock'); // GIVEN a component connected to Onyx @@ -587,7 +587,7 @@ describe('Onyx', () => { expect(AsyncStorageMock.getItem).toHaveBeenCalledTimes(2); }); - it('Expect multiple calls to AsyncStorage.getItem when multiple keys are used', async () => { + it('Expect multiple calls to getItem when multiple keys are used', async () => { const AsyncStorageMock = require('@react-native-community/async-storage/jest/async-storage-mock'); // GIVEN two component @@ -626,7 +626,7 @@ describe('Onyx', () => { ]); }); - it('Expect a single call to AsyncStorage.getItem when at least one component is still subscribed to a key', async () => { + it('Expect a single call to getItem when at least one component is still subscribed to a key', async () => { const AsyncStorageMock = require('@react-native-community/async-storage/jest/async-storage-mock'); // GIVEN a component connected to Onyx From dfe362da024744b355b8321f26138ed62f412030 Mon Sep 17 00:00:00 2001 From: Peter Velkov Date: Fri, 28 May 2021 23:26:18 +0300 Subject: [PATCH 25/40] refactor: handle cache cleanup for collections --- lib/Onyx.js | 55 +++++++++++++++++++++++-- tests/unit/onyxCacheTest.js | 82 +++++++++++++++++++++++++++++++++++++ 2 files changed, 133 insertions(+), 4 deletions(-) diff --git a/lib/Onyx.js b/lib/Onyx.js index 0aee10383..aba68c6e0 100644 --- a/lib/Onyx.js +++ b/lib/Onyx.js @@ -62,6 +62,19 @@ function isCollectionKey(key) { return _.contains(_.values(onyxKeys.COLLECTION), key); } +/** + * Find the collection a collection item belongs to + * or return null if them item is not a part of a collection + * @param {string} key + * @returns {string|null} + */ +function getCollectionKeyForItem(key) { + return _.chain(onyxKeys.COLLECTION) + .values() + .find(name => key.startsWith(name)) + .value(); +} + /** * Checks to see if a given key matches with the * configured key of our connected subscriber @@ -354,6 +367,43 @@ function connect(mapping) { return connectionID; } +/** + * Remove cache items that are no longer connected through Onyx + * @param {string} key + */ +function cleanCache(key) { + const hasRemainingConnections = _.some(callbackToStateMapping, {key}); + + // When the key is still used in other places don't remove it from cache + if (hasRemainingConnections) { + return; + } + + // When this is a collection - also recursively removed any unused individual items + if (isCollectionKey(key)) { + cache.remove(key); + + getAllKeys().then(cachedKeys => _.chain(cachedKeys) + .filter(name => name.startsWith(key)) + .forEach(cleanCache)); + + return; + } + + // When this is a collection item - check if the collection is still used + const collectionKey = getCollectionKeyForItem(key); + if (collectionKey) { + // When there's an active subscription for a collection don't remove the item + const hasRemainingConnectionsForCollection = _.some(callbackToStateMapping, {key: collectionKey}); + if (hasRemainingConnectionsForCollection) { + return; + } + } + + // Otherwise remove the item from cache + cache.remove(key); +} + /** * Remove the listener for a react component * @@ -375,10 +425,7 @@ function disconnect(connectionID, keyToRemoveFromEvictionBlocklist) { delete callbackToStateMapping[connectionID]; // When the last subscriber disconnects, drop cache as well - const remainingConnectionsForKey = _.filter(callbackToStateMapping, {key}); - if (remainingConnectionsForKey.length === 0) { - cache.remove(key); - } + cleanCache(key); } /** diff --git a/tests/unit/onyxCacheTest.js b/tests/unit/onyxCacheTest.js index 159a6f48a..f77096b1b 100644 --- a/tests/unit/onyxCacheTest.js +++ b/tests/unit/onyxCacheTest.js @@ -470,6 +470,9 @@ describe('Onyx', () => { let Onyx; let withOnyx; + /** @type OnyxCache */ + let cache; + const ONYX_KEYS = { TEST_KEY: 'test', ANOTHER_TEST: 'anotherTest', @@ -482,6 +485,7 @@ describe('Onyx', () => { const OnyxModule = require('../../index'); Onyx = OnyxModule.default; withOnyx = OnyxModule.withOnyx; + cache = require('../../lib/OnyxCache').default; Onyx.init({ keys: ONYX_KEYS, @@ -659,5 +663,83 @@ describe('Onyx', () => { await waitForPromisesToResolve(); expect(AsyncStorageMock.getItem).toHaveBeenCalledTimes(1); }); + + it('Should remove collection items from cache when collection is disconnected', async () => { + const AsyncStorageMock = require('@react-native-community/async-storage/jest/async-storage-mock'); + + // GIVEN a component subscribing to a collection + const TestComponentWithOnyx = withOnyx({ + collections: { + key: ONYX_KEYS.COLLECTION.MOCK_COLLECTION, + }, + })(ViewWithCollections); + + // GIVEN some collection item values exist in storage + const keys = [`${ONYX_KEYS.COLLECTION.MOCK_COLLECTION}15`, `${ONYX_KEYS.COLLECTION.MOCK_COLLECTION}16`]; + AsyncStorageMock.setItem(keys[0], JSON.stringify({ID: 15})); + AsyncStorageMock.setItem(keys[1], JSON.stringify({ID: 16})); + AsyncStorageMock.getAllKeys.mockResolvedValue(keys); + await initOnyx(); + + // WHEN the collection using components render + const result = render(); + const result2 = render(); + await waitForPromisesToResolve(); + + // THEN the collection items should be in cache + expect(cache.hasCacheForKey(keys[0])).toBe(true); + expect(cache.hasCacheForKey(keys[1])).toBe(true); + + // WHEN one of the components unmounts + result.unmount(); + await waitForPromisesToResolve(); + + // THEN the collection items should still be in cache + expect(cache.hasCacheForKey(keys[0])).toBe(true); + expect(cache.hasCacheForKey(keys[1])).toBe(true); + + // WHEN the last component using the collection unmounts + result2.unmount(); + await waitForPromisesToResolve(); + + // THEN the collection items should be removed from cache + expect(cache.hasCacheForKey(keys[0])).toBe(false); + expect(cache.hasCacheForKey(keys[1])).toBe(false); + }); + + it('Should not remove item from cache when it still used in a collection', async () => { + const AsyncStorageMock = require('@react-native-community/async-storage/jest/async-storage-mock'); + + // GIVEN component that uses a collection and a component that uses a collection item + const COLLECTION_ITEM_KEY = `${ONYX_KEYS.COLLECTION.MOCK_COLLECTION}10`; + const TestComponentWithOnyx = withOnyx({ + collections: { + key: ONYX_KEYS.COLLECTION.MOCK_COLLECTION, + }, + })(ViewWithCollections); + + const AnotherTestComponentWithOnyx = withOnyx({ + testObject: { + key: COLLECTION_ITEM_KEY, + }, + })(ViewWithCollections); + + // GIVEN some values exist in storage + AsyncStorageMock.setItem(COLLECTION_ITEM_KEY, JSON.stringify({ID: 10})); + AsyncStorageMock.getAllKeys.mockResolvedValue([COLLECTION_ITEM_KEY]); + await initOnyx(); + + // WHEN both components render + render(); + const result = render(); + await waitForPromisesToResolve(); + + // WHEN the component using the individual item unmounts + result.unmount(); + await waitForPromisesToResolve(); + + // THEN The item should not be removed from cache as it's used in a collection + expect(cache.hasCacheForKey(COLLECTION_ITEM_KEY)).toBe(true); + }); }); }); From f14e5d758e06ea5effa22b156a7eadd4620743d7 Mon Sep 17 00:00:00 2001 From: Peter Velkov Date: Fri, 28 May 2021 23:26:54 +0300 Subject: [PATCH 26/40] test: fix react warning that items don't have a key --- tests/components/ViewWithCollections.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/components/ViewWithCollections.js b/tests/components/ViewWithCollections.js index fb7f2e721..c11e8cb43 100644 --- a/tests/components/ViewWithCollections.js +++ b/tests/components/ViewWithCollections.js @@ -24,8 +24,8 @@ const ViewWithCollections = (props) => { return ( - {_.map(props.collections, collection => ( - {collection.ID} + {_.map(props.collections, (collection, i) => ( + {collection.ID} ))} ); From 2494a55bd84bdcb27c930423a3dd37765dce8ae8 Mon Sep 17 00:00:00 2001 From: Peter Velkov Date: Fri, 28 May 2021 23:45:49 +0300 Subject: [PATCH 27/40] refactor: rename `OnyxCache.update` to `OnyxCache.set` --- lib/Onyx.js | 2 +- lib/OnyxCache.js | 8 ++-- tests/unit/onyxCacheTest.js | 74 ++++++++++++++++++------------------- 3 files changed, 42 insertions(+), 42 deletions(-) diff --git a/lib/Onyx.js b/lib/Onyx.js index aba68c6e0..53a0fff64 100644 --- a/lib/Onyx.js +++ b/lib/Onyx.js @@ -478,7 +478,7 @@ function evictStorageAndRetry(error, ionMethod, ...args) { */ function set(key, val) { // Adds the key to cache when it's not available - cache.update(key, val); + cache.set(key, val); // Optimistically inform subscribers keyChanged(key, val); diff --git a/lib/OnyxCache.js b/lib/OnyxCache.js index d7a566c32..7feb7f2c4 100644 --- a/lib/OnyxCache.js +++ b/lib/OnyxCache.js @@ -32,7 +32,7 @@ class OnyxCache { // bind all methods to prevent problems with `this` _.bindAll( this, - 'getAllKeys', 'getValue', 'hasCacheForKey', 'update', 'remove', 'merge', 'resolveTask', + 'getAllKeys', 'getValue', 'hasCacheForKey', 'set', 'remove', 'merge', 'resolveTask', ); } @@ -74,7 +74,7 @@ class OnyxCache { } // Get the value using the fallback. This task optimizes concurrent calls see `resolveTask` for details - const startTask = () => fallback(key).then(value => this.update(key, value)); + const startTask = () => fallback(key).then(value => this.set(key, value)); return this.resolveTask(`getValue:${key}`, startTask); } @@ -95,7 +95,7 @@ class OnyxCache { * @param {*} [value] * @returns {*} value - returns the cache value */ - update(key, value) { + set(key, value) { // Keep storage keys up to date so that `getAllKeys` works correctly this.storageKeys.add(key); @@ -145,7 +145,7 @@ class OnyxCache { newValue = [...existingValue, ...valueToMerge]; } - this.update(key, newValue); + this.set(key, newValue); }); } diff --git a/tests/unit/onyxCacheTest.js b/tests/unit/onyxCacheTest.js index f77096b1b..9eb7380c7 100644 --- a/tests/unit/onyxCacheTest.js +++ b/tests/unit/onyxCacheTest.js @@ -36,9 +36,9 @@ describe('Onyx', () => { it('Should keep storage keys', async () => { // GIVEN cache with some items - cache.update('mockKey', 'mockValue'); - cache.update('mockKey2', 'mockValue'); - cache.update('mockKey3', 'mockValue'); + cache.set('mockKey', 'mockValue'); + cache.set('mockKey2', 'mockValue'); + cache.set('mockKey3', 'mockValue'); // GIVEN a fallback function const mockFallback = jest.fn().mockResolvedValue(['a', 'b', 'c']); @@ -53,9 +53,9 @@ describe('Onyx', () => { it('Should keep storage keys even when no values are provided', async () => { // GIVEN cache with some items - cache.update('mockKey'); - cache.update('mockKey2'); - cache.update('mockKey3'); + cache.set('mockKey'); + cache.set('mockKey2'); + cache.set('mockKey3'); // THEN the keys should be stored in cache const allKeys = await cache.getAllKeys(jest.fn()); @@ -64,12 +64,12 @@ describe('Onyx', () => { it('Should not store duplicate keys', async () => { // GIVEN cache with some items - cache.update('mockKey', 'mockValue'); - cache.update('mockKey2', 'mockValue'); - cache.update('mockKey3', 'mockValue'); + cache.set('mockKey', 'mockValue'); + cache.set('mockKey2', 'mockValue'); + cache.set('mockKey3', 'mockValue'); // WHEN an existing keys is later updated - cache.update('mockKey2', 'new mock value'); + cache.set('mockKey2', 'new mock value'); // THEN getAllKeys should not include a duplicate value const allKeys = await cache.getAllKeys(jest.fn()); @@ -110,7 +110,7 @@ describe('Onyx', () => { it('Should return cached value when it exists', async () => { // GIVEN cache with some items - cache.update('mockKey', {items: ['mockValue', 'mockValue2']}); + cache.set('mockKey', {items: ['mockValue', 'mockValue2']}); // GIVEN a fallback function const mockFallback = jest.fn().mockResolvedValue('myResult'); @@ -173,8 +173,8 @@ describe('Onyx', () => { it('Should return cached value when it exists', () => { // GIVEN cache with some items - cache.update('mockKey', {items: ['mockValue', 'mockValue2']}); - cache.update('mockKey2', 'mockValue3'); + cache.set('mockKey', {items: ['mockValue', 'mockValue2']}); + cache.set('mockKey2', 'mockValue3'); // WHEN a value exists in cache // THEN it should return true @@ -183,12 +183,12 @@ describe('Onyx', () => { }); }); - describe('update', () => { + describe('set', () => { it('Should add data to cache when both key and value are provided', async () => { // GIVEN empty cache - // WHEN update is called with key and value - cache.update('mockKey', {value: 'mockValue'}); + // WHEN set is called with key and value + cache.set('mockKey', {value: 'mockValue'}); // THEN data should be cached const data = await cache.getValue('mockKey', jest.fn()); @@ -198,8 +198,8 @@ describe('Onyx', () => { it('Should store only the key when no value is provided', async () => { // GIVEN empty cache - // WHEN update is called with key and value - cache.update('mockKey'); + // WHEN set is called with key and value + cache.set('mockKey'); // THEN there should be no cached value expect(cache.hasCacheForKey('mockKey')).toBe(false); @@ -211,11 +211,11 @@ describe('Onyx', () => { it('Should overwrite existing cache items for the given key', async () => { // GIVEN cache with some items - cache.update('mockKey', {value: 'mockValue'}); - cache.update('mockKey2', {other: 'otherMockValue'}); + cache.set('mockKey', {value: 'mockValue'}); + cache.set('mockKey2', {other: 'otherMockValue'}); - // WHEN update is called for an existing key - cache.update('mockKey2', {value: []}); + // WHEN set is called for an existing key + cache.set('mockKey2', {value: []}); // THEN the value should be overwritten const value = await cache.getValue('mockKey2', jest.fn()); @@ -226,9 +226,9 @@ describe('Onyx', () => { describe('remove', () => { it('Should remove the key from all keys', async () => { // GIVEN cache with some items - cache.update('mockKey', 'mockValue'); - cache.update('mockKey2', 'mockValue'); - cache.update('mockKey3', 'mockValue'); + cache.set('mockKey', 'mockValue'); + cache.set('mockKey2', 'mockValue'); + cache.set('mockKey3', 'mockValue'); // WHEN an key is removed cache.remove('mockKey2'); @@ -240,8 +240,8 @@ describe('Onyx', () => { it('Should remove the key from cache', () => { // GIVEN cache with some items - cache.update('mockKey', {items: ['mockValue', 'mockValue2']}); - cache.update('mockKey2', 'mockValue3'); + cache.set('mockKey', {items: ['mockValue', 'mockValue2']}); + cache.set('mockKey2', 'mockValue3'); // WHEN a key is removed cache.remove('mockKey'); @@ -270,8 +270,8 @@ describe('Onyx', () => { it('Should merge data to existing cache value', async () => { // GIVEN cache with some items - cache.update('mockKey', {value: 'mockValue'}); - cache.update('mockKey2', {other: 'otherMockValue', mock: 'mock'}); + cache.set('mockKey', {value: 'mockValue'}); + cache.set('mockKey2', {other: 'otherMockValue', mock: 'mock'}); // WHEN merge is called with key value pairs cache.merge([ @@ -296,7 +296,7 @@ describe('Onyx', () => { it('Should merge objects correctly', async () => { // GIVEN cache with existing object data - cache.update('mockKey', {value: 'mockValue', anotherValue: 'overwrite me'}); + cache.set('mockKey', {value: 'mockValue', anotherValue: 'overwrite me'}); // WHEN merge is called with an object cache.merge([ @@ -314,7 +314,7 @@ describe('Onyx', () => { it('Should merge arrays correctly', async () => { // GIVEN cache with existing array data - cache.update('mockKey', [1, 2, 3]); + cache.set('mockKey', [1, 2, 3]); // WHEN merge is called with an array cache.merge([ @@ -328,7 +328,7 @@ describe('Onyx', () => { it('Should work with primitive values', async () => { // GIVEN cache with existing data - cache.update('mockKey', {}); + cache.set('mockKey', {}); // WHEN merge is called with bool cache.merge([['mockKey', false]]); @@ -368,8 +368,8 @@ describe('Onyx', () => { it('Should remove a key if the new value is `undefined`', () => { // GIVEN cache with existing data - cache.update('mockKey', {}); - cache.update('mockKey1', {}); + cache.set('mockKey', {}); + cache.set('mockKey1', {}); // WHEN merge is called key value pair and the value is undefined cache.merge([['mockKey', undefined]]); @@ -382,9 +382,9 @@ describe('Onyx', () => { it('Should work with multiple key value pairs', async () => { // GIVEN cache with existing data - cache.update('mockKey1', {ID: 1}); - cache.update('mockKey2', {ID: 2}); - cache.update('mockKey3', {ID: 3}); + cache.set('mockKey1', {ID: 1}); + cache.set('mockKey2', {ID: 2}); + cache.set('mockKey3', {ID: 3}); // WHEN merge is called with multiple key value pairs cache.merge([ From ee177e27eeae0208d3aeeef451c4eedf8badfe3b Mon Sep 17 00:00:00 2001 From: Peter Velkov Date: Fri, 28 May 2021 23:56:47 +0300 Subject: [PATCH 28/40] refactor: extract a separate `addKey` method --- lib/OnyxCache.js | 25 ++++++++++++--------- tests/unit/onyxCacheTest.js | 44 +++++++++++++++++++++++++------------ 2 files changed, 45 insertions(+), 24 deletions(-) diff --git a/lib/OnyxCache.js b/lib/OnyxCache.js index 7feb7f2c4..580f7a3b2 100644 --- a/lib/OnyxCache.js +++ b/lib/OnyxCache.js @@ -32,7 +32,7 @@ class OnyxCache { // bind all methods to prevent problems with `this` _.bindAll( this, - 'getAllKeys', 'getValue', 'hasCacheForKey', 'set', 'remove', 'merge', 'resolveTask', + 'getAllKeys', 'getValue', 'hasCacheForKey', 'addKey', 'set', 'remove', 'merge', 'resolveTask', ); } @@ -88,21 +88,26 @@ class OnyxCache { } /** - * Update or add data to cache + * Saves a key in the storage keys list + * Serves to keep the result of `getAllKeys` up to date + * @param {string} key + */ + addKey(key) { + this.storageKeys.add(key); + } + + /** + * Set's a key value in cache + * Adds the keys to the storage keys list as well * Providing value is optional. Skipping value would just capture the key so that * it's available in `getAllKeys` * @param {string} key - * @param {*} [value] + * @param {*} value * @returns {*} value - returns the cache value */ set(key, value) { - // Keep storage keys up to date so that `getAllKeys` works correctly - this.storageKeys.add(key); - - // When a 2nd parameter (value) is provided - store it under the key - if (isDefined(value)) { - this.storageMap[key] = value; - } + this.addKey(key); + this.storageMap[key] = value; return value; } diff --git a/tests/unit/onyxCacheTest.js b/tests/unit/onyxCacheTest.js index 9eb7380c7..606d3fe58 100644 --- a/tests/unit/onyxCacheTest.js +++ b/tests/unit/onyxCacheTest.js @@ -183,23 +183,12 @@ describe('Onyx', () => { }); }); - describe('set', () => { - it('Should add data to cache when both key and value are provided', async () => { + describe('addKey', () => { + it('Should store the key so that it is returned by `getAllKeys`', async () => { // GIVEN empty cache // WHEN set is called with key and value - cache.set('mockKey', {value: 'mockValue'}); - - // THEN data should be cached - const data = await cache.getValue('mockKey', jest.fn()); - expect(data).toEqual({value: 'mockValue'}); - }); - - it('Should store only the key when no value is provided', async () => { - // GIVEN empty cache - - // WHEN set is called with key and value - cache.set('mockKey'); + cache.addKey('mockKey'); // THEN there should be no cached value expect(cache.hasCacheForKey('mockKey')).toBe(false); @@ -209,6 +198,33 @@ describe('Onyx', () => { expect(allKeys).toEqual(expect.arrayContaining(['mockKey'])); }); + it('Should not make duplicate keys', async () => { + // GIVEN empty cache + + // WHEN the same item is added multiple times + cache.addKey('mockKey'); + cache.addKey('mockKey'); + cache.addKey('mockKey2'); + cache.addKey('mockKey'); + + // THEN getAllKeys should not include a duplicate value + const allKeys = await cache.getAllKeys(jest.fn()); + expect(allKeys).toEqual(['mockKey', 'mockKey2']); + }); + }); + + describe('set', () => { + it('Should add data to cache when both key and value are provided', async () => { + // GIVEN empty cache + + // WHEN set is called with key and value + cache.set('mockKey', {value: 'mockValue'}); + + // THEN data should be cached + const data = await cache.getValue('mockKey', jest.fn()); + expect(data).toEqual({value: 'mockValue'}); + }); + it('Should overwrite existing cache items for the given key', async () => { // GIVEN cache with some items cache.set('mockKey', {value: 'mockValue'}); From 55086b433b2c244df96e9eb7061fbcd3b9fc70f7 Mon Sep 17 00:00:00 2001 From: Peter Velkov Date: Sat, 29 May 2021 00:15:08 +0300 Subject: [PATCH 29/40] fix: Onyx.multiSet should not use cache.merge, but cache.set to overwrite data --- lib/Onyx.js | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/lib/Onyx.js b/lib/Onyx.js index 53a0fff64..2444ea384 100644 --- a/lib/Onyx.js +++ b/lib/Onyx.js @@ -509,11 +509,11 @@ function prepareKeyValuePairsForStorage(data) { function multiSet(data) { const keyValuePairs = prepareKeyValuePairsForStorage(data); - // Capture (non-stringified) changes to cache - cache.merge(_.pairs(data)); - - // Optimistically inform subscribers - _.each(data, (val, key) => keyChanged(key, val)); + _.each(data, (val, key) => { + // Update cache and optimistically inform subscribers + cache.set(key, val); + keyChanged(key, val); + }); return AsyncStorage.multiSet(keyValuePairs) .catch(error => evictStorageAndRetry(error, multiSet, data)); From afd91ca24c38e8e4cb8bd6233a68d30de896c0db Mon Sep 17 00:00:00 2001 From: Peter Velkov Date: Tue, 1 Jun 2021 16:31:44 +0300 Subject: [PATCH 30/40] refactor: Update cache merge to work like AsyncStorage.merge and lodashMerge --- lib/Onyx.js | 4 +- lib/OnyxCache.js | 35 +++----------- tests/unit/onyxCacheTest.js | 93 +++++++++++++++---------------------- 3 files changed, 45 insertions(+), 87 deletions(-) diff --git a/lib/Onyx.js b/lib/Onyx.js index 2444ea384..94d2e8ea6 100644 --- a/lib/Onyx.js +++ b/lib/Onyx.js @@ -644,8 +644,8 @@ function mergeCollection(collectionKey, collection) { const existingCollectionPromise = AsyncStorage.multiMerge(keyValuePairsForExistingCollection); const newCollectionPromise = AsyncStorage.multiSet(keyValuePairsForNewCollection); - // Capture (non-stringified) changes to cache - cache.merge(_.pairs(collection)); + // Merge original data to cache + cache.merge(collection); // Optimistically inform collection subscribers keysChanged(collectionKey, collection); diff --git a/lib/OnyxCache.js b/lib/OnyxCache.js index 580f7a3b2..b5eed314d 100644 --- a/lib/OnyxCache.js +++ b/lib/OnyxCache.js @@ -1,4 +1,6 @@ import _ from 'underscore'; +import lodashMerge from 'lodash/merge'; + const isDefined = _.negate(_.isUndefined); @@ -122,36 +124,11 @@ class OnyxCache { } /** - * Merge data to cache, any non existing keys will be crated - * Array items are concatenated - * Object items are merged - * If the existing value in cache and the new value are not the same type - * the existing value is replaced by the new value - * In case the merge value for a key is undefined - the key is removed from cache - * @param {Array<[string, *]>} pairs - array of key value pairs + * Deep merge data to cache, any non existing keys will be crated + * @param {Record} data - a map of (cache) key - values */ - merge(pairs) { - _.forEach(pairs, ([key, valueToMerge]) => { - if (_.isUndefined(valueToMerge)) { - this.remove(key); - return; - } - - const existingValue = this.storageMap[key]; - let newValue = valueToMerge; - - /* Perform merge or concatenation for complex values otherwise just replace the old value */ - - if (_.isObject(valueToMerge) && _.isObject(existingValue)) { - newValue = {...existingValue, ...valueToMerge}; - } - - if (_.isArray(valueToMerge) && _.isArray(existingValue)) { - newValue = [...existingValue, ...valueToMerge]; - } - - this.set(key, newValue); - }); + merge(data) { + this.storageMap = lodashMerge({}, this.storageMap, data); } /** diff --git a/tests/unit/onyxCacheTest.js b/tests/unit/onyxCacheTest.js index 606d3fe58..2af481a9c 100644 --- a/tests/unit/onyxCacheTest.js +++ b/tests/unit/onyxCacheTest.js @@ -271,11 +271,11 @@ describe('Onyx', () => { it('Should create the value in cache when it does not exist', async () => { // GIVEN empty cache - // WHEN merge is called with key value pairs - cache.merge([ - ['mockKey', {value: 'mockValue'}], - ['mockKey2', {value: 'mockValue2'}], - ]); + // WHEN merge is called with new key value pairs + cache.merge({ + mockKey: {value: 'mockValue'}, + mockKey2: {value: 'mockValue2'} + }); // THEN data should be created in cache const value1 = await cache.getValue('mockKey', jest.fn()); @@ -287,13 +287,13 @@ describe('Onyx', () => { it('Should merge data to existing cache value', async () => { // GIVEN cache with some items cache.set('mockKey', {value: 'mockValue'}); - cache.set('mockKey2', {other: 'otherMockValue', mock: 'mock'}); + cache.set('mockKey2', {other: 'otherMockValue', mock: 'mock', items: [3, 4, 5]}); - // WHEN merge is called with key value pairs - cache.merge([ - ['mockKey', {mockItems: []}], - ['mockKey2', {items: [1, 2], other: 'overwrittenMockValue'}] - ]); + // WHEN merge is called with existing key value pairs + cache.merge({ + mockKey: {mockItems: []}, + mockKey2: {items: [1, 2], other: 'overwrittenMockValue'} + }); // THEN the values should be merged together in cache const value1 = await cache.getValue('mockKey', jest.fn()); @@ -305,7 +305,7 @@ describe('Onyx', () => { const value2 = await cache.getValue('mockKey2', jest.fn()); expect(value2).toEqual({ other: 'overwrittenMockValue', - items: [1, 2], + items: [1, 2, 5], mock: 'mock', }); }); @@ -314,10 +314,10 @@ describe('Onyx', () => { // GIVEN cache with existing object data cache.set('mockKey', {value: 'mockValue', anotherValue: 'overwrite me'}); - // WHEN merge is called with an object - cache.merge([ - ['mockKey', {mockItems: [], anotherValue: 'overwritten'}], - ]); + // WHEN merge is called for a key with object value + cache.merge({ + mockKey: {mockItems: [], anotherValue: 'overwritten'} + }); // THEN the values should be merged together in cache const value = await cache.getValue('mockKey', jest.fn()); @@ -330,16 +330,18 @@ describe('Onyx', () => { it('Should merge arrays correctly', async () => { // GIVEN cache with existing array data - cache.set('mockKey', [1, 2, 3]); + cache.set('mockKey', [{ID: 1}, {ID: 2}, {ID: 3}]); // WHEN merge is called with an array - cache.merge([ - ['mockKey', [3, 4, 5]], - ]); + cache.merge({ + mockKey: [{ID: 3}, {added: 'field'}, {}, {ID: 1000}] + }); - // THEN the arrays should be concatenated + // THEN the arrays should be merged as expected const value = await cache.getValue('mockKey', jest.fn()); - expect(value).toEqual([1, 2, 3, 3, 4, 5]); + expect(value).toEqual([ + {ID: 3}, {ID: 2, added: 'field'}, {ID: 3}, {ID: 1000} + ]); }); it('Should work with primitive values', async () => { @@ -347,75 +349,54 @@ describe('Onyx', () => { cache.set('mockKey', {}); // WHEN merge is called with bool - cache.merge([['mockKey', false]]); + cache.merge({mockKey: false}); // THEN the object should be overwritten with a bool value const bool = await cache.getValue('mockKey', jest.fn()); expect(bool).toBe(false); // WHEN merge is called with number - cache.merge([['mockKey', 0]]); + cache.merge({mockKey: 0}); // THEN the value should be overwritten const number = await cache.getValue('mockKey', jest.fn()); expect(number).toEqual(0); // WHEN merge is called with string - cache.merge([['mockKey', '123']]); + cache.merge({mockKey: '123'}); // THEN the value should be overwritten const string = await cache.getValue('mockKey', jest.fn()); expect(string).toEqual('123'); // WHEN merge is called with string again - cache.merge([['mockKey', '123']]); + cache.merge({mockKey: '123'}); // THEN strings should not have been concatenated const string2 = await cache.getValue('mockKey', jest.fn()); expect(string2).toEqual('123'); // WHEN merge is called with an object - cache.merge([['mockKey', {value: 'myMockObject'}]]); + cache.merge({mockKey: {value: 'myMockObject'}}); // THEN the old primitive value should be overwritten with the object const object = await cache.getValue('mockKey', jest.fn()); expect(object).toEqual({value: 'myMockObject'}); }); - it('Should remove a key if the new value is `undefined`', () => { + it('Should do nothing to a key which value is `undefined`', async () => { // GIVEN cache with existing data - cache.set('mockKey', {}); - cache.set('mockKey1', {}); + cache.set('mockKey', {ID: 5}); // WHEN merge is called key value pair and the value is undefined - cache.merge([['mockKey', undefined]]); - cache.merge([['mockKey2']]); - - // THEN the key should be removed from cache - expect(cache.hasCacheForKey('mockKey')).toBe(false); - expect(cache.hasCacheForKey('mockKey2')).toBe(false); - }); + cache.merge({mockKey: undefined}); - it('Should work with multiple key value pairs', async () => { - // GIVEN cache with existing data - cache.set('mockKey1', {ID: 1}); - cache.set('mockKey2', {ID: 2}); - cache.set('mockKey3', {ID: 3}); - - // WHEN merge is called with multiple key value pairs - cache.merge([ - ['mockKey2', {value: 'mockValue2'}], - ['mockKey4', 'Some string data'], - ['mockKey3', {ID: '3'}], - ['mockKey5', {ID: '3'}], - ]); + // THEN the key should still be in cache + expect(cache.hasCacheForKey('mockKey')).toBe(true); - // THEN values new values should be added and existing values updated - expect(await cache.getValue('mockKey1', jest.fn())).toEqual({ID: 1}); - expect(await cache.getValue('mockKey2', jest.fn())).toEqual({ID: 2, value: 'mockValue2'}); - expect(await cache.getValue('mockKey3', jest.fn())).toEqual({ID: '3'}); - expect(await cache.getValue('mockKey4', jest.fn())).toEqual('Some string data'); - expect(await cache.getValue('mockKey5', jest.fn())).toEqual({ID: '3'}); + // THEN and the value should be unchanged + const value = await cache.getValue('mockKey', jest.fn()); + expect(value).toEqual({ID: 5}); }); }); From 92314759e12f587b0d25d99a47673ffd82187474 Mon Sep 17 00:00:00 2001 From: Peter Velkov Date: Tue, 1 Jun 2021 18:41:47 +0300 Subject: [PATCH 31/40] Revert "refactor: encapsulate cache checking and resolving logic for get in the cache service" This reverts commit 4af3abf7 It simpler and easier to maintain when we leave `get` and `getAllKeys` deal with cache instead of trying to move the logic to the cache module --- lib/Onyx.js | 17 ++++- lib/OnyxCache.js | 17 +---- tests/unit/onyxCacheTest.js | 126 ++++++++++-------------------------- 3 files changed, 51 insertions(+), 109 deletions(-) diff --git a/lib/Onyx.js b/lib/Onyx.js index 94d2e8ea6..79033d17c 100644 --- a/lib/Onyx.js +++ b/lib/Onyx.js @@ -35,12 +35,23 @@ let defaultKeyStates = {}; * @returns {Promise<*>} */ function get(key) { - return cache.getValue(key, () => AsyncStorage.getItem(key) + // When we already have the value in cache - resolve right away + if (cache.hasCacheForKey(key)) { + return Promise.resolve(cache.getValue(key)); + } + + /* Otherwise setup a task to retrieve it. + This ensures concurrent usages would not start the same task over and over */ + const getFromStorage = () => AsyncStorage.getItem(key) .then((val) => { - const parsed = JSON.parse(val); + // Add values to cache and return parsed result + const parsed = val && JSON.parse(val); + cache.set(key, parsed); return parsed; - })) + }) .catch(err => logInfo(`Unable to get item from persistent storage. Key: ${key} Error: ${err}`)); + + return cache.resolveTask(`get:${key}`, getFromStorage); } /** diff --git a/lib/OnyxCache.js b/lib/OnyxCache.js index b5eed314d..9adad1172 100644 --- a/lib/OnyxCache.js +++ b/lib/OnyxCache.js @@ -62,22 +62,11 @@ class OnyxCache { /** * Get a cached value from storage - * Provide a promise returning fallback that will be executed when - * the value is not available in cache - * @template T * @param {string} key - * @param {function(key: string): Promise} fallback - a fallback to retrieve the item from storage - * @returns {Promise} + * @returns {*} */ - getValue(key, fallback) { - // When we have cache resolve right away - if (this.hasCacheForKey(key)) { - return Promise.resolve(this.storageMap[key]); - } - - // Get the value using the fallback. This task optimizes concurrent calls see `resolveTask` for details - const startTask = () => fallback(key).then(value => this.set(key, value)); - return this.resolveTask(`getValue:${key}`, startTask); + getValue(key) { + return this.storageMap[key]; } /** diff --git a/tests/unit/onyxCacheTest.js b/tests/unit/onyxCacheTest.js index 2af481a9c..c6ab9d2d6 100644 --- a/tests/unit/onyxCacheTest.js +++ b/tests/unit/onyxCacheTest.js @@ -96,69 +96,25 @@ describe('Onyx', () => { }); describe('getValue', () => { - it('Should use the fallback function when there is no stored value', async () => { - // GIVEN empty cache and a fallback function - const mockFallback = jest.fn().mockResolvedValue('myResult'); + it('Should return undefined when there is no stored value', () => { + // GIVEN empty cache // WHEN a value is retrieved - const result = await cache.getValue('mockKey', mockFallback); + const result = cache.getValue('mockKey'); - // THEN the fallback should be used to retrieve the value - expect(result).toEqual('myResult'); - expect(mockFallback).toHaveBeenCalledTimes(1); + // THEN it should be undefined + expect(result).not.toBeDefined(); }); - it('Should return cached value when it exists', async () => { + it('Should return cached value when it exists', () => { // GIVEN cache with some items cache.set('mockKey', {items: ['mockValue', 'mockValue2']}); - - // GIVEN a fallback function - const mockFallback = jest.fn().mockResolvedValue('myResult'); - - // WHEN a value is retrieved - const value = await cache.getValue('mockKey', mockFallback); - - // THEN it should have the correct content - expect(value).toEqual({items: ['mockValue', 'mockValue2']}); - - // AND the fallback function should be executed - expect(mockFallback).not.toHaveBeenCalled(); - }); - - it('Should execute the fallback only once per key for concurrent calls', async () => { - // GIVEN empty cache and a fallback function - const mockFallback = jest.fn().mockImplementation(key => Promise.resolve(`Result for ${key}`)); - - // WHEN keys are retrieved in parallel - const promise1 = cache.getValue('mockKey1', mockFallback); - const promise2 = cache.getValue('mockKey2', mockFallback); - const promise3 = cache.getValue('mockKey1', mockFallback); - const promise4 = cache.getValue('mockKey1', mockFallback); - const promise5 = cache.getValue('mockKey2', mockFallback); - - const results = await Promise.all([promise1, promise2, promise3, promise4, promise5]); - - // THEN the fallback should be called 2 times - once per key - expect(mockFallback).toHaveBeenCalledTimes(2); - expect(results[0]).toEqual('Result for mockKey1'); - expect(results[1]).toEqual('Result for mockKey2'); - expect(results[2]).toEqual('Result for mockKey1'); - expect(results[3]).toEqual('Result for mockKey1'); - expect(results[4]).toEqual('Result for mockKey2'); - }); - - it('Should not store cache if the fallback method failed', async () => { - // GIVEN empty cache and a fallback function that rejects - const mockFallback = jest.fn().mockRejectedValue( - new Error('Unable to get item from persistent storage') - ); + cache.set('mockKey2', 'mockValue3'); // WHEN a value is retrieved - return cache.getValue('mockKey', mockFallback) - .catch(() => { - // THEN no value should be in cache - expect(cache.hasCacheForKey('mockKey')).toBe(false); - }); + // THEN it should be the correct value + expect(cache.getValue('mockKey')).toEqual({items: ['mockValue', 'mockValue2']}); + expect(cache.getValue('mockKey2')).toEqual('mockValue3'); }); }); @@ -214,18 +170,18 @@ describe('Onyx', () => { }); describe('set', () => { - it('Should add data to cache when both key and value are provided', async () => { + it('Should add data to cache when both key and value are provided', () => { // GIVEN empty cache // WHEN set is called with key and value cache.set('mockKey', {value: 'mockValue'}); // THEN data should be cached - const data = await cache.getValue('mockKey', jest.fn()); + const data = cache.getValue('mockKey'); expect(data).toEqual({value: 'mockValue'}); }); - it('Should overwrite existing cache items for the given key', async () => { + it('Should overwrite existing cache items for the given key', () => { // GIVEN cache with some items cache.set('mockKey', {value: 'mockValue'}); cache.set('mockKey2', {other: 'otherMockValue'}); @@ -234,8 +190,7 @@ describe('Onyx', () => { cache.set('mockKey2', {value: []}); // THEN the value should be overwritten - const value = await cache.getValue('mockKey2', jest.fn()); - expect(value).toEqual({value: []}); + expect(cache.getValue('mockKey2')).toEqual({value: []}); }); }); @@ -264,11 +219,12 @@ describe('Onyx', () => { // THEN a value should not be available in cache expect(cache.hasCacheForKey('mockKey')).toBe(false); + expect(cache.getValue('mockKey')).not.toBeDefined(); }); }); describe('merge', () => { - it('Should create the value in cache when it does not exist', async () => { + it('Should create the value in cache when it does not exist', () => { // GIVEN empty cache // WHEN merge is called with new key value pairs @@ -278,13 +234,11 @@ describe('Onyx', () => { }); // THEN data should be created in cache - const value1 = await cache.getValue('mockKey', jest.fn()); - const value2 = await cache.getValue('mockKey2', jest.fn()); - expect(value1).toEqual({value: 'mockValue'}); - expect(value2).toEqual({value: 'mockValue2'}); + expect(cache.getValue('mockKey')).toEqual({value: 'mockValue'}); + expect(cache.getValue('mockKey2')).toEqual({value: 'mockValue2'}); }); - it('Should merge data to existing cache value', async () => { + it('Should merge data to existing cache value', () => { // GIVEN cache with some items cache.set('mockKey', {value: 'mockValue'}); cache.set('mockKey2', {other: 'otherMockValue', mock: 'mock', items: [3, 4, 5]}); @@ -296,21 +250,19 @@ describe('Onyx', () => { }); // THEN the values should be merged together in cache - const value1 = await cache.getValue('mockKey', jest.fn()); - expect(value1).toEqual({ + expect(cache.getValue('mockKey')).toEqual({ value: 'mockValue', mockItems: [], }); - const value2 = await cache.getValue('mockKey2', jest.fn()); - expect(value2).toEqual({ + expect(cache.getValue('mockKey2')).toEqual({ other: 'overwrittenMockValue', items: [1, 2, 5], mock: 'mock', }); }); - it('Should merge objects correctly', async () => { + it('Should merge objects correctly', () => { // GIVEN cache with existing object data cache.set('mockKey', {value: 'mockValue', anotherValue: 'overwrite me'}); @@ -320,15 +272,14 @@ describe('Onyx', () => { }); // THEN the values should be merged together in cache - const value = await cache.getValue('mockKey', jest.fn()); - expect(value).toEqual({ + expect(cache.getValue('mockKey')).toEqual({ value: 'mockValue', mockItems: [], anotherValue: 'overwritten', }); }); - it('Should merge arrays correctly', async () => { + it('Should merge arrays correctly', () => { // GIVEN cache with existing array data cache.set('mockKey', [{ID: 1}, {ID: 2}, {ID: 3}]); @@ -338,13 +289,12 @@ describe('Onyx', () => { }); // THEN the arrays should be merged as expected - const value = await cache.getValue('mockKey', jest.fn()); - expect(value).toEqual([ + expect(cache.getValue('mockKey')).toEqual([ {ID: 3}, {ID: 2, added: 'field'}, {ID: 3}, {ID: 1000} ]); }); - it('Should work with primitive values', async () => { + it('Should work with primitive values', () => { // GIVEN cache with existing data cache.set('mockKey', {}); @@ -352,51 +302,43 @@ describe('Onyx', () => { cache.merge({mockKey: false}); // THEN the object should be overwritten with a bool value - const bool = await cache.getValue('mockKey', jest.fn()); - expect(bool).toBe(false); + expect(cache.getValue('mockKey')).toEqual(false); // WHEN merge is called with number cache.merge({mockKey: 0}); // THEN the value should be overwritten - const number = await cache.getValue('mockKey', jest.fn()); - expect(number).toEqual(0); + expect(cache.getValue('mockKey')).toEqual(0); // WHEN merge is called with string cache.merge({mockKey: '123'}); // THEN the value should be overwritten - const string = await cache.getValue('mockKey', jest.fn()); - expect(string).toEqual('123'); + expect(cache.getValue('mockKey')).toEqual('123'); // WHEN merge is called with string again cache.merge({mockKey: '123'}); // THEN strings should not have been concatenated - const string2 = await cache.getValue('mockKey', jest.fn()); - expect(string2).toEqual('123'); + expect(cache.getValue('mockKey')).toEqual('123'); // WHEN merge is called with an object cache.merge({mockKey: {value: 'myMockObject'}}); // THEN the old primitive value should be overwritten with the object - const object = await cache.getValue('mockKey', jest.fn()); - expect(object).toEqual({value: 'myMockObject'}); + expect(cache.getValue('mockKey')).toEqual({value: 'myMockObject'}); }); - it('Should do nothing to a key which value is `undefined`', async () => { + it('Should do nothing to a key which value is `undefined`', () => { // GIVEN cache with existing data cache.set('mockKey', {ID: 5}); // WHEN merge is called key value pair and the value is undefined cache.merge({mockKey: undefined}); - // THEN the key should still be in cache + // THEN the key should still be in cache and the value unchanged expect(cache.hasCacheForKey('mockKey')).toBe(true); - - // THEN and the value should be unchanged - const value = await cache.getValue('mockKey', jest.fn()); - expect(value).toEqual({ID: 5}); + expect(cache.getValue('mockKey')).toEqual({ID: 5}); }); }); From 4e5785782219594a497c817cd76de00d846c514c Mon Sep 17 00:00:00 2001 From: Peter Velkov Date: Tue, 1 Jun 2021 18:49:26 +0300 Subject: [PATCH 32/40] Revert "refactor: encapsulate cache checking and resolving logic for getAllKeys in the cache service" This reverts commit b33e6969 --- lib/Onyx.js | 17 ++++++++- lib/OnyxCache.js | 20 ++-------- tests/unit/onyxCacheTest.js | 76 +++++++++++++------------------------ 3 files changed, 46 insertions(+), 67 deletions(-) diff --git a/lib/Onyx.js b/lib/Onyx.js index 79033d17c..4f5c3f08f 100644 --- a/lib/Onyx.js +++ b/lib/Onyx.js @@ -59,7 +59,22 @@ function get(key) { * @returns {Promise} */ function getAllKeys() { - return cache.getAllKeys(() => AsyncStorage.getAllKeys()); + // When we've already read stored keys, resolve right away + const storedKeys = cache.getAllKeys(); + if (storedKeys.length > 0) { + return Promise.resolve(storedKeys); + } + + /* Otherwise setup a task to retrieve them. + This ensures concurrent usages would not start the same task over and over */ + const getKeysFromStorage = () => AsyncStorage.getAllKeys() + .then((keys) => { + // Add values to cache and return original result + _.each(keys, key => cache.addKey(key)); + return keys; + }); + + return cache.resolveTask('getAllKeys', getKeysFromStorage); } /** diff --git a/lib/OnyxCache.js b/lib/OnyxCache.js index 9adad1172..2fd2a9def 100644 --- a/lib/OnyxCache.js +++ b/lib/OnyxCache.js @@ -40,24 +40,10 @@ class OnyxCache { /** * Get all the storage keys - * Provide a promise returning fallback that will be executed when - * keys aren't available in cache - * @param {function(): Promise} fallback - a callback to retrieve keys from storage - * @returns {Promise} + * @returns {string[]} */ - getAllKeys(fallback) { - // When we have storage keys in cache resolve right away - if (this.storageKeys.size > 0) { - return Promise.resolve([...this.storageKeys]); - } - - // Read keys using the fallback. This task optimizes concurrent calls see `resolveTask` for details - const startTask = () => fallback().then((keys) => { - this.storageKeys = new Set(keys); - return keys; - }); - - return this.resolveTask('getAllKeys', startTask); + getAllKeys() { + return [...this.storageKeys]; } /** diff --git a/tests/unit/onyxCacheTest.js b/tests/unit/onyxCacheTest.js index c6ab9d2d6..231d9ada0 100644 --- a/tests/unit/onyxCacheTest.js +++ b/tests/unit/onyxCacheTest.js @@ -17,52 +17,39 @@ describe('Onyx', () => { }); describe('getAllKeys', () => { - it('Should be empty and resolve from fallback initially', async () => { - // GIVEN empty cache and a fallback function - const mockFallback = jest.fn().mockResolvedValue(['a', 'b', 'c']); + it('Should be empty initially', () => { + // GIVEN empty cache // WHEN all keys are retrieved - const result = await cache.getAllKeys(mockFallback); - - // THEN the result should be provided from the fallback - expect(mockFallback).toHaveBeenCalledTimes(1); - expect(result).toEqual(['a', 'b', 'c']); + const allKeys = cache.getAllKeys(); - // THEN fallback result should be stored to cache and fallback not executed - const cachedResult = await cache.getAllKeys(mockFallback); - expect(cachedResult).toEqual(['a', 'b', 'c']); - expect(mockFallback).toHaveBeenCalledTimes(1); + // THEN the result should be empty + expect(allKeys).toEqual([]); }); - it('Should keep storage keys', async () => { + it('Should keep storage keys', () => { // GIVEN cache with some items cache.set('mockKey', 'mockValue'); cache.set('mockKey2', 'mockValue'); cache.set('mockKey3', 'mockValue'); - // GIVEN a fallback function - const mockFallback = jest.fn().mockResolvedValue(['a', 'b', 'c']); - // THEN the keys should be stored in cache - const allKeys = await cache.getAllKeys(mockFallback); + const allKeys = cache.getAllKeys(); expect(allKeys).toEqual(['mockKey', 'mockKey2', 'mockKey3']); - - // AND the fallback should be executed - expect(mockFallback).not.toHaveBeenCalled(); }); - it('Should keep storage keys even when no values are provided', async () => { + it('Should keep storage keys even when no values are provided', () => { // GIVEN cache with some items cache.set('mockKey'); cache.set('mockKey2'); cache.set('mockKey3'); // THEN the keys should be stored in cache - const allKeys = await cache.getAllKeys(jest.fn()); + const allKeys = cache.getAllKeys(); expect(allKeys).toEqual(['mockKey', 'mockKey2', 'mockKey3']); }); - it('Should not store duplicate keys', async () => { + it('Should not store duplicate keys', () => { // GIVEN cache with some items cache.set('mockKey', 'mockValue'); cache.set('mockKey2', 'mockValue'); @@ -72,27 +59,9 @@ describe('Onyx', () => { cache.set('mockKey2', 'new mock value'); // THEN getAllKeys should not include a duplicate value - const allKeys = await cache.getAllKeys(jest.fn()); + const allKeys = cache.getAllKeys(); expect(allKeys).toEqual(['mockKey', 'mockKey2', 'mockKey3']); }); - - it('Should execute the fallback only once for concurrent calls', async () => { - // GIVEN empty cache and a fallback function - const mockFallback = jest.fn().mockResolvedValue(['a', 'b', 'c']); - - // WHEN all keys are retrieved in parallel - const promise1 = cache.getAllKeys(mockFallback); - const promise2 = cache.getAllKeys(mockFallback); - const promise3 = cache.getAllKeys(mockFallback); - - const [result1, result2, result3] = await Promise.all([promise1, promise2, promise3]); - - // THEN the fallback should be called only once - expect(mockFallback).toHaveBeenCalledTimes(1); - expect(result1).toEqual(['a', 'b', 'c']); - expect(result2).toEqual(['a', 'b', 'c']); - expect(result3).toEqual(['a', 'b', 'c']); - }); }); describe('getValue', () => { @@ -140,7 +109,7 @@ describe('Onyx', () => { }); describe('addKey', () => { - it('Should store the key so that it is returned by `getAllKeys`', async () => { + it('Should store the key so that it is returned by `getAllKeys`', () => { // GIVEN empty cache // WHEN set is called with key and value @@ -150,11 +119,10 @@ describe('Onyx', () => { expect(cache.hasCacheForKey('mockKey')).toBe(false); // THEN but a key should be available - const allKeys = await cache.getAllKeys(jest.fn()); - expect(allKeys).toEqual(expect.arrayContaining(['mockKey'])); + expect(cache.getAllKeys()).toEqual(expect.arrayContaining(['mockKey'])); }); - it('Should not make duplicate keys', async () => { + it('Should not make duplicate keys', () => { // GIVEN empty cache // WHEN the same item is added multiple times @@ -164,7 +132,7 @@ describe('Onyx', () => { cache.addKey('mockKey'); // THEN getAllKeys should not include a duplicate value - const allKeys = await cache.getAllKeys(jest.fn()); + const allKeys = cache.getAllKeys(); expect(allKeys).toEqual(['mockKey', 'mockKey2']); }); }); @@ -181,6 +149,16 @@ describe('Onyx', () => { expect(data).toEqual({value: 'mockValue'}); }); + it('Should store the key so that it is returned by `getAllKeys`', () => { + // GIVEN empty cache + + // WHEN set is called with key and value + cache.set('mockKey', {value: 'mockValue'}); + + // THEN but a key should be available + expect(cache.getAllKeys()).toEqual(expect.arrayContaining(['mockKey'])); + }); + it('Should overwrite existing cache items for the given key', () => { // GIVEN cache with some items cache.set('mockKey', {value: 'mockValue'}); @@ -195,7 +173,7 @@ describe('Onyx', () => { }); describe('remove', () => { - it('Should remove the key from all keys', async () => { + it('Should remove the key from all keys', () => { // GIVEN cache with some items cache.set('mockKey', 'mockValue'); cache.set('mockKey2', 'mockValue'); @@ -205,7 +183,7 @@ describe('Onyx', () => { cache.remove('mockKey2'); // THEN getAllKeys should not include the removed value - const allKeys = await cache.getAllKeys(jest.fn()); + const allKeys = cache.getAllKeys(); expect(allKeys).toEqual(['mockKey', 'mockKey3']); }); From a27841daced2d9f39cefd9d7fd1ac4e6e6511174 Mon Sep 17 00:00:00 2001 From: Peter Velkov Date: Tue, 1 Jun 2021 20:14:58 +0300 Subject: [PATCH 33/40] refactor: extract separate methods from `resolveTask` so usage in Onyx is more clear --- lib/Onyx.js | 30 ++++++++++++++++++---------- lib/OnyxCache.js | 52 +++++++++++++++++++++++++++++------------------- 2 files changed, 51 insertions(+), 31 deletions(-) diff --git a/lib/Onyx.js b/lib/Onyx.js index 4f5c3f08f..0efb9c701 100644 --- a/lib/Onyx.js +++ b/lib/Onyx.js @@ -40,18 +40,23 @@ function get(key) { return Promise.resolve(cache.getValue(key)); } - /* Otherwise setup a task to retrieve it. - This ensures concurrent usages would not start the same task over and over */ - const getFromStorage = () => AsyncStorage.getItem(key) + const taskName = `get:${key}`; + + // When a value retrieving task for this key is still running hook to it + if (cache.hasPendingTask(taskName)) { + return cache.getTaskPromise(taskName); + } + + // Otherwise retrieve the value from storage and capture a promise to aid concurrent usages + const promise = AsyncStorage.getItem(key) .then((val) => { - // Add values to cache and return parsed result const parsed = val && JSON.parse(val); cache.set(key, parsed); return parsed; }) .catch(err => logInfo(`Unable to get item from persistent storage. Key: ${key} Error: ${err}`)); - return cache.resolveTask(`get:${key}`, getFromStorage); + return cache.captureTask(taskName, promise); } /** @@ -65,16 +70,21 @@ function getAllKeys() { return Promise.resolve(storedKeys); } - /* Otherwise setup a task to retrieve them. - This ensures concurrent usages would not start the same task over and over */ - const getKeysFromStorage = () => AsyncStorage.getAllKeys() + const taskName = 'getAllKeys'; + + // When a value retrieving task for this key is still running hook to it + if (cache.hasPendingTask(taskName)) { + return cache.getTaskPromise(taskName); + } + + // Otherwise retrieve the value from storage and capture a promise to aid concurrent usages + const promise = AsyncStorage.getAllKeys() .then((keys) => { - // Add values to cache and return original result _.each(keys, key => cache.addKey(key)); return keys; }); - return cache.resolveTask('getAllKeys', getKeysFromStorage); + return cache.captureTask(taskName, promise); } /** diff --git a/lib/OnyxCache.js b/lib/OnyxCache.js index 2fd2a9def..e4cb434f3 100644 --- a/lib/OnyxCache.js +++ b/lib/OnyxCache.js @@ -34,7 +34,8 @@ class OnyxCache { // bind all methods to prevent problems with `this` _.bindAll( this, - 'getAllKeys', 'getValue', 'hasCacheForKey', 'addKey', 'set', 'remove', 'merge', 'resolveTask', + 'getAllKeys', 'getValue', 'hasCacheForKey', 'addKey', 'set', 'remove', 'merge', + 'hasPendingTask', 'getTaskPromise', 'captureTask', ); } @@ -107,29 +108,38 @@ class OnyxCache { } /** - * Use this method to prevents additional calls from the same thing - * When a promise for a given call already exists the caller would - * be hooked to it. Otherwise the task is started and captured so - * that other callers don't have to start over - * Particularly useful for data retrieving methods + * Check whether the give task is already running + * @param {string} taskName - unique name given for the task + * @returns {*} + */ + hasPendingTask(taskName) { + return isDefined(this.pendingPromises[taskName]); + } + + /** + * Use this method to prevent concurrent calls for the same thing + * Instead of calling the same task again use the existing promise + * provided from this function + * @template T + * @param {string} taskName - unique name given for the task + * @returns {Promise} + */ + getTaskPromise(taskName) { + return this.pendingPromises[taskName]; + } + + /** + * Capture a promise for a given task so other caller can + * hook up to the promise if it's still pending * @template T - * @param {string} taskName - * @param {function(): Promise} startTask - Provide a promise returning function that - * will be invoked if there is no pending promise for this task + * @param {string} taskName - unique name for the task + * @param {Promise} promise * @returns {Promise} */ - resolveTask(taskName, startTask) { - // When a task is already running return it right away - if (isDefined(this.pendingPromises[taskName])) { - return this.pendingPromises[taskName]; - } - - // Otherwise start the task and store a reference - this.pendingPromises[taskName] = startTask() - .finally(() => { - // Cleanup after the task is over - delete this.pendingPromises[taskName]; - }); + captureTask(taskName, promise) { + this.pendingPromises[taskName] = promise.finally(() => { + delete this.pendingPromises[taskName]; + }); return this.pendingPromises[taskName]; } From 1ff60fcbbd576ac6a7aefdc91c8c9f9b6bbb492a Mon Sep 17 00:00:00 2001 From: Peter Velkov Date: Tue, 1 Jun 2021 20:27:44 +0300 Subject: [PATCH 34/40] test: update tests related to task capturing after changes --- tests/unit/onyxCacheTest.js | 91 +++++++++++++++++-------------------- 1 file changed, 41 insertions(+), 50 deletions(-) diff --git a/tests/unit/onyxCacheTest.js b/tests/unit/onyxCacheTest.js index 231d9ada0..e8018a49a 100644 --- a/tests/unit/onyxCacheTest.js +++ b/tests/unit/onyxCacheTest.js @@ -96,7 +96,7 @@ describe('Onyx', () => { expect(cache.hasCacheForKey('mockKey')).toBe(false); }); - it('Should return cached value when it exists', () => { + it('Should return true when cached value exists', () => { // GIVEN cache with some items cache.set('mockKey', {items: ['mockValue', 'mockValue2']}); cache.set('mockKey2', 'mockValue3'); @@ -320,65 +320,56 @@ describe('Onyx', () => { }); }); - describe('resolveTask', () => { - it('Should start a new task when no pending task exists', () => { - // GIVEN empty cache and a function returning a promise - const task = jest.fn().mockResolvedValue({data: 'mockData'}); - - // WHEN resolve task is called with this task - cache.resolveTask('mockTask', task); - - // THEN the task should be triggered - expect(task).toHaveBeenCalledTimes(1); + describe('hasPendingTask', () => { + it('Should return false when there is no started task', () => { + // GIVEN empty cache with no started tasks + // WHEN a task has not been started + // THEN it should return false + expect(cache.hasPendingTask('mockTask')).toBe(false); }); - it('Should not start a task again when it is already captured and running', () => { - // GIVEN cache that have captured a promise for a pending task - const task = jest.fn().mockResolvedValue({data: 'mockData'}); - cache.resolveTask('mockTask', task); - task.mockClear(); + it('Should return true when a task is running', () => { + // GIVEN empty cache with no started tasks + // WHEN a unique task is started + const promise = Promise.resolve(); + cache.captureTask('mockTask', promise); - // WHEN a function tries to run the same task - cache.resolveTask('mockTask', task); - cache.resolveTask('mockTask', task); - cache.resolveTask('mockTask', task); + // THEN `hasPendingTask` should return true + expect(cache.hasPendingTask('mockTask')).toBe(true); - // THEN the task should not have been called again - expect(task).not.toHaveBeenCalled(); + // WHEN the promise is completed + return waitForPromisesToResolve() + .then(() => { + // THEN `hasPendingTask` should return false + expect(cache.hasPendingTask('mockTask')).toBe(false); + }); }); + }); - it('Should start a new task when a previous task was completed and removed', async () => { - // GIVEN cache that have captured a promise for a pending task - const task = jest.fn().mockResolvedValue({data: 'mockData'}); - cache.resolveTask('mockTask', task); - task.mockClear(); - - // WHEN the task is completed - await waitForPromisesToResolve(); + describe('getTaskPromise', () => { + it('Should return undefined when there is no stored value', () => { + // GIVEN empty cache with no started tasks - // WHEN a function tries to run the same task - cache.resolveTask('mockTask', task); + // WHEN a task is retrieved + const task = cache.getTaskPromise('mockTask'); - // THEN a new task should be started - expect(task).toHaveBeenCalledTimes(1); + // THEN it should be undefined + expect(task).not.toBeDefined(); }); - it('Should resolve all tasks with the same result', () => { - // GIVEN empty cache and a function returning a promise - const task = jest.fn().mockResolvedValue({data: 'mockData'}); - - // WHEN multiple tasks are executed at the same time - const promise1 = cache.resolveTask('mockTask', task); - const promise2 = cache.resolveTask('mockTask', task); - const promise3 = cache.resolveTask('mockTask', task); - - // THEN they all should have the same result - Promise.all([promise1, promise2, promise3]) - .then(([result1, result2, result3]) => { - expect(result1).toEqual({data: 'mockData'}); - expect(result2).toEqual({data: 'mockData'}); - expect(result3).toEqual({data: 'mockData'}); - }); + it('Should return captured task when it exists', () => { + // GIVEN empty cache with no started tasks + // WHEN a unique task is started + const promise = Promise.resolve({mockResult: true}); + cache.captureTask('mockTask', promise); + + // WHEN a task is retrieved + const taskPromise = cache.getTaskPromise('mockTask'); + + // THEN it should resolve with the same result as the captured task + return taskPromise.then((result) => { + expect(result).toEqual({mockResult: true}); + }); }); }); }); From 4eed7268cdc7132c3d8d88435c61d719db6cfb14 Mon Sep 17 00:00:00 2001 From: Peter Velkov Date: Tue, 1 Jun 2021 20:45:01 +0300 Subject: [PATCH 35/40] convert async/await tests to promise based chains --- tests/unit/onyxCacheTest.js | 289 ++++++++++++++++++++---------------- 1 file changed, 158 insertions(+), 131 deletions(-) diff --git a/tests/unit/onyxCacheTest.js b/tests/unit/onyxCacheTest.js index e8018a49a..de5f0a356 100644 --- a/tests/unit/onyxCacheTest.js +++ b/tests/unit/onyxCacheTest.js @@ -389,7 +389,7 @@ describe('Onyx', () => { }, }; - async function initOnyx() { + function initOnyx() { const OnyxModule = require('../../index'); Onyx = OnyxModule.default; withOnyx = OnyxModule.withOnyx; @@ -401,18 +401,18 @@ describe('Onyx', () => { }); // Onyx init introduces some side effects e.g. calls the getAllKeys - // We're clearing to have a fresh mock calls history - await waitForPromisesToResolve(); - jest.clearAllMocks(); + // We're clearing mocks to have a fresh calls history + return waitForPromisesToResolve() + .then(() => jest.clearAllMocks()); } - // Always use a "fresh" (and undecorated) instance + // Always use a "fresh" instance beforeEach(() => { jest.resetModules(); return initOnyx(); }); - it('Expect a single call to getItem when multiple components use the same key', async () => { + it('Expect a single call to getItem when multiple components use the same key', () => { const AsyncStorageMock = require('@react-native-community/async-storage/jest/async-storage-mock'); // GIVEN a component connected to Onyx @@ -425,23 +425,25 @@ describe('Onyx', () => { // GIVEN some string value for that key exists in storage AsyncStorageMock.getItem.mockResolvedValue('"mockValue"'); AsyncStorageMock.getAllKeys.mockResolvedValue([ONYX_KEYS.TEST_KEY]); - await initOnyx(); - - // WHEN multiple components are rendered - render( - <> - - - - - ); - - // THEN Async storage `getItem` should be called only once - await waitForPromisesToResolve(); - expect(AsyncStorageMock.getItem).toHaveBeenCalledTimes(1); + return initOnyx() + .then(() => { + // WHEN multiple components are rendered + render( + <> + + + + + ); + }) + .then(waitForPromisesToResolve) + .then(() => { + // THEN Async storage `getItem` should be called only once + expect(AsyncStorageMock.getItem).toHaveBeenCalledTimes(1); + }); }); - it('Expect a single call to getAllKeys when multiple components use the same key', async () => { + it('Expect a single call to getAllKeys when multiple components use the same key', () => { const AsyncStorageMock = require('@react-native-community/async-storage/jest/async-storage-mock'); // GIVEN a component connected to Onyx @@ -452,25 +454,28 @@ describe('Onyx', () => { })(ViewWithText); // GIVEN some string value for that key exists in storage - await initOnyx(); - AsyncStorageMock.getItem.mockResolvedValue('"mockValue"'); - AsyncStorageMock.getAllKeys.mockResolvedValue([ONYX_KEYS.TEST_KEY]); - - // WHEN multiple components are rendered - render( - <> - - - - - ); - - // THEN Async storage `getItem` should be called only once - await waitForPromisesToResolve(); - expect(AsyncStorageMock.getAllKeys).toHaveBeenCalledTimes(1); + return initOnyx() + .then(() => { + AsyncStorageMock.getItem.mockResolvedValue('"mockValue"'); + AsyncStorageMock.getAllKeys.mockResolvedValue([ONYX_KEYS.TEST_KEY]); + + // WHEN multiple components are rendered + render( + <> + + + + + ); + }) + .then(waitForPromisesToResolve) + .then(() => { + // THEN Async storage `getItem` should be called only once + expect(AsyncStorageMock.getAllKeys).toHaveBeenCalledTimes(1); + }); }); - it('Expect multiple calls to getItem when no existing component is using a key', async () => { + it('Expect multiple calls to getItem when no existing component is using a key', () => { const AsyncStorageMock = require('@react-native-community/async-storage/jest/async-storage-mock'); // GIVEN a component connected to Onyx @@ -483,23 +488,27 @@ describe('Onyx', () => { // GIVEN some string value for that key exists in storage AsyncStorageMock.getItem.mockResolvedValue('"mockValue"'); AsyncStorageMock.getAllKeys.mockResolvedValue([ONYX_KEYS.TEST_KEY]); - await initOnyx(); - - // WHEN a component is rendered and unmounted and no longer available - const result = render(); - await waitForPromisesToResolve(); - result.unmount(); - await waitForPromisesToResolve(); - - // THEN When another component using the same storage key is rendered - render(); - - // THEN Async storage `getItem` should be called twice - await waitForPromisesToResolve(); - expect(AsyncStorageMock.getItem).toHaveBeenCalledTimes(2); + let result; + + return initOnyx() + .then(() => { + // WHEN a component is rendered and unmounted and no longer available + result = render(); + }) + .then(waitForPromisesToResolve) + .then(() => result.unmount()) + .then(waitForPromisesToResolve) + + // THEN When another component using the same storage key is rendered + .then(() => render()) + .then(waitForPromisesToResolve) + .then(() => { + // THEN Async storage `getItem` should be called twice + expect(AsyncStorageMock.getItem).toHaveBeenCalledTimes(2); + }); }); - it('Expect multiple calls to getItem when multiple keys are used', async () => { + it('Expect multiple calls to getItem when multiple keys are used', () => { const AsyncStorageMock = require('@react-native-community/async-storage/jest/async-storage-mock'); // GIVEN two component @@ -519,26 +528,28 @@ describe('Onyx', () => { AsyncStorageMock.setItem(ONYX_KEYS.TEST_KEY, JSON.stringify({ID: 15, data: 'mock object with ID'})); AsyncStorageMock.setItem(ONYX_KEYS.ANOTHER_TEST, JSON.stringify('mock text')); AsyncStorageMock.getAllKeys.mockResolvedValue([ONYX_KEYS.TEST_KEY, ONYX_KEYS.ANOTHER_TEST]); - await initOnyx(); - - // WHEN the components are rendered multiple times - render(); - render(); - render(); - render(); - render(); - render(); - - // THEN Async storage `getItem` should be called exactly two times (once for each key) - await waitForPromisesToResolve(); - expect(AsyncStorageMock.getItem).toHaveBeenCalledTimes(2); - expect(AsyncStorageMock.getItem.mock.calls).toEqual([ - [ONYX_KEYS.TEST_KEY], - [ONYX_KEYS.ANOTHER_TEST] - ]); + return initOnyx() + .then(() => { + // WHEN the components are rendered multiple times + render(); + render(); + render(); + render(); + render(); + render(); + }) + .then(waitForPromisesToResolve) + .then(() => { + // THEN Async storage `getItem` should be called exactly two times (once for each key) + expect(AsyncStorageMock.getItem).toHaveBeenCalledTimes(2); + expect(AsyncStorageMock.getItem.mock.calls).toEqual([ + [ONYX_KEYS.TEST_KEY], + [ONYX_KEYS.ANOTHER_TEST] + ]); + }); }); - it('Expect a single call to getItem when at least one component is still subscribed to a key', async () => { + it('Expect a single call to getItem when at least one component is still subscribed to a key', () => { const AsyncStorageMock = require('@react-native-community/async-storage/jest/async-storage-mock'); // GIVEN a component connected to Onyx @@ -551,28 +562,33 @@ describe('Onyx', () => { // GIVEN some string value for that key exists in storage AsyncStorageMock.getItem.mockResolvedValue('"mockValue"'); AsyncStorageMock.getAllKeys.mockResolvedValue([ONYX_KEYS.TEST_KEY]); - await initOnyx(); - - // WHEN multiple components are rendered - render(); - const result2 = render(); - const result3 = render(); - await waitForPromisesToResolve(); - - // WHEN components unmount but at least one remain mounted - result2.unmount(); - result3.unmount(); - await waitForPromisesToResolve(); - - // THEN When another component using the same storage key is rendered - render(); - - // THEN Async storage `getItem` should be called once - await waitForPromisesToResolve(); - expect(AsyncStorageMock.getItem).toHaveBeenCalledTimes(1); + let result2; + let result3; + return initOnyx() + .then(() => { + // WHEN multiple components are rendered + render(); + result2 = render(); + result3 = render(); + }) + .then(waitForPromisesToResolve) + .then(() => { + // WHEN components unmount but at least one remain mounted + result2.unmount(); + result3.unmount(); + }) + .then(waitForPromisesToResolve) + + // THEN When another component using the same storage key is rendered + .then(() => render()) + .then(waitForPromisesToResolve) + .then(() => { + // THEN Async storage `getItem` should be called once + expect(AsyncStorageMock.getItem).toHaveBeenCalledTimes(1); + }); }); - it('Should remove collection items from cache when collection is disconnected', async () => { + it('Should remove collection items from cache when collection is disconnected', () => { const AsyncStorageMock = require('@react-native-community/async-storage/jest/async-storage-mock'); // GIVEN a component subscribing to a collection @@ -587,35 +603,41 @@ describe('Onyx', () => { AsyncStorageMock.setItem(keys[0], JSON.stringify({ID: 15})); AsyncStorageMock.setItem(keys[1], JSON.stringify({ID: 16})); AsyncStorageMock.getAllKeys.mockResolvedValue(keys); - await initOnyx(); - - // WHEN the collection using components render - const result = render(); - const result2 = render(); - await waitForPromisesToResolve(); - - // THEN the collection items should be in cache - expect(cache.hasCacheForKey(keys[0])).toBe(true); - expect(cache.hasCacheForKey(keys[1])).toBe(true); - - // WHEN one of the components unmounts - result.unmount(); - await waitForPromisesToResolve(); - - // THEN the collection items should still be in cache - expect(cache.hasCacheForKey(keys[0])).toBe(true); - expect(cache.hasCacheForKey(keys[1])).toBe(true); - - // WHEN the last component using the collection unmounts - result2.unmount(); - await waitForPromisesToResolve(); - - // THEN the collection items should be removed from cache - expect(cache.hasCacheForKey(keys[0])).toBe(false); - expect(cache.hasCacheForKey(keys[1])).toBe(false); + let result; + let result2; + return initOnyx() + .then(() => { + // WHEN the collection using components render + result = render(); + result2 = render(); + }) + .then(waitForPromisesToResolve) + .then(() => { + // THEN the collection items should be in cache + expect(cache.hasCacheForKey(keys[0])).toBe(true); + expect(cache.hasCacheForKey(keys[1])).toBe(true); + + // WHEN one of the components unmounts + result.unmount(); + return waitForPromisesToResolve(); + }) + .then(() => { + // THEN the collection items should still be in cache + expect(cache.hasCacheForKey(keys[0])).toBe(true); + expect(cache.hasCacheForKey(keys[1])).toBe(true); + + // WHEN the last component using the collection unmounts + result2.unmount(); + return waitForPromisesToResolve(); + }) + .then(() => { + // THEN the collection items should be removed from cache + expect(cache.hasCacheForKey(keys[0])).toBe(false); + expect(cache.hasCacheForKey(keys[1])).toBe(false); + }); }); - it('Should not remove item from cache when it still used in a collection', async () => { + it('Should not remove item from cache when it still used in a collection', () => { const AsyncStorageMock = require('@react-native-community/async-storage/jest/async-storage-mock'); // GIVEN component that uses a collection and a component that uses a collection item @@ -635,19 +657,24 @@ describe('Onyx', () => { // GIVEN some values exist in storage AsyncStorageMock.setItem(COLLECTION_ITEM_KEY, JSON.stringify({ID: 10})); AsyncStorageMock.getAllKeys.mockResolvedValue([COLLECTION_ITEM_KEY]); - await initOnyx(); - - // WHEN both components render - render(); - const result = render(); - await waitForPromisesToResolve(); - - // WHEN the component using the individual item unmounts - result.unmount(); - await waitForPromisesToResolve(); - - // THEN The item should not be removed from cache as it's used in a collection - expect(cache.hasCacheForKey(COLLECTION_ITEM_KEY)).toBe(true); + let result; + + return initOnyx() + .then(() => { + // WHEN both components render + render(); + result = render(); + return waitForPromisesToResolve(); + }) + .then(() => { + // WHEN the component using the individual item unmounts + result.unmount(); + return waitForPromisesToResolve(); + }) + .then(() => { + // THEN The item should not be removed from cache as it's used in a collection + expect(cache.hasCacheForKey(COLLECTION_ITEM_KEY)).toBe(true); + }); }); }); }); From 7b35a24701690e8124bc01708058241538b40016 Mon Sep 17 00:00:00 2001 From: Peter Velkov Date: Tue, 1 Jun 2021 20:46:27 +0300 Subject: [PATCH 36/40] docs: update comment --- lib/Onyx.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/Onyx.js b/lib/Onyx.js index 0efb9c701..d23da7c6b 100644 --- a/lib/Onyx.js +++ b/lib/Onyx.js @@ -72,12 +72,12 @@ function getAllKeys() { const taskName = 'getAllKeys'; - // When a value retrieving task for this key is still running hook to it + // When a value retrieving task for all keys is still running hook to it if (cache.hasPendingTask(taskName)) { return cache.getTaskPromise(taskName); } - // Otherwise retrieve the value from storage and capture a promise to aid concurrent usages + // Otherwise retrieve the keys from storage and capture a promise to aid concurrent usages const promise = AsyncStorage.getAllKeys() .then((keys) => { _.each(keys, key => cache.addKey(key)); From d6f153c475f65ef0185d3108803be6d401e4b3c3 Mon Sep 17 00:00:00 2001 From: Peter Velkov Date: Wed, 2 Jun 2021 12:33:12 +0300 Subject: [PATCH 37/40] docs: Apply suggestions from code review Co-authored-by: Rory Abraham <47436092+roryabraham@users.noreply.github.com> --- lib/Onyx.js | 2 +- lib/OnyxCache.js | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/Onyx.js b/lib/Onyx.js index d23da7c6b..a1a136560 100644 --- a/lib/Onyx.js +++ b/lib/Onyx.js @@ -415,7 +415,7 @@ function cleanCache(key) { return; } - // When this is a collection - also recursively removed any unused individual items + // When this is a collection - also recursively remove any unused individual items if (isCollectionKey(key)) { cache.remove(key); diff --git a/lib/OnyxCache.js b/lib/OnyxCache.js index e4cb434f3..62368f20e 100644 --- a/lib/OnyxCache.js +++ b/lib/OnyxCache.js @@ -108,7 +108,7 @@ class OnyxCache { } /** - * Check whether the give task is already running + * Check whether the given task is already running * @param {string} taskName - unique name given for the task * @returns {*} */ From 27c0ba8ba1dee20171062ff249d03f2696d078a8 Mon Sep 17 00:00:00 2001 From: Peter Velkov Date: Wed, 2 Jun 2021 12:59:55 +0300 Subject: [PATCH 38/40] docs: Remove leftover method description --- lib/OnyxCache.js | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/lib/OnyxCache.js b/lib/OnyxCache.js index 62368f20e..7257dbb0e 100644 --- a/lib/OnyxCache.js +++ b/lib/OnyxCache.js @@ -76,9 +76,7 @@ class OnyxCache { /** * Set's a key value in cache - * Adds the keys to the storage keys list as well - * Providing value is optional. Skipping value would just capture the key so that - * it's available in `getAllKeys` + * Adds the key to the storage keys list as well * @param {string} key * @param {*} value * @returns {*} value - returns the cache value From 5254568c183666c924cada5585ffc943f5351a4e Mon Sep 17 00:00:00 2001 From: Peter Velkov Date: Wed, 2 Jun 2021 22:42:12 +0300 Subject: [PATCH 39/40] Skip creating an arrow function for AsyncStorage.clear Co-authored-by: Rory Abraham <47436092+roryabraham@users.noreply.github.com> --- lib/Onyx.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/Onyx.js b/lib/Onyx.js index a1a136560..d6a2d13a8 100644 --- a/lib/Onyx.js +++ b/lib/Onyx.js @@ -642,7 +642,7 @@ function clear() { cache.remove(key); }); }) - .then(() => AsyncStorage.clear()) + .then(AsyncStorage.clear) .then(initializeWithDefaultKeyStates); } From 77b07e10dfd10831380c197a132a032e812b31f9 Mon Sep 17 00:00:00 2001 From: Peter Velkov Date: Fri, 4 Jun 2021 01:02:51 +0300 Subject: [PATCH 40/40] Explicitly convert set to array --- lib/OnyxCache.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/OnyxCache.js b/lib/OnyxCache.js index 7257dbb0e..841f6bc97 100644 --- a/lib/OnyxCache.js +++ b/lib/OnyxCache.js @@ -44,7 +44,7 @@ class OnyxCache { * @returns {string[]} */ getAllKeys() { - return [...this.storageKeys]; + return Array.from(this.storageKeys); } /**