-
Notifications
You must be signed in to change notification settings - Fork 74
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
Call setState once when handling initial withOnyx data to reduce JS scripting time #97
Changes from 5 commits
65888e2
6ba1be3
1fed363
31c8a40
fe2de12
9718d3c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -20,15 +20,26 @@ function getDisplayName(component) { | |
} | ||
|
||
export default function (mapOnyxToState) { | ||
// A list of keys that must be present in tempState before we can render the WrappedComponent | ||
const requiredKeysForInit = _.chain(mapOnyxToState) | ||
.omit(config => config.initWithStoredValues === false) | ||
.keys() | ||
.value(); | ||
|
||
return (WrappedComponent) => { | ||
class withOnyx extends React.Component { | ||
constructor(props) { | ||
super(props); | ||
|
||
this.setInitialState = this.setInitialState.bind(this); | ||
|
||
// This stores all the Onyx connection IDs to be used when the component unmounts so everything can be | ||
// disconnected. It is a key value store with the format {[mapping.key]: connectionID}. | ||
this.activeConnectionIDs = {}; | ||
|
||
// Object holding the temporary initial state for the component while we load the various Onyx keys | ||
this.tempState = {}; | ||
|
||
this.state = { | ||
loading: true, | ||
}; | ||
|
@@ -39,7 +50,7 @@ export default function (mapOnyxToState) { | |
_.each(mapOnyxToState, (mapping, propertyName) => { | ||
this.connectMappingToOnyx(mapping, propertyName); | ||
}); | ||
this.checkAndUpdateLoading(); | ||
this.checkEvictableKeys(); | ||
} | ||
|
||
componentDidUpdate(prevProps) { | ||
|
@@ -55,7 +66,7 @@ export default function (mapOnyxToState) { | |
this.connectMappingToOnyx(mapping, propertyName); | ||
} | ||
}); | ||
this.checkAndUpdateLoading(); | ||
this.checkEvictableKeys(); | ||
} | ||
|
||
componentWillUnmount() { | ||
|
@@ -67,12 +78,33 @@ export default function (mapOnyxToState) { | |
}); | ||
} | ||
|
||
/** | ||
* This method is used externally by sendDataToConnection to prevent unnecessary renders while a component | ||
* still in a loading state. The temporary initial state is saved to the component instance and setState() | ||
* only called once all the necessary data has been collected. | ||
* | ||
* @param {String} statePropertyName | ||
* @param {*} val | ||
*/ | ||
setInitialState(statePropertyName, val) { | ||
this.tempState[statePropertyName] = val; | ||
|
||
// All state keys should exist and at least have a value of null | ||
if (!_.every(requiredKeysForInit, key => !_.isUndefined(this.tempState[key]))) { | ||
return; | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. IMO something like this is more readable: if (_.some(requiredKeys, key => _.isUndefined(this.tempState[key])) {
return;
} |
||
|
||
this.setState({...this.tempState, loading: false}, () => { | ||
this.tempState = {}; | ||
}); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We don't need to reset this.setState({...this.tempState, loading: false});
this.tempState = {}; Also we could |
||
} | ||
|
||
/** | ||
* Makes sure each Onyx key we requested has been set to state with a value of some kind. | ||
* We are doing this so that the wrapped component will only render when all the data | ||
* it needs is available to it. | ||
*/ | ||
checkAndUpdateLoading() { | ||
checkEvictableKeys() { | ||
// We will add this key to our list of recently accessed keys | ||
// if the canEvict function returns true. This is necessary criteria | ||
// we MUST use to specify if a key can be removed or not. | ||
|
@@ -95,24 +127,6 @@ export default function (mapOnyxToState) { | |
Onyx.addToEvictionBlockList(key, mapping.connectionID); | ||
} | ||
}); | ||
|
||
if (!this.state.loading) { | ||
return; | ||
} | ||
|
||
// Filter all keys by those which we do want to init with stored values | ||
// since keys that are configured to not init with stored values will | ||
// never appear on state when the component mounts - only after they update | ||
// organically. | ||
const requiredKeysForInit = _.chain(mapOnyxToState) | ||
.omit(config => config.initWithStoredValues === false) | ||
.keys() | ||
.value(); | ||
|
||
// All state keys should exist and at least have a value of null | ||
if (_.every(requiredKeysForInit, key => !_.isUndefined(this.state[key]))) { | ||
this.setState({loading: false}); | ||
} | ||
} | ||
|
||
/** | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 tofalse