From bb607e37019896fe1a19b007d0571e39fd5bc871 Mon Sep 17 00:00:00 2001 From: Tim Golen Date: Thu, 21 Sep 2023 05:30:16 +0800 Subject: [PATCH 1/6] Testing dependent keys --- tests/unit/withOnyxTest.js | 20 ++++++++------------ 1 file changed, 8 insertions(+), 12 deletions(-) diff --git a/tests/unit/withOnyxTest.js b/tests/unit/withOnyxTest.js index 1f36cfe0..d4b00894 100644 --- a/tests/unit/withOnyxTest.js +++ b/tests/unit/withOnyxTest.js @@ -250,18 +250,14 @@ describe('withOnyxTest', () => { Onyx.mergeCollection(ONYX_KEYS.COLLECTION.RELATED_KEY, {related_1: 'Test'}); return waitForPromisesToResolve() .then(() => { - const TestComponentWithOnyx = compose( - withOnyx({ - testObject: { - key: `${ONYX_KEYS.COLLECTION.TEST_KEY}${collectionItemID}`, - }, - }), - withOnyx({ - testThing: { - key: ({testObject}) => `${ONYX_KEYS.COLLECTION.RELATED_KEY}${testObject.id}`, - }, - }), - )(ViewWithCollections); + const TestComponentWithOnyx = withOnyx({ + testObject: { + key: `${ONYX_KEYS.COLLECTION.TEST_KEY}${collectionItemID}`, + }, + testThing: { + key: ({testObject}) => `${ONYX_KEYS.COLLECTION.RELATED_KEY}${testObject.id}`, + }, + })(ViewWithCollections); render(); }) .then(() => { From e9c911a53a931a8957fbeb13591d13daa66c98aa Mon Sep 17 00:00:00 2001 From: Tim Golen Date: Thu, 21 Sep 2023 06:34:14 +0800 Subject: [PATCH 2/6] Pass withOnyx state to key functions --- .gitignore | 1 + lib/withOnyx.js | 8 ++++---- tests/unit/withOnyxTest.js | 3 ++- 3 files changed, 7 insertions(+), 5 deletions(-) diff --git a/.gitignore b/.gitignore index 41f9bece..4ee70c35 100644 --- a/.gitignore +++ b/.gitignore @@ -11,6 +11,7 @@ dist/ # IDEs .idea +.vscode # Decrypted private key we do not want to commit .github/OSBotify-private-key.asc diff --git a/lib/withOnyx.js b/lib/withOnyx.js index 97615284..bbd61009 100644 --- a/lib/withOnyx.js +++ b/lib/withOnyx.js @@ -92,12 +92,12 @@ export default function (mapOnyxToState, shouldDelayUpdates = false) { this.checkEvictableKeys(); } - componentDidUpdate(prevProps) { + componentDidUpdate(prevProps, prevState) { // If any of the mappings use data from the props, then when the props change, all the // connections need to be reconnected with the new props _.each(mapOnyxToState, (mapping, propertyName) => { - const previousKey = Str.result(mapping.key, prevProps); - const newKey = Str.result(mapping.key, this.props); + const previousKey = Str.result(mapping.key, {...prevProps, ...prevState}); + const newKey = Str.result(mapping.key, {...this.props, ...this.state}); if (previousKey !== newKey) { Onyx.disconnect(this.activeConnectionIDs[previousKey], previousKey); delete this.activeConnectionIDs[previousKey]; @@ -238,7 +238,7 @@ export default function (mapOnyxToState, shouldDelayUpdates = false) { * component */ connectMappingToOnyx(mapping, statePropertyName) { - const key = Str.result(mapping.key, this.props); + const key = Str.result(mapping.key, {...this.props, ...this.state}); // eslint-disable-next-line rulesdir/prefer-onyx-connect-in-libs this.activeConnectionIDs[key] = Onyx.connect({ diff --git a/tests/unit/withOnyxTest.js b/tests/unit/withOnyxTest.js index d4b00894..1f510ae1 100644 --- a/tests/unit/withOnyxTest.js +++ b/tests/unit/withOnyxTest.js @@ -255,10 +255,11 @@ describe('withOnyxTest', () => { key: `${ONYX_KEYS.COLLECTION.TEST_KEY}${collectionItemID}`, }, testThing: { - key: ({testObject}) => `${ONYX_KEYS.COLLECTION.RELATED_KEY}${testObject.id}`, + key: ({testObject}) => `${ONYX_KEYS.COLLECTION.RELATED_KEY}${(testObject && testObject.id) || 0}`, }, })(ViewWithCollections); render(); + return waitForPromisesToResolve(); }) .then(() => { expect(onRender).toHaveBeenLastCalledWith({ From 6c5a65d3f600fafe47d0db1896cecbb0e26bc4e1 Mon Sep 17 00:00:00 2001 From: Tim Golen Date: Thu, 21 Sep 2023 06:38:38 +0800 Subject: [PATCH 3/6] Remove unused import --- tests/unit/withOnyxTest.js | 1 - 1 file changed, 1 deletion(-) diff --git a/tests/unit/withOnyxTest.js b/tests/unit/withOnyxTest.js index 1f510ae1..f3c76cfd 100644 --- a/tests/unit/withOnyxTest.js +++ b/tests/unit/withOnyxTest.js @@ -5,7 +5,6 @@ import Onyx, {withOnyx} from '../../lib'; import ViewWithText from '../components/ViewWithText'; import ViewWithCollections from '../components/ViewWithCollections'; import waitForPromisesToResolve from '../utils/waitForPromisesToResolve'; -import compose from '../../lib/compose'; import ViewWithObject from '../components/ViewWithObject'; const ONYX_KEYS = { From 0a066647b4081f3992acf8c885d5065db76b015a Mon Sep 17 00:00:00 2001 From: Tim Golen Date: Thu, 21 Sep 2023 07:10:21 +0800 Subject: [PATCH 4/6] Filter out unrelated data before passing state --- lib/withOnyx.js | 18 +++++++++++++----- 1 file changed, 13 insertions(+), 5 deletions(-) diff --git a/lib/withOnyx.js b/lib/withOnyx.js index bbd61009..e9ea7097 100644 --- a/lib/withOnyx.js +++ b/lib/withOnyx.js @@ -93,15 +93,21 @@ export default function (mapOnyxToState, shouldDelayUpdates = false) { } componentDidUpdate(prevProps, prevState) { + // When the state is passed to the key functions with Str.result(), omit anything + // from state that was not part of the mapped keys. + const mappingPropNames = _.pluck(mapOnyxToState, 'key'); + const prevStateOnlyWithOnyxData = _.pick(prevState, mappingPropNames); + const stateOnlyWithOnyxData = _.pick(this.state, mappingPropNames); + // If any of the mappings use data from the props, then when the props change, all the // connections need to be reconnected with the new props - _.each(mapOnyxToState, (mapping, propertyName) => { - const previousKey = Str.result(mapping.key, {...prevProps, ...prevState}); - const newKey = Str.result(mapping.key, {...this.props, ...this.state}); + _.each(mapOnyxToState, (mapping, propName) => { + const previousKey = Str.result(mapping.key, {...prevProps, prevStateOnlyWithOnyxData}); + const newKey = Str.result(mapping.key, {...this.props, stateOnlyWithOnyxData}); if (previousKey !== newKey) { Onyx.disconnect(this.activeConnectionIDs[previousKey], previousKey); delete this.activeConnectionIDs[previousKey]; - this.connectMappingToOnyx(mapping, propertyName); + this.connectMappingToOnyx(mapping, propName); } }); this.checkEvictableKeys(); @@ -238,7 +244,9 @@ export default function (mapOnyxToState, shouldDelayUpdates = false) { * component */ connectMappingToOnyx(mapping, statePropertyName) { - const key = Str.result(mapping.key, {...this.props, ...this.state}); + const mappingPropNames = _.pluck(mapOnyxToState, 'key'); + const stateOnlyWithOnyxData = _.pick(this.state, mappingPropNames); + const key = Str.result(mapping.key, {...this.props, stateOnlyWithOnyxData}); // eslint-disable-next-line rulesdir/prefer-onyx-connect-in-libs this.activeConnectionIDs[key] = Onyx.connect({ From 7865b475e4df4d9cbe0b40f94e13e1fa21ea8cd0 Mon Sep 17 00:00:00 2001 From: Tim Golen Date: Thu, 21 Sep 2023 08:23:15 +0800 Subject: [PATCH 5/6] Add tests for nested dependencies and filter the state before passing --- lib/withOnyx.js | 18 +++++++----- tests/unit/withOnyxTest.js | 59 ++++++++++++++++++++++++++++++++------ 2 files changed, 61 insertions(+), 16 deletions(-) diff --git a/lib/withOnyx.js b/lib/withOnyx.js index e9ea7097..01221b7a 100644 --- a/lib/withOnyx.js +++ b/lib/withOnyx.js @@ -92,18 +92,17 @@ export default function (mapOnyxToState, shouldDelayUpdates = false) { this.checkEvictableKeys(); } - componentDidUpdate(prevProps, prevState) { + componentDidUpdate() { // When the state is passed to the key functions with Str.result(), omit anything // from state that was not part of the mapped keys. - const mappingPropNames = _.pluck(mapOnyxToState, 'key'); - const prevStateOnlyWithOnyxData = _.pick(prevState, mappingPropNames); + const mappingPropNames = _.keys(mapOnyxToState); const stateOnlyWithOnyxData = _.pick(this.state, mappingPropNames); // If any of the mappings use data from the props, then when the props change, all the // connections need to be reconnected with the new props _.each(mapOnyxToState, (mapping, propName) => { - const previousKey = Str.result(mapping.key, {...prevProps, prevStateOnlyWithOnyxData}); - const newKey = Str.result(mapping.key, {...this.props, stateOnlyWithOnyxData}); + const previousKey = mapping.previousKey; + const newKey = Str.result(mapping.key, {...this.props, ...stateOnlyWithOnyxData}); if (previousKey !== newKey) { Onyx.disconnect(this.activeConnectionIDs[previousKey], previousKey); delete this.activeConnectionIDs[previousKey]; @@ -244,9 +243,14 @@ export default function (mapOnyxToState, shouldDelayUpdates = false) { * component */ connectMappingToOnyx(mapping, statePropertyName) { - const mappingPropNames = _.pluck(mapOnyxToState, 'key'); + const mappingPropNames = _.keys(mapOnyxToState); const stateOnlyWithOnyxData = _.pick(this.state, mappingPropNames); - const key = Str.result(mapping.key, {...this.props, stateOnlyWithOnyxData}); + const key = Str.result(mapping.key, {...this.props, ...stateOnlyWithOnyxData}); + + // Remember the previous key so that if it ever changes, the component will reconnect to Onyx + // in componentDidUpdate + // eslint-disable-next-line no-param-reassign + mapOnyxToState[statePropertyName].previousKey = key; // eslint-disable-next-line rulesdir/prefer-onyx-connect-in-libs this.activeConnectionIDs[key] = Onyx.connect({ diff --git a/tests/unit/withOnyxTest.js b/tests/unit/withOnyxTest.js index f3c76cfd..c6ee2140 100644 --- a/tests/unit/withOnyxTest.js +++ b/tests/unit/withOnyxTest.js @@ -11,7 +11,10 @@ const ONYX_KEYS = { TEST_KEY: 'test', COLLECTION: { TEST_KEY: 'test_', - RELATED_KEY: 'related_', + STATIC: 'static_', + DEPENDS_ON_STATIC: 'dependsOnStatic_', + DEPENDS_ON_DEPENDS_ON_STATIC: 'dependsOnDependsOnStatic_', + DEPENDS_ON_DEPENDS_ON_DEPENDS_ON_STATIC: 'dependsOnDependsOnDependsOnStatic_', }, SIMPLE_KEY: 'simple', SIMPLE_KEY_2: 'simple2', @@ -242,27 +245,65 @@ describe('withOnyxTest', () => { }); it('should pass a prop from one connected component to another', () => { - const collectionItemID = 1; const onRender = jest.fn(); const markReadyForHydration = jest.fn(); - Onyx.mergeCollection(ONYX_KEYS.COLLECTION.TEST_KEY, {test_1: {id: 1}}); - Onyx.mergeCollection(ONYX_KEYS.COLLECTION.RELATED_KEY, {related_1: 'Test'}); + + // Given three collections with multiple items in each + Onyx.mergeCollection(ONYX_KEYS.COLLECTION.STATIC, { + static_1: {name: 'Static 1', id: 1}, + static_2: {name: 'Static 2', id: 2}, + }); + + // And one collection will depends on data being loaded from the static collection + Onyx.mergeCollection(ONYX_KEYS.COLLECTION.DEPENDS_ON_STATIC, { + dependsOnStatic_1: {name: 'dependsOnStatic 1', id: 3}, + dependsOnStatic_2: {name: 'dependsOnStatic 2', id: 4}, + }); + + // And one collection will depend on the data being loaded from the collection that depends on the static collection (multiple nested dependencies) + Onyx.mergeCollection(ONYX_KEYS.COLLECTION.DEPENDS_ON_DEPENDS_ON_STATIC, { + dependsOnDependsOnStatic_3: {name: 'dependsOnDependsOnStatic 1', id: 5}, + dependsOnDependsOnStatic_4: {name: 'dependsOnDependsOnStatic 2', id: 6}, + }); + + // And another collection with one more layer of dependency just to prove it works + Onyx.mergeCollection(ONYX_KEYS.COLLECTION.DEPENDS_ON_DEPENDS_ON_DEPENDS_ON_STATIC, { + dependsOnDependsOnDependsOnStatic_5: {name: 'dependsOnDependsOnDependsOnStatic 1'}, + dependsOnDependsOnDependsOnStatic_6: {name: 'dependsOnDependsOnDependsOnStatic 2'}, + }); + + // When a component is rendered using withOnyx and several nested dependencies on the keys return waitForPromisesToResolve() .then(() => { const TestComponentWithOnyx = withOnyx({ - testObject: { - key: `${ONYX_KEYS.COLLECTION.TEST_KEY}${collectionItemID}`, + staticObject: { + key: `${ONYX_KEYS.COLLECTION.STATIC}1`, }, - testThing: { - key: ({testObject}) => `${ONYX_KEYS.COLLECTION.RELATED_KEY}${(testObject && testObject.id) || 0}`, + dependentObject: { + key: ({staticObject}) => `${ONYX_KEYS.COLLECTION.DEPENDS_ON_STATIC}${(staticObject && staticObject.id) || 0}`, + }, + multiDependentObject: { + key: ({dependentObject}) => `${ONYX_KEYS.COLLECTION.DEPENDS_ON_DEPENDS_ON_STATIC}${(dependentObject && dependentObject.id) || 0}`, + }, + extremeMultiDependentObject: { + key: ({multiDependentObject}) => `${ONYX_KEYS.COLLECTION.DEPENDS_ON_DEPENDS_ON_DEPENDS_ON_STATIC}${(multiDependentObject && multiDependentObject.id) || 0}`, }, })(ViewWithCollections); render(); return waitForPromisesToResolve(); }) + + // Then all of the data gets properly loaded into the component as expected with the nested dependencies resolved .then(() => { expect(onRender).toHaveBeenLastCalledWith({ - collections: {}, markReadyForHydration, onRender, testObject: {id: 1}, testThing: 'Test', + markReadyForHydration, + onRender, + collections: {}, + testObject: {isDefaultProp: true}, + staticObject: {name: 'Static 1', id: 1}, + dependentObject: {name: 'dependsOnStatic 1', id: 3}, + multiDependentObject: {name: 'dependsOnDependsOnStatic 1', id: 5}, + extremeMultiDependentObject: {name: 'dependsOnDependsOnDependsOnStatic 1'}, }); }); }); From ac7e983bac934c0c7d49fc505422471fdd148667 Mon Sep 17 00:00:00 2001 From: Tim Golen Date: Thu, 21 Sep 2023 08:35:47 +0800 Subject: [PATCH 6/6] DRY up method --- lib/withOnyx.js | 18 ++++++++++++------ 1 file changed, 12 insertions(+), 6 deletions(-) diff --git a/lib/withOnyx.js b/lib/withOnyx.js index 01221b7a..58e2a87e 100644 --- a/lib/withOnyx.js +++ b/lib/withOnyx.js @@ -20,6 +20,15 @@ function getDisplayName(component) { return component.displayName || component.name || 'Component'; } +/** + * Removes all the keys from state that are unrelated to the onyx data being mapped to the component. + * + * @param {Object} state of the component + * @param {Object} onyxToStateMapping the object holding all of the mapping configuration for the component + * @returns {Object} + */ +const getOnyxDataFromState = (state, onyxToStateMapping) => _.pick(state, _.keys(onyxToStateMapping)); + export default function (mapOnyxToState, shouldDelayUpdates = false) { // A list of keys that must be present in tempState before we can render the WrappedComponent const requiredKeysForInit = _.chain(mapOnyxToState) @@ -95,14 +104,13 @@ export default function (mapOnyxToState, shouldDelayUpdates = false) { componentDidUpdate() { // When the state is passed to the key functions with Str.result(), omit anything // from state that was not part of the mapped keys. - const mappingPropNames = _.keys(mapOnyxToState); - const stateOnlyWithOnyxData = _.pick(this.state, mappingPropNames); + const onyxDataFromState = getOnyxDataFromState(this.state, mapOnyxToState); // If any of the mappings use data from the props, then when the props change, all the // connections need to be reconnected with the new props _.each(mapOnyxToState, (mapping, propName) => { const previousKey = mapping.previousKey; - const newKey = Str.result(mapping.key, {...this.props, ...stateOnlyWithOnyxData}); + const newKey = Str.result(mapping.key, {...this.props, ...onyxDataFromState}); if (previousKey !== newKey) { Onyx.disconnect(this.activeConnectionIDs[previousKey], previousKey); delete this.activeConnectionIDs[previousKey]; @@ -243,9 +251,7 @@ export default function (mapOnyxToState, shouldDelayUpdates = false) { * component */ connectMappingToOnyx(mapping, statePropertyName) { - const mappingPropNames = _.keys(mapOnyxToState); - const stateOnlyWithOnyxData = _.pick(this.state, mappingPropNames); - const key = Str.result(mapping.key, {...this.props, ...stateOnlyWithOnyxData}); + const key = Str.result(mapping.key, {...this.props, ...getOnyxDataFromState(this.state, mapOnyxToState)}); // Remember the previous key so that if it ever changes, the component will reconnect to Onyx // in componentDidUpdate