Skip to content
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 multiSet when pairs is empty #231

Merged
merged 1 commit into from
Jan 20, 2023
Merged

Fix multiSet when pairs is empty #231

merged 1 commit into from
Jan 20, 2023

Conversation

marcochavezf
Copy link
Contributor

@marcochavezf marcochavezf commented Jan 20, 2023

Details

The native apps are crashing because there's an action (unknown at the moment) that calls Onyx.multiSet with an empty object as argument. This happens because the query for multiSet is expecting an array with key-value pairs.

Screenshot 2023-01-20 at 10 32 59

Additionally, this bug was reported here.

Related Issues

Expensify/App#14437

Automated Tests

N/A

Linked PRs

N/A atm, a new PR will be created when this change is deployed to npm

@marcochavezf marcochavezf self-assigned this Jan 20, 2023
@marcochavezf marcochavezf requested a review from a team as a code owner January 20, 2023 21:51
@github-actions
Copy link
Contributor

github-actions bot commented Jan 20, 2023

CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅

@melvin-bot melvin-bot bot requested review from techievivek and removed request for a team January 20, 2023 21:52
@marcochavezf
Copy link
Contributor Author

I have read the CLA Document and I hereby sign the CLA

@s77rt
Copy link
Contributor

s77rt commented Jan 20, 2023

While you are fixing this, can you check other methods too? such as mergeCollection etc..

Copy link
Contributor

@mountiny mountiny left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@marcochavezf Have you been able to test this patch? Could you please upload a video of the sign-out on native working? The changes look good to me, but it would be good to confirm just now

@marcochavezf
Copy link
Contributor Author

While you are fixing this, can you check other methods too? such as mergeCollection etc..

Good call, seems we're fine with other methods. For mergeCollection we're already checking here if the collection is empty.

@marcochavezf
Copy link
Contributor Author

@marcochavezf Have you been able to test this patch? Could you please upload a video of the sign-out on native working? The changes look good to me, but it would be good to confirm just now

Yeap, sure I attach a video with the fix:

Screen.Recording.2023-01-20.at.17.21.35.mov

@marcochavezf
Copy link
Contributor Author

Merging it now to fix the deploy blocker. Thanks @mountiny!

@marcochavezf marcochavezf merged commit 5a51638 into main Jan 20, 2023
@mountiny
Copy link
Contributor

thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants