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

@marcaaron

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.

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);

.

We start with a userWallet:{} value.
After the component is mounted we make an API call to get the new data.
But If the data coming from the backend is the same as the stored value in Onyx, there will not be any rerendered.
Due to this, we will never be able to show the real data on the screen.
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:

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

@arielgreen

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

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).

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

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,

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.

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

@kidroca

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?

@marcaaron

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

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.
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.

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()?

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.

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

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 👍🏽

@kidroca

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.

@marcaaron

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

@parasharrajat

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.

@laurenreidexpensify

Any update on Upwork @laurenreidexpensify ?

@roryabraham

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

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

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)

@MitchExpensify

cc: @MitchExpensify Out of nowhere. Sorry.

@parasharrajat

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

@MitchExpensify

@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.

@MitchExpensify

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

@parasharrajat

Paid with bonus! Thanks @parasharrajat

Navigation Menu

Repository navigation

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

ISSUE

Expected Result:

Actual Result:

Workaround:

Platform:

1. AdditionalDetailsStep (walletAdditionalDetails)

2. EnablePaymentsPage (userWallet)

3. OnfidoStep (walletOnfidoData)

4. TermsStep (walletTerms)

5. Expensify (updateAvailable)

6. LHN loading state

58 remaining items

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

	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);

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

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

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

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

	function setSidebarLoaded() {
	if (isSidebarLoaded) {
	return;
	}

	Onyx.set(ONYXKEYS.IS_SIDEBAR_LOADED, true);
	Firebase.stopTrace(CONST.TIMING.SIDEBAR_LOADED);
	}

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

Description

ISSUE

Expected Result:

Actual Result:

Workaround:

Platform:

Activity

MelvinBot commented on Aug 7, 2021

parasharrajat commented on Aug 7, 2021

kidroca commented on Aug 9, 2021

kidroca commented on Aug 9, 2021

1. AdditionalDetailsStep (walletAdditionalDetails)

2. EnablePaymentsPage (userWallet)

3. OnfidoStep (walletOnfidoData)

4. TermsStep (walletTerms)

5. Expensify (updateAvailable)

6. LHN loading state

parasharrajat commented on Aug 9, 2021

marcaaron commented on Aug 9, 2021

parasharrajat commented on Aug 9, 2021

marcaaron commented on Aug 9, 2021

parasharrajat commented on Aug 9, 2021

58 remaining items

kidroca commented on Aug 18, 2021

laurenreidexpensify commented on Aug 18, 2021

parasharrajat commented on Aug 18, 2021

parasharrajat commented on Aug 18, 2021

marcaaron commented on Aug 18, 2021

parasharrajat commented on Aug 25, 2021

botify commented on Aug 30, 2021

parasharrajat commented on Sep 7, 2021

parasharrajat commented on Sep 8, 2021

MitchExpensify commented on Sep 8, 2021

parasharrajat commented on Sep 8, 2021

parasharrajat commented on Sep 10, 2021

MitchExpensify commented on Sep 10, 2021

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions