From 1998ca0170f021e75419a8880ee0e0c69734fb4a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Hanno=20J=2E=20G=C3=B6decke?= Date: Fri, 14 Jul 2023 16:56:45 +0200 Subject: [PATCH 01/43] Use cache when wrapping a component with onyx --- lib/Onyx.js | 162 +++++++++++++++++++++---------------- lib/withOnyx.js | 23 +++++- tests/unit/withOnyxTest.js | 16 ++-- 3 files changed, 120 insertions(+), 81 deletions(-) diff --git a/lib/Onyx.js b/lib/Onyx.js index 381b4f28..75ff934a 100644 --- a/lib/Onyx.js +++ b/lib/Onyx.js @@ -181,6 +181,51 @@ function isSafeEvictionKey(testKey) { return _.some(evictionAllowList, key => isKeyMatch(key, testKey)); } +/** + * Tries to return all keys + * from the current cache. + * @returns {Array} + */ +function getAllKeysSync() { + return cache.getAllKeys(); +} + +function tryGetCachedValue(key, mapping = {}) { + if (isCollectionKey(key)) { + const allKeys = getAllKeysSync(); + const matchingKeys = _.filter(allKeys, k => k.startsWith(key)); + const values = _.reduce(matchingKeys, (finalObject, matchedKey) => { + const val = cache.getValue(matchedKey); + if (val) { + // We know a 100% where the object is coming from, so its fine + // eslint-disable-next-line no-param-reassign + finalObject[matchedKey] = val; + } + return finalObject; + }, {}); + if (_.keys(values).length > 0) { + return values; + } + } + + if (!cache.hasCacheForKey(key)) { + return defaultKeyStates[key]; // We might have the value in the default key state + } + + const val = cache.getValue(key); + + // If the mapping has a selector, then the component's state must only be updated with the data + // returned by the selector. + if (mapping.selector) { + if (isCollectionKey(key)) { + return reduceCollectionWithSelector(val, mapping.selector, mapping.withOnyxInstance?.state); + } + return getSubsetOfData(val, mapping.selector, mapping.withOnyxInstance?.state); + } + + return val; +} + /** * Remove a key from the recently accessed key list. * @@ -367,24 +412,30 @@ function keysChanged(collectionKey, partialCollection) { const previousData = prevState[subscriber.statePropertyName]; const newData = reduceCollectionWithSelector(cachedCollection, subscriber.selector, subscriber.withOnyxInstance.state); - if (!deepEqual(previousData, newData)) { - return { - [subscriber.statePropertyName]: newData, - }; + if (deepEqual(previousData, newData)) { + return null; } - return null; + + return { + [subscriber.statePropertyName]: newData, + }; }); continue; } subscriber.withOnyxInstance.setState((prevState) => { - const finalCollection = _.clone(prevState[subscriber.statePropertyName] || {}); + const prevCollection = prevState[subscriber.statePropertyName]; + const finalCollection = _.clone(prevCollection || {}); const dataKeys = _.keys(partialCollection); for (let j = 0; j < dataKeys.length; j++) { const dataKey = dataKeys[j]; finalCollection[dataKey] = cachedCollection[dataKey]; } + if (deepEqual(prevCollection, finalCollection)) { + return null; + } + PerformanceUtils.logSetStateCall(subscriber, prevState[subscriber.statePropertyName], finalCollection, 'keysChanged', collectionKey); return { [subscriber.statePropertyName]: finalCollection, @@ -405,32 +456,18 @@ function keysChanged(collectionKey, partialCollection) { // If the subscriber has a selector, then the component's state must only be updated with the data // returned by the selector and the state should only change when the subset of data changes from what // it was previously. - if (subscriber.selector) { - subscriber.withOnyxInstance.setState((prevState) => { - const prevData = prevState[subscriber.statePropertyName]; - const newData = getSubsetOfData(cachedCollection[subscriber.key], subscriber.selector, subscriber.withOnyxInstance.state); - if (!deepEqual(prevData, newData)) { - PerformanceUtils.logSetStateCall(subscriber, prevData, newData, 'keysChanged', collectionKey); - return { - [subscriber.statePropertyName]: newData, - }; - } - - return null; - }); - continue; - } - subscriber.withOnyxInstance.setState((prevState) => { + const prevData = prevState[subscriber.statePropertyName]; const data = cachedCollection[subscriber.key]; - const previousData = prevState[subscriber.statePropertyName]; - if (data === previousData) { + const newData = subscriber.selector ? getSubsetOfData(data, subscriber.selector, subscriber.withOnyxInstance.state) : data; + + if (deepEqual(prevData, newData)) { return null; } - PerformanceUtils.logSetStateCall(subscriber, previousData, data, 'keysChanged', collectionKey); + PerformanceUtils.logSetStateCall(subscriber, prevData, newData, 'keysChanged', collectionKey); return { - [subscriber.statePropertyName]: data, + [subscriber.statePropertyName]: newData, }; }); } @@ -486,34 +523,21 @@ function keyChanged(key, data, canUpdateSubscriber) { if (isCollectionKey(subscriber.key)) { // If the subscriber has a selector, then the consumer of this data must only be given the data // returned by the selector and only when the selected data has changed. - if (subscriber.selector) { - subscriber.withOnyxInstance.setState((prevState) => { - const prevData = prevState[subscriber.statePropertyName]; - const newData = { - [key]: getSubsetOfData(data, subscriber.selector, subscriber.withOnyxInstance.state), - }; - const prevDataWithNewData = { - ...prevData, - ...newData, - }; - if (!deepEqual(prevData, prevDataWithNewData)) { - PerformanceUtils.logSetStateCall(subscriber, prevData, newData, 'keyChanged', key); - return { - [subscriber.statePropertyName]: prevDataWithNewData, - }; - } - return null; - }); - continue; - } - subscriber.withOnyxInstance.setState((prevState) => { - const collection = prevState[subscriber.statePropertyName] || {}; + const prevData = prevState[subscriber.statePropertyName] || {}; + const newData = { + [key]: subscriber.selector ? getSubsetOfData(data, subscriber.selector, subscriber.withOnyxInstance.state) : data, + }; const newCollection = { - ...collection, - [key]: data, + ...prevData, + ...newData, }; - PerformanceUtils.logSetStateCall(subscriber, collection, newCollection, 'keyChanged', key); + + if (deepEqual(prevData, newCollection)) { + return null; + } + + PerformanceUtils.logSetStateCall(subscriber, prevData, newCollection, 'keyChanged', key); return { [subscriber.statePropertyName]: newCollection, }; @@ -523,30 +547,16 @@ function keyChanged(key, data, canUpdateSubscriber) { // If the subscriber has a selector, then the component's state must only be updated with the data // returned by the selector and only if the selected data has changed. - if (subscriber.selector) { - subscriber.withOnyxInstance.setState((prevState) => { - const previousValue = getSubsetOfData(prevState[subscriber.statePropertyName], subscriber.selector, subscriber.withOnyxInstance.state); - const newValue = getSubsetOfData(data, subscriber.selector, subscriber.withOnyxInstance.state); - if (!deepEqual(previousValue, newValue)) { - return { - [subscriber.statePropertyName]: newValue, - }; - } - return null; - }); - continue; - } - - // If we did not match on a collection key then we just set the new data to the state property subscriber.withOnyxInstance.setState((prevState) => { - const previousData = prevState[subscriber.statePropertyName]; - if (previousData === data) { + const previousValue = prevState[subscriber.statePropertyName]; + const newValue = subscriber.selector ? getSubsetOfData(data, subscriber.selector, subscriber.withOnyxInstance.state) : data; + if (deepEqual(previousValue, newValue)) { return null; } - PerformanceUtils.logSetStateCall(subscriber, previousData, data, 'keyChanged', key); + PerformanceUtils.logSetStateCall(subscriber, previousValue, newValue, 'keyChanged', key); return { - [subscriber.statePropertyName]: data, + [subscriber.statePropertyName]: newValue, }; }); continue; @@ -561,6 +571,8 @@ function keyChanged(key, data, canUpdateSubscriber) { * - sets state on the withOnyxInstances * - triggers the callback function * + * Note: This is only invoked from Onyx.connect + * * @private * @param {Object} mapping * @param {Object} [mapping.withOnyxInstance] @@ -590,6 +602,15 @@ function sendDataToConnection(mapping, val, matchedKey) { } } + // sendDataToConnection only get's called in the connect code. + // withOnyx is already setting data from cache initially. If we now + // want to call sendDataToConnection, we want to make sure, we + // are not sending again the same data from cache, if the component + // already has the cached data. + if (deepEqual(mapping.withOnyxInstance.state[mapping.statePropertyName], newData)) { + return; + } + PerformanceUtils.logSetStateCall(mapping, null, newData, 'sendDataToConnection'); mapping.withOnyxInstance.setWithOnyxState(mapping.statePropertyName, newData); return; @@ -1327,6 +1348,7 @@ const Onyx = { isSafeEvictionKey, METHOD, setMemoryOnlyKeys, + tryGetCachedValue, }; /** diff --git a/lib/withOnyx.js b/lib/withOnyx.js index e6b9ce19..40f1eb47 100644 --- a/lib/withOnyx.js +++ b/lib/withOnyx.js @@ -37,12 +37,29 @@ export default function (mapOnyxToState) { // disconnected. It is a key value store with the format {[mapping.key]: connectionID}. this.activeConnectionIDs = {}; + let cachedAssigments = 0; + this.cachedState = {}; + + // Check if we can get all the data synchronously from cache + _.each(mapOnyxToState, (mapping, propertyName) => { + const key = Str.result(mapping.key, props); + const value = Onyx.tryGetCachedValue(key, mapping); + + if (value !== undefined) { + this.cachedState[propertyName] = value; + cachedAssigments++; + } + }); + + // If there are no required keys for init then we can render the wrapped component immediately + const loading = cachedAssigments < requiredKeysForInit.length; + // Object holding the temporary initial state for the component while we load the various Onyx keys - this.tempState = {}; + this.tempState = this.cachedState; this.state = { - // If there are no required keys for init then we can render the wrapped component immediately - loading: requiredKeysForInit.length > 0, + loading, + ...(loading ? {} : this.cachedState), }; } diff --git a/tests/unit/withOnyxTest.js b/tests/unit/withOnyxTest.js index 58ed912a..1e8ab33c 100644 --- a/tests/unit/withOnyxTest.js +++ b/tests/unit/withOnyxTest.js @@ -23,10 +23,13 @@ Onyx.init({ beforeEach(() => Onyx.clear()); describe('withOnyx', () => { - it('should render with the test data when using withOnyx', () => { - let result; - - return Onyx.set(ONYX_KEYS.TEST_KEY, 'test1') + it('should render immediately with the test data when using withOnyx', () => { + // Note: earlier, after rendering a component we had to do another + // waitForPromisesToResolve() to wait for Onyx's next tick updating + // the component from {loading: true} to {loading:false, ...data}. + // We now changed the architecture, so that when a key can be retrieved + // synchronously from cache, we expect the component to be rendered immediately. + Onyx.set(ONYX_KEYS.TEST_KEY, 'test1') .then(() => { const TestComponentWithOnyx = withOnyx({ text: { @@ -34,10 +37,7 @@ describe('withOnyx', () => { }, })(ViewWithText); - result = render(); - return waitForPromisesToResolve(); - }) - .then(() => { + const result = render(); const textComponent = result.getByText('test1'); expect(textComponent).not.toBeNull(); }); From 09b94948c3c205d7e0253044240c985b7f025c41 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Hanno=20J=2E=20G=C3=B6decke?= Date: Fri, 14 Jul 2023 17:02:55 +0200 Subject: [PATCH 02/43] tests: removed now unnecessary waitForPromiseToResolve calls --- tests/unit/withOnyxTest.js | 87 +++++++++++++------------------------- 1 file changed, 29 insertions(+), 58 deletions(-) diff --git a/tests/unit/withOnyxTest.js b/tests/unit/withOnyxTest.js index 1e8ab33c..89519d09 100644 --- a/tests/unit/withOnyxTest.js +++ b/tests/unit/withOnyxTest.js @@ -56,9 +56,8 @@ describe('withOnyx', () => { .then(() => { Onyx.merge(`${ONYX_KEYS.COLLECTION.TEST_KEY}1`, {ID: 123}); Onyx.merge(`${ONYX_KEYS.COLLECTION.TEST_KEY}2`, {ID: 234}); - Onyx.merge(`${ONYX_KEYS.COLLECTION.TEST_KEY}3`, {ID: 345}); + return Onyx.merge(`${ONYX_KEYS.COLLECTION.TEST_KEY}3`, {ID: 345}); }) - .then(waitForPromisesToResolve) .then(() => { expect(onRender).toHaveBeenCalledTimes(4); }); @@ -73,12 +72,9 @@ describe('withOnyx', () => { const onRender = jest.fn(); render(); return waitForPromisesToResolve() - .then(() => { - Onyx.mergeCollection(ONYX_KEYS.COLLECTION.TEST_KEY, { - test_1: {ID: 123}, test_2: {ID: 234}, test_3: {ID: 345}, - }); - return waitForPromisesToResolve(); - }) + .then(() => Onyx.mergeCollection(ONYX_KEYS.COLLECTION.TEST_KEY, { + test_1: {ID: 123}, test_2: {ID: 234}, test_3: {ID: 345}, + })) .then(() => { expect(onRender).toHaveBeenCalledTimes(2); }); @@ -94,12 +90,9 @@ describe('withOnyx', () => { const onRender = jest.fn(); render(); return waitForPromisesToResolve() - .then(() => { - Onyx.mergeCollection(ONYX_KEYS.COLLECTION.TEST_KEY, { - test_1: {ID: 123}, test_2: {ID: 234}, test_3: {ID: 345}, - }); - return waitForPromisesToResolve(); - }) + .then(() => Onyx.mergeCollection(ONYX_KEYS.COLLECTION.TEST_KEY, { + test_1: {ID: 123}, test_2: {ID: 234}, test_3: {ID: 345}, + })) .then(() => { expect(onRender).toHaveBeenCalledTimes(2); }); @@ -115,18 +108,12 @@ describe('withOnyx', () => { const onRender = jest.fn(); render(); return waitForPromisesToResolve() - .then(() => { - Onyx.mergeCollection(ONYX_KEYS.COLLECTION.TEST_KEY, { - test_1: {list: [1, 2]}, - }); - return waitForPromisesToResolve(); - }) - .then(() => { - Onyx.mergeCollection(ONYX_KEYS.COLLECTION.TEST_KEY, { - test_1: {list: [7]}, - }); - return waitForPromisesToResolve(); - }) + .then(() => Onyx.mergeCollection(ONYX_KEYS.COLLECTION.TEST_KEY, { + test_1: {list: [1, 2]}, + })) + .then(() => Onyx.mergeCollection(ONYX_KEYS.COLLECTION.TEST_KEY, { + test_1: {list: [7]}, + })) .then(() => { expect(onRender).toHaveBeenCalledTimes(3); expect(onRender).toHaveBeenLastCalledWith({ @@ -147,12 +134,11 @@ describe('withOnyx', () => { 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, { + return 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).toHaveBeenCalledTimes(3); @@ -178,14 +164,13 @@ describe('withOnyx', () => { const result = render(); rerender = result.rerender; getByTestId = result.getByTestId; - return waitForPromisesToResolve(); }) .then(() => { expect(getByTestId('text-element').props.children).toEqual('test_1'); }) .then(() => { rerender(); - return waitForPromisesToResolve(); + return waitForPromisesToResolve(); // TODO: interesting, here we need to await. Why? }) .then(() => { expect(getByTestId('text-element').props.children).toEqual('test_2'); @@ -231,7 +216,6 @@ describe('withOnyx', () => { }), )(ViewWithCollections); render(); - return waitForPromisesToResolve(); }) .then(() => { expect(onRender).toHaveBeenLastCalledWith({ @@ -275,16 +259,12 @@ describe('withOnyx', () => { }, })(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(); }) + + // When a single item in the collection is updated with mergeCollect() + .then(() => Onyx.mergeCollection(ONYX_KEYS.COLLECTION.TEST_KEY, { + test_1: {ID: 1, newProperty: 'yay'}, + })) .then(() => { // Then the component subscribed to the modified item should have the new version of the item // and all other components should be unchanged. @@ -319,16 +299,12 @@ describe('withOnyx', () => { }, })(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(); }) + + // When the `number` property is updated using mergeCollection to be 2 + .then(() => Onyx.mergeCollection(ONYX_KEYS.COLLECTION.TEST_KEY, { + test_1: {number: 2}, + })) .then(() => { // Then the component subscribed to the modified item should be rendered twice. // The first time it will render with number === 1 @@ -354,22 +330,17 @@ describe('withOnyx', () => { }, })(ViewWithCollections); render(); - - return waitForPromisesToResolve(); - }) - .then(() => { - // And we set the value to the same value it was before - Onyx.merge(ONYX_KEYS.SIMPLE_KEY, 'string'); - return waitForPromisesToResolve(); }) + + // And we set the value to the same value it was before + .then(() => Onyx.merge(ONYX_KEYS.SIMPLE_KEY, 'string')) .then(() => { // Then the component subscribed to the modified item should only render once expect(onRender).toHaveBeenCalledTimes(1); expect(onRender.mock.calls[0][0].simple).toBe('string'); // If we change the value to something new - Onyx.merge(ONYX_KEYS.SIMPLE_KEY, 'long_string'); - return waitForPromisesToResolve(); + return Onyx.merge(ONYX_KEYS.SIMPLE_KEY, 'long_string'); }) .then(() => { // Then the component subscribed to the modified item should only render once From 0f5e115d89ba7f0639db9dc9bbf8905f6a6252f0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Hanno=20J=2E=20G=C3=B6decke?= Date: Fri, 14 Jul 2023 18:08:10 +0200 Subject: [PATCH 03/43] wip: one approach to solve prop updates --- lib/withOnyx.js | 26 ++++++++++++++++++++------ tests/unit/withOnyxTest.js | 13 ++++--------- 2 files changed, 24 insertions(+), 15 deletions(-) diff --git a/lib/withOnyx.js b/lib/withOnyx.js index 40f1eb47..efd8a843 100644 --- a/lib/withOnyx.js +++ b/lib/withOnyx.js @@ -38,7 +38,7 @@ export default function (mapOnyxToState) { this.activeConnectionIDs = {}; let cachedAssigments = 0; - this.cachedState = {}; + const cachedState = {}; // Check if we can get all the data synchronously from cache _.each(mapOnyxToState, (mapping, propertyName) => { @@ -46,7 +46,7 @@ export default function (mapOnyxToState) { const value = Onyx.tryGetCachedValue(key, mapping); if (value !== undefined) { - this.cachedState[propertyName] = value; + cachedState[propertyName] = value; cachedAssigments++; } }); @@ -55,11 +55,11 @@ export default function (mapOnyxToState) { const loading = cachedAssigments < requiredKeysForInit.length; // Object holding the temporary initial state for the component while we load the various Onyx keys - this.tempState = this.cachedState; + this.tempState = cachedState; this.state = { loading, - ...(loading ? {} : this.cachedState), + ...(loading ? {} : cachedState), }; } @@ -72,19 +72,33 @@ export default function (mapOnyxToState) { } componentDidUpdate(prevProps) { + const stateUpdates = {}; + // 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) => { + // Check if we can update from cache already const previousKey = Str.result(mapping.key, prevProps); const newKey = Str.result(mapping.key, this.props); - if (previousKey !== newKey) { + // Check if we can update the state synchronously from cache + const value = Onyx.tryGetCachedValue(newKey, mapping); + if (value !== undefined && value !== this.state[propertyName]) { + stateUpdates[propertyName] = value; + } + + // Renew the connection with the new key Onyx.disconnect(this.activeConnectionIDs[previousKey], previousKey); delete this.activeConnectionIDs[previousKey]; this.connectMappingToOnyx(mapping, propertyName); } }); this.checkEvictableKeys(); + + // TODO: i am bothered by the extra render that happens here. + if (!_.isEmpty(stateUpdates)) { + this.setState(stateUpdates); + } } componentWillUnmount() { @@ -105,7 +119,7 @@ export default function (mapOnyxToState) { * @param {*} val */ setWithOnyxState(statePropertyName, val) { - if (!this.state.loading) { + if (!this.state.loading && this.state[statePropertyName] !== val) { this.setState({[statePropertyName]: val}); return; } diff --git a/tests/unit/withOnyxTest.js b/tests/unit/withOnyxTest.js index 89519d09..1de268d1 100644 --- a/tests/unit/withOnyxTest.js +++ b/tests/unit/withOnyxTest.js @@ -151,7 +151,6 @@ describe('withOnyx', () => { it('should update if a prop dependent key changes', () => { let rerender; let getByTestId; - const onRender = jest.fn(); const TestComponentWithOnyx = withOnyx({ text: { key: props => `${ONYX_KEYS.COLLECTION.TEST_KEY}${props.collectionID}`, @@ -161,18 +160,14 @@ describe('withOnyx', () => { Onyx.set(`${ONYX_KEYS.COLLECTION.TEST_KEY}2`, 'test_2'); return waitForPromisesToResolve() .then(() => { - const result = render(); + const result = render(); rerender = result.rerender; getByTestId = result.getByTestId; - }) - .then(() => { + expect(getByTestId('text-element').props.children).toEqual('test_1'); - }) - .then(() => { + rerender(); - return waitForPromisesToResolve(); // TODO: interesting, here we need to await. Why? - }) - .then(() => { + expect(getByTestId('text-element').props.children).toEqual('test_2'); }); }); From 98a1672ad03a15bf8fb0cddaffb5538a46caa988 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Hanno=20J=2E=20G=C3=B6decke?= Date: Mon, 17 Jul 2023 08:36:24 +0200 Subject: [PATCH 04/43] fix test --- tests/unit/withOnyxTest.js | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/tests/unit/withOnyxTest.js b/tests/unit/withOnyxTest.js index 1de268d1..5805c963 100644 --- a/tests/unit/withOnyxTest.js +++ b/tests/unit/withOnyxTest.js @@ -163,11 +163,17 @@ describe('withOnyx', () => { const result = render(); rerender = result.rerender; getByTestId = result.getByTestId; - + }) + .then(() => { expect(getByTestId('text-element').props.children).toEqual('test_1'); - + }) + .then(() => { rerender(); + // Note, when we change the prop, we need to wait for the next tick: + return waitForPromisesToResolve(); + }) + .then(() => { expect(getByTestId('text-element').props.children).toEqual('test_2'); }); }); From 916b9b0c0dac4baec713a12fab1e7120d5ceeb37 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Hanno=20J=2E=20G=C3=B6decke?= Date: Mon, 17 Jul 2023 08:36:53 +0200 Subject: [PATCH 05/43] remove componentDidUpdate code --- lib/withOnyx.js | 14 -------------- 1 file changed, 14 deletions(-) diff --git a/lib/withOnyx.js b/lib/withOnyx.js index efd8a843..db3a6097 100644 --- a/lib/withOnyx.js +++ b/lib/withOnyx.js @@ -72,8 +72,6 @@ export default function (mapOnyxToState) { } componentDidUpdate(prevProps) { - const stateUpdates = {}; - // 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) => { @@ -81,24 +79,12 @@ export default function (mapOnyxToState) { const previousKey = Str.result(mapping.key, prevProps); const newKey = Str.result(mapping.key, this.props); if (previousKey !== newKey) { - // Check if we can update the state synchronously from cache - const value = Onyx.tryGetCachedValue(newKey, mapping); - if (value !== undefined && value !== this.state[propertyName]) { - stateUpdates[propertyName] = value; - } - - // Renew the connection with the new key Onyx.disconnect(this.activeConnectionIDs[previousKey], previousKey); delete this.activeConnectionIDs[previousKey]; this.connectMappingToOnyx(mapping, propertyName); } }); this.checkEvictableKeys(); - - // TODO: i am bothered by the extra render that happens here. - if (!_.isEmpty(stateUpdates)) { - this.setState(stateUpdates); - } } componentWillUnmount() { From 35becc3f300bf3a6124db2737f40ed26a7c8353a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Hanno=20J=2E=20G=C3=B6decke?= Date: Mon, 17 Jul 2023 08:41:26 +0200 Subject: [PATCH 06/43] add jsdoc --- lib/Onyx.js | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/lib/Onyx.js b/lib/Onyx.js index 75ff934a..f3ee0f6b 100644 --- a/lib/Onyx.js +++ b/lib/Onyx.js @@ -184,12 +184,24 @@ function isSafeEvictionKey(testKey) { /** * Tries to return all keys * from the current cache. + * (internal) * @returns {Array} */ function getAllKeysSync() { return cache.getAllKeys(); } +/** + * Tries to get a value from the cache. + * If the value is not present in cache it will + * return the default value or undefined. + * If the requested key is a collection, it will + * return an object with all the collection members. + * + * @param {string} key + * @param {Object} mapping + * @returns {string} + */ function tryGetCachedValue(key, mapping = {}) { if (isCollectionKey(key)) { const allKeys = getAllKeysSync(); From 4b7f2670715622386929deccf53da47c33abbd06 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Hanno=20J=2E=20G=C3=B6decke?= Date: Mon, 17 Jul 2023 08:44:56 +0200 Subject: [PATCH 07/43] clean --- lib/withOnyx.js | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/lib/withOnyx.js b/lib/withOnyx.js index db3a6097..1b6bcab0 100644 --- a/lib/withOnyx.js +++ b/lib/withOnyx.js @@ -37,7 +37,6 @@ export default function (mapOnyxToState) { // disconnected. It is a key value store with the format {[mapping.key]: connectionID}. this.activeConnectionIDs = {}; - let cachedAssigments = 0; const cachedState = {}; // Check if we can get all the data synchronously from cache @@ -47,12 +46,11 @@ export default function (mapOnyxToState) { if (value !== undefined) { cachedState[propertyName] = value; - cachedAssigments++; } }); - // If there are no required keys for init then we can render the wrapped component immediately - const loading = cachedAssigments < requiredKeysForInit.length; + // If we have all the data we need, then we can render the component immediately + const loading = _.keys(cachedState).length < requiredKeysForInit.length; // Object holding the temporary initial state for the component while we load the various Onyx keys this.tempState = cachedState; From 6943d360327a7d637e1c680c9889bd8210acb17e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Hanno=20J=2E=20G=C3=B6decke?= Date: Mon, 17 Jul 2023 08:56:05 +0200 Subject: [PATCH 08/43] fix eslint --- lib/Onyx.js | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/lib/Onyx.js b/lib/Onyx.js index f3ee0f6b..83e8e105 100644 --- a/lib/Onyx.js +++ b/lib/Onyx.js @@ -229,10 +229,11 @@ function tryGetCachedValue(key, mapping = {}) { // If the mapping has a selector, then the component's state must only be updated with the data // returned by the selector. if (mapping.selector) { + const state = mapping.withOnyxInstance ? mapping.withOnyxInstance.state : undefined; if (isCollectionKey(key)) { - return reduceCollectionWithSelector(val, mapping.selector, mapping.withOnyxInstance?.state); + return reduceCollectionWithSelector(val, mapping.selector, state); } - return getSubsetOfData(val, mapping.selector, mapping.withOnyxInstance?.state); + return getSubsetOfData(val, mapping.selector, state); } return val; From 790def1a246492e46e5791e3be783ed226099cc4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Hanno=20J=2E=20G=C3=B6decke?= Date: Mon, 17 Jul 2023 11:52:50 +0200 Subject: [PATCH 09/43] use cache.getAllKeys directly --- lib/Onyx.js | 12 +----------- 1 file changed, 1 insertion(+), 11 deletions(-) diff --git a/lib/Onyx.js b/lib/Onyx.js index 83e8e105..1aa0b6c2 100644 --- a/lib/Onyx.js +++ b/lib/Onyx.js @@ -181,16 +181,6 @@ function isSafeEvictionKey(testKey) { return _.some(evictionAllowList, key => isKeyMatch(key, testKey)); } -/** - * Tries to return all keys - * from the current cache. - * (internal) - * @returns {Array} - */ -function getAllKeysSync() { - return cache.getAllKeys(); -} - /** * Tries to get a value from the cache. * If the value is not present in cache it will @@ -204,7 +194,7 @@ function getAllKeysSync() { */ function tryGetCachedValue(key, mapping = {}) { if (isCollectionKey(key)) { - const allKeys = getAllKeysSync(); + const allKeys = cache.getAllKeys(); const matchingKeys = _.filter(allKeys, k => k.startsWith(key)); const values = _.reduce(matchingKeys, (finalObject, matchedKey) => { const val = cache.getValue(matchedKey); From f71294ef5e83854c6a3f3ae9e58c3a10e39396fa Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Hanno=20J=2E=20G=C3=B6decke?= Date: Mon, 17 Jul 2023 11:53:49 +0200 Subject: [PATCH 10/43] add better comment --- lib/Onyx.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/Onyx.js b/lib/Onyx.js index 1aa0b6c2..c9bdd270 100644 --- a/lib/Onyx.js +++ b/lib/Onyx.js @@ -199,7 +199,7 @@ function tryGetCachedValue(key, mapping = {}) { const values = _.reduce(matchingKeys, (finalObject, matchedKey) => { const val = cache.getValue(matchedKey); if (val) { - // We know a 100% where the object is coming from, so its fine + // This is permissible because we're in the process of constructing the final object in a reduce function. // eslint-disable-next-line no-param-reassign finalObject[matchedKey] = val; } From 9358b41cb279457c3f3f01383f88682066fc7f4d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Hanno=20J=2E=20G=C3=B6decke?= Date: Mon, 17 Jul 2023 11:55:39 +0200 Subject: [PATCH 11/43] foeward default value eventually to selector --- lib/Onyx.js | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/lib/Onyx.js b/lib/Onyx.js index c9bdd270..be2df4b2 100644 --- a/lib/Onyx.js +++ b/lib/Onyx.js @@ -210,11 +210,7 @@ function tryGetCachedValue(key, mapping = {}) { } } - if (!cache.hasCacheForKey(key)) { - return defaultKeyStates[key]; // We might have the value in the default key state - } - - const val = cache.getValue(key); + const val = cache.hasCacheForKey(key) ? cache.getValue(key) : defaultKeyStates[key]; // If the mapping has a selector, then the component's state must only be updated with the data // returned by the selector. From 3ee92947406b9cfee13f4f0ddb4b8bcebefa0335 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Hanno=20J=2E=20G=C3=B6decke?= Date: Mon, 17 Jul 2023 11:59:01 +0200 Subject: [PATCH 12/43] return early --- lib/Onyx.js | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/lib/Onyx.js b/lib/Onyx.js index be2df4b2..3e6c04e0 100644 --- a/lib/Onyx.js +++ b/lib/Onyx.js @@ -205,9 +205,10 @@ function tryGetCachedValue(key, mapping = {}) { } return finalObject; }, {}); - if (_.keys(values).length > 0) { - return values; + if (_.keys(values).length === 0) { + return undefined; } + return values; } const val = cache.hasCacheForKey(key) ? cache.getValue(key) : defaultKeyStates[key]; From fcc49ac5416777adb95aa0244a61977ec34ae917 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Hanno=20J=2E=20G=C3=B6decke?= Date: Mon, 17 Jul 2023 12:01:53 +0200 Subject: [PATCH 13/43] remove ambigous comment --- lib/Onyx.js | 2 -- 1 file changed, 2 deletions(-) diff --git a/lib/Onyx.js b/lib/Onyx.js index 3e6c04e0..a075a4ce 100644 --- a/lib/Onyx.js +++ b/lib/Onyx.js @@ -213,8 +213,6 @@ function tryGetCachedValue(key, mapping = {}) { const val = cache.hasCacheForKey(key) ? cache.getValue(key) : defaultKeyStates[key]; - // If the mapping has a selector, then the component's state must only be updated with the data - // returned by the selector. if (mapping.selector) { const state = mapping.withOnyxInstance ? mapping.withOnyxInstance.state : undefined; if (isCollectionKey(key)) { From 853390e4bc370ee536185cd93137fa0f2c168045 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Hanno=20J=2E=20G=C3=B6decke?= Date: Mon, 17 Jul 2023 12:09:07 +0200 Subject: [PATCH 14/43] reverted back cleanups --- lib/Onyx.js | 105 ++++++++++++++++++++++++++++++++++++---------------- 1 file changed, 74 insertions(+), 31 deletions(-) diff --git a/lib/Onyx.js b/lib/Onyx.js index a075a4ce..8d5c4c96 100644 --- a/lib/Onyx.js +++ b/lib/Onyx.js @@ -410,13 +410,12 @@ function keysChanged(collectionKey, partialCollection) { const previousData = prevState[subscriber.statePropertyName]; const newData = reduceCollectionWithSelector(cachedCollection, subscriber.selector, subscriber.withOnyxInstance.state); - if (deepEqual(previousData, newData)) { - return null; + if (!deepEqual(previousData, newData)) { + return { + [subscriber.statePropertyName]: newData, + }; } - - return { - [subscriber.statePropertyName]: newData, - }; + return null; }); continue; } @@ -454,18 +453,32 @@ function keysChanged(collectionKey, partialCollection) { // If the subscriber has a selector, then the component's state must only be updated with the data // returned by the selector and the state should only change when the subset of data changes from what // it was previously. + if (subscriber.selector) { + subscriber.withOnyxInstance.setState((prevState) => { + const prevData = prevState[subscriber.statePropertyName]; + const newData = getSubsetOfData(cachedCollection[subscriber.key], subscriber.selector, subscriber.withOnyxInstance.state); + if (!deepEqual(prevData, newData)) { + PerformanceUtils.logSetStateCall(subscriber, prevData, newData, 'keysChanged', collectionKey); + return { + [subscriber.statePropertyName]: newData, + }; + } + + return null; + }); + continue; + } + subscriber.withOnyxInstance.setState((prevState) => { - const prevData = prevState[subscriber.statePropertyName]; const data = cachedCollection[subscriber.key]; - const newData = subscriber.selector ? getSubsetOfData(data, subscriber.selector, subscriber.withOnyxInstance.state) : data; - - if (deepEqual(prevData, newData)) { + const previousData = prevState[subscriber.statePropertyName]; + if (deepEqual(data, previousData)) { return null; } - PerformanceUtils.logSetStateCall(subscriber, prevData, newData, 'keysChanged', collectionKey); + PerformanceUtils.logSetStateCall(subscriber, previousData, data, 'keysChanged', collectionKey); return { - [subscriber.statePropertyName]: newData, + [subscriber.statePropertyName]: data, }; }); } @@ -521,40 +534,70 @@ function keyChanged(key, data, canUpdateSubscriber) { if (isCollectionKey(subscriber.key)) { // If the subscriber has a selector, then the consumer of this data must only be given the data // returned by the selector and only when the selected data has changed. + if (subscriber.selector) { + subscriber.withOnyxInstance.setState((prevState) => { + const prevData = prevState[subscriber.statePropertyName]; + const newData = { + [key]: getSubsetOfData(data, subscriber.selector, subscriber.withOnyxInstance.state), + }; + const prevDataWithNewData = { + ...prevData, + ...newData, + }; + if (!deepEqual(prevData, prevDataWithNewData)) { + PerformanceUtils.logSetStateCall(subscriber, prevData, newData, 'keyChanged', key); + return { + [subscriber.statePropertyName]: prevDataWithNewData, + }; + } + return null; + }); + continue; + } + subscriber.withOnyxInstance.setState((prevState) => { - const prevData = prevState[subscriber.statePropertyName] || {}; - const newData = { - [key]: subscriber.selector ? getSubsetOfData(data, subscriber.selector, subscriber.withOnyxInstance.state) : data, - }; + const collection = prevState[subscriber.statePropertyName] || {}; const newCollection = { - ...prevData, - ...newData, + ...collection, + [key]: data, }; - - if (deepEqual(prevData, newCollection)) { - return null; + if (!deepEqual(collection, newCollection)) { + PerformanceUtils.logSetStateCall(subscriber, collection, newCollection, 'keyChanged', key); + return { + [subscriber.statePropertyName]: newCollection, + }; } - - PerformanceUtils.logSetStateCall(subscriber, prevData, newCollection, 'keyChanged', key); - return { - [subscriber.statePropertyName]: newCollection, - }; + return null; }); continue; } // If the subscriber has a selector, then the component's state must only be updated with the data // returned by the selector and only if the selected data has changed. + if (subscriber.selector) { + subscriber.withOnyxInstance.setState((prevState) => { + const previousValue = getSubsetOfData(prevState[subscriber.statePropertyName], subscriber.selector, subscriber.withOnyxInstance.state); + const newValue = getSubsetOfData(data, subscriber.selector, subscriber.withOnyxInstance.state); + if (!deepEqual(previousValue, newValue)) { + return { + [subscriber.statePropertyName]: newValue, + }; + } + return null; + }); + continue; + } + + // If we did not match on a collection key then we just set the new data to the state property subscriber.withOnyxInstance.setState((prevState) => { - const previousValue = prevState[subscriber.statePropertyName]; - const newValue = subscriber.selector ? getSubsetOfData(data, subscriber.selector, subscriber.withOnyxInstance.state) : data; - if (deepEqual(previousValue, newValue)) { + const previousData = prevState[subscriber.statePropertyName]; + if (previousData === data) { return null; } - PerformanceUtils.logSetStateCall(subscriber, previousValue, newValue, 'keyChanged', key); + PerformanceUtils.logSetStateCall(subscriber, previousData, data, 'keyChanged', key); return { - [subscriber.statePropertyName]: newValue, + [subscriber.statePropertyName]: data, }; }); continue; From 0c72be18a345891f63fc498bb776934b00d00260 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Hanno=20J=2E=20G=C3=B6decke?= Date: Mon, 17 Jul 2023 12:49:53 +0200 Subject: [PATCH 15/43] fix: selector not getting applied to collection --- lib/Onyx.js | 24 ++++++++++++++++++------ 1 file changed, 18 insertions(+), 6 deletions(-) diff --git a/lib/Onyx.js b/lib/Onyx.js index 8d5c4c96..66c77fd0 100644 --- a/lib/Onyx.js +++ b/lib/Onyx.js @@ -154,6 +154,18 @@ function isCollectionMemberKey(collectionKey, key) { return Str.startsWith(key, collectionKey) && key.length > collectionKey.length; } +function isCollectionKeyAndNoMember(key) { + const keys = _.keys(onyxKeys.COLLECTION); + // eslint-disable-next-line no-restricted-syntax + for (const collectionKeyCandidate in keys) { + if (Str.contains(key, collectionKeyCandidate)) { + return !isCollectionMemberKey(collectionKeyCandidate, key); + } + } + + return false; +} + /** * Checks to see if a provided key is the exact configured key of our connected subscriber * or if the provided key is a collection member key (in case our configured key is a "collection key") @@ -193,26 +205,26 @@ function isSafeEvictionKey(testKey) { * @returns {string} */ function tryGetCachedValue(key, mapping = {}) { + let val = cache.hasCacheForKey(key) ? cache.getValue(key) : defaultKeyStates[key]; + if (isCollectionKey(key)) { const allKeys = cache.getAllKeys(); const matchingKeys = _.filter(allKeys, k => k.startsWith(key)); const values = _.reduce(matchingKeys, (finalObject, matchedKey) => { - const val = cache.getValue(matchedKey); - if (val) { + const cachedValue = cache.getValue(matchedKey); + if (cachedValue) { // This is permissible because we're in the process of constructing the final object in a reduce function. // eslint-disable-next-line no-param-reassign - finalObject[matchedKey] = val; + finalObject[matchedKey] = cachedValue; } return finalObject; }, {}); if (_.keys(values).length === 0) { return undefined; } - return values; + val = values; } - const val = cache.hasCacheForKey(key) ? cache.getValue(key) : defaultKeyStates[key]; - if (mapping.selector) { const state = mapping.withOnyxInstance ? mapping.withOnyxInstance.state : undefined; if (isCollectionKey(key)) { From 294aa31db9ce233003cf1c83794e17974ff0acf9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Hanno=20J=2E=20G=C3=B6decke?= Date: Mon, 17 Jul 2023 19:13:06 +0200 Subject: [PATCH 16/43] fix tests --- tests/unit/subscribeToPropertiesTest.js | 55 +++++++------------------ 1 file changed, 16 insertions(+), 39 deletions(-) diff --git a/tests/unit/subscribeToPropertiesTest.js b/tests/unit/subscribeToPropertiesTest.js index 69e4c750..af13b73e 100644 --- a/tests/unit/subscribeToPropertiesTest.js +++ b/tests/unit/subscribeToPropertiesTest.js @@ -56,7 +56,6 @@ describe('Only the specific property changes when using withOnyx() and ', () => .then(() => Onyx.merge(ONYX_KEYS.TEST_KEY, {a: 'one', b: 'two'})) .then(() => { renderedComponent = render(); - return waitForPromisesToResolve(); }) // Then the props passed to the component should only include the property "a" that was specified @@ -68,7 +67,6 @@ describe('Only the specific property changes when using withOnyx() and ', () => .then(() => Onyx.merge(ONYX_KEYS.TEST_KEY, {a: 'two'})) .then(() => { renderedComponent = render(); - return waitForPromisesToResolve(); }) // Then the props passed should have the new value of property "a" @@ -80,7 +78,6 @@ describe('Only the specific property changes when using withOnyx() and ', () => .then(() => Onyx.merge(ONYX_KEYS.TEST_KEY, {b: 'two'})) .then(() => { renderedComponent = render(); - return waitForPromisesToResolve(); }) // Then the props passed should not have changed @@ -126,16 +123,12 @@ describe('Only the specific property changes when using withOnyx() and ', () => return waitForPromisesToResolve() // When Onyx is updated with an object that has multiple properties - .then(() => { - Onyx.mergeCollection(ONYX_KEYS.COLLECTION.TEST_KEY, { - [`${ONYX_KEYS.COLLECTION.TEST_KEY}1`]: {a: 'one', b: 'two'}, - [`${ONYX_KEYS.COLLECTION.TEST_KEY}2`]: {c: 'three', d: 'four'}, - }); - return waitForPromisesToResolve(); - }) + .then(() => Onyx.mergeCollection(ONYX_KEYS.COLLECTION.TEST_KEY, { + [`${ONYX_KEYS.COLLECTION.TEST_KEY}1`]: {a: 'one', b: 'two'}, + [`${ONYX_KEYS.COLLECTION.TEST_KEY}2`]: {c: 'three', d: 'four'}, + })) .then(() => { renderedComponent = render(); - return waitForPromisesToResolve(); }) // Then the props passed to the component should only include the property "a" that was specified @@ -146,10 +139,7 @@ describe('Only the specific property changes when using withOnyx() and ', () => // When Onyx is updated with a change to property a using merge() // This uses merge() just to make sure that everything works as expected when mixing merge() // and mergeCollection() - .then(() => { - Onyx.merge(`${ONYX_KEYS.COLLECTION.TEST_KEY}1`, {a: 'two'}); - return waitForPromisesToResolve(); - }) + .then(() => Onyx.merge(`${ONYX_KEYS.COLLECTION.TEST_KEY}1`, {a: 'two'})) // Then the props passed should have the new value of property "a" .then(() => { @@ -157,12 +147,9 @@ describe('Only the specific property changes when using withOnyx() and ', () => }) // When Onyx is updated with a change to property b using mergeCollection() - .then(() => { - Onyx.mergeCollection(ONYX_KEYS.COLLECTION.TEST_KEY, { - [`${ONYX_KEYS.COLLECTION.TEST_KEY}1`]: {b: 'three'}, - }); - return waitForPromisesToResolve(); - }) + .then(() => Onyx.mergeCollection(ONYX_KEYS.COLLECTION.TEST_KEY, { + [`${ONYX_KEYS.COLLECTION.TEST_KEY}1`]: {b: 'three'}, + })) // Then the props passed should not have changed .then(() => { @@ -214,16 +201,12 @@ describe('Only the specific property changes when using withOnyx() and ', () => return waitForPromisesToResolve() // When Onyx is updated with an object that has multiple properties - .then(() => { - Onyx.mergeCollection(ONYX_KEYS.COLLECTION.TEST_KEY, { - [`${ONYX_KEYS.COLLECTION.TEST_KEY}1`]: {a: 'one', b: 'two'}, - [`${ONYX_KEYS.COLLECTION.TEST_KEY}2`]: {c: 'three', d: 'four'}, - }); - return waitForPromisesToResolve(); - }) + .then(() => Onyx.mergeCollection(ONYX_KEYS.COLLECTION.TEST_KEY, { + [`${ONYX_KEYS.COLLECTION.TEST_KEY}1`]: {a: 'one', b: 'two'}, + [`${ONYX_KEYS.COLLECTION.TEST_KEY}2`]: {c: 'three', d: 'four'}, + })) .then(() => { renderedComponent = render(); - return waitForPromisesToResolve(); }) // Then the props passed to the component should only include the property "a" that was specified @@ -234,10 +217,7 @@ describe('Only the specific property changes when using withOnyx() and ', () => // When Onyx is updated with a change to property a using merge() // This uses merge() just to make sure that everything works as expected when mixing merge() // and mergeCollection() - .then(() => { - Onyx.merge(`${ONYX_KEYS.COLLECTION.TEST_KEY}1`, {a: 'two'}); - return waitForPromisesToResolve(); - }) + .then(() => Onyx.merge(`${ONYX_KEYS.COLLECTION.TEST_KEY}1`, {a: 'two'})) // Then the props passed should have the new value of property "a" .then(() => { @@ -245,12 +225,9 @@ describe('Only the specific property changes when using withOnyx() and ', () => }) // When Onyx is updated with a change to property b using mergeCollection() - .then(() => { - Onyx.mergeCollection(ONYX_KEYS.COLLECTION.TEST_KEY, { - [`${ONYX_KEYS.COLLECTION.TEST_KEY}1`]: {b: 'three'}, - }); - return waitForPromisesToResolve(); - }) + .then(() => Onyx.mergeCollection(ONYX_KEYS.COLLECTION.TEST_KEY, { + [`${ONYX_KEYS.COLLECTION.TEST_KEY}1`]: {b: 'three'}, + })) // Then the props passed should not have changed .then(() => { From d2a5390b706eb2371aae9d42b49c8a06b4c4716b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Hanno=20J=2E=20G=C3=B6decke?= Date: Mon, 17 Jul 2023 19:13:32 +0200 Subject: [PATCH 17/43] remove unused function --- lib/Onyx.js | 12 ------------ 1 file changed, 12 deletions(-) diff --git a/lib/Onyx.js b/lib/Onyx.js index 66c77fd0..e032f68d 100644 --- a/lib/Onyx.js +++ b/lib/Onyx.js @@ -154,18 +154,6 @@ function isCollectionMemberKey(collectionKey, key) { return Str.startsWith(key, collectionKey) && key.length > collectionKey.length; } -function isCollectionKeyAndNoMember(key) { - const keys = _.keys(onyxKeys.COLLECTION); - // eslint-disable-next-line no-restricted-syntax - for (const collectionKeyCandidate in keys) { - if (Str.contains(key, collectionKeyCandidate)) { - return !isCollectionMemberKey(collectionKeyCandidate, key); - } - } - - return false; -} - /** * Checks to see if a provided key is the exact configured key of our connected subscriber * or if the provided key is a collection member key (in case our configured key is a "collection key") From 5b007156caaf685b63fac209b87a30fda033c023 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Hanno=20J=2E=20G=C3=B6decke?= Date: Wed, 19 Jul 2023 08:43:09 +0200 Subject: [PATCH 18/43] Update lib/Onyx.js Co-authored-by: Marc Glasser --- lib/Onyx.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/Onyx.js b/lib/Onyx.js index e032f68d..9b968eeb 100644 --- a/lib/Onyx.js +++ b/lib/Onyx.js @@ -643,7 +643,7 @@ function sendDataToConnection(mapping, val, matchedKey) { } } - // sendDataToConnection only get's called in the connect code. + // sendDataToConnection only gets called in the connect code. // withOnyx is already setting data from cache initially. If we now // want to call sendDataToConnection, we want to make sure, we // are not sending again the same data from cache, if the component From 8a3bc95d8b5954b53998f88eaf0ea5d576556e7c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Hanno=20J=2E=20G=C3=B6decke?= Date: Wed, 19 Jul 2023 08:43:18 +0200 Subject: [PATCH 19/43] Update lib/Onyx.js Co-authored-by: Marc Glasser --- lib/Onyx.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/Onyx.js b/lib/Onyx.js index 9b968eeb..29ff029e 100644 --- a/lib/Onyx.js +++ b/lib/Onyx.js @@ -208,7 +208,7 @@ function tryGetCachedValue(key, mapping = {}) { return finalObject; }, {}); if (_.keys(values).length === 0) { - return undefined; + return; } val = values; } From bf48eb3a96b517d1ac61fd9cb6a43b776d90b58d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Hanno=20J=2E=20G=C3=B6decke?= Date: Wed, 19 Jul 2023 13:29:40 +0200 Subject: [PATCH 20/43] simplify cached state usement --- lib/withOnyx.js | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/lib/withOnyx.js b/lib/withOnyx.js index 1b6bcab0..77f48215 100644 --- a/lib/withOnyx.js +++ b/lib/withOnyx.js @@ -55,10 +55,8 @@ export default function (mapOnyxToState) { // Object holding the temporary initial state for the component while we load the various Onyx keys this.tempState = cachedState; - this.state = { - loading, - ...(loading ? {} : cachedState), - }; + cachedState.loading = loading; + this.state = cachedState; } componentDidMount() { From fbc16fc917a41274fdeb42e07b8c2684986a58be Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Hanno=20J=2E=20G=C3=B6decke?= Date: Wed, 19 Jul 2023 13:59:22 +0200 Subject: [PATCH 21/43] remove reundand code --- lib/Onyx.js | 2 -- lib/withOnyx.js | 2 +- 2 files changed, 1 insertion(+), 3 deletions(-) diff --git a/lib/Onyx.js b/lib/Onyx.js index 29ff029e..5429e5b3 100644 --- a/lib/Onyx.js +++ b/lib/Onyx.js @@ -612,8 +612,6 @@ function keyChanged(key, data, canUpdateSubscriber) { * - sets state on the withOnyxInstances * - triggers the callback function * - * Note: This is only invoked from Onyx.connect - * * @private * @param {Object} mapping * @param {Object} [mapping.withOnyxInstance] diff --git a/lib/withOnyx.js b/lib/withOnyx.js index 77f48215..d8eefc10 100644 --- a/lib/withOnyx.js +++ b/lib/withOnyx.js @@ -101,7 +101,7 @@ export default function (mapOnyxToState) { * @param {*} val */ setWithOnyxState(statePropertyName, val) { - if (!this.state.loading && this.state[statePropertyName] !== val) { + if (!this.state.loading) { this.setState({[statePropertyName]: val}); return; } From 48570e8eb7eebb6aabdc43a86e503686bdd74321 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Hanno=20J=2E=20G=C3=B6decke?= Date: Wed, 19 Jul 2023 14:06:46 +0200 Subject: [PATCH 22/43] avoid too many deepEqual calls --- lib/Onyx.js | 17 +++++++++++------ 1 file changed, 11 insertions(+), 6 deletions(-) diff --git a/lib/Onyx.js b/lib/Onyx.js index 5429e5b3..d78252f0 100644 --- a/lib/Onyx.js +++ b/lib/Onyx.js @@ -629,6 +629,7 @@ function sendDataToConnection(mapping, val, matchedKey) { } if (mapping.withOnyxInstance) { + const prevData = mapping.withOnyxInstance.state[mapping.statePropertyName]; let newData = val; // If the mapping has a selector, then the component's state must only be updated with the data @@ -639,14 +640,18 @@ function sendDataToConnection(mapping, val, matchedKey) { } else { newData = getSubsetOfData(val, mapping.selector, mapping.withOnyxInstance.state); } + + // The data might have been loaded from cache alrady, + // in which case we want to avoid triggering a re-render. + // As the data is coming from a selector, we need to use deepEqual. + if (deepEqual(prevData, newData)) { + return; + } } - // sendDataToConnection only gets called in the connect code. - // withOnyx is already setting data from cache initially. If we now - // want to call sendDataToConnection, we want to make sure, we - // are not sending again the same data from cache, if the component - // already has the cached data. - if (deepEqual(mapping.withOnyxInstance.state[mapping.statePropertyName], newData)) { + // The data might have been loaded from cache alrady, + // in which case we want to avoid triggering a re-render. + if (mapping.withOnyxInstance.state[mapping.statePropertyName] === newData) { return; } From 68c3d04d28ef263a5d2958c308ed3da891fcd591 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Hanno=20J=2E=20G=C3=B6decke?= Date: Wed, 19 Jul 2023 14:09:48 +0200 Subject: [PATCH 23/43] remove other perf fix --- lib/Onyx.js | 11 ++++------- 1 file changed, 4 insertions(+), 7 deletions(-) diff --git a/lib/Onyx.js b/lib/Onyx.js index d78252f0..c52a070b 100644 --- a/lib/Onyx.js +++ b/lib/Onyx.js @@ -561,13 +561,10 @@ function keyChanged(key, data, canUpdateSubscriber) { ...collection, [key]: data, }; - if (!deepEqual(collection, newCollection)) { - PerformanceUtils.logSetStateCall(subscriber, collection, newCollection, 'keyChanged', key); - return { - [subscriber.statePropertyName]: newCollection, - }; - } - return null; + PerformanceUtils.logSetStateCall(subscriber, collection, newCollection, 'keyChanged', key); + return { + [subscriber.statePropertyName]: newCollection, + }; }); continue; } From 937c3f79209c83510d73d6ed1cb5d84c8e68453f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Hanno=20J=2E=20G=C3=B6decke?= Date: Wed, 19 Jul 2023 14:12:22 +0200 Subject: [PATCH 24/43] clean --- lib/Onyx.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/Onyx.js b/lib/Onyx.js index c52a070b..25a44207 100644 --- a/lib/Onyx.js +++ b/lib/Onyx.js @@ -648,7 +648,7 @@ function sendDataToConnection(mapping, val, matchedKey) { // The data might have been loaded from cache alrady, // in which case we want to avoid triggering a re-render. - if (mapping.withOnyxInstance.state[mapping.statePropertyName] === newData) { + if (prevData === newData) { return; } From cb505e322a705354b6fd11d9f920bcad8c3b7046 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Hanno=20J=2E=20G=C3=B6decke?= Date: Wed, 19 Jul 2023 14:17:51 +0200 Subject: [PATCH 25/43] remove all premature deepEquals --- lib/Onyx.js | 9 ++------- 1 file changed, 2 insertions(+), 7 deletions(-) diff --git a/lib/Onyx.js b/lib/Onyx.js index 25a44207..300ae3bc 100644 --- a/lib/Onyx.js +++ b/lib/Onyx.js @@ -421,18 +421,13 @@ function keysChanged(collectionKey, partialCollection) { } subscriber.withOnyxInstance.setState((prevState) => { - const prevCollection = prevState[subscriber.statePropertyName]; - const finalCollection = _.clone(prevCollection || {}); + const finalCollection = _.clone(prevState[subscriber.statePropertyName] || {}); const dataKeys = _.keys(partialCollection); for (let j = 0; j < dataKeys.length; j++) { const dataKey = dataKeys[j]; finalCollection[dataKey] = cachedCollection[dataKey]; } - if (deepEqual(prevCollection, finalCollection)) { - return null; - } - PerformanceUtils.logSetStateCall(subscriber, prevState[subscriber.statePropertyName], finalCollection, 'keysChanged', collectionKey); return { [subscriber.statePropertyName]: finalCollection, @@ -472,7 +467,7 @@ function keysChanged(collectionKey, partialCollection) { subscriber.withOnyxInstance.setState((prevState) => { const data = cachedCollection[subscriber.key]; const previousData = prevState[subscriber.statePropertyName]; - if (deepEqual(data, previousData)) { + if (data === previousData) { return null; } From 46f73e162a227008d4baec73d93b7283c22290f4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Hanno=20J=2E=20G=C3=B6decke?= Date: Wed, 19 Jul 2023 14:29:41 +0200 Subject: [PATCH 26/43] simplify --- lib/withOnyx.js | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/lib/withOnyx.js b/lib/withOnyx.js index d8eefc10..61f357bd 100644 --- a/lib/withOnyx.js +++ b/lib/withOnyx.js @@ -50,12 +50,11 @@ export default function (mapOnyxToState) { }); // If we have all the data we need, then we can render the component immediately - const loading = _.keys(cachedState).length < requiredKeysForInit.length; + cachedState.loading = _.keys(cachedState).length < requiredKeysForInit.length; // Object holding the temporary initial state for the component while we load the various Onyx keys this.tempState = cachedState; - cachedState.loading = loading; this.state = cachedState; } From a2e20d0dcb8ceb053c06b3efb6ada9d18db2ce64 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Hanno=20J=2E=20G=C3=B6decke?= Date: Wed, 19 Jul 2023 17:57:01 +0200 Subject: [PATCH 27/43] remove checks in onyx for prev state as its withOnyxInstance responsibility to check for that --- lib/Onyx.js | 14 -------------- lib/withOnyx.js | 2 +- 2 files changed, 1 insertion(+), 15 deletions(-) diff --git a/lib/Onyx.js b/lib/Onyx.js index 300ae3bc..5d126952 100644 --- a/lib/Onyx.js +++ b/lib/Onyx.js @@ -621,7 +621,6 @@ function sendDataToConnection(mapping, val, matchedKey) { } if (mapping.withOnyxInstance) { - const prevData = mapping.withOnyxInstance.state[mapping.statePropertyName]; let newData = val; // If the mapping has a selector, then the component's state must only be updated with the data @@ -632,19 +631,6 @@ function sendDataToConnection(mapping, val, matchedKey) { } else { newData = getSubsetOfData(val, mapping.selector, mapping.withOnyxInstance.state); } - - // The data might have been loaded from cache alrady, - // in which case we want to avoid triggering a re-render. - // As the data is coming from a selector, we need to use deepEqual. - if (deepEqual(prevData, newData)) { - return; - } - } - - // The data might have been loaded from cache alrady, - // in which case we want to avoid triggering a re-render. - if (prevData === newData) { - return; } PerformanceUtils.logSetStateCall(mapping, null, newData, 'sendDataToConnection'); diff --git a/lib/withOnyx.js b/lib/withOnyx.js index 61f357bd..070cc612 100644 --- a/lib/withOnyx.js +++ b/lib/withOnyx.js @@ -58,6 +58,7 @@ export default function (mapOnyxToState) { this.state = cachedState; } + // TODO: I think we don't have to wait until mount to subscribe to Onyx keys. We can do it in the constructor as well. componentDidMount() { // Subscribe each of the state properties to the proper Onyx key _.each(mapOnyxToState, (mapping, propertyName) => { @@ -101,7 +102,6 @@ export default function (mapOnyxToState) { */ setWithOnyxState(statePropertyName, val) { if (!this.state.loading) { - this.setState({[statePropertyName]: val}); return; } From 617ff64b2e410edb19cf90c06864a11bdb821dd7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Hanno=20J=2E=20G=C3=B6decke?= Date: Wed, 19 Jul 2023 19:39:43 +0200 Subject: [PATCH 28/43] fix too many state updates --- lib/withOnyx.js | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/lib/withOnyx.js b/lib/withOnyx.js index 070cc612..24606641 100644 --- a/lib/withOnyx.js +++ b/lib/withOnyx.js @@ -101,7 +101,16 @@ export default function (mapOnyxToState) { * @param {*} val */ setWithOnyxState(statePropertyName, val) { + // This method gets called by Onyx.connect no matter if the cached + // data is already there or not. Thus we have to check whether + // the data has changed or not before we set the state. + const prevValue = this.state[statePropertyName]; + if (!this.state.loading && prevValue === val) { + return; + } + if (!this.state.loading) { + this.setState({[statePropertyName]: val}); return; } From 5ffdefa22dcfda070255fa8bc107a08ad67e71a5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Hanno=20J=2E=20G=C3=B6decke?= Date: Tue, 25 Jul 2023 08:50:47 +0200 Subject: [PATCH 29/43] remove comments and todos --- lib/withOnyx.js | 2 -- 1 file changed, 2 deletions(-) diff --git a/lib/withOnyx.js b/lib/withOnyx.js index 24606641..acb0c808 100644 --- a/lib/withOnyx.js +++ b/lib/withOnyx.js @@ -58,7 +58,6 @@ export default function (mapOnyxToState) { this.state = cachedState; } - // TODO: I think we don't have to wait until mount to subscribe to Onyx keys. We can do it in the constructor as well. componentDidMount() { // Subscribe each of the state properties to the proper Onyx key _.each(mapOnyxToState, (mapping, propertyName) => { @@ -71,7 +70,6 @@ export default function (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, propertyName) => { - // Check if we can update from cache already const previousKey = Str.result(mapping.key, prevProps); const newKey = Str.result(mapping.key, this.props); if (previousKey !== newKey) { From 2c45edb4993a93a5adfb358b53ee5a5b720f2e91 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Hanno=20J=2E=20G=C3=B6decke?= Date: Tue, 25 Jul 2023 10:32:11 +0200 Subject: [PATCH 30/43] manually update the loading state --- lib/withOnyx.js | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/lib/withOnyx.js b/lib/withOnyx.js index acb0c808..547ac521 100644 --- a/lib/withOnyx.js +++ b/lib/withOnyx.js @@ -119,7 +119,17 @@ export default function (mapOnyxToState) { return; } - this.setState({...this.tempState, loading: false}); + const stateUpdate = {...this.tempState, loading: false}; + + // Why are we setting the state here manually, instead of just using setState? + // Explanation: setState is a async operation, meaning it might execute on the next task, + // or to a later point in the microtask queue. That means, that there is a chance that + // setWithOnyxState is called again before the state is actually set. This causes issues + // with the above checks for the loading state. + // Note: This behaviour has been mainly observed on fabric. + this.state = stateUpdate; + + this.setState(stateUpdate); // Trigger a render delete this.tempState; } From d479c12c421cf5e1a5078370dab16c1ed4957881 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Hanno=20J=2E=20G=C3=B6decke?= Date: Tue, 25 Jul 2023 15:57:22 +0200 Subject: [PATCH 31/43] add condition to check for cache usage --- tests/components/ViewWithText.js | 20 +++++++++++++++----- tests/unit/withOnyxTest.js | 14 +++++++++++++- 2 files changed, 28 insertions(+), 6 deletions(-) diff --git a/tests/components/ViewWithText.js b/tests/components/ViewWithText.js index e1660ddf..e72b424c 100644 --- a/tests/components/ViewWithText.js +++ b/tests/components/ViewWithText.js @@ -5,13 +5,23 @@ import {View, Text} from 'react-native'; const propTypes = { text: PropTypes.string.isRequired, + onRender: PropTypes.func, }; -const ViewWithText = props => ( - - {props.text} - -); +const defaultProps = { + onRender: () => {}, +}; + +const ViewWithText = (props) => { + props.onRender(); + + return ( + + {props.text} + + ); +}; ViewWithText.propTypes = propTypes; +ViewWithText.defaultProps = defaultProps; export default ViewWithText; diff --git a/tests/unit/withOnyxTest.js b/tests/unit/withOnyxTest.js index 5805c963..b53476b5 100644 --- a/tests/unit/withOnyxTest.js +++ b/tests/unit/withOnyxTest.js @@ -24,6 +24,8 @@ beforeEach(() => Onyx.clear()); describe('withOnyx', () => { it('should render immediately with the test data when using withOnyx', () => { + const onRender = jest.fn(); + // Note: earlier, after rendering a component we had to do another // waitForPromisesToResolve() to wait for Onyx's next tick updating // the component from {loading: true} to {loading:false, ...data}. @@ -37,9 +39,19 @@ describe('withOnyx', () => { }, })(ViewWithText); - const result = render(); + const result = render(); const textComponent = result.getByText('test1'); expect(textComponent).not.toBeNull(); + + jest.runAllTimers(); + return waitForPromisesToResolve(); + }) + .then(() => { + // As the component immediately rendered from cache, we want to make + // sure it wasn't updated a second time with the same value (the connect + // calls gets the data another time and tries to forward it to the component, + // which could cause a re-render if the right checks aren't in place): + expect(onRender).toHaveBeenCalledTimes(1); }); }); From 0756a277608ccb6e99525a3a25d1f56d7bd0f89d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Hanno=20J=2E=20G=C3=B6decke?= Date: Tue, 25 Jul 2023 16:06:19 +0200 Subject: [PATCH 32/43] remove now obsolete eslint warnings --- tests/unit/onyxCacheTest.js | 1 - tests/unit/subscribeToPropertiesTest.js | 1 - tests/unit/withOnyxTest.js | 1 - 3 files changed, 3 deletions(-) diff --git a/tests/unit/onyxCacheTest.js b/tests/unit/onyxCacheTest.js index d72e3c44..3ffbb4da 100644 --- a/tests/unit/onyxCacheTest.js +++ b/tests/unit/onyxCacheTest.js @@ -1,4 +1,3 @@ -/* eslint-disable rulesdir/onyx-props-must-have-default */ import React from 'react'; import {render} from '@testing-library/react-native'; import _ from 'underscore'; diff --git a/tests/unit/subscribeToPropertiesTest.js b/tests/unit/subscribeToPropertiesTest.js index 27017ad6..af13b73e 100644 --- a/tests/unit/subscribeToPropertiesTest.js +++ b/tests/unit/subscribeToPropertiesTest.js @@ -1,4 +1,3 @@ -/* eslint-disable rulesdir/onyx-props-must-have-default */ import React from 'react'; import {render, cleanup} from '@testing-library/react-native'; import Onyx, {withOnyx} from '../../lib'; diff --git a/tests/unit/withOnyxTest.js b/tests/unit/withOnyxTest.js index fd2f0000..b53476b5 100644 --- a/tests/unit/withOnyxTest.js +++ b/tests/unit/withOnyxTest.js @@ -1,4 +1,3 @@ -/* eslint-disable rulesdir/onyx-props-must-have-default */ import React from 'react'; import {render} from '@testing-library/react-native'; import Onyx, {withOnyx} from '../../lib'; From 7904d58a069c9cd60fee70c31b18b02f6abe7a98 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Hanno=20J=2E=20G=C3=B6decke?= Date: Tue, 25 Jul 2023 16:19:26 +0200 Subject: [PATCH 33/43] Update tests/unit/withOnyxTest.js Co-authored-by: Vit Horacek <36083550+mountiny@users.noreply.github.com> --- tests/unit/withOnyxTest.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/unit/withOnyxTest.js b/tests/unit/withOnyxTest.js index b53476b5..8bdde31f 100644 --- a/tests/unit/withOnyxTest.js +++ b/tests/unit/withOnyxTest.js @@ -274,7 +274,7 @@ describe('withOnyx', () => { render(); }) - // When a single item in the collection is updated with mergeCollect() + // When a single item in the collection is updated with mergeCollection() .then(() => Onyx.mergeCollection(ONYX_KEYS.COLLECTION.TEST_KEY, { test_1: {ID: 1, newProperty: 'yay'}, })) From c7ac445ff84b8f83a3e686090f48af9766ba6280 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Hanno=20J=2E=20G=C3=B6decke?= Date: Tue, 25 Jul 2023 16:21:50 +0200 Subject: [PATCH 34/43] fix lint --- tests/components/ViewWithText.js | 4 ++-- tests/unit/onyxCacheTest.js | 1 + tests/unit/subscribeToPropertiesTest.js | 1 + tests/unit/withOnyxTest.js | 1 + 4 files changed, 5 insertions(+), 2 deletions(-) diff --git a/tests/components/ViewWithText.js b/tests/components/ViewWithText.js index e72b424c..e172f295 100644 --- a/tests/components/ViewWithText.js +++ b/tests/components/ViewWithText.js @@ -12,7 +12,7 @@ const defaultProps = { onRender: () => {}, }; -const ViewWithText = (props) => { +function ViewWithText(props) { props.onRender(); return ( @@ -20,7 +20,7 @@ const ViewWithText = (props) => { {props.text} ); -}; +} ViewWithText.propTypes = propTypes; ViewWithText.defaultProps = defaultProps; diff --git a/tests/unit/onyxCacheTest.js b/tests/unit/onyxCacheTest.js index 3ffbb4da..d72e3c44 100644 --- a/tests/unit/onyxCacheTest.js +++ b/tests/unit/onyxCacheTest.js @@ -1,3 +1,4 @@ +/* eslint-disable rulesdir/onyx-props-must-have-default */ import React from 'react'; import {render} from '@testing-library/react-native'; import _ from 'underscore'; diff --git a/tests/unit/subscribeToPropertiesTest.js b/tests/unit/subscribeToPropertiesTest.js index af13b73e..27017ad6 100644 --- a/tests/unit/subscribeToPropertiesTest.js +++ b/tests/unit/subscribeToPropertiesTest.js @@ -1,3 +1,4 @@ +/* eslint-disable rulesdir/onyx-props-must-have-default */ import React from 'react'; import {render, cleanup} from '@testing-library/react-native'; import Onyx, {withOnyx} from '../../lib'; diff --git a/tests/unit/withOnyxTest.js b/tests/unit/withOnyxTest.js index 8bdde31f..c0397cf1 100644 --- a/tests/unit/withOnyxTest.js +++ b/tests/unit/withOnyxTest.js @@ -1,3 +1,4 @@ +/* eslint-disable rulesdir/onyx-props-must-have-default */ import React from 'react'; import {render} from '@testing-library/react-native'; import Onyx, {withOnyx} from '../../lib'; From 127f56c18bc82397ee2d07f6fed79c15d1f893f5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Hanno=20J=2E=20G=C3=B6decke?= Date: Tue, 25 Jul 2023 17:10:44 +0200 Subject: [PATCH 35/43] Update lib/withOnyx.js Co-authored-by: Tim Golen --- lib/withOnyx.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/withOnyx.js b/lib/withOnyx.js index 547ac521..325c4421 100644 --- a/lib/withOnyx.js +++ b/lib/withOnyx.js @@ -50,7 +50,7 @@ export default function (mapOnyxToState) { }); // If we have all the data we need, then we can render the component immediately - cachedState.loading = _.keys(cachedState).length < requiredKeysForInit.length; + cachedState.loading = _.size(cachedState) < requiredKeysForInit.length; // Object holding the temporary initial state for the component while we load the various Onyx keys this.tempState = cachedState; From 9a4135f71f2018087d8146f829f8b56e66d8ce52 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Hanno=20J=2E=20G=C3=B6decke?= Date: Tue, 25 Jul 2023 17:10:57 +0200 Subject: [PATCH 36/43] Update lib/Onyx.js Co-authored-by: Tim Golen --- lib/Onyx.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/Onyx.js b/lib/Onyx.js index 5baa84bc..7b0b2e07 100644 --- a/lib/Onyx.js +++ b/lib/Onyx.js @@ -210,7 +210,7 @@ function tryGetCachedValue(key, mapping = {}) { } return finalObject; }, {}); - if (_.keys(values).length === 0) { + if (_.size(values) === 0) { return; } val = values; From 9a41243fe65d04e6797e4be0d0d2342a6696af8f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Hanno=20J=2E=20G=C3=B6decke?= Date: Tue, 25 Jul 2023 17:11:46 +0200 Subject: [PATCH 37/43] Update lib/withOnyx.js Co-authored-by: Tim Golen --- lib/withOnyx.js | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/lib/withOnyx.js b/lib/withOnyx.js index 325c4421..74c8e80e 100644 --- a/lib/withOnyx.js +++ b/lib/withOnyx.js @@ -121,12 +121,9 @@ export default function (mapOnyxToState) { const stateUpdate = {...this.tempState, loading: false}; - // Why are we setting the state here manually, instead of just using setState? - // Explanation: setState is a async operation, meaning it might execute on the next task, - // or to a later point in the microtask queue. That means, that there is a chance that - // setWithOnyxState is called again before the state is actually set. This causes issues - // with the above checks for the loading state. - // Note: This behaviour has been mainly observed on fabric. + // The state is set here manually, instead of using setState because setState is an async operation, meaning it might execute on the next tick, + // or at a later point in the microtask queue. That can lead to a race condition where + // setWithOnyxState is called before the state is actually set. This results in unreliable behavior when checking the loading state and has been mainly observed on fabric. this.state = stateUpdate; this.setState(stateUpdate); // Trigger a render From cdb9e8214676364f079381613cac868947b8a210 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Hanno=20J=2E=20G=C3=B6decke?= Date: Tue, 25 Jul 2023 17:16:20 +0200 Subject: [PATCH 38/43] use reduce --- lib/withOnyx.js | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/lib/withOnyx.js b/lib/withOnyx.js index 74c8e80e..f4781116 100644 --- a/lib/withOnyx.js +++ b/lib/withOnyx.js @@ -37,17 +37,17 @@ export default function (mapOnyxToState) { // disconnected. It is a key value store with the format {[mapping.key]: connectionID}. this.activeConnectionIDs = {}; - const cachedState = {}; - - // Check if we can get all the data synchronously from cache - _.each(mapOnyxToState, (mapping, propertyName) => { + const cachedState = _.reduce(mapOnyxToState, (resultObj, mapping, propertyName) => { const key = Str.result(mapping.key, props); const value = Onyx.tryGetCachedValue(key, mapping); if (value !== undefined) { - cachedState[propertyName] = value; + // eslint-disable-next-line no-param-reassign + resultObj[propertyName] = value; } - }); + + return resultObj; + }, {}); // If we have all the data we need, then we can render the component immediately cachedState.loading = _.size(cachedState) < requiredKeysForInit.length; From 1caf3f950a77667b5101f239ee05b903ea30efdd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Hanno=20J=2E=20G=C3=B6decke?= Date: Tue, 25 Jul 2023 17:18:15 +0200 Subject: [PATCH 39/43] fix documentation --- lib/Onyx.js | 11 ++++------- 1 file changed, 4 insertions(+), 7 deletions(-) diff --git a/lib/Onyx.js b/lib/Onyx.js index 7b0b2e07..71da1b55 100644 --- a/lib/Onyx.js +++ b/lib/Onyx.js @@ -185,15 +185,12 @@ function isSafeEvictionKey(testKey) { } /** - * Tries to get a value from the cache. - * If the value is not present in cache it will - * return the default value or undefined. - * If the requested key is a collection, it will - * return an object with all the collection members. + * Tries to get a value from the cache. If the value is not present in cache it will return the default value or undefined. + * If the requested key is a collection, it will return an object with all the collection members. * - * @param {string} key + * @param {String} key * @param {Object} mapping - * @returns {string} + * @returns {Mixed} */ function tryGetCachedValue(key, mapping = {}) { let val = cache.hasCacheForKey(key) ? cache.getValue(key) : defaultKeyStates[key]; From ad4758de6864ea2d3d7aa6c4ef606b38b237c9dd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Hanno=20J=2E=20G=C3=B6decke?= Date: Tue, 25 Jul 2023 17:23:12 +0200 Subject: [PATCH 40/43] update documentation --- lib/withOnyx.js | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/lib/withOnyx.js b/lib/withOnyx.js index f4781116..8c5196b5 100644 --- a/lib/withOnyx.js +++ b/lib/withOnyx.js @@ -99,9 +99,11 @@ export default function (mapOnyxToState) { * @param {*} val */ setWithOnyxState(statePropertyName, val) { - // This method gets called by Onyx.connect no matter if the cached - // data is already there or not. Thus we have to check whether - // the data has changed or not before we set the state. + // We might have loaded the values for the onyx keys/mappings already from the cache. + // In case we were able to load all the values upfront, the loading state will be false. + // However, Onyx.js will always call setWithOnyxState, as it doesn't now that this implementation + // already loaded the values from cache. Thus we have to check whether the value has changed + // before we set the state to prevent unnecessary renders. const prevValue = this.state[statePropertyName]; if (!this.state.loading && prevValue === val) { return; From e5312700ccf04a3a0b7e04f81d1137e55fe71154 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Hanno=20J=2E=20G=C3=B6decke?= Date: Tue, 25 Jul 2023 18:59:49 +0200 Subject: [PATCH 41/43] Update lib/withOnyx.js Co-authored-by: Tim Golen --- lib/withOnyx.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/withOnyx.js b/lib/withOnyx.js index 8c5196b5..7dc8e5a4 100644 --- a/lib/withOnyx.js +++ b/lib/withOnyx.js @@ -101,7 +101,7 @@ export default function (mapOnyxToState) { setWithOnyxState(statePropertyName, val) { // We might have loaded the values for the onyx keys/mappings already from the cache. // In case we were able to load all the values upfront, the loading state will be false. - // However, Onyx.js will always call setWithOnyxState, as it doesn't now that this implementation + // However, Onyx.js will always call setWithOnyxState, as it doesn't know that this implementation // already loaded the values from cache. Thus we have to check whether the value has changed // before we set the state to prevent unnecessary renders. const prevValue = this.state[statePropertyName]; From 69fac6026b6fb891adf9bfd69304cd0cee4d90be Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Hanno=20J=2E=20G=C3=B6decke?= Date: Wed, 26 Jul 2023 08:37:48 +0200 Subject: [PATCH 42/43] Update lib/Onyx.js Co-authored-by: Carlos Martins --- lib/Onyx.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/Onyx.js b/lib/Onyx.js index 71da1b55..0fca676b 100644 --- a/lib/Onyx.js +++ b/lib/Onyx.js @@ -207,7 +207,7 @@ function tryGetCachedValue(key, mapping = {}) { } return finalObject; }, {}); - if (_.size(values) === 0) { + if (_.isEmpty(values)) { return; } val = values; From ff82a94e4d5981d98ce8babff087172eabf86813 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Hanno=20J=2E=20G=C3=B6decke?= Date: Wed, 26 Jul 2023 08:41:15 +0200 Subject: [PATCH 43/43] Use cache directly without falling back to default state --- lib/Onyx.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/Onyx.js b/lib/Onyx.js index 0fca676b..a79cccb1 100644 --- a/lib/Onyx.js +++ b/lib/Onyx.js @@ -193,7 +193,7 @@ function isSafeEvictionKey(testKey) { * @returns {Mixed} */ function tryGetCachedValue(key, mapping = {}) { - let val = cache.hasCacheForKey(key) ? cache.getValue(key) : defaultKeyStates[key]; + let val = cache.getValue(key); if (isCollectionKey(key)) { const allKeys = cache.getAllKeys();