From ed3b96b2a076c4bf0e01d0befbff8a90c25ad331 Mon Sep 17 00:00:00 2001 From: Janic Duplessis Date: Thu, 12 Oct 2023 21:51:35 -0400 Subject: [PATCH 1/3] Fix cache for missing keys and empty collections --- lib/Onyx.js | 14 +++- tests/components/ViewWithCollections.js | 4 ++ tests/components/ViewWithText.js | 5 +- tests/unit/withOnyxTest.js | 95 +++++++++++++++++++++++++ 4 files changed, 113 insertions(+), 5 deletions(-) diff --git a/lib/Onyx.js b/lib/Onyx.js index a940afc9..72e10eec 100644 --- a/lib/Onyx.js +++ b/lib/Onyx.js @@ -236,6 +236,12 @@ function tryGetCachedValue(key, mapping = {}) { if (isCollectionKey(key)) { const allKeys = cache.getAllKeys(); + + // It is possible we haven't loaded all keys yet so we do not know if the + // collection actually exists. + if (allKeys.length === 0) { + return; + } const matchingKeys = _.filter(allKeys, k => k.startsWith(key)); const values = _.reduce(matchingKeys, (finalObject, matchedKey) => { const cachedValue = cache.getValue(matchedKey); @@ -246,9 +252,7 @@ function tryGetCachedValue(key, mapping = {}) { } return finalObject; }, {}); - if (_.isEmpty(values)) { - return; - } + val = values; } @@ -826,6 +830,10 @@ function connect(mapping) { // since there are none matched. In withOnyx() we wait for all connected keys to return a value before rendering the child // component. This null value will be filtered out so that the connected component can utilize defaultProps. if (matchingKeys.length === 0) { + if (mapping.key && !isCollectionKey(mapping.key)) { + cache.set(mapping.key, null); + } + // Here we cannot use batching because the null value is expected to be set immediately for default props // or they will be undefined. sendDataToConnection(mapping, null, undefined, false); diff --git a/tests/components/ViewWithCollections.js b/tests/components/ViewWithCollections.js index ecf380ef..aa05d0b9 100644 --- a/tests/components/ViewWithCollections.js +++ b/tests/components/ViewWithCollections.js @@ -29,6 +29,10 @@ const ViewWithCollections = React.forwardRef((props, ref) => { props.onRender(props); + if (_.size(props.collections) === 0) { + return empty; + } + return ( {_.map(props.collections, (collection, i) => ( diff --git a/tests/components/ViewWithText.js b/tests/components/ViewWithText.js index e172f295..0a872077 100644 --- a/tests/components/ViewWithText.js +++ b/tests/components/ViewWithText.js @@ -4,12 +4,13 @@ import PropTypes from 'prop-types'; import {View, Text} from 'react-native'; const propTypes = { - text: PropTypes.string.isRequired, + text: PropTypes.string, onRender: PropTypes.func, }; const defaultProps = { onRender: () => {}, + text: null, }; function ViewWithText(props) { @@ -17,7 +18,7 @@ function ViewWithText(props) { return ( - {props.text} + {props.text || 'null'} ); } diff --git a/tests/unit/withOnyxTest.js b/tests/unit/withOnyxTest.js index 82fc544b..cbb39c4c 100644 --- a/tests/unit/withOnyxTest.js +++ b/tests/unit/withOnyxTest.js @@ -6,6 +6,7 @@ import ViewWithText from '../components/ViewWithText'; import ViewWithCollections from '../components/ViewWithCollections'; import waitForPromisesToResolve from '../utils/waitForPromisesToResolve'; import ViewWithObject from '../components/ViewWithObject'; +import StorageMock from '../../lib/storage'; const ONYX_KEYS = { TEST_KEY: 'test', @@ -522,4 +523,98 @@ describe('withOnyxTest', () => { expect(onRender.mock.calls[3][0].simple).toBe('long_string'); }); }); + + it('should render immediately when a onyx component is mounted a 2nd time', () => { + const TestComponentWithOnyx = withOnyx({ + text: { + key: ONYX_KEYS.TEST_KEY, + }, + })(ViewWithText); + const onRender = jest.fn(); + let renderResult; + + // Set the value in storage, but not in cache. + return StorageMock.setItem(ONYX_KEYS.TEST_KEY, 'test1') + .then(() => { + renderResult = render(); + + // The component should not render initially since we have no data in cache. + expect(onRender).not.toHaveBeenCalled(); + return waitForPromisesToResolve(); + }) + .then(waitForPromisesToResolve) + .then(() => { + let textComponent = renderResult.getByText('test1'); + expect(textComponent).not.toBeNull(); + expect(onRender).toHaveBeenCalledTimes(1); + + // Unmount the component and mount it again. It should now render immediately. + renderResult.unmount(); + renderResult = render(); + + textComponent = renderResult.getByText('test1'); + expect(textComponent).not.toBeNull(); + expect(onRender).toHaveBeenCalledTimes(2); + }); + }); + + it('should cache missing keys', () => { + const TestComponentWithOnyx = withOnyx({ + text: { + key: ONYX_KEYS.TEST_KEY, + }, + })(ViewWithText); + const onRender = jest.fn(); + + // Render with a key that doesn't exist in cache or storage. + let renderResult = render(); + + // The component should not render initially since we have no data in cache. + expect(onRender).not.toHaveBeenCalled(); + return waitForPromisesToResolve().then(() => { + let textComponent = renderResult.getByText('null'); + expect(textComponent).not.toBeNull(); + expect(onRender).toHaveBeenCalledTimes(1); + + // Unmount the component and mount it again. It should now render immediately. + renderResult.unmount(); + renderResult = render(); + textComponent = renderResult.getByText('null'); + expect(textComponent).not.toBeNull(); + expect(onRender).toHaveBeenCalledTimes(2); + }); + }); + + it('should cache empty collections', () => { + const TestComponentWithOnyx = withOnyx({ + text: { + key: ONYX_KEYS.COLLECTION.TEST_KEY, + }, + })(ViewWithCollections); + const onRender = jest.fn(); + let renderResult; + + return StorageMock.setItem(ONYX_KEYS.SIMPLE_KEY, 'simple') + .then(() => { + // Render with a collection that doesn't exist in cache or storage. + renderResult = render(); + + // The component should not render initially since we have no data in cache. + expect(onRender).not.toHaveBeenCalled(); + + return waitForPromisesToResolve(); + }) + .then(() => { + let textComponent = renderResult.getByText('empty'); + expect(textComponent).not.toBeNull(); + expect(onRender).toHaveBeenCalledTimes(1); + + // Unmount the component and mount it again. It should now render immediately. + renderResult.unmount(); + renderResult = render(); + textComponent = renderResult.getByText('empty'); + expect(textComponent).not.toBeNull(); + expect(onRender).toHaveBeenCalledTimes(2); + }); + }); }); From adf230c847b5341d7b9b7e4acb84473335316265 Mon Sep 17 00:00:00 2001 From: Janic Duplessis Date: Tue, 17 Oct 2023 00:21:42 -0400 Subject: [PATCH 2/3] Clarify some comments --- tests/unit/withOnyxTest.js | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/tests/unit/withOnyxTest.js b/tests/unit/withOnyxTest.js index cbb39c4c..56207dbd 100644 --- a/tests/unit/withOnyxTest.js +++ b/tests/unit/withOnyxTest.js @@ -539,6 +539,8 @@ describe('withOnyxTest', () => { renderResult = render(); // The component should not render initially since we have no data in cache. + // Use `waitForPromisesToResolve` before making the assertions and make sure + // onRender was not called. expect(onRender).not.toHaveBeenCalled(); return waitForPromisesToResolve(); }) @@ -549,6 +551,8 @@ describe('withOnyxTest', () => { expect(onRender).toHaveBeenCalledTimes(1); // Unmount the component and mount it again. It should now render immediately. + // Do not use `waitForPromisesToResolve` before making the assertions and make sure + // onRender was called. renderResult.unmount(); renderResult = render(); @@ -570,6 +574,8 @@ describe('withOnyxTest', () => { let renderResult = render(); // The component should not render initially since we have no data in cache. + // Use `waitForPromisesToResolve` before making the assertions and make sure + // onRender was not called. expect(onRender).not.toHaveBeenCalled(); return waitForPromisesToResolve().then(() => { let textComponent = renderResult.getByText('null'); @@ -577,6 +583,8 @@ describe('withOnyxTest', () => { expect(onRender).toHaveBeenCalledTimes(1); // Unmount the component and mount it again. It should now render immediately. + // Do not use `waitForPromisesToResolve` before making the assertions and make sure + // onRender was called. renderResult.unmount(); renderResult = render(); textComponent = renderResult.getByText('null'); @@ -594,12 +602,16 @@ describe('withOnyxTest', () => { const onRender = jest.fn(); let renderResult; + // Set a random item in storage since Onyx will only think keys are loaded + // in cache if there are at least one key. return StorageMock.setItem(ONYX_KEYS.SIMPLE_KEY, 'simple') .then(() => { // Render with a collection that doesn't exist in cache or storage. renderResult = render(); // The component should not render initially since we have no data in cache. + // Use `waitForPromisesToResolve` before making the assertions and make sure + // onRender was not called. expect(onRender).not.toHaveBeenCalled(); return waitForPromisesToResolve(); @@ -610,6 +622,8 @@ describe('withOnyxTest', () => { expect(onRender).toHaveBeenCalledTimes(1); // Unmount the component and mount it again. It should now render immediately. + // Do not use `waitForPromisesToResolve` before making the assertions and make sure + // onRender was called. renderResult.unmount(); renderResult = render(); textComponent = renderResult.getByText('empty'); From c12202edab4631ebc005ed99ad06091f981ac538 Mon Sep 17 00:00:00 2001 From: Janic Duplessis Date: Tue, 17 Oct 2023 19:35:08 -0400 Subject: [PATCH 3/3] Update Onyx.js --- lib/Onyx.js | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/lib/Onyx.js b/lib/Onyx.js index 72e10eec..7bb3f4e7 100644 --- a/lib/Onyx.js +++ b/lib/Onyx.js @@ -235,14 +235,14 @@ function tryGetCachedValue(key, mapping = {}) { let val = cache.getValue(key); if (isCollectionKey(key)) { - const allKeys = cache.getAllKeys(); + const allCacheKeys = cache.getAllKeys(); // It is possible we haven't loaded all keys yet so we do not know if the // collection actually exists. - if (allKeys.length === 0) { + if (allCacheKeys.length === 0) { return; } - const matchingKeys = _.filter(allKeys, k => k.startsWith(key)); + const matchingKeys = _.filter(allCacheKeys, k => k.startsWith(key)); const values = _.reduce(matchingKeys, (finalObject, matchedKey) => { const cachedValue = cache.getValue(matchedKey); if (cachedValue) {