-
Notifications
You must be signed in to change notification settings - Fork 82
create onyx utils ts #402
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
create onyx utils ts #402
Conversation
|
Assigning @flodnv as the reviewer, since I see that he is the CM engineer assigned to the related 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.
LGTM
@flodnv Do we need the PR Reviewer Checklist? It's only type declaration addition so nothing to test here actually.
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.
Could you link me to where you use this in your PR?
|
@flodnv I think it will be used in this PR: Expensify/App#29939 |
| * On native, when merging an existing value with new changes, SQLite will use JSON_PATCH, which removes top-level nullish values. | ||
| * To be consistent with the behaviour for merge, we'll also want to remove null values for "set" operations. | ||
| */ | ||
| declare function fastMerge<T>( |
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.
Is "fastMerge" actually a good description? Is it actually faster...? Would something like mergeWithoutNullValues make more sense? Or customMerge...?
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.
@flodnv We already have fastMerge in lib/utils.js. I just declare this function in d.ts file to support ts
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.
OHhhhhh now I understand this PR much better 😂 🤦 Thanks 👍
| * On native, when merging an existing value with new changes, SQLite will use JSON_PATCH, which removes top-level nullish values. | ||
| * To be consistent with the behaviour for merge, we'll also want to remove null values for "set" operations. | ||
| */ | ||
| declare function fastMerge<T>( |
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.
OHhhhhh now I understand this PR much better 😂 🤦 Thanks 👍
Details
Related Issues
Expensify/App#28941
Automated 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
Web
Screen.Recording.2023-10-19.at.11.31.04.mov
Mobile Web - Chrome
Screenrecorder-2023-10-19-11-47-07-323.mp4
Mobile Web - Safari
Screen.Recording.2023-10-19.at.11.45.33.mov
Desktop
Screen.Recording.2023-10-19.at.13.06.03.mov
iOS
Screen.Recording.2023-10-19.at.12.13.12.mov
Android