-
Notifications
You must be signed in to change notification settings - Fork 87
Update batching to be applied to useOnyx updates as well #669
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Update batching to be applied to useOnyx updates as well #669
Conversation
|
Please dont merge this yet, we are performing more tests... |
|
@TMisiukiewicz What is the merge strategy now? I would like to make sure we can first update Onyx in App without regressions. Was the previously found regression fixed already in main (sorry I cant recall now) |
|
Merged that PR |
…dates # Conflicts: # tests/unit/useOnyxTest.ts
|
Great, let's then wait till onyx version is bumped in the E/App - PR |
mollfpr
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The changes look good to me and have tested well!
|
Note: I'll be OOO tomorrow! |
|
Add #571 as fixed issue. |
|
Waiting for the onyx bump go through |
mountiny
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you!
| ): Promise<void> { | ||
| const promise = Promise.resolve().then(() => keyChanged(key, value, previousValue, canUpdateSubscriber, true, false)); | ||
| batchUpdates(() => keyChanged(key, value, previousValue, canUpdateSubscriber, false, true)); | ||
| const promise = Promise.resolve().then(() => keyChanged(key, value, previousValue, canUpdateSubscriber, true, false, false)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@VickyStash I am not sure why but this false seems to cause below issue(passing true worked 😅 ), could you plz check once
Bug: Toggling the language preference back doesn't apply to the application.
Screen.Recording.2025-08-25.at.19.26.46.mov
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let me check!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I was testing PR and came to know about this issue.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Pujan92 I was able to reproduce it on my end, but I need some time to figure out why it happens. I'll return to you tomorrow morning!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, I've found out why it happens.
When we change the language, the app loads it and changes ONYXKEYS.ARE_TRANSLATIONS_LOADING flag here (to true) and here (to false).
The thing is that when the translations were already loaded before, they are cached and both ONYXKEYS.ARE_TRANSLATIONS_LOADING happen almost at the same moment, so they are batched together.
Due to that -> this hook doesn't work as expected -> causing the bug.
I think there can be other places like that in the app...
One of the ideas that I have is apply batching only if updates are coming from Onyx.update.
It also can easily be fixed on the app side, but I don't think it's a good idea
…evert-batch-updates Revert "Merge pull request #669 from callstack-internal/VickyStash/refactor/batch-all-subscribers-updates"
Details
Before, the batch updates logic in the onyx was working for
withOnyxsubscribers only.This PR extends that batching mechanism to also include
useOnyxsubscribers.Previously,
useOnyxsubscribers could receiveOnyx.updateupdates slightly out of sync, as some updates would arrive a bit earlier than others, causing extra re-renders and data missing -> it was reported here. It should be fixed with this PR.Related Issues
#571
The issue was reported here: Expensify/App#66801 (comment)
Automated Tests
Manual Tests
Feel free to test updates over this PR.
That PR has E/App using onyx version 2.0.127 + this PR updates. I've done it this way since onyx upgrade to v2.0.130 was reverted.
ONYXKEYS.COLLECTION.REPORT_METADATAkey specifically) is loaded at the same time as the returned by API report (ONYXKEYS.COLLECTION.REPORT) data.hasOnceLoadedReportActions: trueis applied in metadata, but the report from the response isn't loaded yet. It appears a little later, though this data was provided in one Onyx.updateisLoadingInitialReportActions: truein the metadata. 2. The report is loaded. Successful datahasOnceLoadedReportActions: trueis applied in metadataBesides that, let's make sure the basic app functionality works the same as before:
Author Checklist
### Related Issuessection aboveTestssectiontoggleReportand notonIconClick)myBool && <MyComponent />.STYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)/** comment above it */thisproperly so there are no scoping issues (i.e. foronClick={this.submit}the methodthis.submitshould be bound tothisin the constructor)thisare necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);ifthis.submitis never passed to a component event handler likeonClick)Avataris modified, I verified thatAvataris working as expected in all cases)mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
Android: Native
android.mp4
Android: mWeb Chrome
android-web.mp4
iOS: Native
ios.mp4
iOS: mWeb Safari
ios-web.mp4
MacOS: Chrome / Safari
web.mp4
MacOS: Desktop
desktop.mp4