Fix mergeCollection bug

.github/PULL_REQUEST_TEMPLATE.md

@@ -1,10 +1,10 @@
-<If necessary, assign reviewers that know the area or changes well. Feel free to tag any additional reviewers you see fit.>
+<!-- If necessary, assign reviewers that know the area or changes well. Feel free to tag any additional reviewers you see fit. -->
 
 ### Details
-<Explanation of the change or anything fishy that is going on>
+<!-- Explanation of the change or anything fishy that is going on -->
 
 ### Related Issues
-<Please replace GH_LINK with the link to the GitHub issue this Pull Request is related to>
+<!-- Please replace GH_LINK with the link to the GitHub issue this Pull Request is related to -->
 GH_LINK
 
 ### Automated Tests

lib/Onyx.js

@@ -189,15 +189,22 @@ function keysChanged(collectionKey, collection) {
                 });
             }
         } else if (isSubscribedToCollectionMemberKey) {
-            subscriber.withOnyxInstance.setState((prevState) => {
-                let newData = collection[subscriber.key];
-                if (_.isObject(newData)) {
-                    newData = lodashMerge(newData, prevState[subscriber.statePropertyName]);
-                }
-                return {
-                    [subscriber.statePropertyName]: newData,
-                };
-            });
+            const dataFromCollection = collection[subscriber.key];
+
+            // If `dataFromCollection` happens to not exist, then return early so that there are no unnecessary
+            // re-renderings of the component
+            if (_.isUndefined(dataFromCollection)) {
+                return;
+            }
+
+            subscriber.withOnyxInstance.setState(prevState => ({
+                [subscriber.statePropertyName]: _.isObject(dataFromCollection)
+                    ? {
+                        ...prevState[subscriber.statePropertyName],
+                        ...dataFromCollection,
+                    }
+                    : dataFromCollection,
+            }));
         }
     });
 }

tests/components/ViewWithCollections.js

@@ -7,16 +7,20 @@ const propTypes = {
     collections: PropTypes.objectOf(PropTypes.shape({
         ID: PropTypes.number,
     })),
+    testObject: PropTypes.shape({
+        ID: PropTypes.number,
+    }),
     onRender: PropTypes.func,
 };
 
 const defaultProps = {
     collections: {},
+    testObject: {isDefaultProp: true},
     onRender: () => {},
 };
 
 const ViewWithCollections = (props) => {
-    props.onRender();
+    props.onRender(props);
 
     return (
         <View>

tests/unit/withOnyxTest.js

@@ -42,9 +42,7 @@ describe('withOnyx', () => {
                 expect(result).toHaveBeenCalledTimes(999);
             });
     });
-});
 
-describe('withOnyx', () => {
     it('should update withOnyx subscriber multiple times when merge is used', () => {
         const TestComponentWithOnyx = withOnyx({
             text: {
@@ -62,9 +60,7 @@ describe('withOnyx', () => {
                 expect(onRender.mock.calls.length).toBe(4);
             });
     });
-});
 
-describe('withOnyx', () => {
     it('should update withOnyx subscriber just once when mergeCollection is used', () => {
         const TestComponentWithOnyx = withOnyx({
             text: {
@@ -80,9 +76,7 @@ describe('withOnyx', () => {
                 expect(onRender.mock.calls.length).toBe(2);
             });
     });
-});
 
-describe('withOnyx', () => {
     it('should update withOnyx subscribing to individual key if mergeCollection is used', () => {
         const collectionItemID = 1;
         const TestComponentWithOnyx = withOnyx({
@@ -99,9 +93,7 @@ describe('withOnyx', () => {
                 expect(onRender.mock.calls.length).toBe(2);
             });
     });
-});
 
-describe('withOnyx', () => {
     it('should update withOnyx subscribing to individual key with merged value if mergeCollection is used', () => {
         const collectionItemID = 4;
         const TestComponentWithOnyx = withOnyx({
@@ -123,9 +115,7 @@ describe('withOnyx', () => {
                 expect(onRender.mock.instances[2].text).toEqual({ID: 456, Name: 'Test4'});
             });
     });
-});
 
-describe('withOnyx', () => {
     it('should pass a prop from one connected component to another', () => {
         const collectionItemID = 1;
         const onRender = jest.fn();
@@ -152,4 +142,111 @@ describe('withOnyx', () => {
                 expect(onRender.mock.instances[0].testThing).toBe('Test');
             });
     });
+
+    it('using mergeCollection to modify one item should only effect one component', () => {
+        const onRender1 = jest.fn();
+        const onRender2 = jest.fn();
+        const onRender3 = jest.fn();
+
+        // GIVEN there is a collection with three simple items in it
+        Onyx.mergeCollection(ONYX_KEYS.COLLECTION.TEST_KEY, {
+            test_1: {ID: 1},
+            test_2: {ID: 2},
+            test_3: {ID: 3},
+        });
+
+        return waitForPromisesToResolve()
+            .then(() => {
+                // WHEN three components subscribe to each of the items in that collection
+                const TestComponentWithOnyx1 = compose(
+                    withOnyx({
+                        testObject: {
+                            key: `${ONYX_KEYS.COLLECTION.TEST_KEY}1`,
+                        },
+                    }),
+                )(ViewWithCollections);
+                render(<TestComponentWithOnyx1 onRender={onRender1} />);
+
+                const TestComponentWithOnyx2 = compose(
+                    withOnyx({
+                        testObject: {
+                            key: `${ONYX_KEYS.COLLECTION.TEST_KEY}2`,
+                        },
+                    }),
+                )(ViewWithCollections);
+                render(<TestComponentWithOnyx2 onRender={onRender2} />);
+
+                const TestComponentWithOnyx3 = compose(
+                    withOnyx({
+                        testObject: {
+                            key: `${ONYX_KEYS.COLLECTION.TEST_KEY}3`,
+                        },
+                    }),
+                )(ViewWithCollections);
+                render(<TestComponentWithOnyx3 onRender={onRender3} />);
+
+                return waitForPromisesToResolve();
+            })
+            .then(() => {
+                // WHEN a single item in the collection is updated with mergeCollect()
+                Onyx.mergeCollection(ONYX_KEYS.COLLECTION.TEST_KEY, {
+                    test_1: {ID: 1, newProperty: 'yay'},
+                });
+                return waitForPromisesToResolve();
+            })
+            .then(() => {
+                // THEN the component subscribed to the modified item should have the new version of the item
+                // and all other components should be unchanged.
+                // Note: each component is rendered twice. Once when it is initially rendered, and then again
+                // when the collection is updated. That's why there are two checks here for each component.
+                expect(onRender1).toHaveBeenCalledTimes(2);
+                expect(onRender1.mock.calls[0][0].testObject).toStrictEqual({ID: 1});
+                expect(onRender1.mock.calls[1][0].testObject).toStrictEqual({ID: 1, newProperty: 'yay'});
+
+                expect(onRender2).toHaveBeenCalledTimes(1);
+                expect(onRender2.mock.calls[0][0].testObject).toStrictEqual({ID: 2});
+
+                expect(onRender3).toHaveBeenCalledTimes(1);
+                expect(onRender3.mock.calls[0][0].testObject).toStrictEqual({ID: 3});
+            });
+    });
+
+    it('mergeCollection should merge previous props correctly to the new state', () => {
+        const onRender = jest.fn();
+
+        // GIVEN there is a collection with a simple item in it that has a `number` property set to 1
+        Onyx.mergeCollection(ONYX_KEYS.COLLECTION.TEST_KEY, {
+            test_1: {ID: 1, number: 1},
+        });
+
+        return waitForPromisesToResolve()
+            .then(() => {
+                // WHEN a component subscribes to the one item in that collection
+                const TestComponentWithOnyx = compose(
+                    withOnyx({
+                        testObject: {
+                            key: `${ONYX_KEYS.COLLECTION.TEST_KEY}1`,
+                        },
+                    }),
+                )(ViewWithCollections);
+                render(<TestComponentWithOnyx onRender={onRender} />);
+
+                return waitForPromisesToResolve();
+            })
+            .then(() => {
+                // WHEN the `number` property is updated using mergeCollection to be 2
+                Onyx.mergeCollection(ONYX_KEYS.COLLECTION.TEST_KEY, {
+                    test_1: {number: 2},
+                });
+                return waitForPromisesToResolve();
+            })
+            .then(() => {
+                // THEN the component subscribed to the modified item should be rendered twice.
+                // The first time it will render with number === 1
+                // The second time it will render with number === 2
+                expect(onRender).toHaveBeenCalledTimes(2);
+                expect(onRender.mock.calls[0][0].testObject).toStrictEqual({ID: 1, number: 1});
+                expect(onRender.mock.calls[1][0].testObject).toStrictEqual({ID: 1, number: 2});
+            });
+    });
 });