Skip to content

Commit

Permalink
Merge pull request #280 from margelo/perf/fix-269-use-cached-data
Browse files Browse the repository at this point in the history 
[Fix #269] use cached data
  • Loading branch information
@mountiny
mountiny authored Jul 27, 2023
2 parents 41a1416 + ff82a94 commit 8e76a58
Show file tree
Hide file tree
Showing 5 changed files with 149 additions and 111 deletions.
41 changes: 41 additions & 0 deletions lib/Onyx.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -184,6 +184,46 @@ function isSafeEvictionKey(testKey) {
return _.some(evictionAllowList, key => isKeyMatch(key, testKey));
}

/**
* Tries to get a value from the cache. If the value is not present in cache it will return the default value or undefined.
* If the requested key is a collection, it will return an object with all the collection members.
*
* @param {String} key
* @param {Object} mapping
* @returns {Mixed}
*/
function tryGetCachedValue(key, mapping = {}) {
let val = cache.getValue(key);

if (isCollectionKey(key)) {
const allKeys = cache.getAllKeys();
const matchingKeys = _.filter(allKeys, k => k.startsWith(key));
const values = _.reduce(matchingKeys, (finalObject, matchedKey) => {
const cachedValue = cache.getValue(matchedKey);
if (cachedValue) {
// This is permissible because we're in the process of constructing the final object in a reduce function.
// eslint-disable-next-line no-param-reassign
finalObject[matchedKey] = cachedValue;
}
return finalObject;
}, {});
if (_.isEmpty(values)) {
return;
}
val = values;
}

if (mapping.selector) {
const state = mapping.withOnyxInstance ? mapping.withOnyxInstance.state : undefined;
if (isCollectionKey(key)) {
return reduceCollectionWithSelector(val, mapping.selector, state);
}
return getSubsetOfData(val, mapping.selector, state);
}

return val;
}

/**
* Remove a key from the recently accessed key list.
*
Expand Down Expand Up @@ -1353,6 +1393,7 @@ const Onyx = {
isSafeEvictionKey,
METHOD,
setMemoryOnlyKeys,
tryGetCachedValue,
};

/**
Expand Down
42 changes: 35 additions & 7 deletions lib/withOnyx.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -37,13 +37,25 @@ export default function (mapOnyxToState) {
// disconnected. It is a key value store with the format {[mapping.key]: connectionID}.
this.activeConnectionIDs = {};

const cachedState = _.reduce(mapOnyxToState, (resultObj, mapping, propertyName) => {
const key = Str.result(mapping.key, props);
const value = Onyx.tryGetCachedValue(key, mapping);

if (value !== undefined) {
// eslint-disable-next-line no-param-reassign
resultObj[propertyName] = value;
}

return resultObj;
}, {});

// If we have all the data we need, then we can render the component immediately
cachedState.loading = _.size(cachedState) < requiredKeysForInit.length;

// Object holding the temporary initial state for the component while we load the various Onyx keys
this.tempState = {};
this.tempState = cachedState;

this.state = {
// If there are no required keys for init then we can render the wrapped component immediately
loading: requiredKeysForInit.length > 0,
};
this.state = cachedState;
}

componentDidMount() {
Expand All @@ -60,7 +72,6 @@ export default function (mapOnyxToState) {
_.each(mapOnyxToState, (mapping, propertyName) => {
const previousKey = Str.result(mapping.key, prevProps);
const newKey = Str.result(mapping.key, this.props);

if (previousKey !== newKey) {
Onyx.disconnect(this.activeConnectionIDs[previousKey], previousKey);
delete this.activeConnectionIDs[previousKey];
Expand Down Expand Up @@ -88,6 +99,16 @@ export default function (mapOnyxToState) {
* @param {*} val
*/
setWithOnyxState(statePropertyName, val) {
// We might have loaded the values for the onyx keys/mappings already from the cache.
// In case we were able to load all the values upfront, the loading state will be false.
// However, Onyx.js will always call setWithOnyxState, as it doesn't know that this implementation
// already loaded the values from cache. Thus we have to check whether the value has changed
// before we set the state to prevent unnecessary renders.
const prevValue = this.state[statePropertyName];
if (!this.state.loading && prevValue === val) {
return;
}

if (!this.state.loading) {
this.setState({[statePropertyName]: val});
return;
Expand All @@ -100,7 +121,14 @@ export default function (mapOnyxToState) {
return;
}

this.setState({...this.tempState, loading: false});
const stateUpdate = {...this.tempState, loading: false};

// The state is set here manually, instead of using setState because setState is an async operation, meaning it might execute on the next tick,
// or at a later point in the microtask queue. That can lead to a race condition where
// setWithOnyxState is called before the state is actually set. This results in unreliable behavior when checking the loading state and has been mainly observed on fabric.
this.state = stateUpdate;

this.setState(stateUpdate); // Trigger a render
delete this.tempState;
}

Expand Down
8 changes: 8 additions & 0 deletions tests/components/ViewWithText.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,9 +5,16 @@ import {View, Text} from 'react-native';

const propTypes = {
text: PropTypes.string.isRequired,
onRender: PropTypes.func,
};

const defaultProps = {
onRender: () => {},
};

function ViewWithText(props) {
props.onRender();

return (
<View>
<Text testID="text-element">{props.text}</Text>
Expand All @@ -16,4 +23,5 @@ function ViewWithText(props) {
}

ViewWithText.propTypes = propTypes;
ViewWithText.defaultProps = defaultProps;
export default ViewWithText;
55 changes: 16 additions & 39 deletions tests/unit/subscribeToPropertiesTest.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,6 @@ describe('Only the specific property changes when using withOnyx() and ', () =>
.then(() => Onyx.merge(ONYX_KEYS.TEST_KEY, {a: 'one', b: 'two'}))
.then(() => {
renderedComponent = render(<ErrorBoundary><TestComponentWithOnyx /></ErrorBoundary>);
return waitForPromisesToResolve();
})

// Then the props passed to the component should only include the property "a" that was specified
Expand All @@ -69,7 +68,6 @@ describe('Only the specific property changes when using withOnyx() and ', () =>
.then(() => Onyx.merge(ONYX_KEYS.TEST_KEY, {a: 'two'}))
.then(() => {
renderedComponent = render(<ErrorBoundary><TestComponentWithOnyx /></ErrorBoundary>);
return waitForPromisesToResolve();
})

// Then the props passed should have the new value of property "a"
Expand All @@ -81,7 +79,6 @@ describe('Only the specific property changes when using withOnyx() and ', () =>
.then(() => Onyx.merge(ONYX_KEYS.TEST_KEY, {b: 'two'}))
.then(() => {
renderedComponent = render(<ErrorBoundary><TestComponentWithOnyx /></ErrorBoundary>);
return waitForPromisesToResolve();
})

// Then the props passed should not have changed
Expand Down Expand Up @@ -127,16 +124,12 @@ describe('Only the specific property changes when using withOnyx() and ', () =>
return waitForPromisesToResolve()

// When Onyx is updated with an object that has multiple properties
.then(() => {
Onyx.mergeCollection(ONYX_KEYS.COLLECTION.TEST_KEY, {
[`${ONYX_KEYS.COLLECTION.TEST_KEY}1`]: {a: 'one', b: 'two'},
[`${ONYX_KEYS.COLLECTION.TEST_KEY}2`]: {c: 'three', d: 'four'},
});
return waitForPromisesToResolve();
})
.then(() => Onyx.mergeCollection(ONYX_KEYS.COLLECTION.TEST_KEY, {
[`${ONYX_KEYS.COLLECTION.TEST_KEY}1`]: {a: 'one', b: 'two'},
[`${ONYX_KEYS.COLLECTION.TEST_KEY}2`]: {c: 'three', d: 'four'},
}))
.then(() => {
renderedComponent = render(<ErrorBoundary><TestComponentWithOnyx /></ErrorBoundary>);
return waitForPromisesToResolve();
})

// Then the props passed to the component should only include the property "a" that was specified
Expand All @@ -147,23 +140,17 @@ describe('Only the specific property changes when using withOnyx() and ', () =>
// When Onyx is updated with a change to property a using merge()
// This uses merge() just to make sure that everything works as expected when mixing merge()
// and mergeCollection()
.then(() => {
Onyx.merge(`${ONYX_KEYS.COLLECTION.TEST_KEY}1`, {a: 'two'});
return waitForPromisesToResolve();
})
.then(() => Onyx.merge(`${ONYX_KEYS.COLLECTION.TEST_KEY}1`, {a: 'two'}))

// Then the props passed should have the new value of property "a"
.then(() => {
expect(renderedComponent.getByTestId('text-element').props.children).toEqual('{"collectionWithPropertyA":{"test_1":"two"}}');
})

// When Onyx is updated with a change to property b using mergeCollection()
.then(() => {
Onyx.mergeCollection(ONYX_KEYS.COLLECTION.TEST_KEY, {
[`${ONYX_KEYS.COLLECTION.TEST_KEY}1`]: {b: 'three'},
});
return waitForPromisesToResolve();
})
.then(() => Onyx.mergeCollection(ONYX_KEYS.COLLECTION.TEST_KEY, {
[`${ONYX_KEYS.COLLECTION.TEST_KEY}1`]: {b: 'three'},
}))

// Then the props passed should not have changed
.then(() => {
Expand Down Expand Up @@ -215,16 +202,12 @@ describe('Only the specific property changes when using withOnyx() and ', () =>
return waitForPromisesToResolve()

// When Onyx is updated with an object that has multiple properties
.then(() => {
Onyx.mergeCollection(ONYX_KEYS.COLLECTION.TEST_KEY, {
[`${ONYX_KEYS.COLLECTION.TEST_KEY}1`]: {a: 'one', b: 'two'},
[`${ONYX_KEYS.COLLECTION.TEST_KEY}2`]: {c: 'three', d: 'four'},
});
return waitForPromisesToResolve();
})
.then(() => Onyx.mergeCollection(ONYX_KEYS.COLLECTION.TEST_KEY, {
[`${ONYX_KEYS.COLLECTION.TEST_KEY}1`]: {a: 'one', b: 'two'},
[`${ONYX_KEYS.COLLECTION.TEST_KEY}2`]: {c: 'three', d: 'four'},
}))
.then(() => {
renderedComponent = render(<ErrorBoundary><TestComponentWithOnyx /></ErrorBoundary>);
return waitForPromisesToResolve();
})

// Then the props passed to the component should only include the property "a" that was specified
Expand All @@ -235,23 +218,17 @@ describe('Only the specific property changes when using withOnyx() and ', () =>
// When Onyx is updated with a change to property a using merge()
// This uses merge() just to make sure that everything works as expected when mixing merge()
// and mergeCollection()
.then(() => {
Onyx.merge(`${ONYX_KEYS.COLLECTION.TEST_KEY}1`, {a: 'two'});
return waitForPromisesToResolve();
})
.then(() => Onyx.merge(`${ONYX_KEYS.COLLECTION.TEST_KEY}1`, {a: 'two'}))

// Then the props passed should have the new value of property "a"
.then(() => {
expect(renderedComponent.getByTestId('text-element').props.children).toEqual('{"itemWithPropertyA":"two"}');
})

// When Onyx is updated with a change to property b using mergeCollection()
.then(() => {
Onyx.mergeCollection(ONYX_KEYS.COLLECTION.TEST_KEY, {
[`${ONYX_KEYS.COLLECTION.TEST_KEY}1`]: {b: 'three'},
});
return waitForPromisesToResolve();
})
.then(() => Onyx.mergeCollection(ONYX_KEYS.COLLECTION.TEST_KEY, {
[`${ONYX_KEYS.COLLECTION.TEST_KEY}1`]: {b: 'three'},
}))

// Then the props passed should not have changed
.then(() => {
Expand Down
Loading

0 comments on commit 8e76a58

Please sign in to comment.