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.

Sign up for GitHub

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

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
65888e2
Call setState once when handling initial withOnyx data
marcaaron Aug 12, 2021
6ba1be3
update name
marcaaron Aug 12, 2021
1fed363
Fix failing tests
marcaaron Aug 12, 2021
31c8a40
only create requiredKeysForInit once
marcaaron Aug 12, 2021
fe2de12
return the promise in the test
marcaaron Aug 12, 2021
9718d3c
Make suggested changes
marcaaron Aug 12, 2021
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
View file
Open in desktop
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
60 changes: 39 additions & 21 deletions lib/withOnyx.js
View file
Open in desktop
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,37 @@ 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) {
if (!this.state.loading) {
console.error('withOnyx.setInitialState() called after loading: false');
return;
}
Comment on lines +89 to +93
Copy link
Contributor

@kidroca kidroca Aug 16, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

@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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh I see. The Onyx.connect() method may be called again. That solution makes sense to me.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for. catching this one. Created a follow up here -> #98


this.tempState[statePropertyName] = val;

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

this.setState({...this.tempState, loading: false});
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 +131,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
View file
Open in desktop
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