From 65888e2760335cb3ce683bccc0257339b36c4079 Mon Sep 17 00:00:00 2001 From: Marc Glasser Date: Thu, 12 Aug 2021 06:49:24 -1000 Subject: [PATCH 1/6] Call setState once when handling initial withOnyx data --- lib/Onyx.js | 4 +--- lib/withOnyx.js | 58 +++++++++++++++++++++++++++++++------------------ 2 files changed, 38 insertions(+), 24 deletions(-) diff --git a/lib/Onyx.js b/lib/Onyx.js index f992b57b..014e1565 100644 --- a/lib/Onyx.js +++ b/lib/Onyx.js @@ -328,9 +328,7 @@ function keyChanged(key, data, hasNewValue = true) { */ function sendDataToConnection(config, val, key) { if (config.withOnyxInstance) { - config.withOnyxInstance.setState({ - [config.statePropertyName]: val, - }); + config.withOnyxInstance.setInitialState(config.statePropertyName, val); } else if (_.isFunction(config.callback)) { config.callback(val, key); } diff --git a/lib/withOnyx.js b/lib/withOnyx.js index 7641d75c..93a89d07 100644 --- a/lib/withOnyx.js +++ b/lib/withOnyx.js @@ -25,10 +25,14 @@ export default function (mapOnyxToState) { constructor(props) { super(props); + this.setInitialData = this.setInitialData.bind(this); + // This stores all the Onyx connection IDs to be used when the component unmounts so everything can be // disconnected. It is a key value store with the format {[mapping.key]: connectionID}. this.activeConnectionIDs = {}; + this.tempState = {}; + this.state = { loading: true, }; @@ -39,7 +43,7 @@ export default function (mapOnyxToState) { _.each(mapOnyxToState, (mapping, propertyName) => { this.connectMappingToOnyx(mapping, propertyName); }); - this.checkAndUpdateLoading(); + this.checkEvictableKeys(); } componentDidUpdate(prevProps) { @@ -55,7 +59,7 @@ export default function (mapOnyxToState) { this.connectMappingToOnyx(mapping, propertyName); } }); - this.checkAndUpdateLoading(); + this.checkEvictableKeys(); } componentWillUnmount() { @@ -67,12 +71,42 @@ export default function (mapOnyxToState) { }); } + /** + * This method is used externally by sendDataToConnection to prevent unnecessary renders while a component + * still in a loading state. The temporary initial state is saved to the component instance and setState() + * only called once all the necessary data has been collected. + * + * @param {String} statePropertyName + * @param {*} val + */ + setInitialState(statePropertyName, val) { + this.tempState[statePropertyName] = val; + + // Filter all keys by those which we do want to init with stored values + // since keys that are configured to not init with stored values will + // never appear on state when the component mounts - only after they update + // organically. + const requiredKeysForInit = _.chain(mapOnyxToState) + .omit(config => config.initWithStoredValues === false) + .keys() + .value(); + + // All state keys should exist and at least have a value of null + if (!_.every(requiredKeysForInit, key => !_.isUndefined(this.tempState[key]))) { + return; + } + + this.setState({...this.tempState, loading: false}, () => { + this.tempState = {}; + }); + } + /** * Makes sure each Onyx key we requested has been set to state with a value of some kind. * We are doing this so that the wrapped component will only render when all the data * it needs is available to it. */ - checkAndUpdateLoading() { + checkEvictableKeys() { // We will add this key to our list of recently accessed keys // if the canEvict function returns true. This is necessary criteria // we MUST use to specify if a key can be removed or not. @@ -95,24 +129,6 @@ export default function (mapOnyxToState) { Onyx.addToEvictionBlockList(key, mapping.connectionID); } }); - - if (!this.state.loading) { - return; - } - - // Filter all keys by those which we do want to init with stored values - // since keys that are configured to not init with stored values will - // never appear on state when the component mounts - only after they update - // organically. - const requiredKeysForInit = _.chain(mapOnyxToState) - .omit(config => config.initWithStoredValues === false) - .keys() - .value(); - - // All state keys should exist and at least have a value of null - if (_.every(requiredKeysForInit, key => !_.isUndefined(this.state[key]))) { - this.setState({loading: false}); - } } /** From 6ba1be3314e8a73398f94c2ea3db68ba1f96d412 Mon Sep 17 00:00:00 2001 From: Marc Glasser Date: Thu, 12 Aug 2021 06:50:14 -1000 Subject: [PATCH 2/6] update name --- lib/withOnyx.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/withOnyx.js b/lib/withOnyx.js index 93a89d07..4ff1716e 100644 --- a/lib/withOnyx.js +++ b/lib/withOnyx.js @@ -25,7 +25,7 @@ export default function (mapOnyxToState) { constructor(props) { super(props); - this.setInitialData = this.setInitialData.bind(this); + this.setInitialState = this.setInitialState.bind(this); // This stores all the Onyx connection IDs to be used when the component unmounts so everything can be // disconnected. It is a key value store with the format {[mapping.key]: connectionID}. From 1fed3632290d21d39a90a6b4a8ed453c899023e8 Mon Sep 17 00:00:00 2001 From: Marc Glasser Date: Thu, 12 Aug 2021 07:06:02 -1000 Subject: [PATCH 3/6] Fix failing tests --- tests/unit/withOnyxTest.js | 37 ++++++++++++++++++++++++------------- 1 file changed, 24 insertions(+), 13 deletions(-) diff --git a/tests/unit/withOnyxTest.js b/tests/unit/withOnyxTest.js index 6053bac4..9217f39f 100644 --- a/tests/unit/withOnyxTest.js +++ b/tests/unit/withOnyxTest.js @@ -68,9 +68,13 @@ describe('withOnyx', () => { })(ViewWithCollections); const onRender = jest.fn(); render(); - - Onyx.mergeCollection(ONYX_KEYS.COLLECTION.TEST_KEY, {test_1: {ID: 123}, test_2: {ID: 234}, test_3: {ID: 345}}); - return waitForPromisesToResolve() + waitForPromisesToResolve() + .then(() => { + Onyx.mergeCollection(ONYX_KEYS.COLLECTION.TEST_KEY, { + test_1: {ID: 123}, test_2: {ID: 234}, test_3: {ID: 345}, + }); + return waitForPromisesToResolve(); + }) .then(() => { expect(onRender.mock.calls.length).toBe(2); }); @@ -85,9 +89,13 @@ describe('withOnyx', () => { })(ViewWithCollections); const onRender = jest.fn(); render(); - - Onyx.mergeCollection(ONYX_KEYS.COLLECTION.TEST_KEY, {test_1: {ID: 123}, test_2: {ID: 234}, test_3: {ID: 345}}); - return waitForPromisesToResolve() + waitForPromisesToResolve() + .then(() => { + Onyx.mergeCollection(ONYX_KEYS.COLLECTION.TEST_KEY, { + test_1: {ID: 123}, test_2: {ID: 234}, test_3: {ID: 345}, + }); + return waitForPromisesToResolve(); + }) .then(() => { expect(onRender.mock.calls.length).toBe(2); }); @@ -102,13 +110,16 @@ describe('withOnyx', () => { })(ViewWithCollections); const onRender = jest.fn(); render(); - Onyx.mergeCollection(ONYX_KEYS.COLLECTION.TEST_KEY, {test_4: {ID: 456}, test_5: {ID: 567}}); - Onyx.mergeCollection(ONYX_KEYS.COLLECTION.TEST_KEY, { - test_4: {Name: 'Test4'}, - test_5: {Name: 'Test5'}, - test_6: {ID: 678, Name: 'Test6'}, - }); - return waitForPromisesToResolve() + waitForPromisesToResolve() + .then(() => { + Onyx.mergeCollection(ONYX_KEYS.COLLECTION.TEST_KEY, {test_4: {ID: 456}, test_5: {ID: 567}}); + Onyx.mergeCollection(ONYX_KEYS.COLLECTION.TEST_KEY, { + test_4: {Name: 'Test4'}, + test_5: {Name: 'Test5'}, + test_6: {ID: 678, Name: 'Test6'}, + }); + return waitForPromisesToResolve(); + }) .then(() => { expect(onRender.mock.calls.length).toBe(3); expect(onRender.mock.instances[2].text).toEqual({ID: 456, Name: 'Test4'}); From 31c8a40956a42b11224d6cd6df334ca4ab570059 Mon Sep 17 00:00:00 2001 From: Marc Glasser Date: Thu, 12 Aug 2021 07:11:12 -1000 Subject: [PATCH 4/6] only create requiredKeysForInit once --- lib/withOnyx.js | 16 +++++++--------- 1 file changed, 7 insertions(+), 9 deletions(-) diff --git a/lib/withOnyx.js b/lib/withOnyx.js index 4ff1716e..40054021 100644 --- a/lib/withOnyx.js +++ b/lib/withOnyx.js @@ -20,6 +20,12 @@ function getDisplayName(component) { } export default function (mapOnyxToState) { + // A list of keys that must be present in tempState before we can render the WrappedComponent + const requiredKeysForInit = _.chain(mapOnyxToState) + .omit(config => config.initWithStoredValues === false) + .keys() + .value(); + return (WrappedComponent) => { class withOnyx extends React.Component { constructor(props) { @@ -31,6 +37,7 @@ export default function (mapOnyxToState) { // disconnected. It is a key value store with the format {[mapping.key]: connectionID}. this.activeConnectionIDs = {}; + // Object holding the temporary initial state for the component while we load the various Onyx keys this.tempState = {}; this.state = { @@ -82,15 +89,6 @@ export default function (mapOnyxToState) { setInitialState(statePropertyName, val) { this.tempState[statePropertyName] = val; - // Filter all keys by those which we do want to init with stored values - // since keys that are configured to not init with stored values will - // never appear on state when the component mounts - only after they update - // organically. - const requiredKeysForInit = _.chain(mapOnyxToState) - .omit(config => config.initWithStoredValues === false) - .keys() - .value(); - // All state keys should exist and at least have a value of null if (!_.every(requiredKeysForInit, key => !_.isUndefined(this.tempState[key]))) { return; From fe2de12d073216d69ee2e18f564c3826f5828557 Mon Sep 17 00:00:00 2001 From: Marc Glasser Date: Thu, 12 Aug 2021 07:18:17 -1000 Subject: [PATCH 5/6] return the promise in the test --- tests/unit/withOnyxTest.js | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/tests/unit/withOnyxTest.js b/tests/unit/withOnyxTest.js index 9217f39f..509cb5ff 100644 --- a/tests/unit/withOnyxTest.js +++ b/tests/unit/withOnyxTest.js @@ -68,7 +68,7 @@ describe('withOnyx', () => { })(ViewWithCollections); const onRender = jest.fn(); render(); - waitForPromisesToResolve() + return waitForPromisesToResolve() .then(() => { Onyx.mergeCollection(ONYX_KEYS.COLLECTION.TEST_KEY, { test_1: {ID: 123}, test_2: {ID: 234}, test_3: {ID: 345}, @@ -89,7 +89,7 @@ describe('withOnyx', () => { })(ViewWithCollections); const onRender = jest.fn(); render(); - waitForPromisesToResolve() + return waitForPromisesToResolve() .then(() => { Onyx.mergeCollection(ONYX_KEYS.COLLECTION.TEST_KEY, { test_1: {ID: 123}, test_2: {ID: 234}, test_3: {ID: 345}, @@ -110,7 +110,7 @@ describe('withOnyx', () => { })(ViewWithCollections); const onRender = jest.fn(); render(); - waitForPromisesToResolve() + return waitForPromisesToResolve() .then(() => { Onyx.mergeCollection(ONYX_KEYS.COLLECTION.TEST_KEY, {test_4: {ID: 456}, test_5: {ID: 567}}); Onyx.mergeCollection(ONYX_KEYS.COLLECTION.TEST_KEY, { From 9718d3c549fb55217473b2de1093f86f031c2d26 Mon Sep 17 00:00:00 2001 From: Marc Glasser Date: Thu, 12 Aug 2021 12:08:25 -1000 Subject: [PATCH 6/6] Make suggested changes --- lib/withOnyx.js | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/lib/withOnyx.js b/lib/withOnyx.js index 40054021..834e1ad9 100644 --- a/lib/withOnyx.js +++ b/lib/withOnyx.js @@ -87,16 +87,20 @@ export default function (mapOnyxToState) { * @param {*} val */ setInitialState(statePropertyName, val) { + if (!this.state.loading) { + console.error('withOnyx.setInitialState() called after loading: false'); + return; + } + this.tempState[statePropertyName] = val; // All state keys should exist and at least have a value of null - if (!_.every(requiredKeysForInit, key => !_.isUndefined(this.tempState[key]))) { + if (_.some(requiredKeysForInit, key => _.isUndefined(this.tempState[key]))) { return; } - this.setState({...this.tempState, loading: false}, () => { - this.tempState = {}; - }); + this.setState({...this.tempState, loading: false}); + delete this.tempState; } /**