Fix cache for missing keys and empty collections #401

janicduplessis · 2023-10-13T02:05:21Z

Details

I found 2 cases where onyx component do not properly cache their data and cause it to be fetched from storage every time it mounts, even if it mounted before.

The first case is when the key does not exist in storage. In that case we never end up calling the get function and just send null to the connection. This means that we never cache that this key doesn't exist. To fix that we can manually set null in the cache for this code path.

The 2nd case is when we have an empty collection. Currently tryGetCachedValue always returns undefined when the collection is empty, but we always have all keys available so it is safe to just return an empty collection in that case. The only case where we don't have all keys is if we never called getAllKeys before and in that case cache.getAllKeys will be empty, so that is the only case where we need to return undefined.

Related Issues

N/A

Slack: https://expensify.slack.com/archives/C05LX9D6E07/p1696601323002289

Automated Tests

This adds 3 tests to validate caching behavior of withOnyx. The first one validates a simple case of mounting and unmounting a component. The first time it mounts it should load data from storage and the 2nd time it should be able to synchronously render. This test already passes before these changes.

The 2nd and 3rd test validate behaviors fixed in the PR, missing keys and empty collections. It makes sure that they are also properly cached on remount.

Manual Tests

I tested in expensify app by scrolling to the bottom of the chats list to make sure all data has been loaded in cache, then start profiling with react devtools and scroll back to the top of the list.

Before:

You can see a large number of small renders

They are caused by loading prop change, which should not happen since we already loaded that data.

After:

Only a few bigger renders, seems like there is still an extra render for each chat cell because of a change to the personalDetails prop, we could investigate in a follow up.

Also manually tested the main app screens to make sure it doesn't cause any issues.

Author Checklist

Screenshots/Videos

Web

Mobile Web - Chrome

Mobile Web - Safari

Desktop

iOS

Android

puneetlath

I don't have a great understanding of Onyx, so adding a couple more reviewers. The tests seem to make sense to me though.

tgolen · 2023-10-13T16:50:35Z

lib/Onyx.js

@@ -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


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

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.

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

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

tgolen · 2023-10-13T16:53:14Z

tests/unit/withOnyxTest.js

+                expect(textComponent).not.toBeNull();
+                expect(onRender).toHaveBeenCalledTimes(1);
+
+                // Unmount the component and mount it again. It should now render immediately.


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.

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

mountiny · 2023-10-15T12:25:40Z

tests/unit/withOnyxTest.js

+                expect(textComponent).not.toBeNull();
+                expect(onRender).toHaveBeenCalledTimes(1);
+
+                // Unmount the component and mount it again. It should now render immediately.


mountiny · 2023-10-15T12:34:17Z

tests/unit/withOnyxTest.js

+        const onRender = jest.fn();
+        let renderResult;
+
+        return StorageMock.setItem(ONYX_KEYS.SIMPLE_KEY, 'simple')


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

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.

janicduplessis · 2023-10-15T23:35:04Z

@tgolen @mountiny Thanks for the review, will address the feedback tomorrow!

janicduplessis · 2023-10-17T04:21:56Z

Updated!

tgolen · 2023-10-17T14:43:03Z

lib/Onyx.js

@@ -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


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

tgolen

Thanks!

mountiny

Thank you!

This reverts commit a8c0115, reversing changes made to 9672073.

janicduplessis force-pushed the @janic/cache-fixes branch 2 times, most recently from 4307bfe to 7b98cca Compare October 13, 2023 03:48

janicduplessis marked this pull request as ready for review October 13, 2023 04:12

janicduplessis requested a review from a team as a code owner October 13, 2023 04:12

melvin-bot bot requested review from puneetlath and removed request for a team October 13, 2023 04:12

janicduplessis force-pushed the @janic/cache-fixes branch from 7b98cca to 2111b74 Compare October 13, 2023 04:47

Fix cache for missing keys and empty collections

ed3b96b

janicduplessis force-pushed the @janic/cache-fixes branch from 2111b74 to ed3b96b Compare October 13, 2023 05:00

puneetlath previously approved these changes Oct 13, 2023

View reviewed changes

puneetlath requested review from tgolen and mountiny October 13, 2023 15:20

tgolen requested changes Oct 13, 2023

View reviewed changes

mountiny assigned janicduplessis Oct 15, 2023

mountiny reviewed Oct 15, 2023

View reviewed changes

Clarify some comments

adf230c

janicduplessis dismissed puneetlath’s stale review via adf230c October 17, 2023 04:21

tgolen requested changes Oct 17, 2023

View reviewed changes

Update Onyx.js

c12202e

tgolen approved these changes Oct 18, 2023

View reviewed changes

muttmuure mentioned this pull request Oct 18, 2023

Explore Onyx Data Caching for heavy components Expensify/App#29261

Closed

mountiny approved these changes Oct 19, 2023

View reviewed changes

mountiny merged commit a8c0115 into Expensify:main Oct 19, 2023
3 checks passed

janicduplessis deleted the @janic/cache-fixes branch October 19, 2023 13:58

ospfranco mentioned this pull request Oct 26, 2023

[NoQA] Bump onyx - Fix out of order updates Expensify/App#29169

Merged

59 tasks

tgolen added a commit that referenced this pull request Oct 27, 2023

Revert "Merge pull request #401 from janicduplessis/@janic/cache-fixes"

94372ef

This reverts commit a8c0115, reversing changes made to 9672073.

jjcoffee mentioned this pull request Oct 30, 2023

Fix IDBKeyVal get returning undefined instead of null #408

Merged

42 tasks

janicduplessis mentioned this pull request Nov 2, 2023

Don't remove nulls in cache merge #411

Merged

42 tasks

erquhart mentioned this pull request Nov 14, 2023

[HOLD for payment 2023-11-30] [$500] Composer is very large on first render Expensify/App#31089

Closed

6 tasks

chrispader mentioned this pull request May 29, 2024

Remove null values from cache to improve Onyx read speed #554

Merged

42 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix cache for missing keys and empty collections #401

Fix cache for missing keys and empty collections #401

janicduplessis commented Oct 13, 2023 •

edited

Loading

puneetlath left a comment •

edited

Loading

tgolen Oct 13, 2023

janicduplessis Oct 13, 2023

tgolen Oct 16, 2023

tgolen Oct 17, 2023

tgolen Oct 13, 2023

janicduplessis Oct 13, 2023

mountiny Oct 15, 2023

mountiny Oct 15, 2023

mountiny Oct 15, 2023

janicduplessis Oct 15, 2023

janicduplessis commented Oct 15, 2023

janicduplessis commented Oct 17, 2023

tgolen Oct 17, 2023

tgolen left a comment

mountiny left a comment

Fix cache for missing keys and empty collections #401

Fix cache for missing keys and empty collections #401

Conversation

janicduplessis commented Oct 13, 2023 • edited Loading

Details

Related Issues

Automated Tests

Manual Tests

Author Checklist

Screenshots/Videos

puneetlath left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

janicduplessis commented Oct 15, 2023

janicduplessis commented Oct 17, 2023

Choose a reason for hiding this comment

tgolen left a comment

Choose a reason for hiding this comment

mountiny left a comment

Choose a reason for hiding this comment

janicduplessis commented Oct 13, 2023 •

edited

Loading

puneetlath left a comment •

edited

Loading