[hold for payment 25 august] Onyx changes break Exisiting App architecture.

[parasharrajat]

If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!

---

cc: @marcaaron @kidroca

ISSUE

There few parts that will stop working due to the recent changes to Onyx. We have to either plan for migration or fix Onyx so that those continue working.

There is a component Enablepayment which uses the following implementation.

1. It disregards the stored values in Onyx for userWallet.
   

   App/src/pages/EnablePayments/index.js

   Lines 50 to 58 in 50828ab

   	export default withOnyx({
   	userWallet: {
   	key: ONYXKEYS.USER_WALLET,
   	// We want to refresh the wallet each time the user attempts to activate the wallet so we won't use the
   	// stored values here.
   	initWithStoredValues: false,
   	},
   	})(EnablePaymentsPage);

   .
2. We start with a userWallet:{} value.
3. After the component is mounted we make an API call to get the new data.
4. But If the data coming from the backend is the same as the stored value in Onyx, there will not be any rerendered.
5. Due to this, we will never be able to show the real data on the screen.
6. There will be many instances of this approach in the App and multiple times data will not change but we still want to make a request to check for updated value and show it to the user.

Slack thread => https://expensify.slack.com/archives/C01GTK53T8Q/p1628106963153400

Expected Result:

UI should be updated to show the recent state of data.

Actual Result:

1. No change in the UI state.

Workaround:

None. It breaks the App.

Platform:

Where is this issue occurring?

• Web
• iOS
• Android
• Desktop App
• Mobile Web

Version Number: 1.0.82
Logs: https://stackoverflow.com/c/expensify/questions/4856
Notes/Photos/Videos: Any additional supporting documentation
Expensify/Expensify Issue URL:

View all open jobs on Upwork

[MelvinBot]

Triggered auto assignment to @arielgreen (AutoAssignerTriage), see https://stackoverflow.com/c/expensify/questions/4749 for more details.

[parasharrajat]

My point here is What is an issue with triggering a render even if data is not changed? If user merge(Onyx.merge) or set(Onyx.set) a new data and we should trigger a render.

If we want to block a render then this should be handled in the component level shouldComponentUpdate.

The issue seems to be due to the Object comparison here. https://github.com/Expensify/react-native-onyx/blob/d7553b95e982ab78f6bb2064f6b0549f0ace94c2/lib/Onyx.js#L479

Even if we want to drop a render if data is not changed and then we should only do Instance matching (ob1 === ob2).

[kidroca]

We're not doing it to skip a render, but to skip re-writing the same data to storage
You can add a log point in the code you've linked to see it's often the case we're trying to write the same value

To let components re-render and still skip a write to storage we can move this to the top of the method

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

But do we want to raise a "change" event, when storage didn't really change ?

---

It doesn't seem valid to use something like initWithStoredValue: false to capture loading state

• what if you're offline - you'll be stuck loading and with a fullscreen loader it might be impossible to go back
• if the network call fails - you're stuck loading as well

Instead of relying on storage to raise a "change" event, even though it didn't really change, we can capture a loading state

• either locally in the component state: { isLoading: true }
  • not possible due to current architecture that intentionally hides promises
• or in the storage data itself - { walletKey1, walletKey2, isRefreshing: true }
  • this is already an existing pattern: Onyx.merge(ONYXKEYS.PLAID_BANK_ACCOUNTS, {loading: true});

It's cleaner and easier to reason about instead of having to explain why the component uses initWithStoedValues

[kidroca]

After analysing initWithStoedValues the component shared in the ticket description is the only component relying on initWithStoredValue for its loading state

initWithStoredValue has 6 usages

1. AdditionalDetailsStep (walletAdditionalDetails)

Has defaultProps and tracks loading state

App/src/pages/EnablePayments/AdditionalDetailsStep.js

Lines 39 to 43 in 50828ab

	walletAdditionalDetails: {
	errorFields: [],
	loading: false,
	additionalErrorMessage: '',
	},

2. EnablePaymentsPage (userWallet)

Discussed in the current issue - does not track loading state but relies on initWithStoedValues and storage to change

3. OnfidoStep (walletOnfidoData)

Has defaultProps and tracks loading state

App/src/pages/EnablePayments/OnfidoStep.js

Lines 42 to 47 in 50828ab

	const defaultProps = {
	walletOnfidoData: {
	loading: false,
	hasAcceptedPrivacyPolicy: false,
	},
	};

4. TermsStep (walletTerms)

Has defaultProps and tracks loading state

App/src/pages/EnablePayments/TermsStep.js

Lines 31 to 35 in 50828ab

	const defaultProps = {
	walletTerms: {
	loading: false,
	},
	};

5. Expensify (updateAvailable)

It's possible to have a problem here, updateAvailable is not read from storage, and the storage value could be true for some reason.

App/src/Expensify.js

Lines 53 to 63 in 50828ab

	const defaultProps = {
	session: {
	authToken: null,
	accountID: null,
	redirectToWorkspaceNewAfterSignIn: false,
	},
	updateAvailable: false,
	initialReportDataLoaded: false,
	isSidebarLoaded: false,
	betas: [],
	};

I don't know why we ignore the storage value of updateAvailable, I guess there's a check that would set it to true at some point. If that's the case it can just be made to always init as false here:

App/src/components/OnyxProvider.js

Line 15 in 50828ab

initialKeyStates: {

6. LHN loading state

App/src/libs/actions/App.js

Lines 19 to 24 in 50828ab

	let isSidebarLoaded;
	Onyx.connect({
	key: ONYXKEYS.IS_SIDEBAR_LOADED,
	callback: val => isSidebarLoaded = val,
	initWithStoredValues: false,
	});

Always treated like false until it loads

App/src/libs/actions/App.js

Lines 54 to 61 in 50828ab

	function setSidebarLoaded() {
	if (isSidebarLoaded) {
	return;
	}
	Onyx.set(ONYXKEYS.IS_SIDEBAR_LOADED, true);
	Firebase.stopTrace(CONST.TIMING.SIDEBAR_LOADED);
	}

BTW it's already initalised with false and initWithStoedValues is not needed

App/src/components/OnyxProvider.js

Line 24 in 50828ab

[ONYXKEYS.IS_SIDEBAR_LOADED]: false,

[parasharrajat]

Nice writeup. So what should be done for these cases?
I think initWithStoredValues should be considered a special case. Instead of causing write, can't we just update the state and trigger data flow?

It doesn't seem valid to use something like initWithStoredValue: false to capture loading state

I don't tightly couple the usages of initWithStoredValue with capturing loading state. Although, it is used in this way, overall there is a case to consider.

1. A component initialized with initWithStoredValue: false.
2. After the API call, we get the same data.
3. Component does not update??? <= what about this? shouldn't the data be passed down to the component when we call Onyx.set or merge.

[marcaaron]

Great conversation around this and it sounds like it might be worth exploring a deprecation of initWithStoredValues based on @kidroca's insights.

Is there some interim solution we can go with for now? e.g. could we always notify any subscribers that have initWithStoredValues: false to get around this issue and quickly restore the previous behavior?

[parasharrajat]

@marcaaron yeah we can just send the notification without updating the Onyx Store.
There are two ways to do this as per the code.

1. we move the keychanged call to before checking the cache-store and returning early with an empty promise.
   https://github.com/Expensify/react-native-onyx/blob/d7553b95e982ab78f6bb2064f6b0549f0ace94c2/lib/Onyx.js#L479.

2. we do the above but pass a parameter to keychanged to only trigger notifications for keys which has initWithStoredValues: false.

It solves the issue without breaking new changes of Onyx.

[marcaaron]

I'm not sure 1 will work - bc we need to set the value before calling keyChanged()?

we do the above but pass a parameter to keychanged to only trigger notifications for keys which has initWithStoredValues: false

So something like this... ?

const shouldCacheNewValue = !cache.hasCacheForKey(key) || !_.isEqual(val, cache.getValue(key));

if (shouldCacheNewValue)
    cache.set(key, val);
}

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

Then handle the update to subscribers in keyChanged()?

[parasharrajat]

Oh Yeah. for the other cases, this is necessary. Sorry slipped from my mind.

Then, if shouldCacheNewValue is false, only trigger a notification only for initWithStoredValues: false if we want to skip unnecessary render for all keys.

[arielgreen]

@marcaaron, since you're eng triaging this, can you go ahead and throw External on this when it's ready to be pushed forward?

[marcaaron]

@arielgreen Seems like a regression though. We usually have whoever caused the regression address it and not create separate tickets - so I'm not sure what to do in this case.

[kidroca]

Everything that's needed to achieve the previous behaviour is to move this line to the start of the call

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

It's just a matter of deciding if it's correct to call keysChanged when they actually didn't.

Reverting to the previous behaviour would also degrade performance as it will cause more re-renders, but at least we'll skip writing to storage

[kidroca]

I'm not sure 1 will work - bc we need to set the value before calling keyChanged()?

I don't see a reason for this
If you're talking about cache.set it doesn't matter if we update cache or skip the update if the value is already there

[parasharrajat]

const shouldCacheNewValue = !cache.hasCacheForKey(key) || !_.isEqual(val, cache.getValue(key));

if (shouldCacheNewValue)
    cache.set(key, val);
}

for keys whose value is different from the cache.

[marcaaron]

Reverting to the previous behaviour would also degrade performance as it will cause more re-renders, but at least we'll skip writing to storage

To clarify the suggestion is to only run keysChanged() logic on things that:

1. Changed
2. Did not change but the subscriber has initWithStoredValues: false

So I think this just means things that are flagged with initWithStoredValues won't experience the optimization (but very few things use it).

[parasharrajat]

Also, I think if the component is optimized a render should not be big deal. We are already reusing the Onyx subscriptions so resubscribing to WitOnyx should be fast.

[kidroca]

const shouldCacheNewValue

The problem is not caching the value but writing it to storage - we should not skip caching but the hard write to storage

[kidroca]

I don't see proposals that addresses this:

2. Did not change but the subscriber has initWithStoredValues: false

And I don't think it's possible to address it in set - there's no information regarding the connection that's getting notified

1. How would you know (inside set) that you should call keysChanged because initWithStoredValues is false ?
2. What about consecutive calls
   1. A component is configured to initWithStoredValues: false
   2. The fist set is triggering keysChanged even when the value in storage is the same
   3. Consecutive set calls should skip keysChanged if the value in storage didn't change

We should either always skip notifying when storage didn't change or always notify. There's no way to do it just for the dontInitWithLocalState components and then only for their 2nd render

[kidroca]

I think the optimisation updates just revealed a hidden problem - other components with loading state would capture it in Onyx

[marcaaron]

Alright, I'd still recommend that we fix this to restore the behavior that has changed. Less concerned with how it is resolved as long as the solution is simple.

[OSBotify]

👋 Friendly reminder that deploy blockers are time-sensitive ⏱ issues! Check out the open StagingDeployCash deploy checklist to see the list of PRs included in this release, then work quickly to do one of the following:

1. Identify the pull request that introduced this issue and revert it.
2. Find someone who can quickly fix the issue.
3. Fix the issue yourself.

[parasharrajat]

Thanks for pointing out @marcaaron.

We call initializeWithDefaultKeyStates() then merge the default states into the cache with new values but do not write them to storage.
Some key is dropped because we added more than the limit
Something tries the get() the value after and doesn't find it so it reaches into storage and gets the old value instead of the default key state. The value has not "changed" so we never update it here:

This makes sense. I see that you are reverting both PRs. I will wait to see if new changes are pushed from Expensify/react-native-onyx#92.

[laurenreidexpensify]

Gonna issue payment on 25 August to account for regression 👍🏽

[parasharrajat]

@laurenreidexpensify I have a doubt about the task.

The changes which I did are reverted and the original change which caused this issue is also reverted.

So this issue does not exist as both of the PRs are taken down.

Secondly, I am not sure about Marc's comment about that PR linked to this issue caused that side effect as the things he has mentioned are not related to PR's changes. Evey if they are I am not sure how.

so in short, you can say that this is not valid anymore. Now, I am not sure how to handle such case.

[kidroca]

@laurenreidexpensify, @parasharrajat
The regression is not due to work made in the current ticket

@marcaaron's investigation is correct. The problem is with cache and the LRU update: #4509

I'm not sure the best way to handle this, but my first instinct is to maybe stop dropping things from the Onyx cache unless they are explicitly flagged as safe for eviction? I can't really see a good reason to drop something like a tiny boolean value.

The "old" cache cleaning logic took "default keys" into consideration and never cleared them from cache

We could 1) restrict cache cleanup to clear only the "safe eviction" keys from cache - this aids performance as well or 2) add a check to skip clearing default keys (like we used to do before LRU cache)

One of the optimizations of initializeWithDefaultKeyStates was to skip writing the default keys back to storage. We don't need these (the default values) in hard storage as they are always overridden during init

[kidroca]

I'll be happy to open a PR to address the regressions, as it's caused by a PR of mine
We just need to agree on the solution
We've discussed 1) before, and have agreed it could only improve things.

Both approaches are pretty much handled the same way - we give "Cache" a list of keys that should not be cleared

[laurenreidexpensify]

Okay thanks for the summary - let's wait for Marc to get online later and take a look here to give us a forward steer 👍🏽

[parasharrajat]

Thanks, @kidroca, I agree with the point that changes done in the linked PR are not the cause of the issue. It's just the side case was not handled.

Anyways, I am sure that we are going to keep the LRU cache so a fix to the occurred issue #4500 (comment) and #4509 and changes in Expensify/react-native-onyx#96, all are needed.

[parasharrajat]

@marcaaron Awaiting your response in the discussion. Please let us know what is the next thing to do.

[marcaaron]

Sorry, I will be tied up with some other duties for a bit. But I don't think there is anything that urgently needs to be done here. I understand @parasharrajat was assigned to this issue. The reverts have resolved the original issue. This should also no longer be a deploy blocking issue and can be closed.

I'd suggest we create a new issue to discuss further improvements and just close this one.

[parasharrajat]

Any update on Upwork @laurenreidexpensify ?

[botify]

🚀 Deployed to production by @roryabraham in version: 1.0.88-2 🚀

platform	result
🤖 android 🤖	success ✅
🖥 desktop 🖥	success ✅
🍎 iOS 🍎	failure ❌
🕸 web 🕸	success ✅

[parasharrajat]

Neither I recieved the job offer nor I got paid for reporting the issue and submitting the PR. Would someone like to clarify what's up with this issue?

I am waiting for a clarification from a couple of days now. #4500 (comment)

[parasharrajat]

cc: @MitchExpensify Out of nowhere. Sorry.

[MitchExpensify]

Hey @parasharrajat ! Sorry for the delay, Lauren is out of office right now. I can jump in :)

Hire offer sent, please accept and I will pay for job completion. To confirm, did you identify this issue? It appears so but just making sure

[parasharrajat]

@MitchExpensify
I submitted PR which solved this issue but multiple issues were found with Onyx afterward. Thus a decision was taken to revert PR #4509 which caused all of those issues. And so my PR became unnecessary and was also reverted. But PR did solve this issue.

[parasharrajat]

Any update @MitchExpensify? Sorry for the confusing comment, I have updated it.

[MitchExpensify]

Paid with bonus! Thanks @parasharrajat