-
Notifications
You must be signed in to change notification settings - Fork 82
perf: preserve references of unchanged collection items #693
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
perf: preserve references of unchanged collection items #693
Conversation
|
Performance test failures are expected. We intentionally compare values in collection methods (
|
youssef-lr
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.
LGTM
|
@TMisiukiewicz can you merge main to see if reassure will pass? |
…ces-of-unchanged-collection-items
|
@youssef-lr Updated with latest main, but it won't have any effect, see the comment above |
Revert Merge pull request #693
Details
When
setCollection/mergeCollection/partialSetCollectionare called fromOpenAppandReconnectApp, it should preserve references for all the items that are unchanged to reduce the amount of the re-renders of the components connected to collection items that did not change.Related Issues
GH_LINK Expensify/App#72679
Automated Tests
Added required unit tests
Manual Tests
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.mov
Android: mWeb Chrome
iOS: Native
ios.mov
iOS: mWeb Safari
ios-web.mov
MacOS: Chrome / Safari
web.mov
MacOS: Desktop
desktop.mov