diff --git a/.github/PULL_REQUEST_TEMPLATE.md b/.github/PULL_REQUEST_TEMPLATE.md index 8ded900b..3ccce8a9 100644 --- a/.github/PULL_REQUEST_TEMPLATE.md +++ b/.github/PULL_REQUEST_TEMPLATE.md @@ -1,10 +1,10 @@ - + ### Details - + ### Related Issues - + GH_LINK ### Automated Tests diff --git a/lib/Onyx.js b/lib/Onyx.js index 77fd5280..71835de5 100644 --- a/lib/Onyx.js +++ b/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, + })); } }); } diff --git a/tests/components/ViewWithCollections.js b/tests/components/ViewWithCollections.js index d75a79dc..fb7f2e72 100644 --- a/tests/components/ViewWithCollections.js +++ b/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 ( diff --git a/tests/unit/withOnyxTest.js b/tests/unit/withOnyxTest.js index 26b58cbb..44078647 100644 --- a/tests/unit/withOnyxTest.js +++ b/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(); + + const TestComponentWithOnyx2 = compose( + withOnyx({ + testObject: { + key: `${ONYX_KEYS.COLLECTION.TEST_KEY}2`, + }, + }), + )(ViewWithCollections); + render(); + + const TestComponentWithOnyx3 = compose( + withOnyx({ + testObject: { + key: `${ONYX_KEYS.COLLECTION.TEST_KEY}3`, + }, + }), + )(ViewWithCollections); + render(); + + 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(); + + 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}); + }); + }); });