Call setState once when handling initial withOnyx data to reduce JS scripting time #97
Conversation
tests/unit/withOnyxTest.js
Outdated
|
|
||
| Onyx.mergeCollection(ONYX_KEYS.COLLECTION.TEST_KEY, {test_1: {ID: 123}, test_2: {ID: 234}, test_3: {ID: 345}}); | ||
| return waitForPromisesToResolve() | ||
| waitForPromisesToResolve() |
There was a problem hiding this comment.
I think these tests are fixed correctly. Did not observe any issues with mergeCollection() yet
lib/withOnyx.js
Outdated
| if (!_.every(requiredKeysForInit, key => !_.isUndefined(this.tempState[key]))) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
IMO something like this is more readable:
if (_.some(requiredKeys, key => _.isUndefined(this.tempState[key])) {
return;
}
lib/withOnyx.js
Outdated
| this.setState({...this.tempState, loading: false}, () => { | ||
| this.tempState = {}; | ||
| }); |
There was a problem hiding this comment.
We don't need to reset tempState in a callback
This is equivalent and more performant
this.setState({...this.tempState, loading: false});
this.tempState = {};Also we could delete this.tempState
lib/withOnyx.js
Outdated
| setInitialState(statePropertyName, val) { | ||
| this.tempState[statePropertyName] = val; |
There was a problem hiding this comment.
I think this method should throw an Error or at least print a warning and skip execution if it's called after loading is set to false
| setInitialState(statePropertyName, val) { | ||
| if (!this.state.loading) { | ||
| console.error('withOnyx.setInitialState() called after loading: false'); | ||
| return; | ||
| } |
There was a problem hiding this comment.
@marcaaron
Sorry we've made a mistake here
This method is not called only for initial state but for updates (new connections) made by the logic in componentDidUpdate: https://github.com/Expensify/react-native-onyx/pull/97/files#diff-364fd7349a1af7b52091427c5a0734da8ff56057fbc7fe2be10734a06c97264cL55
We should update the logic to address calls triggered by updates
setInitialState(statePropertyName, val) {
if (this.state.loading) {
this.tempState[statePropertyName] = val;
if (_.some(requiredKeysForInit, key => _.isUndefined(this.tempState[key]))) {
return;
}
this.setState({...this.tempState, loading: false});
delete this.tempState;
}
else {
this.setState({[statePropertyName]: val});
}
}Also we should maybe change the method name and description to reflect that it's not called only for the initial state
There was a problem hiding this comment.
Oh I see. The Onyx.connect() method may be called again. That solution makes sense to me.
There was a problem hiding this comment.
Thanks for. catching this one. Created a follow up here -> #98
Details
Implements the idea discussed in the link below cc @kidroca
Before change
After change:
Small difference of about 81ms here to total scripting time when the app first becomes active. But this should have positive affects in all other places where a
withOnyxconnection is set up.Related Issues
Expensify/App#4592 (comment)
Automated Tests
Linked PRs