Feature: Onyx Cache #76

kidroca · 2021-05-25T22:59:22Z

Details

Added a new file and defined OnyxCache class to encapsulate cache related methods and have some reuse
Update Onyx methods to use cache
Updated Onyx methods to deal with concurrent method calls while data is still being read
Added tests

Related Issues

GH_LINK
Fixes #63
Expensify/App#2762

Automated Tests

Added test annotated with GIVEN, WHEN, THEN. They should be pretty self explanatory
Covering OnyxCache and the integration in Onyx and AsyncStorage calls

Linked PRs

…rray

Fixed an issue with merge collections to cache

… component

kidroca

There are still some things to complete
Like clearing cached keys when there are no subscribers for a given key
Some existing tests started to fail after the update, I'll take a look tomorrow

tests/unit/onyxMetricsDecorationTest.js

tests/unit/withOnyxTest.js

lib/Onyx.js

marcaaron

Just leaving some initial comments since it seems like you are still working through stuff.

lib/Onyx.js

lib/OnyxCache.js

kidroca

Thanks for the review @marcaaron
Added some replies

lib/Onyx.js

lib/OnyxCache.js

…values

…cted

…ys in the cache service

…he cache service

kidroca · 2021-05-27T19:29:42Z

Updated
I've left some threads open for potential discussions
The only thing remaining otherwise is a 26 of the test I've written for the cache use async/await syntax I find the tests more readable that way, but can convert them to promise chains if you want to

kidroca · 2021-06-03T22:08:58Z

Addressed the outstanding comment by explicitly converting the set to array

Jag96

LGTM, will leave open to let @Julesssss get his review in

Jag96

Re-tested this today on the latest E.cash main on an account that has an IOU and I'm getting this error on both web and desktop after signing in (iOS/Android work fine):

TypeError: Currency code is required with currency style.
    at new NumberFormat (<anonymous>)
    at oO (app-6bf1371e025ae4112ce5.bundle.js:78)
    at Object.numberFormat (app-6bf1371e025ae4112ce5.bundle.js:78)
    at IOUBadge (app-6bf1371e025ae4112ce5.bundle.js:78)
    at Yi (app-6bf1371e025ae4112ce5.bundle.js:50)
    at gs (app-6bf1371e025ae4112ce5.bundle.js:50)
    at lc (app-6bf1371e025ae4112ce5.bundle.js:50)
    at sc (app-6bf1371e025ae4112ce5.bundle.js:50)
    at Zs (app-6bf1371e025ae4112ce5.bundle.js:50)
    at app-6bf1371e025ae4112ce5.bundle.js:50

Uncaught (in promise) TypeError: Currency code is required with currency style.
    at new NumberFormat (<anonymous>)
    at oO (app-6bf1371e025ae4112ce5.bundle.js:78)
    at Object.numberFormat (app-6bf1371e025ae4112ce5.bundle.js:78)
    at IOUBadge (app-6bf1371e025ae4112ce5.bundle.js:78)
    at Yi (app-6bf1371e025ae4112ce5.bundle.js:50)
    at gs (app-6bf1371e025ae4112ce5.bundle.js:50)
    at lc (app-6bf1371e025ae4112ce5.bundle.js:50)
    at sc (app-6bf1371e025ae4112ce5.bundle.js:50)
    at Zs (app-6bf1371e025ae4112ce5.bundle.js:50)
    at app-6bf1371e025ae4112ce5.bundle.js:50

This results in a white screen on Web and Desktop. I tested this on dev after running npx react-native-clean-project and wiping the node_modules folder, and I also confirmed it happens when running npm run desktop-build as well. Here's the steps I did to test this, just to make sure I'm not missing anything:

Run npx react-native-clean-project and wipe node_modules folder
Update package.json react-native-onyx commit hash to 77b07e10dfd10831380c197a132a032e812b31f9
Run npm install then npm run web and npm run desktop and npm run desktop-build
Sign into an account that has an IOU
See the above errors in the console and a white screen
Clear changes in package.json so the react-native-onyx hash is back to d0a5d12c08f2f029e3038fb3516a93a9403e9746
Run npx react-native-clean-project and wipe node_modules folder
Run npm install then npm run web and npm run desktop and npm run desktop-build
Confirm there is no white screen and the error doesn't appear after signing in

I chatted w/ @roryabraham and he is also going to test this to confirm

Jag96 · 2021-06-04T18:43:53Z

It looks like setting a default value for currency here fixes the issue, going to create an E.cash PR.

Jag96 · 2021-06-04T21:25:03Z

The e.cash PR has been merged, going to test again on the main branch to confirm everything is working

quinthar · 2021-06-04T22:24:20Z

Woo!!!!

…

On Fri, Jun 4, 2021 at 2:25 PM Joe Gambino ***@***.***> wrote: The e.cash PR has been merged, going to test again on the main branch to confirm everything is working — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#76 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAEMNUWYIQEA4Z3FWTESQDTTRFAD7ANCNFSM45QMDVUQ> .

roryabraham

Spent a while testing this on iOS, web, desktop, and I couldn't find any regressions or obvious errors. Seems to work well.

Jag96

This looks good to me as well, tested on all platforms and it's working great! I'll let @marcaaron have a final review/merge for this one

Julesssss

Sorry for the late review! I ran this locally and while it was tricky to do a visual comparison between dev and prod builds the improvement is noticeable.

I had some of the same concerns as @Jag96 mentioned here, but as discussed there are further optimizations that might take care of this. The main one is that even pages without huge lists feel slightly slower to display. For example, the initial delay when tapping an option in the setting page feels like input lag (I know this isn't actually the case, but this might seem like input lag to a user). Also, it would be great to clear up the issue where FlatList items are added one by one.

Anyway, thanks for leading this huge improvement. I look forward to testing on a prod build!

marcaaron

Left a few minor comments (mostly about tests and not blockers - but should be addressed). Still think we should move usages of captureTask and AsyncStorage into the cache itself, but it's not a hill I will die on so I'll let it go for now.

lib/OnyxCache.js

tests/unit/onyxCacheTest.js

tests/unit/withOnyxTest.js

marcaaron · 2021-06-07T17:04:56Z

tests/unit/onyxCacheTest.js

+import ViewWithCollections from '../components/ViewWithCollections';
+
+describe('Onyx', () => {
+    describe('Cache Service', () => {


NAB, appreciate the thoroughness here, but rather than test the individual cache methods we could have just tested the behavior that we'd expect when Onyx is actually using the cache vs. using the cache in isolation.

NAB, appreciate the thoroughness here, but rather than test the individual cache methods we could have just tested the behavior that we'd expect when Onyx is actually using the cache

Actually we've tested both in isolation here and the behavior that we'd expect when Onyx is using cache (in the next section)

It's much harder to test all the expectations written here in Onyx, you have to

setup component(s)

configure async blocks and flush promises at specific times

not as straightforward if you want to debug a test

The test for Onyx validate complete flows covering multiple cache methods - e.g. a key is not retrieved from storage when a rendered component uses it

marcaaron · 2021-06-07T17:07:22Z

tests/unit/onyxCacheTest.js

+        /** @type OnyxCache */
+        let cache;
+
+        // Always use a "fresh" instance


NAB, Is that what requiring again would do? When would this not be creating a "fresh" instance? Why should we use one instead of just clearing out the cache?

NAB, Is that what requiring again would do? When would this not be creating a "fresh" instance? Why should we use one instead of just clearing out the cache?

Simply requiring again would not do the trick. It would return the instance that was already created from the first time the script was required somewhere

There's no function that completely clears the cache. You could implemented something here or in the cache file, but if it's only for the test you could just tell jest you want a clean env

marcaaron · 2021-06-07T17:15:48Z

Next we will just need to create a Expensify/Expensify.cash PR referencing this commit 4f341cfca83bcc4f9a8d89b593d81ea78903f04c for react-native-onyx.

kidroca

Still think we should move usages of captureTask and AsyncStorage into the cache itself, but it's not a hill I will die on so I'll let it go for now.

Sorry I thought that's what you meant by #76 (comment)

Thanks for your thoughts. Let's go ahead and make this change.

I thought it was a reply to

Adding Onyx specifics here makes this util less flexible it should just provide some low level cache handling heplers like hasCacheForKey, get, set, merge and the resolveTask helper for concurrency calls and leave the caller the flexibility to use it however needed. Then it's transparent how items are retrieved in Onyx.js and the cache handling is on the same abstraction level

And so I switched to how Cache was set up initially

Right now we have separation - cache logic in Cache. Onyx logic and flows in Onyx. It's transparent how Onyx uses and updates cache

It would be confusing to me if I read the high level of the code for Onyx.get and see that it just does return cache.get(key). "What happens if there's no cache?" is my first question, then I have to open OnyxCache and have another question "Why does cache knows how to retrieve values from storage?"

From the current code it's clear that it doesn't have to know and works

If it's just a Proxy - the "startTask" implementation where it doesn't know how the task would provide the value is OK, but it made it harder to test and it was confusing for everybody but me

kidroca · 2021-06-07T18:01:34Z

tests/unit/onyxCacheTest.js

+import ViewWithCollections from '../components/ViewWithCollections';
+
+describe('Onyx', () => {
+    describe('Cache Service', () => {


NAB, appreciate the thoroughness here, but rather than test the individual cache methods we could have just tested the behavior that we'd expect when Onyx is actually using the cache

Actually we've tested both in isolation here and the behavior that we'd expect when Onyx is using cache (in the next section)

It's much harder to test all the expectations written here in Onyx, you have to

setup component(s)

configure async blocks and flush promises at specific times

not as straightforward if you want to debug a test

The test for Onyx validate complete flows covering multiple cache methods - e.g. a key is not retrieved from storage when a rendered component uses it

kidroca · 2021-06-07T18:11:25Z

tests/unit/onyxCacheTest.js

+        /** @type OnyxCache */
+        let cache;
+
+        // Always use a "fresh" instance


NAB, Is that what requiring again would do? When would this not be creating a "fresh" instance? Why should we use one instead of just clearing out the cache?

Simply requiring again would not do the trick. It would return the instance that was already created from the first time the script was required somewhere

There's no function that completely clears the cache. You could implemented something here or in the cache file, but if it's only for the test you could just tell jest you want a clean env

tests/unit/onyxCacheTest.js

tests/unit/withOnyxTest.js

kidroca · 2021-06-07T18:56:52Z

Next we will just need to create a Expensify/Expensify.cash PR referencing this commit 4f341cfca83bcc4f9a8d89b593d81ea78903f04c for react-native-onyx.

Should I create this?
I guess yes, and I'll need to provide samples of my tests for all the platforms
I can do this tomorrow

marcaaron · 2021-06-07T19:52:22Z

Sorry I thought that's what you meant by #76 (comment)

No. I was thanking you for your thoughts and requesting that you take my suggestion anyway.

Julesssss · 2021-06-09T11:16:47Z

Moving this comment here, as the linked issue was locked.

This morning @trjExpensify and I noticed a potential regression with the Onyx changes:

The iou loading state values that we initialize here are returned null here when opening the IOUModal. The data is valid within local storage and this is only reproducible about 1 in 4 attempts.

While the simple solution would be to add a conditional check to confirm the iou object is valid, we need to confirm whether this requires a fix, or is now expected behavior.

CC @kidroca

IOURequestOnyxCrash.mov

Julesssss · 2021-06-09T11:38:25Z

It looks like the cleanup PR supposedly resolved this issue. But as far as I can tell, these commits are also on the staging build. So it seems to me that the issue is still occurring:

Tim's PR bumps the Onyx commit to 4ff94b6c068582a75f50c6e159d3ff69fa123138
Commit 4ff94b6 is more recent than the onyx cache fix
Tim's PR was deployed to staging

kidroca · 2021-06-09T12:20:14Z

@Julesssss

This morning @trjExpensify and I noticed a potential regression with the Onyx changes

I'll investigate

react-native-onyx/lib/Onyx.js

Lines 723 to 724 in 9b03d95

    
           // Set our default key states to use when initializing and clearing Onyx data 
        
           defaultKeyStates = initialKeyStates;

Are we supposed to put these initialKeyStates in cache like so?

    // Set our default key states to use when initializing and clearing Onyx data
    defaultKeyStates = initialKeyStates;
    cache.merge(initialKeyStates);

I think they automatically go there:

react-native-onyx/lib/Onyx.js

Lines 730 to 731 in 9b03d95

    
           // Initialize all of our keys with data provided 
        
           initializeWithDefaultKeyStates();

This function would internally call merge which would call set and this will update the values in cache

Maybe, this happens after enough delay that the initial value is null because it's not yet set ?

Anyway, I'll see what can I find

trjExpensify · 2021-06-09T13:16:04Z

Thanks, @kidroca! We're imminently sending a newsletter out to all our users to announce the IOU features and welcome them into E.cash to test out what we’ve been built so far. We shipped the latest prod build off to the app stores last night for review, which was the last item on the list before hitting send.

If this regression makes it to production, it delays our timeline for the announcement, as the IOU feature will be broken on Web/Desktop/mWeb. With that, it would be awesome if we could get a fix up ASAP to minimise the delay. Thanks again!

Julesssss · 2021-06-09T13:17:29Z

Are we supposed to put these initialKeyStates in cache like so?

Yeah, I think that would make sense. Perhaps we should just expect these to be initially null... but I think having the defaults added to cache would be preferable.

kidroca · 2021-06-09T14:06:52Z

I traced what is causing the issue. It's our cache cleanup logic and a bug in cache.remove

When you close the IOU modal, since it's the last and only connection that uses the iou key
on disconnect it tells cache to remove the iou from cache - because there are no more usages on screen
but cache.remove would also remove it from the storageKeys (the bug)
the next time someone tries to connect to the iou key (You open the IOU modal)
getAllKeys is not returning the iou because it was removed
since getAllKeys does not return it connect short-circuits the value to null

react-native-onyx/lib/Onyx.js

Lines 373 to 382 in 9b03d95

    
           getAllKeys() 
        
               .then((keys) => { 
        
                   // Find all the keys matched by the config key 
        
                   const matchingKeys = _.filter(keys, key => isKeyMatch(mapping.key, key)); 
        
                   // If the key being connected to does not exist, initialize the value with null 
        
                   if (matchingKeys.length === 0) { 
        
                       sendDataToConnection(mapping, null); 
        
                       return; 
        
                   }

cache.remove should not remove the key from storageKeys so that getAllKeys continues to return correct result

react-native-onyx/lib/OnyxCache.js

Lines 91 to 98 in 9b03d95

    
               /** 
        
                * Remove data from cache 
        
                * @param {string} key 
        
                */ 
        
               remove(key) { 
        
                   this.storageKeys.delete(key); 
        
                   delete this.storageMap[key]; 
        
               }

It should just drop the cached value for that key

@Julesssss I'll open a PR shortly

`OnyxCache.remove` was renamed to `drop` and updated so that it does not remove the `key` but only the value This way the result of `getAllKeys` is unaffected Related to Expensify#76

kidroca · 2021-06-09T15:21:30Z

@Julesssss pushed a fix here #81

And confirmed it against the main branch of E.cash

Expensify.cash.-.Google.Chrome.2021-06-09.18-19-25.mp4

kidroca added 13 commits May 25, 2021 11:00

test: move metrics decoration tests to separate file

a343ef1

test: move Onyx require call to beforeEach

a7ee841

test: fix withOnyx test not returning promise

d23b8eb

test: add a test suite for Onyx cache

a6df3db

feat: Add separate service for cache

231fe00

test: add OnyxCache tests

2f76d80

feat: Use OnyxCache to cache storage operations

1046786

test: update Onyx with Cache to run correctly

273afe1

refactor: cache.merge should throw when a value is not an object or a…

47739e4

…rray

refactor: extract prepareKeyValuePairsForStorage method

f4a523f

Fixed an issue with merge collections to cache

refactor: simplify mergeCollection keys partitioning

6b659d7

docs: add todo notes on withOnyx test not passing expected props to…

f7feafc

… component

fix: get should capture different tasks for different keys

16d42ac

kidroca commented May 25, 2021

View reviewed changes

marcaaron reviewed May 26, 2021

View reviewed changes

kidroca commented May 26, 2021

View reviewed changes

tgolen assigned kidroca May 26, 2021

kidroca added 9 commits May 27, 2021 16:59

fix: clear async storage

5b11545

feat: enhance cache.merge to work with primitives as well as complex …

0c51923

…values

refactor: update hasCacheForKey to use isDefined

22ad916

docs: cache.update try to better explain this method

d0c62c5

feat: clean up cache when the last key for a subscription is disconne…

1b35161

…cted

refactor: encapsulate cache checking and resolving logic for getAllKe…

b33e696

…ys in the cache service

refactor: encapsulate cache checking and resolving logic for get in t…

4af3abf

…he cache service

Rename tests/decorateWithMetrics.js to tests/decorateWithMetricsTest.js

ceb4acf

test: check what happens with cache when async storage rejects

5435063

kidroca force-pushed the kidroca/onyx-cache branch from 61c0c82 to 5435063 Compare May 27, 2021 18:32

kidroca added 2 commits May 27, 2021 22:18

docs: update getAllKeys and getValue documentation

dc2a540

test: Shorten test case names

34cb2aa

Jag96 approved these changes Jun 4, 2021

View reviewed changes

Jag96 suggested changes Jun 4, 2021

View reviewed changes

Jag96 mentioned this pull request Jun 4, 2021

Set default currency for IOUBadge Expensify/App#3382

Merged

5 tasks

roryabraham approved these changes Jun 4, 2021

View reviewed changes

Jag96 approved these changes Jun 4, 2021

View reviewed changes

Julesssss approved these changes Jun 7, 2021

View reviewed changes

marcaaron approved these changes Jun 7, 2021

View reviewed changes

marcaaron merged commit 4f341cf into Expensify:master Jun 7, 2021

kidroca commented Jun 7, 2021

View reviewed changes

kidroca mentioned this pull request Jun 8, 2021

Update react-native-onyx with the latest version Expensify/App#3423

Merged

5 tasks

kidroca deleted the kidroca/onyx-cache branch June 8, 2021 12:16

kidroca mentioned this pull request Jun 8, 2021

Kidroca/onyx cache cleanup #79

Merged

Julesssss mentioned this pull request Jun 9, 2021

Persist IOU data in Onyx Expensify/App#1785

Merged

5 tasks

kidroca mentioned this pull request Jun 9, 2021

Fix: Onyx.cleanCache #81

Merged

Julesssss mentioned this pull request Jun 15, 2021

[HOLD #12054] [$1000] Conversation DM bounces, flickers, and lags when scrolling downward in a long chat Expensify/App#2545

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature: Onyx Cache #76

Feature: Onyx Cache #76

kidroca commented May 25, 2021

kidroca left a comment •

edited

Loading

marcaaron left a comment

kidroca left a comment

kidroca commented May 27, 2021

kidroca commented Jun 3, 2021

Jag96 left a comment

Jag96 left a comment

Jag96 commented Jun 4, 2021

Jag96 commented Jun 4, 2021 •

edited

Loading

quinthar commented Jun 4, 2021 via email

roryabraham left a comment

Jag96 left a comment

Julesssss left a comment •

edited

Loading

marcaaron left a comment

marcaaron Jun 7, 2021

kidroca Jun 7, 2021

marcaaron Jun 7, 2021

kidroca Jun 7, 2021

marcaaron commented Jun 7, 2021

kidroca left a comment

kidroca Jun 7, 2021

kidroca Jun 7, 2021

kidroca commented Jun 7, 2021

marcaaron commented Jun 7, 2021

Julesssss commented Jun 9, 2021

Julesssss commented Jun 9, 2021

kidroca commented Jun 9, 2021

trjExpensify commented Jun 9, 2021

Julesssss commented Jun 9, 2021 •

edited

Loading

kidroca commented Jun 9, 2021

kidroca commented Jun 9, 2021

Feature: Onyx Cache #76

Feature: Onyx Cache #76

Conversation

kidroca commented May 25, 2021

Details

Related Issues

Automated Tests

Linked PRs

kidroca left a comment • edited Loading

Choose a reason for hiding this comment

marcaaron left a comment

Choose a reason for hiding this comment

kidroca left a comment

Choose a reason for hiding this comment

kidroca commented May 27, 2021

kidroca commented Jun 3, 2021

Jag96 left a comment

Choose a reason for hiding this comment

Jag96 left a comment

Choose a reason for hiding this comment

Jag96 commented Jun 4, 2021

Jag96 commented Jun 4, 2021 • edited Loading

quinthar commented Jun 4, 2021 via email

roryabraham left a comment

Choose a reason for hiding this comment

Jag96 left a comment

Choose a reason for hiding this comment

Julesssss left a comment • edited Loading

Choose a reason for hiding this comment

marcaaron left a comment

Choose a reason for hiding this comment

marcaaron Jun 7, 2021

Choose a reason for hiding this comment

kidroca Jun 7, 2021

Choose a reason for hiding this comment

marcaaron Jun 7, 2021

Choose a reason for hiding this comment

kidroca Jun 7, 2021

Choose a reason for hiding this comment

marcaaron commented Jun 7, 2021

kidroca left a comment

Choose a reason for hiding this comment

kidroca Jun 7, 2021

Choose a reason for hiding this comment

kidroca Jun 7, 2021

Choose a reason for hiding this comment

kidroca commented Jun 7, 2021

marcaaron commented Jun 7, 2021

Julesssss commented Jun 9, 2021

Julesssss commented Jun 9, 2021

kidroca commented Jun 9, 2021

trjExpensify commented Jun 9, 2021

Julesssss commented Jun 9, 2021 • edited Loading

kidroca commented Jun 9, 2021

kidroca commented Jun 9, 2021

kidroca left a comment •

edited

Loading

Jag96 commented Jun 4, 2021 •

edited

Loading

Julesssss left a comment •

edited

Loading

Julesssss commented Jun 9, 2021 •

edited

Loading