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

[Fix #269] use cached data #280

Merged
merged 44 commits into from
Jul 27, 2023
Merged

[Fix #269] use cached data #280

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
44 commits
Select commit Hold shift + click to select a range
1998ca0
Use cache when wrapping a component with onyx
hannojg Jul 14, 2023
09b9494
tests: removed now unnecessary waitForPromiseToResolve calls
hannojg Jul 14, 2023
0f5e115
wip: one approach to solve prop updates
hannojg Jul 14, 2023
98a1672
fix test
hannojg Jul 17, 2023
916b9b0
remove componentDidUpdate code
hannojg Jul 17, 2023
35becc3
add jsdoc
hannojg Jul 17, 2023
4b7f267
clean
hannojg Jul 17, 2023
6943d36
fix eslint
hannojg Jul 17, 2023
790def1
use cache.getAllKeys directly
hannojg Jul 17, 2023
f71294e
add better comment
hannojg Jul 17, 2023
9358b41
foeward default value eventually to selector
hannojg Jul 17, 2023
3ee9294
return early
hannojg Jul 17, 2023
fcc49ac
remove ambigous comment
hannojg Jul 17, 2023
853390e
reverted back cleanups
hannojg Jul 17, 2023
0c72be1
fix: selector not getting applied to collection
hannojg Jul 17, 2023
294aa31
fix tests
hannojg Jul 17, 2023
d2a5390
remove unused function
hannojg Jul 17, 2023
5b00715
Update lib/Onyx.js
hannojg Jul 19, 2023
8a3bc95
Update lib/Onyx.js
hannojg Jul 19, 2023
bf48eb3
simplify cached state usement
hannojg Jul 19, 2023
fbc16fc
remove reundand code
hannojg Jul 19, 2023
48570e8
avoid too many deepEqual calls
hannojg Jul 19, 2023
68c3d04
remove other perf fix
hannojg Jul 19, 2023
937c3f7
clean
hannojg Jul 19, 2023
cb505e3
remove all premature deepEquals
hannojg Jul 19, 2023
46f73e1
simplify
hannojg Jul 19, 2023
a2e20d0
remove checks in onyx for prev state as its withOnyxInstance responsi…
hannojg Jul 19, 2023
617ff64
fix too many state updates
hannojg Jul 19, 2023
5ffdefa
remove comments and todos
hannojg Jul 25, 2023
2c45edb
manually update the loading state
hannojg Jul 25, 2023
d479c12
add condition to check for cache usage
hannojg Jul 25, 2023
020ccb2
Merge branch 'main' of github.com:margelo/react-native-onyx into perf…
hannojg Jul 25, 2023
0756a27
remove now obsolete eslint warnings
hannojg Jul 25, 2023
7904d58
Update tests/unit/withOnyxTest.js
hannojg Jul 25, 2023
c7ac445
fix lint
hannojg Jul 25, 2023
127f56c
Update lib/withOnyx.js
hannojg Jul 25, 2023
9a4135f
Update lib/Onyx.js
hannojg Jul 25, 2023
9a41243
Update lib/withOnyx.js
hannojg Jul 25, 2023
cdb9e82
use reduce
hannojg Jul 25, 2023
1caf3f9
fix documentation
hannojg Jul 25, 2023
ad4758d
update documentation
hannojg Jul 25, 2023
e531270
Update lib/withOnyx.js
hannojg Jul 25, 2023
69fac60
Update lib/Onyx.js
hannojg Jul 26, 2023
ff82a94
Use cache directly without falling back to default state
hannojg Jul 26, 2023
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
41 changes: 41 additions & 0 deletions lib/Onyx.js
View file
Open in desktop
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);
luacmartins marked this conversation as resolved.
Show resolved Hide resolved
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 @@ -1328,6 +1368,7 @@ const Onyx = {
isSafeEvictionKey,
METHOD,
setMemoryOnlyKeys,
tryGetCachedValue,
};

/**
Expand Down
42 changes: 35 additions & 7 deletions lib/withOnyx.js
View file
Open in desktop
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;
Comment on lines 55 to +56
Copy link
Contributor

Choose a reason for hiding this comment

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

I am not sure why do we set the tempState to the same cachedState as the normal state. I might be missing something but could you help me understand this? Should we update the comment above?

Copy link
Contributor

Choose a reason for hiding this comment

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

The intention here is that tempState is still used in the same way as it was before.

If we did not get all the values from the cache then the values are held in the tempState until all are "loaded". This way we avoid calling setState() each time a value comes back from storage. That was the original optimization added here and is a good thing because otherwise you may have like several calls to setState() happening in a kind of burst before the first render (at least that's what we saw and why we changed how this works).

Though the question did make me wonder about something... if this.tempState and this.state are initialized with the same object reference then will we be mutating the state directly on this line:

react-native-onyx/lib/withOnyx.js

Line 117 in 80c6f00

this.tempState[statePropertyName] = val;

Seems like that would be undesirable in the case where you did not initialize all the values in the constructor.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for the write up. @hannojg what do you think of the concern Marc raised above?


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
Open in desktop
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
Open in desktop
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