Set default key states when clearing the cache #129

neil-marcellini · 2022-05-03T16:53:25Z

Details

When we clear Onyx and reset the cache set every value to null OR the default key state if one exists for that key. Remove the call to initializeWithDefaultKeyStates since it is no longer needed. These changes prevent updating Onyx subscribers with out of date information by sending only the defaultKeyStates. See the issue for more info and an example.

Related Issues

#125

Automated Tests

onyxClearNativeStorageTest.js and onyxClearWebStorageTest.js

Manual Tests

I had a lot of trouble adding an automated tests earlier so I also added manual tests.

Checkout the App branch for Case 1
npm i to install this Onyx version that has been modified for testing

For each case below:

Checkout the App branch
npm run web
Sign in to any account
Open the console
Sign out
Make sure there are no assertion errors in the console
For cases 1 and 2 only:
command + f in the console and search 'Onyx test'
Make sure that the cache clears, then data is set or merged, and then Onyx finishes clearing.

Case 1: neil-test-onyx-clear-merge

Onyx.clear();
Onyx.merge('someDefaultKey', 'someValue');

// Expect the value of the merge to replace the default key state
// Expect that storage matches what we have in the cache and that our call to 
// `Storage.clear()` did not clear the value of merge from storage after we called Onyx.clear()

Case 2: neil-test-onyx-clear-set

Onyx.clear();
Onyx.set('someDefaultKey', 'someValue');

// Expect the value of the set to replace the default key state
// Expect that storage matches what we have in the cache and that our call 
// to `Storage.clear()` did not prevent any values from being set

Case 3: neil-test-onyx-set-clear

Onyx.set('someDefaultKey', 'someValue');
Onyx.clear();

// Expect the value from the `set()` to be replaced with the default key state value

Case 4: neil-test-onyx-merge-clear

Onyx.merge('someDefaultKey', 'someValue');
Onyx.clear();

// Expect the value from the `merge()` to be replaced with the default key state value

Linked PRs

Expensify/App#8855

lib/Onyx.js

neil-marcellini · 2022-05-03T19:17:24Z

How can I get Onyx.merge or Onyx.set to run between clearing the cache and clearing the storage? I tried inserting a lot of data into storage before clearing, but that didn't work. I need some kind of callback in between, or I need to make Storage.clear take a really long time.

Right now I have this test for Case 1:

it('should store merged values when calling merge on a default key after clear', () => {
        let defaultValue;
        connectionID = Onyx.connect({
            key: ONYX_KEYS.DEFAULT_KEY,
            initWithStoredValues: false,
            callback: val => defaultValue = val,
        });
        console.log('Calling clear in test');
        const afterClear = Onyx.clear();
        console.log('Calling merge');
        const afterMerge = Onyx.merge(ONYX_KEYS.DEFAULT_KEY, 'merged');
        return Promise.all([afterClear, afterMerge])
            .then(() => {
                console.log('defaultValue = ', defaultValue);
                expect(defaultValue).toEqual('merged');
                return Storage.getItem(ONYX_KEYS.DEFAULT_KEY);
            })
            .then((storedValue) => {
                const cachedValue = cache.getValue(ONYX_KEYS.DEFAULT_KEY);
                console.log('cachedValue = ', cachedValue);
                console.log('storedValue = ', storedValue);
                expect(cachedValue).toEqual(storedValue);
            });
    });

I'm also console logging when Onyx starts clearing Storage and when it stops:

console.log
      Calling clear in test

      at Object.<anonymous> (tests/unit/onyxTest.js:329:17)

    console.log
      [Onyx.clear] clearing storage

      at Object.clear (lib/Onyx.js:721:23)

    console.log
      [Onyx.clear] finished clearing storage

      at Object.clear (lib/Onyx.js:723:23)

    console.log
      Calling merge

      at Object.<anonymous> (tests/unit/onyxTest.js:331:17)

    console.log
      defaultValue =  merged

      at fn (tests/unit/onyxTest.js:335:25)

    console.log
      cachedValue =  undefined

      at fn (tests/unit/onyxTest.js:341:25)

marcaaron · 2022-05-03T20:14:03Z

How can I get Onyx.merge or Onyx.set to run between clearing the cache and clearing the storage? I tried inserting a lot of data into storage before clearing, but that didn't work. I need some kind of callback in between, or I need to make Storage.clear take a really long time.

I'm not sure if I understand the question exactly, but merge() or set() will start working as soon as it can. Which would be when any synchronous logic in Onyx.clear() finishes - which looks like getAllKeys(). I'd expect something like this:

clear() -> getAllKeys() - which is a Promise
merge() -> push value into merge queue or possibly call get() - which is a Promise
What happens next depends on when those promises resolve...

We can't call really call Onyx.merge() while Storage.clear() is running if we are using Onyx.clear(). We can only wait for that entire Onyx.clear() promise to resolve.

One idea - though I'm not sure if it will work - is to add the ability to make the Storage.clear mock delayed. Then we can use fake timers to have more control over how long different parts of a larger promise are taking.

neil-marcellini · 2022-05-03T23:36:32Z

One idea - though I'm not sure if it will work - is to add the ability to make the Storage.clear mock delayed. Then we can use fake timers to have more control over how long different parts of a larger promise are taking.

I can understand how we could create a version of Storage.clear that is delayed, but I don't get how we would make Onyx.clear() call that version.

I thought about adding an optional callback param to Onyx.clear(). By doing this I can get merge or set to run in between. The problem with this approach of course is that the code is only used for the tests. I also tried injecting the code into Onyx.clear during the test and then calling eval, based on this SO answer, but I couldn't get it to work and it's kind of a messy solution.

function clear(testingCallback = null) {
    return getAllKeys()
        .then((keys) => {
            _.each(keys, (key) => {
                const resetValue = lodashGet(defaultKeyStates, key, null);
                keyChanged(key, resetValue);
                cache.set(key, resetValue);
            });
        })
        .then(() => new Promise((resolve) => {
            if (testingCallback) {
                resolve(testingCallback());
            } else {
                resolve();
            }
        }))
        .then(Storage.clear)
}

marcaaron · 2022-05-04T01:44:47Z

I can understand how we could create a version of Storage.clear that is delayed, but I don't get how we would make Onyx.clear() call that version.

With jest you can mock whatever you want which will force Onyx.clear() to use that instead of what it would have used.

Actually, there are some tests using a mock already since node doesn't have "storage"...

react-native-onyx/__mocks__/localforage.js

Lines 29 to 34 in f6c2b03

    
           clear() { 
        
               return new Promise((resolve) => { 
        
                   storageMap = {}; 
        
                   resolve(); 
        
               }); 
        
           },

Maybe just create something like Storage.setDelayForClear() and call it from your test?

https://jestjs.io/docs/manual-mocks

marcaaron · 2022-05-04T01:47:14Z

Here's an example where we took the mock and added our own test functionality to it by setting some "initial data"

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

Lines 3 to 18 in 2d42566

    
           import localforageMock from '../../__mocks__/localforage'; 
        
           const ONYX_KEYS = { 
        
               COLLECTION: { 
        
                   TEST_KEY: 'test_', 
        
               }, 
        
           }; 
        
           const initialTestObject = {a: 'a'}; 
        
           const initialData = { 
        
               test_1: initialTestObject, 
        
               test_2: initialTestObject, 
        
               test_3: initialTestObject, 
        
           }; 
        
           localforageMock.setInitialMockData(initialData);

neil-marcellini · 2022-05-04T15:25:29Z

Awesome, thanks so much for steering me in the right direction here. Jest is new to me and I struggled with it quite a bit yesterday, but hopefully I can get it working today.

neil-marcellini · 2022-05-05T21:34:30Z

I went down a bit of a rabbit hole again unfortunately, but I learned a few things in the process.

When calling merge or set after clear the value will get set in the cache right away. However, Storage.setItem from set won't get called until Storage.clear finishes. I refactored my test for Case 1 to address that. I'll finish up the other tests later.

If we merge and then clear storage the defaultKeyState will not be set in the storage. Default key states are not set in storage after clearing on this branch or on main. I don't think this is a problem because if we close the tab and kill the cache, the default key states will be re-set in the cache when Onyx initializes.

neil-marcellini · 2022-05-06T19:07:24Z

Ready for review now. For cases 3 and 4, I did not include an assertion that the stored value matches the cached value after setting data and then clearing Onyx.

If we merge and then clear storage the defaultKeyState will not be set in the storage. Default key states are not set in storage after clearing on this branch or on main. I don't think this is a problem because if we close the tab and kill the cache, the default key states will be re-set in the cache when Onyx initializes.

marcaaron · 2022-05-06T19:44:42Z

Sorry, @neil-marcellini I'm going OOO for a bit.

List of all contributors here might be a good bet for a replacement reviewer:

https://github.com/Expensify/react-native-onyx/graphs/contributors

@kidroca and @tgolen may be able to help as well

tgolen · 2022-05-09T03:27:05Z

Where are the unit tests being added?

tgolen

These changes look fine.

kidroca · 2022-05-09T12:06:51Z

I had a lot of trouble adding an automated test where I could mock the Storage.clear method because keepInstancesSync overrides the clear method when Onyx initializes. I added manual tests instead.

Depending on how you want to mock Storage, you can either init Onyx with shouldSyncMultipleInstances: false and then monitor localForage calls. Initializing Onyx with shouldSyncMultipleInstances: false would skip applying keepInstancesSync

react-native-onyx/lib/Onyx.js

Lines 844 to 848 in ce5bd2c

    
           if (shouldSyncMultipleInstances && _.isFunction(Storage.keepInstancesSync)) { 
        
               Storage.keepInstancesSync(keysToDisableSyncEvents, (key, value) => { 
        
                   cache.set(key, value); 
        
                   keyChanged(key, value); 
        
               });

Or you can override the storage mock like so:

unit-test.js

import Storage from '../../lib/storage';

// makes `Storage` imported above to be the one from `storage/__mocks__`
jest.mock('../../lib/storage');

// we can now override methods like remove the `keepInstancesSync` and/or change the `clear` method
delete Storage.keepInstancesSync;
Storage.clear = jest.fn(() => Promise.resolve());

// The important bit is to import/init Onyx only after `Storage` had been overridden
const Onyx = require('../../index').default;

describe('My tests', () => {
  /* ... */
});

I can help writing unit tests if you'd like

neil-marcellini · 2022-05-09T16:38:22Z

Based on this slack thread Marc suggested that we should not set shouldSyncMultipleInstances: false since we wouldn't be testing the actual Storage.clear behavior.

However, you made me realize that I could mock Storage.keepInstancesInSync and add a delay to the clear method when I mock it. Also, how could I create a storage mock for AsyncStorage rather than WebStorage and tell Onyx to use that mock for Storage?

Since I have a lot of fixes blocked on this change and the manual tests work well, may I go ahead and merge it now? Or should we add the unit tests first? @tgolen @kidroca

tgolen · 2022-05-09T16:49:47Z

I personally feel like any change to Onyx (especially fixing or changing behavior) should always be done from a unit-test first approach. So, unless there is any reason to move forward without having unit tests (like it's fixing a fire or something), then I think we should wait until the unit tests are done.

Admittedly, I don't know the context of all the fixes blocked on this change though.

neil-marcellini · 2022-05-09T16:59:28Z

Ok, that makes sense. I'm not fixing a fire here. All of the transition flow fixes are blocked on this but I don't think it's super urgent. I will work on the unit tests again and I'll ask @kidroca for help when I need it. Thanks for reviewing!

kidroca · 2022-05-09T17:49:35Z

Also, how could I create a storage mock for AsyncStorage rather than WebStorage and tell Onyx to use that mock for Storage?

By default Onyx uses NativeStorage which is based on AsyncStorage. I can't remember why but in unit tests code resolves and uses the native case here:

react-native-onyx/lib/storage/index.js

Lines 3 to 6 in ce5bd2c

    
           const Storage = Platform.select({ 
        
               default: () => require('./WebStorage').default, 
        
               native: () => require('./NativeStorage').default, 
        
           })();

So unless we call jest.mock('../../lib/storage') the unit test (file) would use NativeStorage by default

In unit tests you can simply import Storage and override whatever you need, everything else would be based on AsyncStorage

specifically the mock they provide for unit tests: '@react-native-async-storage/async-storage/jest/async-storage-mock'

unit-test.js

import Storage from '../../lib/storage';

Storage.clear = jest.fn(() => Promise.resolve());

describe('My tests', () => {
  /* ... */
});

If you want to be more explicit about it you can instead specify a mock like so:

unit-test.js

import Storage from '../../lib/storage';

jest.mock('../../lib/storage', () => {
  const NativeStorage = require('../../lib/storage/NativeStorage').default;
  NativeStorage.clear = jest.fn(() => Promise.resolve());
  return NativeStorage;
});

describe('My tests', () => {
  /* ... */
});

kidroca · 2022-05-09T18:01:44Z

Since in unit tests it's not possible to use a real storage provider, trying to mimic specific storage provider behavior (like keepInstancesInSync) shouldn't be necessary - that's internal to the storage provider and can be tested separately

The functionality you're changing depends solely on Storage.clear - e.g. it assumes that Storage.clear works no matter the underlying storage provider, so we should just provide a mock that stubs Storage.clear in all desired scenarios we want to test

To assert keepInstancesInSync works as expected we should write a test for the WebStorage provider where we verify clear works as expected

notifies other tabs
returns a promise
etc...

neil-marcellini · 2022-05-24T23:04:58Z

I addressed Tim's comments and I followed up with the plan from our call. I still have a question from #129 (comment) and I would appreciate another draft review. I'm assuming that I should still add these test cases for web storage as well?

marcaaron

Thanks @neil-marcellini, I gave this a more thorough review. Ultimately, I think the code is looking fine, but I'm having a really difficult time making sense out of the tests (but appreciate you taking the time to look into them).

I think at this point it would be best if we greatly simplified the testing portion here and just test whatever we reasonably can without introducing delays and creative mocks. If we run into an actual issue with the code we can try to resolve it and react by creating a test.

lib/Onyx.js

marcaaron · 2022-05-24T23:48:25Z

lib/Onyx.js

+                const resetValue = lodashGet(defaultKeyStates, key, null);
+                cache.set(key, resetValue);
+
+                // Optimistically inform subscribers on the next tick


I'll add the context here so we can stay in this thread. @neil-marcellini said

On our call I forgot to mention that I added this line to fix test case 4 (merge then clear). This line sends the default value to subscribers after the merge value. I will update the comment to explain that.

Not sure I get it. If we are calling a merge() then a clear() in the same tick then wouldn't the correct behavior be for the clear() to cancel the merge()... ?

tests/unit/onyxClearNativeStorageTest.js

tests/unit/onyxClearWebStorageTest.js

neil-marcellini · 2022-05-25T15:56:56Z

Thanks for the review Marc! I should have mentioned that I'm not done with the WebStorage tests yet. Overall I think I made these tests too complicated. I was tracking the resolve order of the Storage calls to be explicit about when each call completes, but it's really not necessary so I will remove all of that complexity.

I was putting the Storage calls into a queue based off of this comment, but maybe I interpreted this wrong and it is also unnecessary.

I think IndexedDB (web) and AsyncStorage (native) has mechanisms to queue transactions so merge would happen after clear, but our cache has no such logic

I will try to greatly simplify these tests while still making sure that merge() or set() happens between the cache and storage clearing.

neil-marcellini · 2022-05-25T18:35:20Z

tests/unit/onyxClearNativeStorageTest.js

+                const cachedValue = cache.getValue(ONYX_KEYS.DEFAULT_KEY);
+                expect(cachedValue).toBe(MERGED_VALUE);
+                const storedValue = Storage.getItem(ONYX_KEYS.DEFAULT_KEY);
+                return expect(storedValue).resolves.toBe(MERGED_VALUE);


The storedValue is resolving to null and I have no idea why. From some logging I added it looks like Storage.setItem(ONYXKEYS.DEFAULT_KEY, MERGED_VALUE) called from Onyx.set is the last call to resolve before the assertions, so why is the value null?

Let's look at what Storage.getItem() doing in the context of this test...

It will get the value inside the mock here so I'd imagine that this code is running and clearing the value.

It seems like this validates my concern that a value merged after Onyx.clear() will not actually make it into device storage, but does make it to the cache. Though, I'm not sure whether this is a problem or not for our applications using Onyx.

If it causes no issues for us in App I think it would be best to create an issue to improve this later and document that it's a known issue.

It seems like this validates my concern that a value merged after Onyx.clear() will not actually make it into device storage, but does make it to the cache.

Yeah I think you are right. I did some logging with the web storage test and what I'm seeing is that merge() is called before the cache is cleared from clear(). I would like to go back to my previous test where I call merge() in between the cache and storage clearing. That test works and it is the case that this PR aims to fix.

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

Lines 50 to 56 in 1781223

Storage.clear = jest.fn(() => {

// WHEN merge is called between the cache and storage clearing, on a key with a default key state

Onyx.merge(ONYX_KEYS.DEFAULT_KEY, MERGED_VALUE);

AsyncStorageMock.clear();

return waitForPromisesToResolve();

});

Onyx.clear();

If it causes no issues for us in App I think it would be best to create an issue to improve this later and document that it's a known issue.

I don't think it creates any issues in the App and I agree that we should create an issue to improve it later.

I would like to go back to my previous test where I call merge() in between the cache and storage clearing. That test works and it is the case that this PR aims to fix.

Can we back up and re-state what it is exactly that we are trying to test with recreating the "in between the cache and storage clearing" condition?

If this test is failing I think that's fine, but we must change the assertion and add some documentation that basically explains how there is a remote possibility that values might not be saved to device storage and that it's a known issue.

Can we back up and re-state what it is exactly that we are trying to test with recreating the "in between the cache and storage clearing" condition?

I think I was trying to test to see if Storage.setItem() waits for Storage.clear() to finish before it runs, based on this Slack comment. However, embarrassingly, I'm still not quite sure why I tried to test in between the cache and storage clearing.

If this test is failing I think that's fine, but we must change the assertion and add some documentation that basically explains how there is remove possibility that values might not be saved to device storage and that it's a known issue.

Ok. It seems that any Onyx action to set data around Onyx.clear() might be unreliable because the data that gets set in Onyx, the cache, and Storage depends on how long it takes async calls to run. The async calls include Onyx.get(), Onyx.getAllKeys() and Storage.clear() and Storage.setItem().

It seems that any Onyx action to set data around Onyx.clear() might be unreliable because the data that gets set in Onyx, the cache, and Storage depends on how long it takes async calls to run.

I think it's less drastic and probably more common that Storage.clear() happens after we set a value because of the general order of operations and cached values that we are working with.

Onyx.set() immediately saves to storage

react-native-onyx/lib/Onyx.js

Lines 540 to 556 in b61c904

function set(key, value) {

// Logging properties only since values could be sensitive things we don't want to log

Logger.logInfo(`set() called for key: ${key}${_.isObject(value) ? ` properties: ${_.keys(value).join(',')}` : ''}`);

// eslint-disable-next-line no-use-before-define

if (hasPendingMergeForKey(key)) {

Logger.logAlert(`Onyx.set() called after Onyx.merge() for key: ${key}. It is recommended to use set() or merge() not both.`);

}

// Adds the key to cache when it's not available

cache.set(key, value);

// Optimistically inform subscribers on the next tick

Promise.resolve().then(() => keyChanged(key, value));

// Write the thing to persistent storage, which will trigger a storage event for any other tabs open on this domain

return Storage.setItem(key, value)

Onyx.merge() has to wait for get() before calling set() (the get value is usually cached so we will 99% of the time just continue on the next tick and not a random time)

react-native-onyx/lib/Onyx.js

Lines 667 to 683 in b61c904

function merge(key, value) {

if (mergeQueue[key]) {

mergeQueue[key].push(value);

return Promise.resolve();

}

mergeQueue[key] = [value];

return get(key)

.then((data) => {

try {

const modifiedData = applyMerge(key, data);

// Clean up the write queue so we

// don't apply these changes again

delete mergeQueue[key];

return set(key, modifiedData);

Onyx.clear() has to wait for getAllKeys() before it calls Storage.clear() (this value is also cached so we will continue on the next tick not a random time)

react-native-onyx/lib/Onyx.js

Lines 713 to 721 in b61c904

function clear() {

return getAllKeys()

.then((keys) => {

_.each(keys, (key) => {

keyChanged(key, null);

cache.set(key, null);

});

})

.then(Storage.clear)

Here are my thoughts...

clear -> set - 100% of the time a set after clear will not get saved to storage

clear -> merge - If we have a merge after clear it is not possible for Storage.clear() to get called after the set() inside merge() because it will always run before the get() in merge() resolves - no matter how long the get() takes.

merge -> clear - Our get() in merge() would have to take longer than getAllKeys() and Storage.clear() and in that case it seems possible for it to set a stale value into storage. This would be very rare since most values will immediately return from the cache.

set -> clear - There is no promise in set() so it's impossible for us to set a stale value as Storage.setItem() will always run before Storage.clear()

Only 1 and 3 seem like problems we might want to solve, but it's not clear if we need to yet.

One idea for how to fix this would be something like:

set after clear solution:

Set an internal flag that Onyx.clear() is still running and hasn't fully cleared storage.

While that happens batch any new writes to storage or flag which keys need to be saved after we are done, but allow cached values to be set, merged, whatever.

Once Storage.clear() is done we can sync up any queued "writes" with the values in the cache.

merge before clear solution:

Add a "cancelled" state to any merges going into the merge queue here:

react-native-onyx/lib/Onyx.js

Lines 668 to 671 in b61c904

if (mergeQueue[key]) {

mergeQueue[key].push(value);

return Promise.resolve();

}

When we call clear() flag all the things in the merge queue as cancelled.

Don't process any cancelled items when applying the merge.

There could be other solutions but I'm not sure if we need these ones or others.

Currently case 2 is the only one that fails in my automated tests. Case 1 also fails but I have deleted it. All cases work in the manual tests.

Thank you for outlining the potential solutions. Could we please leave those for a follow up? Should I add an example and documentation for case 2 to the Onyx.clear() JSDoc? Can I remove case 2 like I removed case 1?

Could we please leave those for a follow up?

Yeah I am not sure we even need to follow up. Creating an issue could be good but it doesn't seem like a priority right now.

Should I add an example and documentation for case 2 to the Onyx.clear() JSDoc?

That sounds good and yes remove whichever cases you think make sense to remove based on this.

neil-marcellini · 2022-05-26T23:23:12Z

@marcaaron I was tinkering around and found that if I call Storage.clear() in the same callback when the cache clears then clear -> merge works. I think this change makes sense because it makes Onyx.clear() more "atomic". Do you think that change is good? If not I will revert it and add some documentation for why this case doesn't work.

Also, with this change and for the native storage test exclusively, it is escaping the quotes in the value. Do you know why that is or how to get around it? I did find this SO that sort of explains why, but not how to fix it.

marcaaron · 2022-05-27T00:09:50Z

I was tinkering around and found that if I call Storage.clear() in the same callback when the cache clears then clear -> merge works.

Ok so moving the Storage.clear() out of the .then() move it up in the order of operations so that what I am describing here works?

clear -> merge - If we have a merge after clear it is not possible for Storage.clear() to get called after the set() inside merge() because it will always run before the get() in merge() resolves - no matter how long the get() takes.

Maybe I was wrong about that, but the change you are suggesting now makes it true?

marcaaron · 2022-05-27T00:11:28Z

for the native storage test exclusively, it is escaping the quotes in the value

Hmm not entirely sure but seems maybe related to stringifying JSON. Maybe try something that isn't a string as a value - object e.g.

neil-marcellini · 2022-05-27T00:48:30Z

Maybe I was wrong about that, but the change you are suggesting now makes it true?

Yes

Hmm not entirely sure but seems maybe related to stringifying JSON. Maybe try something that isn't a string as a value - object e.g.

It's super weird. It seems like the mock isn't very good. I wrote a work-around using integers and parseInt since the storage returned them as a string. Hopefully that's ok. All of the tests are passing now.

marcaaron · 2022-05-27T00:53:26Z

lib/Onyx.js

-        })
-        .then(Storage.clear)
-        .then(initializeWithDefaultKeyStates);
+            Storage.clear();


NAB

Suggested change

Storage.clear();

return Storage.clear();

marcaaron · 2022-05-27T00:54:41Z

lib/Onyx.js

+                const resetValue = lodashGet(defaultKeyStates, key, null);
+                cache.set(key, resetValue);
+
+                // Optimistically inform subscribers on the next tick


I still think we should improve this later but the context is here so I'm good with this.

OOO

Set default key states when clearing the cache

a1068f2

neil-marcellini requested a review from marcaaron May 3, 2022 16:53

neil-marcellini self-assigned this May 3, 2022

neil-marcellini mentioned this pull request May 3, 2022

Trigger callbacks with merged cache data when initializing with default key states #126

Closed

marcaaron reviewed May 3, 2022

View reviewed changes

lib/Onyx.js Outdated Show resolved Hide resolved

neil-marcellini marked this pull request as ready for review May 6, 2022 19:04

neil-marcellini requested a review from a team as a code owner May 6, 2022 19:04

melvin-bot bot requested review from danieldoglas and removed request for a team May 6, 2022 19:04

neil-marcellini requested a review from tgolen May 6, 2022 20:27

neil-marcellini mentioned this pull request May 6, 2022

Web - Inbox - User has to click "Continue setup" CTA twice in olddot to display "Connect bank account" page in the right sidebar Expensify/App#8295

Closed

tgolen previously approved these changes May 9, 2022

View reviewed changes

Test clear then merge native storage with logs

dcb7f01

neil-marcellini dismissed tgolen’s stale review via dcb7f01 May 9, 2022 19:13

neil-marcellini added 5 commits May 24, 2022 15:36

Remove test for clear then set

8037915

Add a comment to Onyx.clear about clear then set

5478ec9

Add a given comment to the clear then merge test

62d94fc

Finish GWT comments for onyxClearNativeStorageTest

0db8b95

Add a comment about the storageCallQueue

a37d766

neil-marcellini requested review from tgolen and marcaaron May 24, 2022 23:02

marcaaron requested changes May 25, 2022

View reviewed changes

neil-marcellini added 2 commits May 25, 2022 09:05

Remove storage call tracking and queue

1781223

Simple clear then merge test fails

a1909b4

neil-marcellini commented May 25, 2022

View reviewed changes

neil-marcellini added 5 commits May 25, 2022 13:05

Update web storage tests

94196a3

Add parentheses to method calls in clear() comment

aa1d333

Example and more explanation for the clear comment

f9de727

Use the web storage mock in Onyx clear tests

08b07dc

Clear the cache and storage in the same tick

6a2ce58

Fix test by reading integers from storage

b3eb9d9

marcaaron marked this pull request as ready for review May 27, 2022 00:51

marcaaron approved these changes May 27, 2022

View reviewed changes

marcaaron merged commit bac8d30 into main May 27, 2022

This was referenced May 27, 2022

initializeWithDefaultKeyStates overrides data that exists in the Onyx cache #125

Closed

Onyx.clear() will wipe Onyx.set() from Storage #144

Open

Beamanator mentioned this pull request Jul 8, 2022

Allow clear in onyx update #156

Merged

	Storage.clear = jest.fn(() => {
	// WHEN merge is called between the cache and storage clearing, on a key with a default key state
	Onyx.merge(ONYX_KEYS.DEFAULT_KEY, MERGED_VALUE);
	AsyncStorageMock.clear();
	return waitForPromisesToResolve();
	});
	Onyx.clear();

	function set(key, value) {
	// Logging properties only since values could be sensitive things we don't want to log
	Logger.logInfo(`set() called for key: ${key}${_.isObject(value) ? ` properties: ${_.keys(value).join(',')}` : ''}`);

	// eslint-disable-next-line no-use-before-define
	if (hasPendingMergeForKey(key)) {
	Logger.logAlert(`Onyx.set() called after Onyx.merge() for key: ${key}. It is recommended to use set() or merge() not both.`);
	}

	// Adds the key to cache when it's not available
	cache.set(key, value);

	// Optimistically inform subscribers on the next tick
	Promise.resolve().then(() => keyChanged(key, value));

	// Write the thing to persistent storage, which will trigger a storage event for any other tabs open on this domain
	return Storage.setItem(key, value)

	function merge(key, value) {
	if (mergeQueue[key]) {
	mergeQueue[key].push(value);
	return Promise.resolve();
	}

	mergeQueue[key] = [value];
	return get(key)
	.then((data) => {
	try {
	const modifiedData = applyMerge(key, data);

	// Clean up the write queue so we
	// don't apply these changes again
	delete mergeQueue[key];

	return set(key, modifiedData);

	function clear() {
	return getAllKeys()
	.then((keys) => {
	_.each(keys, (key) => {
	keyChanged(key, null);
	cache.set(key, null);
	});
	})
	.then(Storage.clear)

Set default key states when clearing the cache #129

Set default key states when clearing the cache #129

Conversation

neil-marcellini commented May 3, 2022 • edited Loading

Details

Related Issues

Automated Tests

Manual Tests

Linked PRs

neil-marcellini commented May 3, 2022

marcaaron commented May 3, 2022

neil-marcellini commented May 3, 2022

marcaaron commented May 4, 2022

marcaaron commented May 4, 2022

neil-marcellini commented May 4, 2022

neil-marcellini commented May 5, 2022

neil-marcellini commented May 6, 2022

marcaaron commented May 6, 2022

tgolen commented May 9, 2022

tgolen left a comment

Choose a reason for hiding this comment

kidroca commented May 9, 2022

unit-test.js

neil-marcellini commented May 9, 2022

tgolen commented May 9, 2022

neil-marcellini commented May 9, 2022

kidroca commented May 9, 2022

unit-test.js

unit-test.js

kidroca commented May 9, 2022 • edited Loading

neil-marcellini commented May 24, 2022

marcaaron left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

neil-marcellini commented May 25, 2022

neil-marcellini May 25, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marcaaron May 26, 2022 • 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

neil-marcellini commented May 26, 2022

marcaaron commented May 27, 2022

marcaaron commented May 27, 2022

neil-marcellini commented May 27, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

neil-marcellini commented May 3, 2022 •

edited

Loading

kidroca commented May 9, 2022 •

edited

Loading

marcaaron left a comment •

edited

Loading

neil-marcellini May 25, 2022 •

edited

Loading

marcaaron May 26, 2022 •

edited

Loading

neil-marcellini commented May 27, 2022 •

edited

Loading