-
Notifications
You must be signed in to change notification settings - Fork 72
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
Add setState style callback to Onyx.merge() #103
Conversation
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 👍
I am confused why we have to do Promise.resolve().then(() => keyChanged(key, finalValue));
, but that was there before so NAB.
Good instinct, but not sure what to do about it at this point. IIRC there's some code in |
Actually now that I've begun to test this. I'm not sure if it's going to work / might be causing some weird side effects - gonna throw a hold on this for a sec. |
Ok I think I figured something out. Unsure if we should switch things up.. but just want to note that it's possible using this could lead to weird race conditions if not used carefully. e.g. if you call I'm not sure if this is really much different from what would happen if you mixed and matched a |
Gonna switch this up and start looking into a My reasoning is that:
Edit: Actually thinking more on this and I don't know if merge or set will really work as expected on these values. We identified the weird race condition where an Will think about it some more. |
Gonna close this until we are ready to implement and come to some conclusions here |
Details
cc @roryabraham @aldo-expensify
This adds a
setState()
style callback toOnyx.get()
so that we can remove or overwrite object properties without storing local copies. For now, it is up to whoever uses this feature to make sure they are handling a value that doesn't exist and the type. We won't assume what the value should be only that if you pass a function instead of a value you are handling the possibility that there might not be any value to use at all.Related Issues
https://github.com/Expensify/Expensify/issues/176419
Automated Tests
Added a small test to verify the change should work
Linked PRs
To Do