Merge pull request #401 from janicduplessis/@janic/cache-fixes

Fix cache for missing keys and empty collections

lib/Onyx.js

@@ -235,8 +235,14 @@ function tryGetCachedValue(key, mapping = {}) {
     let val = cache.getValue(key);
 
     if (isCollectionKey(key)) {
-        const allKeys = cache.getAllKeys();
-        const matchingKeys = _.filter(allKeys, k => k.startsWith(key));
+        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 (allCacheKeys.length === 0) {
+            return;
+        }
+        const matchingKeys = _.filter(allCacheKeys, k => k.startsWith(key));
         const values = _.reduce(matchingKeys, (finalObject, matchedKey) => {
             const cachedValue = cache.getValue(matchedKey);
             if (cachedValue) {
@@ -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);

tests/components/ViewWithCollections.js

@@ -29,6 +29,10 @@ const ViewWithCollections = React.forwardRef((props, ref) => {
 
     props.onRender(props);
 
+    if (_.size(props.collections) === 0) {
+        return <Text>empty</Text>;
+    }
+
     return (
         <View>
             {_.map(props.collections, (collection, i) => (

tests/components/ViewWithText.js

@@ -4,20 +4,21 @@ 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) {
     props.onRender();
 
     return (
         <View>
-            <Text testID="text-element">{props.text}</Text>
+            <Text testID="text-element">{props.text || 'null'}</Text>
         </View>
     );
 }

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,112 @@ 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(<TestComponentWithOnyx onRender={onRender} />);
+
+                // 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(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.
+                // Do not use `waitForPromisesToResolve` before making the assertions and make sure
+                // onRender was called.
+                renderResult.unmount();
+                renderResult = render(<TestComponentWithOnyx onRender={onRender} />);
+
+                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(<TestComponentWithOnyx onRender={onRender} />);
+
+        // 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');
+            expect(textComponent).not.toBeNull();
+            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(<TestComponentWithOnyx onRender={onRender} />);
+            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;
+
+        // 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(<TestComponentWithOnyx onRender={onRender} />);
+
+                // 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('empty');
+                expect(textComponent).not.toBeNull();
+                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(<TestComponentWithOnyx onRender={onRender} />);
+                textComponent = renderResult.getByText('empty');
+                expect(textComponent).not.toBeNull();
+                expect(onRender).toHaveBeenCalledTimes(2);
+            });
+    });
 });