Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add logs to show why Onyx data is changing. Prevent updates when simple values change. #188

Merged
merged 3 commits into from
Oct 6, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down
46 changes: 37 additions & 9 deletions lib/Onyx.js
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import * as Logger from './Logger';
import cache from './OnyxCache';
import createDeferredTask from './createDeferredTask';
import fastMerge from './fastMerge';
import * as PerformanceUtils from './metrics/PerformanceUtils';

// Keeps track of the last connectionID that was used so we can keep incrementing it
let lastConnectionID = 0;
Expand Down Expand Up @@ -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,
};
Expand All @@ -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,
};
});
}
}
Expand Down Expand Up @@ -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;
}
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -988,6 +1010,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,
Expand All @@ -1004,13 +1027,18 @@ 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
// eslint-disable-next-line no-use-before-define
applyDecorators();
}

if (debugSetState) {
PerformanceUtils.setShouldDebugSetState(true);
}

if (maxCachedKeysCount > 0) {
cache.setRecentKeysLimit(maxCachedKeysCount);
}
Expand Down
68 changes: 68 additions & 0 deletions lib/metrics/PerformanceUtils.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,68 @@
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 = {};
if (keyThatChanged) {
logParams.keyThatChanged = 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,
};
5 changes: 3 additions & 2 deletions lib/withOnyx.js
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down Expand Up @@ -152,6 +152,7 @@ export default function (mapOnyxToState) {
key,
statePropertyName,
withOnyxInstance: this,
displayName,
});
}

Expand Down Expand Up @@ -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
Expand Down
40 changes: 40 additions & 0 deletions tests/unit/withOnyxTest.js
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ const ONYX_KEYS = {
TEST_KEY: 'test_',
RELATED_KEY: 'related_',
},
SIMPLE_KEY: 'simple',
};

Onyx.init({
Expand Down Expand Up @@ -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(<TestComponentWithOnyx onRender={onRender} />);

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');
});
});
});