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

neil-marcellini · 2022-04-19T00:08:47Z

Details

Merge user provided default key value pairs with not only the data in storage, but also the data in the cache. See the issue for more context.

Related Issues

#125

Automated Tests

Linked PRs

Expensify/App#8676

github-actions · 2022-04-19T00:09:04Z

CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅

neil-marcellini · 2022-04-19T00:10:35Z

I have read the CLA Document and I hereby sign the CLA

marcaaron · 2022-04-19T02:17:20Z

lib/Onyx.js

            const merged = lodashMerge(asObject, defaultKeyStates);
            cache.merge(merged);
-            _.each(merged, (val, key) => keyChanged(key, val));
+            _.each(merged, (val, key) => {
+                const currentValue = cache.getValue(key);
+                keyChanged(key, currentValue);
+            });


question: It looks like merged is going to end up as the combined result of any existing values for these keys + the defaultKeyStates object? I might just be missing it, but why would it be necessary to ask the cache for this data if merged is the combined result already?

In the case that I'm trying to fix, merged contains only the defaultKeyStates as the Storage is empty. However, the cache is not empty. So if the defaultKeyStates are not merged with the value in the cache then keyChanged is called without up to date values.

However, the cache is not empty.

thought: If we are signing out then both the cache and storage should be empty. Am I missing something?

react-native-onyx/lib/Onyx.js

Lines 711 to 721 in 2d42566

function clear() {

return getAllKeys()

.then((keys) => {

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

keyChanged(key, null);

cache.set(key, null);

});

})

.then(Storage.clear)

.then(initializeWithDefaultKeyStates);

}

It is also surprising to me that the cache is not empty but the storage is. My guess is that Storage.clear takes a while to complete, and during that time the new user is logged in and lots of values are set in Onyx.

When are values written from the cache to storage? Can the storage be out of date temporarily? I will try to look through the code and answer my own questions, but if you can give me a quick explanation / point me in the right direction that would be great.

When are values written from the cache to storage?

I don't think values from the cache are ever written to storage. We should be setting values in the cache and then writing them to storage (async) at the same time.

Can the storage be out of date temporarily?

I'm not sure if I have enough context to explain a situation where the storage could be out of date. But it certainly seems possible.

I have verified that signing in the new user occurs before Onyx.clear() finishes. Waiting to sign the user in until the storage clears could work, and I guess we could do that, but wouldn't it be better to not wait for that? I also think that this solution makes more sense. Please see the issue for more context and an example if you haven't seen that already.

Waiting to sign the user in until the storage clears could work, and I guess we could do that, but wouldn't it be better to not wait for that?

clarification: By better do we mean faster? Or something else.

thought: Signing out a user and signing in a new one seems like a rare event. So maybe performance is not a concern (assuming that is the concern).

I also think that this solution makes more sense.

I do see how it solves the immediate issue. But just want to make sure we're picking the best solution. I'm not sure if allowing data to be set while we are in the process of re-initializing storage makes sense. I think my expectation is that we'd defer those updates until the app has finished one "clean" cycle.

Mind if we go back to this question?

Should we completely sign the previous user out before starting a new sign in process?

clarification: By better do we mean faster? Or something else.

Yes. I think you're right that it probably wouldn't slow things down too much and that this flow is rare.

I'm not sure if allowing data to be set while we are in the process of re-initializing storage makes sense.

It would be up to the user of Onyx to make sure that no data is set while re-initializing storage right? That seems slightly less robust. If any initialKey is set while re-initializing storage then it could send an out of date value to its subscribers. How do you make your whole app wait while Onyx clears and re-initializes?

Should we completely sign the previous user out before starting a new sign in process?

Probably, yes. I will work on a PR to do that. If that works and we agree that it's a better solution, then I'm happy to do that instead.

It would be up to the user of Onyx to make sure that no data is set while re-initializing storage right?

Yep!

That seems slightly less robust. If any initialKey is set while re-initializing storage then it could send an out of date value to its subscribers. How do you make your whole app wait while Onyx clears and re-initializes?

Onyx.clear() returns a promise so that would be my first instinct. But maybe there's a different solution. Might be something we can get some more opinions on too? I'm not 100% what the best answer would be, but just wanted to think about it for a second. Thanks for hearing me out!

neil-marcellini · 2022-04-20T18:09:42Z

I'm going to convert this to a draft while I add a unit test for updating a key with the initialKeyStates after Onyx.clear().

neil-marcellini · 2022-04-20T22:27:12Z

I couldn't figure out how to add a test where I read a value when the cache is out of date.

marcaaron · 2022-04-20T22:46:22Z

I couldn't figure out how to add a test where I read a value when the cache is out of date.

Can you describe the conditions that would lead the storage to go out of date?

neil-marcellini · 2022-04-20T23:38:17Z

Can you describe the conditions that would lead the storage to go out of date?

Storage is cleared
While storage is being cleared Onyx.set is called
The value is set in the cache
initializeWithDefaultKeyStates runs
The object created from Storage.multiGet(_.keys(defaultKeyStates)) is empty. The storage is out of date.
keyChanged is called with the value being only the defaultKeyStates

Does that make sense? Am I missing something?

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

Closing in favor of #129.

Trigger callbacks with merged default key states

0553d40

neil-marcellini requested a review from a team as a code owner April 19, 2022 00:08

neil-marcellini self-assigned this Apr 19, 2022

melvin-bot bot requested review from AndrewGable and removed request for a team April 19, 2022 00:09

Fix style

a9af6fe

marcaaron requested changes Apr 19, 2022

View reviewed changes

neil-marcellini marked this pull request as draft April 20, 2022 18:10

neil-marcellini marked this pull request as ready for review April 20, 2022 22:27

neil-marcellini closed this May 3, 2022

neil-marcellini deleted the neil-merge-default-keys branch May 3, 2022 16:54

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

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

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

Uh oh!

Conversation

neil-marcellini commented Apr 19, 2022

Details

Related Issues

Automated Tests

Linked PRs

Uh oh!

github-actions bot commented Apr 19, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

neil-marcellini commented Apr 19, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

marcaaron Apr 20, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

neil-marcellini commented Apr 20, 2022

Uh oh!

neil-marcellini commented Apr 20, 2022

Uh oh!

marcaaron commented Apr 20, 2022

Uh oh!

neil-marcellini commented Apr 20, 2022

Uh oh!

neil-marcellini commented May 3, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

github-actions bot commented Apr 19, 2022 •

edited

Loading

marcaaron Apr 20, 2022 •

edited

Loading