-
Notifications
You must be signed in to change notification settings - Fork 74
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
fix: avoid using multiSet in setCollection #609
fix: avoid using multiSet in setCollection #609
Conversation
All contributors have signed the CLA ✍️ ✅ |
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.
LGTM 🚀 please make sure to run E/App unit tests agaisnt this change, and do some test/screenshots to prove it won't break the App.
I tested this out on an account with 10K reports. Without this change, signing into the account results in a locked-up application that never recovers. Sometimes the tab crashes in Chrome, or sometimes I just have to kill the tab from Chrome task manager. With this change, the account loaded and was totally usable in about 10-12 seconds. |
Thanks, both Onyx and E/App tests are passing. Here's the video where heavy account is succesfully loaded without freezing the browser: vid.mov |
I have read the CLA Document and I hereby sign the CLA |
Updated PR with videos, ready for review ✅ |
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.
LGTM, great work!
Adding @puneetlath and @tgolen as they participated in the Slack thread. |
Details
setCollection
is usingmultiSet
which is usingkeyChanged
under the hood to notify subscribers about the changes. It means it runs callbacks for connections that matches also collection key, which is very inefficient as it calls them for each collection item.Related Issues
GH_LINK
Proposal and discussion on slack: https://expensify.slack.com/archives/C05LX9D6E07/p1736520252961669
Automated Tests
No tests added because there are no tests for the Onyx methods in the codebase.
Manual Tests
OpenApp
API endpointAuthor Checklist
### Related Issues
section aboveTests
sectiontoggleReport
and notonIconClick
)myBool && <MyComponent />
.STYLE.md
) were followedAvatar
, I verified the components usingAvatar
are working as expected)/** comment above it */
this
properly so there are no scoping issues (i.e. foronClick={this.submit}
the methodthis.submit
should be bound tothis
in the constructor)this
are necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);
ifthis.submit
is never passed to a component event handler likeonClick
)Avatar
is modified, I verified thatAvatar
is working as expected in all cases)main
branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTest
steps.Screenshots/Videos
Android: Native
android.mov
Android: mWeb Chrome
android-web.mov
iOS: Native
ios.mp4
iOS: mWeb Safari
ios-web.mp4
MacOS: Chrome / Safari
web.mov
MacOS: Desktop
desktop.mov