updateSnapshots - remove picking only existing props #638
Conversation
|
Requesting review from @parasharrajat and @iwiznia |
|
@FitseTLT Could you please update the tests for snapshots? |
What do you mean? @parasharrajat |
|
I mean to ask for updating the unit tests around the snapshots as we changed the functionality and Onyx is core lib so it is important we keep full coverage. |
iwiznia
left a comment
There was a problem hiding this comment.
Looks good to me, but agree on adding tests
|
Added @parasharrajat |
|
|
||
| const initialValue = {name: 'Fluffy'}; | ||
| const finalValue = {name: 'Kitty'}; | ||
| const finalValue = {name: 'Kitty', nickName: 'Fitse'}; |
There was a problem hiding this comment.
I don't quite get it, how is this change exercising the changed code?
There was a problem hiding this comment.
Earlier, if we couldn't add new keys(change the Object structure) to the existing values in snapshots in Onyx. But now we can do that. Here in this test, a new key is now added which confirms that change work and allows adding new keys to the existing items in snapshots.
There was a problem hiding this comment.
nickName is a new prop that didn't exist in intialValue. Evidence: if u test it on main it will fail.
There was a problem hiding this comment.
Ah got it! Thanks for clarifying
Details
When updating snapshots from BE response we used to only pick existing props in the snapshot from the update object data but there are cases where the BE can add new props that didn't exist in the snapshot data so in this PR we are removing the picking.
Related Issues
Expensify/App#60116
Automated Tests
Manual Tests
Go to node_modules react-native-onyx and update this line
react-native-onyx/lib/OnyxUtils.ts
Line 1422 in fe8af83
to
Precondition: a workspace has an employee with an expense without receipt
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
2025-05-27.00-20-02.mp4
Android: mWeb Chrome
2025-05-27.00-20-33.mp4
iOS: Native
2025-05-26.22-59-26.mp4
iOS: mWeb Safari
2025-05-26.23-01-47.mp4
MacOS: Chrome / Safari
2025-05-26.22-57-34.mp4
MacOS: Desktop
2025-05-26.22-58-36.mp4