From 115af5936b76cb17c4664840a1d86aae7d5b7555 Mon Sep 17 00:00:00 2001 From: Marc Glasser Date: Fri, 23 Sep 2022 13:28:39 -1000 Subject: [PATCH 1/2] Do not call setState() on a connected component if it does not need to update Add test add dequal and debug setState calls remove throw Add setState logging methods remove dequal for now new PR Only include difference not previous/new value Remove dequal stuff Fix test Change param to use so we dont clash with the metrics --- README.md | 3 ++ lib/Onyx.js | 46 ++++++++++++++++++----- lib/metrics/PerformanceUtils.js | 65 +++++++++++++++++++++++++++++++++ lib/withOnyx.js | 5 ++- tests/unit/withOnyxTest.js | 40 ++++++++++++++++++++ 5 files changed, 148 insertions(+), 11 deletions(-) create mode 100644 lib/metrics/PerformanceUtils.js diff --git a/README.md b/README.md index a5fa61da9..a72f81508 100644 --- a/README.md +++ b/README.md @@ -319,6 +319,9 @@ Sample output of `Onyx.printMetrics()` | 1.17sec | 2.20sec | 1.03sec | currentURL, / | ``` +# Debug mode + +It can be useful to log why Onyx is calling `setState()` on a particular React component so that we can understand which key changed, what changed about the value, and the connected component that ultimately rendered as a result. When used correctly this can help isolate problem areas and unnecessary renders in the code. To enable this feature, pass `debugSetState: true` to the config and grep JS console logs for `[Onyx-Debug]`. # Development diff --git a/lib/Onyx.js b/lib/Onyx.js index 0337555e8..f150257ad 100644 --- a/lib/Onyx.js +++ b/lib/Onyx.js @@ -7,6 +7,7 @@ import * as Logger from './Logger'; import cache from './OnyxCache'; import createDeferredTask from './createDeferredTask'; import mergeWithCustomized from './mergeWithCustomized'; +import * as PerformanceUtils from './metrics/PerformanceUtils'; // Keeps track of the last connectionID that was used so we can keep incrementing it let lastConnectionID = 0; @@ -323,12 +324,13 @@ function keysChanged(collectionKey, partialCollection) { if (isSubscribedToCollectionKey) { subscriber.withOnyxInstance.setState((prevState) => { 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]; } + + PerformanceUtils.logSetStateCall(subscriber, prevState[subscriber.statePropertyName], finalCollection, 'keysChanged', collectionKey); return { [subscriber.statePropertyName]: finalCollection, }; @@ -345,8 +347,17 @@ function keysChanged(collectionKey, partialCollection) { continue; } - subscriber.withOnyxInstance.setState({ - [subscriber.statePropertyName]: cachedCollection[subscriber.key], + subscriber.withOnyxInstance.setState((prevState) => { + const data = cachedCollection[subscriber.key]; + const previousData = prevState[subscriber.statePropertyName]; + if (data === previousData) { + return null; + } + + PerformanceUtils.logSetStateCall(subscriber, previousData, data, 'keysChanged', collectionKey); + return { + [subscriber.statePropertyName]: data, + }; }); } } @@ -397,19 +408,29 @@ function keyChanged(key, data) { if (isCollectionKey(subscriber.key)) { subscriber.withOnyxInstance.setState((prevState) => { const collection = prevState[subscriber.statePropertyName] || {}; + const newCollection = { + ...collection, + [key]: data, + }; + PerformanceUtils.logSetStateCall(subscriber, collection, newCollection, 'keyChanged', key); return { - [subscriber.statePropertyName]: { - ...collection, - [key]: data, - }, + [subscriber.statePropertyName]: newCollection, }; }); continue; } // If we did not match on a collection key then we just set the new data to the state property - subscriber.withOnyxInstance.setState({ - [subscriber.statePropertyName]: data, + subscriber.withOnyxInstance.setState((prevState) => { + const previousData = prevState[subscriber.statePropertyName]; + if (previousData === data) { + return null; + } + + PerformanceUtils.logSetStateCall(subscriber, previousData, data, 'keyChanged', key); + return { + [subscriber.statePropertyName]: data, + }; }); continue; } @@ -439,6 +460,7 @@ function sendDataToConnection(config, val, matchedKey) { } if (config.withOnyxInstance) { + PerformanceUtils.logSetStateCall(config, null, val, 'sendDataToConnection'); config.withOnyxInstance.setWithOnyxState(config.statePropertyName, val); } else if (_.isFunction(config.callback)) { config.callback(val, matchedKey); @@ -986,6 +1008,7 @@ function update(data) { * of Onyx running in different tabs/windows. Defaults to true for platforms that support local storage (web/desktop) * @param {String[]} [option.keysToDisableSyncEvents=[]] Contains keys for which * we want to disable sync event across tabs. + * @param {Boolean} [options.debugSetState] Enables debugging setState() calls to connected components. * @example * Onyx.init({ * keys: ONYXKEYS, @@ -1002,6 +1025,7 @@ function init({ captureMetrics = false, shouldSyncMultipleInstances = Boolean(global.localStorage), keysToDisableSyncEvents = [], + debugSetState = false, } = {}) { if (captureMetrics) { // The code here is only bundled and applied when the captureMetrics is set @@ -1009,6 +1033,10 @@ function init({ applyDecorators(); } + if (debugSetState) { + PerformanceUtils.setShouldDebugSetState(true); + } + if (maxCachedKeysCount > 0) { cache.setRecentKeysLimit(maxCachedKeysCount); } diff --git a/lib/metrics/PerformanceUtils.js b/lib/metrics/PerformanceUtils.js new file mode 100644 index 000000000..176f07b86 --- /dev/null +++ b/lib/metrics/PerformanceUtils.js @@ -0,0 +1,65 @@ +import lodashTransform from 'lodash/transform'; +import _ from 'underscore'; + +let debugSetState = false; + +/** + * @param {Boolean} debug + */ +function setShouldDebugSetState(debug) { + debugSetState = debug; +} + +/** + * Deep diff between two objects. Useful for figuring out what changed about an object from one render to the next so + * that state and props updates can be optimized. + * + * @param {Object} object + * @param {Object} base + * @return {Object} + */ +function diffObject(object, base) { + function changes(obj, comparisonObject) { + return lodashTransform(obj, (result, value, key) => { + if (_.isEqual(value, comparisonObject[key])) { + return; + } + + // eslint-disable-next-line no-param-reassign + result[key] = (_.isObject(value) && _.isObject(comparisonObject[key])) + ? changes(value, comparisonObject[key]) + : value; + }); + } + return changes(object, base); +} + +/** + * Provide insights into why a setState() call occurred by diffing the before and after values. + * + * @param {Object} mapping + * @param {*} previousValue + * @param {*} newValue + * @param {String} caller + * @param {String} keyThatChanged + */ +function logSetStateCall(mapping, previousValue, newValue, caller, keyThatChanged) { + if (!debugSetState) { + return; + } + + const logParams = {keyThatChanged}; + if (_.isObject(newValue) || _.isObject(previousValue)) { + logParams.difference = diffObject(previousValue, newValue); + } else { + logParams.previousValue = previousValue; + logParams.newValue = newValue; + } + + console.debug(`[Onyx-Debug] ${mapping.displayName} setState() called. Subscribed to key '${mapping.key}' (${caller})`, logParams); +} + +export { + logSetStateCall, + setShouldDebugSetState, +}; diff --git a/lib/withOnyx.js b/lib/withOnyx.js index aaefce797..f8154d748 100644 --- a/lib/withOnyx.js +++ b/lib/withOnyx.js @@ -25,8 +25,8 @@ export default function (mapOnyxToState) { .omit(config => config.initWithStoredValues === false) .keys() .value(); - return (WrappedComponent) => { + const displayName = getDisplayName(WrappedComponent); class withOnyx extends React.Component { constructor(props) { super(props); @@ -152,6 +152,7 @@ export default function (mapOnyxToState) { key, statePropertyName, withOnyxInstance: this, + displayName, }); } @@ -190,7 +191,7 @@ export default function (mapOnyxToState) { withOnyx.defaultProps = { forwardedRef: undefined, }; - withOnyx.displayName = `withOnyx(${getDisplayName(WrappedComponent)})`; + withOnyx.displayName = `withOnyx(${displayName})`; return React.forwardRef((props, ref) => { const Component = withOnyx; // eslint-disable-next-line react/jsx-props-no-spreading diff --git a/tests/unit/withOnyxTest.js b/tests/unit/withOnyxTest.js index 94efa1f08..ea5a5b535 100644 --- a/tests/unit/withOnyxTest.js +++ b/tests/unit/withOnyxTest.js @@ -12,6 +12,7 @@ const ONYX_KEYS = { TEST_KEY: 'test_', RELATED_KEY: 'related_', }, + SIMPLE_KEY: 'simple', }; Onyx.init({ @@ -328,4 +329,43 @@ describe('withOnyx', () => { expect(onRender.mock.calls[1][0].testObject).toStrictEqual({ID: 1, number: 2}); }); }); + + it('merge with a simple value should not update a connected component unless the data has changed', () => { + const onRender = jest.fn(); + + // Given there is a simple key that is not an array or object value + Onyx.merge(ONYX_KEYS.SIMPLE_KEY, 'string'); + + return waitForPromisesToResolve() + .then(() => { + // When a component subscribes to the simple key + const TestComponentWithOnyx = withOnyx({ + simple: { + key: ONYX_KEYS.SIMPLE_KEY, + }, + })(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(); + }) + .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(); + }) + .then(() => { + // THEN the component subscribed to the modified item should only render once + expect(onRender).toHaveBeenCalledTimes(2); + expect(onRender.mock.calls[1][0].simple).toBe('long_string'); + }); + }); }); From 993bd3032b866122945ccf58722635f04dd41bdb Mon Sep 17 00:00:00 2001 From: Marc Glasser Date: Tue, 4 Oct 2022 15:35:25 -1000 Subject: [PATCH 2/2] fix difference check --- lib/metrics/PerformanceUtils.js | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/lib/metrics/PerformanceUtils.js b/lib/metrics/PerformanceUtils.js index 176f07b86..ff2c801af 100644 --- a/lib/metrics/PerformanceUtils.js +++ b/lib/metrics/PerformanceUtils.js @@ -41,15 +41,18 @@ function diffObject(object, base) { * @param {*} previousValue * @param {*} newValue * @param {String} caller - * @param {String} keyThatChanged + * @param {String} [keyThatChanged] */ function logSetStateCall(mapping, previousValue, newValue, caller, keyThatChanged) { if (!debugSetState) { return; } - const logParams = {keyThatChanged}; - if (_.isObject(newValue) || _.isObject(previousValue)) { + const logParams = {}; + if (keyThatChanged) { + logParams.keyThatChanged = keyThatChanged; + } + if (_.isObject(newValue) && _.isObject(previousValue)) { logParams.difference = diffObject(previousValue, newValue); } else { logParams.previousValue = previousValue;