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

Call setState once when handling initial withOnyx data to reduce JS scripting time #97

Merged
merged 6 commits into from
Aug 13, 2021
Merged
Show file tree
Hide file tree
Changes from 5 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
4 changes: 1 addition & 3 deletions lib/Onyx.js
Original file line number Diff line number Diff line change
Expand Up @@ -328,9 +328,7 @@ function keyChanged(key, data, hasNewValue = true) {
*/
function sendDataToConnection(config, val, key) {
if (config.withOnyxInstance) {
config.withOnyxInstance.setState({
[config.statePropertyName]: val,
});
config.withOnyxInstance.setInitialState(config.statePropertyName, val);
} else if (_.isFunction(config.callback)) {
config.callback(val, key);
}
Expand Down
56 changes: 35 additions & 21 deletions lib/withOnyx.js
Original file line number Diff line number Diff line change
Expand Up @@ -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,
};
Expand All @@ -39,7 +50,7 @@ export default function (mapOnyxToState) {
_.each(mapOnyxToState, (mapping, propertyName) => {
this.connectMappingToOnyx(mapping, propertyName);
});
this.checkAndUpdateLoading();
this.checkEvictableKeys();
}

componentDidUpdate(prevProps) {
Expand All @@ -55,7 +66,7 @@ export default function (mapOnyxToState) {
this.connectMappingToOnyx(mapping, propertyName);
}
});
this.checkAndUpdateLoading();
this.checkEvictableKeys();
}

componentWillUnmount() {
Expand All @@ -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;
Copy link
Contributor

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 to false


// All state keys should exist and at least have a value of null
if (!_.every(requiredKeysForInit, key => !_.isUndefined(this.tempState[key]))) {
return;
}
Copy link
Contributor

Choose a reason for hiding this comment

The 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 = {};
});
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}

/**
* 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.
Expand All @@ -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});
}
}

/**
Expand Down
31 changes: 21 additions & 10 deletions tests/unit/withOnyxTest.js
Original file line number Diff line number Diff line change
Expand Up @@ -68,9 +68,13 @@ describe('withOnyx', () => {
})(ViewWithCollections);
const onRender = jest.fn();
render(<TestComponentWithOnyx onRender={onRender} />);

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},
});
return waitForPromisesToResolve();
})
.then(() => {
expect(onRender.mock.calls.length).toBe(2);
});
Expand All @@ -85,9 +89,13 @@ describe('withOnyx', () => {
})(ViewWithCollections);
const onRender = jest.fn();
render(<TestComponentWithOnyx onRender={onRender} />);

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},
});
return waitForPromisesToResolve();
})
.then(() => {
expect(onRender.mock.calls.length).toBe(2);
});
Expand All @@ -102,13 +110,16 @@ describe('withOnyx', () => {
})(ViewWithCollections);
const onRender = jest.fn();
render(<TestComponentWithOnyx onRender={onRender} />);
Onyx.mergeCollection(ONYX_KEYS.COLLECTION.TEST_KEY, {test_4: {ID: 456}, test_5: {ID: 567}});
Onyx.mergeCollection(ONYX_KEYS.COLLECTION.TEST_KEY, {
test_4: {Name: 'Test4'},
test_5: {Name: 'Test5'},
test_6: {ID: 678, Name: 'Test6'},
});
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, {
test_4: {Name: 'Test4'},
test_5: {Name: 'Test5'},
test_6: {ID: 678, Name: 'Test6'},
});
return waitForPromisesToResolve();
})
.then(() => {
expect(onRender.mock.calls.length).toBe(3);
expect(onRender.mock.instances[2].text).toEqual({ID: 456, Name: 'Test4'});
Expand Down