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

Fix cache for missing keys and empty collections #401

Merged
merged 3 commits into from
Oct 19, 2023
Merged
Show file tree
Hide file tree
Changes from 1 commit
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
14 changes: 11 additions & 3 deletions lib/Onyx.js
Original file line number Diff line number Diff line change
Expand Up @@ -236,6 +236,12 @@ function tryGetCachedValue(key, mapping = {}) {

if (isCollectionKey(key)) {
const allKeys = cache.getAllKeys();

// It is possible we haven't loaded all keys yet so we do not know if the
Copy link
Collaborator

Choose a reason for hiding this comment

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

How is it possible to not have all keys loaded yet (can you please add this to the code comment)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If we haven't called getAllKeys yet (from Onyx, not OnyxCache). Not sure if that can actually happen but to be safe this is prob better. I can clarify.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ah, I did not notice the subtle difference between getAllKeys from Onyx vs OnyxCache.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Thinking a little more on this, can you please change allKeys to be renamed to allCacheKeys?

// collection actually exists.
if (allKeys.length === 0) {
return;
}
const matchingKeys = _.filter(allKeys, k => k.startsWith(key));
const values = _.reduce(matchingKeys, (finalObject, matchedKey) => {
const cachedValue = cache.getValue(matchedKey);
Expand All @@ -246,9 +252,7 @@ function tryGetCachedValue(key, mapping = {}) {
}
return finalObject;
}, {});
if (_.isEmpty(values)) {
return;
}

val = values;
}

Expand Down Expand Up @@ -826,6 +830,10 @@ function connect(mapping) {
// since there are none matched. In withOnyx() we wait for all connected keys to return a value before rendering the child
// component. This null value will be filtered out so that the connected component can utilize defaultProps.
if (matchingKeys.length === 0) {
if (mapping.key && !isCollectionKey(mapping.key)) {
cache.set(mapping.key, null);
}

// Here we cannot use batching because the null value is expected to be set immediately for default props
// or they will be undefined.
sendDataToConnection(mapping, null, undefined, false);
Expand Down
4 changes: 4 additions & 0 deletions tests/components/ViewWithCollections.js
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,10 @@ const ViewWithCollections = React.forwardRef((props, ref) => {

props.onRender(props);

if (_.size(props.collections) === 0) {
return <Text>empty</Text>;
}

return (
<View>
{_.map(props.collections, (collection, i) => (
Expand Down
5 changes: 3 additions & 2 deletions tests/components/ViewWithText.js
Original file line number Diff line number Diff line change
Expand Up @@ -4,20 +4,21 @@ import PropTypes from 'prop-types';
import {View, Text} from 'react-native';

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

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

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

return (
<View>
<Text testID="text-element">{props.text}</Text>
<Text testID="text-element">{props.text || 'null'}</Text>
</View>
);
}
Expand Down
95 changes: 95 additions & 0 deletions tests/unit/withOnyxTest.js
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import ViewWithText from '../components/ViewWithText';
import ViewWithCollections from '../components/ViewWithCollections';
import waitForPromisesToResolve from '../utils/waitForPromisesToResolve';
import ViewWithObject from '../components/ViewWithObject';
import StorageMock from '../../lib/storage';

const ONYX_KEYS = {
TEST_KEY: 'test',
Expand Down Expand Up @@ -522,4 +523,98 @@ describe('withOnyxTest', () => {
expect(onRender.mock.calls[3][0].simple).toBe('long_string');
});
});

it('should render immediately when a onyx component is mounted a 2nd time', () => {
const TestComponentWithOnyx = withOnyx({
text: {
key: ONYX_KEYS.TEST_KEY,
},
})(ViewWithText);
const onRender = jest.fn();
let renderResult;

// Set the value in storage, but not in cache.
return StorageMock.setItem(ONYX_KEYS.TEST_KEY, 'test1')
.then(() => {
renderResult = render(<TestComponentWithOnyx onRender={onRender} />);

// The component should not render initially since we have no data in cache.
expect(onRender).not.toHaveBeenCalled();
return waitForPromisesToResolve();
})
.then(waitForPromisesToResolve)
.then(() => {
let textComponent = renderResult.getByText('test1');
expect(textComponent).not.toBeNull();
expect(onRender).toHaveBeenCalledTimes(1);

// Unmount the component and mount it again. It should now render immediately.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is the only way to know that it wasn't rendered immediately because the test doesn't need to add a waitForPromisesToResolve in order for the assertions to pass? If so, can you please clarify that in the comment. It's not very obvious.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, if the assertion passes right away, before waitForPromisesToResolve this means it rendered synchronously since all data is cached. I can clarify.

Copy link
Contributor

Choose a reason for hiding this comment

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

+1

renderResult.unmount();
renderResult = render(<TestComponentWithOnyx onRender={onRender} />);

textComponent = renderResult.getByText('test1');
expect(textComponent).not.toBeNull();
expect(onRender).toHaveBeenCalledTimes(2);
});
});

it('should cache missing keys', () => {
const TestComponentWithOnyx = withOnyx({
text: {
key: ONYX_KEYS.TEST_KEY,
},
})(ViewWithText);
const onRender = jest.fn();

// Render with a key that doesn't exist in cache or storage.
let renderResult = render(<TestComponentWithOnyx onRender={onRender} />);

// The component should not render initially since we have no data in cache.
expect(onRender).not.toHaveBeenCalled();
return waitForPromisesToResolve().then(() => {
let textComponent = renderResult.getByText('null');
expect(textComponent).not.toBeNull();
expect(onRender).toHaveBeenCalledTimes(1);

// Unmount the component and mount it again. It should now render immediately.
renderResult.unmount();
renderResult = render(<TestComponentWithOnyx onRender={onRender} />);
textComponent = renderResult.getByText('null');
expect(textComponent).not.toBeNull();
expect(onRender).toHaveBeenCalledTimes(2);
});
});

it('should cache empty collections', () => {
const TestComponentWithOnyx = withOnyx({
text: {
key: ONYX_KEYS.COLLECTION.TEST_KEY,
},
})(ViewWithCollections);
const onRender = jest.fn();
let renderResult;

return StorageMock.setItem(ONYX_KEYS.SIMPLE_KEY, 'simple')
Copy link
Contributor

Choose a reason for hiding this comment

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

Curious why do we need to add this key? is it just so there is something in the onyx storage?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yea to know if all keys are in cache we currently check if the array length is > 0 so if the storage is empty the check never works.

.then(() => {
// Render with a collection that doesn't exist in cache or storage.
renderResult = render(<TestComponentWithOnyx onRender={onRender} />);

// The component should not render initially since we have no data in cache.
expect(onRender).not.toHaveBeenCalled();

return waitForPromisesToResolve();
})
.then(() => {
let textComponent = renderResult.getByText('empty');
expect(textComponent).not.toBeNull();
expect(onRender).toHaveBeenCalledTimes(1);

// Unmount the component and mount it again. It should now render immediately.
renderResult.unmount();
renderResult = render(<TestComponentWithOnyx onRender={onRender} />);
textComponent = renderResult.getByText('empty');
expect(textComponent).not.toBeNull();
expect(onRender).toHaveBeenCalledTimes(2);
});
});
});
Loading