-
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 fastMerge
not correctly merging all nested null values
#518
Fix fastMerge
not correctly merging all nested null values
#518
Conversation
4a379dc
to
88b51ba
Compare
@chrispader thanks for looking into this! |
88b51ba
to
f738b16
Compare
f738b16
to
2b402c1
Compare
This PR fixes only the problem in Thinking about this, there might also be a discrepancy between |
@paultsimura i can't reproduce the issue from #408 (comment), so i think this PR should be ready for merge! |
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 for the wonderful comments to explain why the logic is doing what it is.
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.
Looks great, thanks!
@paultsimura
Details
Previously
fastMerge
wouldn't recursively call itself when merging an object with atarget
value ofundefined
ornull
. Therefore, nested null values wouldn't be removed either, because this only happens if we usefastMerge
for nested keys and not set the value directly.This PR fixes this issue, renames some variables for more clarity and adds comments to further explain the code.
It also adds 3 more unit tests, that @paultsimura suggested in #301 (comment) and #301 (comment).
Related Issues
#508
Automated Tests
Run the tests in
fastMergeTests
. I added an additional test that checks for exactly this issue.Manual Tests
This was reported based on #490, so we might want to test if these changes (partially) fix the issues there.
Author 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: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari
MacOS: Desktop
No screenshots, as this only internally affects Onyx operations.