Remove AsyncStorage dependency #240

chrispader · 2023-03-09T11:18:20Z

Details

This PR removes all remaining dependencies to @react-native-async-storage/async-storage and re-writes tests so they use LocalForage mock instead.

Related Issues

$ Expensify/App#16455

Came up in the discussion here: #238 (comment)

Gonals · 2023-03-09T11:26:17Z

Should we create an issue for this, @marcaaron?

kidroca · 2023-03-09T12:48:32Z

jestSetup.js

-jest.mock('./lib/storage/providers/SQLiteStorage', () => require('./lib/storage/providers/__mocks__/SQLiteStorage'));
+jest.mock('./lib/storage/NativeStorage', () => require('./lib/storage/__mocks__/NativeStorage'));


We're using __mocks__/localforage.js for both Native and Web - I endorse this, but let's just call it Storage mock

Then we can:

move __mocks__/localforage.js to libs/storage/__mocks__/index.js

adjust the code to make it match the storage provider interface

remove any other storage mocks

Because the specific storage implementation does not really matter - they all have the same interface

I remember @marcaaron came up with a mock for sqlite that can be used to validate actual syntax or possible sqlite errors (because the mock executes sqlite under the hood)
When desired (specific test suite) it can be put in place, while the default can be to always use the generic storage mock (after all we're don't have any other mocks right now but localforage mock)

For example a good place to use sqlite mock is if we write tests for the provider

tests/unit/storage/providers/SQLiteStorageTest.js

Agree with all of the above.

When desired (specific test suite) it can be put in place, while the default can be to always use the generic storage mock (after all we're don't have any other mocks right now but localforage mock)

Agree and we can always come back later to do it. And who knows, maybe one day we will see SQLite in the browser?

kidroca · 2023-03-09T12:50:52Z

tests/unit/onyxCacheTest.js

@@ -400,8 +400,8 @@ describe('Onyx', () => {

    describe('Onyx with Cache', () => {
        let Onyx;
+        let LocalForageMock;


Let's just call this Storage or StorageMock - specific storage provider (or mock) shouldn't matter in tests - a feature should work the same regardless of the underlying storage provider (as long as interface match)

This means that we don't have to write tests for every storage provider - but use one generic storage mock

Technically if we have mocks for each storage provider we can re-run the test suite with another storage provider mock

But usually this has no benefit, because in either case it's just a mock - not the real thing
A test does not depend on a storage provider (unless it tests the actual provider), but on the assumption that if storage works "correctly" it results in the expected result

getItem returns a Promise with an item

setItem returns a Promise

the test is about whether we use the result of getItem correctly
the test is whether we call setItem with expected parameters
the specific storage provider doesn't matter

I think it would also make sense, to remove the LocalForageProviderTest.js tests, since we're not really testing localForage, only the mock

I think it would also make sense, to remove the LocalForageProviderTest.js tests, since we're not really testing localForage, only the mock

Not exactly - the test verifies we're using the underlying library localforage as intended
For example a test verifies that if we call StorageProvider.multiSet(pairs) then for each pair we call localforage.setItem with the expected key and value

So the test is would multiSet call setItem correctly for each pair - we don't have to use the real localforage library to test that

marcaaron · 2023-03-23T16:38:02Z

These changes look good to me. Like that we are cleaning it up. @Gonals is right that maybe we should create an issue for it. I'll do it now and update the description.

chrispader · 2023-03-28T08:32:31Z

Still need to fix some tests here... I'll let you know when those are done @kidroca @marcaaron

Should be done by tomorrow hopefully

chrispader · 2023-03-30T14:05:24Z

@marcaaron @kidroca I now got all tests except for 2 running. Could one of you maybe have a look at the remaining 2 tests?

Somehow, "onyxCacheTest.js > Should clean cache when connections to eviction keys happen" sometimes fails, and sometimes not 🤔 If you run the test by itself, it succeeds

"withOnyxTest > should update withOnyx subscriber multiple times when merge is used" also fails atm, i can't figure out why..

thank you in advance!

marcaaron · 2023-03-31T13:02:02Z

Hmm I would get something is wrong with the mock resolved value. Maybe the mock is getting reset at an unexpected time.

When I debug the test suite I can see the value is not resolved as expected and we get null it is probably something with the beforeEach() here:

react-native-onyx/tests/unit/onyxCacheTest.js

Lines 417 to 442 in 7ce6ae4

    
           function initOnyx(overrides) { 
        
               const OnyxModule = require('../../lib'); 
        
               Onyx = OnyxModule.default; 
        
               withOnyx = OnyxModule.withOnyx; 
        
               StorageMock = require('../../lib/storage').default; 
        
               cache = require('../../lib/OnyxCache').default; 
        
               Onyx.init({ 
        
                   keys: ONYX_KEYS, 
        
                   safeEvictionKeys: [ONYX_KEYS.COLLECTION.MOCK_COLLECTION], 
        
                   registerStorageEventListener: jest.fn(), 
        
                   maxCachedKeysCount: 10, 
        
                   ...overrides, 
        
               }); 
        
               // Onyx init introduces some side effects e.g. calls the getAllKeys 
        
               // We're clearing mocks to have a fresh calls history 
        
               return waitForPromisesToResolve() 
        
                   .then(() => jest.clearAllMocks()); 
        
           } 
        
           // Always use a "fresh" instance 
        
           beforeEach(() => { 
        
               jest.resetModules(); 
        
               return initOnyx(); 
        
           });

marcaaron · 2023-03-31T13:15:14Z

yes, confirmed jest.clearAllMocks(); is running after the test starts

marcaaron · 2023-03-31T13:21:20Z

Oh I see it now haha. We are clearing them when calling initOnyx() so you need to set up the resolved value after.

marcaaron · 2023-03-31T13:24:38Z

try this

diff --git a/tests/unit/onyxCacheTest.js b/tests/unit/onyxCacheTest.js
index bd2bf54..9cce84d 100644
--- a/tests/unit/onyxCacheTest.js
+++ b/tests/unit/onyxCacheTest.js
@@ -613,16 +613,16 @@ describe('Onyx', () => {
         });

         it('Should clean cache when connections to eviction keys happen', () => {
-            // Given storage with some data
-            StorageMock.getItem.mockResolvedValue('"mockValue"');
-            const range = _.range(1, 10);
-            StorageMock.getAllKeys.mockResolvedValue(_.map(range, n => `key${n}`));
-
             jest.useFakeTimers();

             // Given Onyx with LRU size of 3
             return initOnyx({maxCachedKeysCount: 3})
                 .then(() => {
+                    // Given storage with some data
+                    StorageMock.getItem.mockResolvedValue('"mockValue"');
+                    const range = _.range(1, 10);
+                    StorageMock.getAllKeys.mockResolvedValue(_.map(range, n => `key${n}`));
+
                     // When 4 connections for different keys happen
                     Onyx.connect({key: 'key1', callback: jest.fn()});
                     Onyx.connect({key: 'key2', callback: jest.fn()});

marcaaron · 2023-03-31T13:40:50Z

Oh hmm not quite actually, sorry. Definitely Onyx library is using the old StorageMock resolved values so that's the problem, but not entirely sure what the fix is.

kidroca · 2023-03-31T14:46:37Z

Hey, I'll take a look tonight as well

kidroca · 2023-03-31T19:01:34Z

This was a hard one

One of the problems is the sync queue used in the mock

I guess because it only resolves one promise at a time it does not integrate correctly with our waitForPromisesToResolve helper

Let's not have that in the mock, as a rule of thumb mocks should be as dumb as possible

we just want to verify they are getting called correctly
we can test actual queue and storage specific functionality when we test the actual storage provider, no need to simulate it in the mock

The other thing is with the new mock Storage.setItem would actually keep items in memory and in turn cause Storage.getAllKeys to return a result of some previously set keys
we have a before each that triggers

initOnyx
Onyx.init
addAllSafeEvictionKeysToRecentlyAccessedList
getAllKeys (This here adds the "old" keys to cache)
- in the past getAllKeys returned 0 keys and so the result was not cached

So when we run initOnyx for the first time some keys get cached, and even though we override Storage.getAllKeys to return other keys it is never used

Here's a small diff that fixes onyxCacheTest.js

Index: lib/storage/__mocks__/index.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/lib/storage/__mocks__/index.js b/lib/storage/__mocks__/index.js
--- a/lib/storage/__mocks__/index.js	(revision 7ce6ae45ea42bad394014f2b855ffc010dff5a1c)
+++ b/lib/storage/__mocks__/index.js	(date 1680287765397)
@@ -24,7 +24,8 @@
 
 const localForageMock = {
     setItem(key, value) {
-        return setItemQueue.push({key, value});
+        storageMapInternal[key] = value;
+        return Promise.resolve();
     },
     multiSet(pairs) {
         const setPromises = _.map(pairs, ([key, value]) => this.setItem(key, value));
Index: tests/unit/onyxCacheTest.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/tests/unit/onyxCacheTest.js b/tests/unit/onyxCacheTest.js
--- a/tests/unit/onyxCacheTest.js	(revision 7ce6ae45ea42bad394014f2b855ffc010dff5a1c)
+++ b/tests/unit/onyxCacheTest.js	(date 1680287882477)
@@ -414,13 +414,18 @@
             },
         };
 
-        function initOnyx(overrides) {
+        // Initialize clean modules before each test
+        // This reset top level static variables (in Onyx.js, OnyxCache.js, etc.)
+        beforeEach(() => {
+           jest.resetModules();
             const OnyxModule = require('../../lib');
             Onyx = OnyxModule.default;
             withOnyx = OnyxModule.withOnyx;
             StorageMock = require('../../lib/storage').default;
             cache = require('../../lib/OnyxCache').default;
+        });
 
+        function initOnyx(overrides) {
             Onyx.init({
                 keys: ONYX_KEYS,
                 safeEvictionKeys: [ONYX_KEYS.COLLECTION.MOCK_COLLECTION],
@@ -435,12 +440,6 @@
                 .then(() => jest.clearAllMocks());
         }
 
-        // Always use a "fresh" instance
-        beforeEach(() => {
-            jest.resetModules();
-            return initOnyx();
-        });
-
         it('Expect a single call to getItem when multiple components use the same key', () => {
             // Given a component connected to Onyx
             const TestComponentWithOnyx = withOnyx({

I see you've also added/modified some tests and now they implicitly depend on the mock having a sync queue
Can you write them in a different way, for example since our mock now has a internal storage map - multiMerge can just examine the end value in the storage map

I'll check why the other test is failing next week

kidroca · 2023-03-31T19:21:07Z

Actually here's the fix for the 2nd one

It's seems there's some promise we need to wait before starting calling Onyx.merge and trigger re-renders...

Index: tests/unit/withOnyxTest.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/tests/unit/withOnyxTest.js b/tests/unit/withOnyxTest.js
--- a/tests/unit/withOnyxTest.js	(revision 7ce6ae45ea42bad394014f2b855ffc010dff5a1c)
+++ b/tests/unit/withOnyxTest.js	(date 1680290315506)
@@ -1,5 +1,5 @@
 import React from 'react';
-import {render} from '@testing-library/react-native';
+import {act, render} from '@testing-library/react-native';
 import Onyx, {withOnyx} from '../../lib';
 import ViewWithText from '../components/ViewWithText';
 import ViewWithCollections from '../components/ViewWithCollections';
@@ -52,10 +52,13 @@
         const onRender = jest.fn();
         render(<TestComponentWithOnyx onRender={onRender} />);
 
-        Onyx.merge(`${ONYX_KEYS.COLLECTION.TEST_KEY}1`, {ID: 123});
-        Onyx.merge(`${ONYX_KEYS.COLLECTION.TEST_KEY}2`, {ID: 234});
-        Onyx.merge(`${ONYX_KEYS.COLLECTION.TEST_KEY}3`, {ID: 345});
-        return waitForPromisesToResolve()
+        return waitForPromisesToResolve()
+            .then(() => {
+                Onyx.merge(`${ONYX_KEYS.COLLECTION.TEST_KEY}1`, {ID: 123});
+                Onyx.merge(`${ONYX_KEYS.COLLECTION.TEST_KEY}2`, {ID: 234});
+                Onyx.merge(`${ONYX_KEYS.COLLECTION.TEST_KEY}3`, {ID: 345});
+            })
+            .then(waitForPromisesToResolve)
             .then(() => {
                 expect(onRender).toHaveBeenCalledTimes(4);
             });

marcaaron · 2023-04-03T15:01:53Z

damn! nice work @kidroca, you are a true JS 🕵️‍♂️

chrispader · 2023-04-05T13:27:49Z

Thanks a lot @kidroca and @marcaaron . This bug almost killed me 😅

chrispader · 2023-04-05T13:29:16Z

One of the problems is the sync queue used in the mock

I guess because it only resolves one promise at a time it does not integrate correctly with our waitForPromisesToResolve helper

Let's not have that in the mock, as a rule of thumb mocks should be as dumb as possible

we just want to verify they are getting called correctly

we can test actual queue and storage specific functionality when we test the actual storage provider, no need to simulate it in the mock

@kidroca did i get you correctly, that you suggest removing the sync queue from the mock completely? Would agree with that, makes sense that there should be no unnecessary overhead in the mock

chrispader · 2023-04-05T13:51:18Z

I see you've also added/modified some tests and now they implicitly depend on the mock having a sync queue
Can you write them in a different way, for example since our mock now has a internal storage map - multiMerge can just examine the end value in the storage map

Which tests do you mean?

This one is the only one i find that somehow depends on setItemQueue? https://github.com/chrispader/react-native-onyx/blob/remove-async-storage/tests/unit/storage/providers/LocalForageProviderTest.js#L106

chrispader · 2023-04-05T13:51:39Z

Also i have changed a few things in the storage mock, so maybe this is already obsolete

chrispader · 2023-04-05T13:52:11Z

Should be ready for review from my side! All tests are passing 🚀 Thanks a lot @kidroca @marcaaron

chrispader · 2023-04-05T13:56:03Z

Once this one is merged, we should test the changes in the JSON_PATCH PR..

Christoph Pader added 4 commits March 9, 2023 11:40

fix: remove async-storage

5374c72

fix: missing jest.fn() in localforage mock

f0f9091

fix: re-instantiate localForageMock and cache

24a8cd2

fix: onyxCacheTest

115de95

chrispader requested a review from a team as a code owner March 9, 2023 11:18

chrispader mentioned this pull request Mar 9, 2023

Implement JSON_PATCH() in Onyx.merge() in react-native-onyx(native) #238

Merged

melvin-bot bot requested review from Gonals and removed request for a team March 9, 2023 11:19

kidroca reviewed Mar 9, 2023

View reviewed changes

Christoph Pader added 3 commits March 10, 2023 16:22

re-write storage mock (WIP)

da58dd2

implement the rest of the storage mock

84833cc

use abstract storage mock

c50ef55

chrispader force-pushed the remove-async-storage branch from 2e52c5a to c50ef55 Compare March 14, 2023 13:25

Christoph Pader added 6 commits March 14, 2023 17:32

replace map with forEach in mock

c935ca0

mock LocalForage storage provider with storage mock

f302205

improve LocalForage mock and tests

d37b039

remove getter in storage mock

15dcd03

explicitly set storage mock in jestSetup

90adaa1

fix: typo

d970808

chrispader mentioned this pull request Mar 16, 2023

[Tracking] Implement JSON_PATCH() in Onyx.merge() in react-native-onyx(native) Expensify/App#13972

Closed

19 tasks

Merge branch 'main' into remove-async-storage

6dc3e7d

Christoph Pader added 4 commits March 29, 2023 16:45

simplify mock

9ccd49d

fix: mock

4b64d1a

rename mock to "StorageMock" everywhere

a67956c

use getter function instead of object directly

7ce6ae4

Christoph Pader added 4 commits April 5, 2023 15:36

remove SyncQueue from storage mock

5c1c60f

fix onyxCacheTest

624ddec

Fix withOnyxTest

bffefd0

Fix merge function in storage mock

bafc128

marcaaron assigned chrispader Apr 5, 2023

marcaaron approved these changes Apr 5, 2023

View reviewed changes

marcaaron merged commit ef462a2 into Expensify:main Apr 5, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove AsyncStorage dependency #240

Remove AsyncStorage dependency #240

chrispader commented Mar 9, 2023 •

edited by marcaaron

Loading

Gonals commented Mar 9, 2023

kidroca Mar 9, 2023 •

edited

Loading

marcaaron Mar 23, 2023

kidroca Mar 9, 2023

kidroca Mar 9, 2023

chrispader Mar 11, 2023

kidroca Mar 13, 2023

marcaaron commented Mar 23, 2023

chrispader commented Mar 28, 2023

chrispader commented Mar 30, 2023 •

edited

Loading

marcaaron commented Mar 31, 2023

marcaaron commented Mar 31, 2023

marcaaron commented Mar 31, 2023

marcaaron commented Mar 31, 2023

marcaaron commented Mar 31, 2023 •

edited

Loading

kidroca commented Mar 31, 2023

kidroca commented Mar 31, 2023

kidroca commented Mar 31, 2023

marcaaron commented Apr 3, 2023

chrispader commented Apr 5, 2023 •

edited

Loading

chrispader commented Apr 5, 2023

One of the problems is the sync queue used in the mock

chrispader commented Apr 5, 2023

chrispader commented Apr 5, 2023

chrispader commented Apr 5, 2023

chrispader commented Apr 5, 2023 •

edited

Loading

		jest.mock('./lib/storage/providers/SQLiteStorage', () => require('./lib/storage/providers/__mocks__/SQLiteStorage'));
		jest.mock('./lib/storage/NativeStorage', () => require('./lib/storage/__mocks__/NativeStorage'));

Remove AsyncStorage dependency #240

Remove AsyncStorage dependency #240

Conversation

chrispader commented Mar 9, 2023 • edited by marcaaron Loading

Details

Related Issues

Gonals commented Mar 9, 2023

kidroca Mar 9, 2023 • edited Loading

Choose a reason for hiding this comment

marcaaron Mar 23, 2023

Choose a reason for hiding this comment

kidroca Mar 9, 2023

Choose a reason for hiding this comment

kidroca Mar 9, 2023

Choose a reason for hiding this comment

chrispader Mar 11, 2023

Choose a reason for hiding this comment

kidroca Mar 13, 2023

Choose a reason for hiding this comment

marcaaron commented Mar 23, 2023

chrispader commented Mar 28, 2023

chrispader commented Mar 30, 2023 • edited Loading

marcaaron commented Mar 31, 2023

marcaaron commented Mar 31, 2023

marcaaron commented Mar 31, 2023

marcaaron commented Mar 31, 2023

marcaaron commented Mar 31, 2023 • edited Loading

kidroca commented Mar 31, 2023

kidroca commented Mar 31, 2023

One of the problems is the sync queue used in the mock

kidroca commented Mar 31, 2023

marcaaron commented Apr 3, 2023

chrispader commented Apr 5, 2023 • edited Loading

chrispader commented Apr 5, 2023

One of the problems is the sync queue used in the mock

chrispader commented Apr 5, 2023

chrispader commented Apr 5, 2023

chrispader commented Apr 5, 2023

chrispader commented Apr 5, 2023 • edited Loading

chrispader commented Mar 9, 2023 •

edited by marcaaron

Loading

kidroca Mar 9, 2023 •

edited

Loading

chrispader commented Mar 30, 2023 •

edited

Loading

marcaaron commented Mar 31, 2023 •

edited

Loading

chrispader commented Apr 5, 2023 •

edited

Loading

chrispader commented Apr 5, 2023 •

edited

Loading