Add setState style callback to Onyx.merge()

[marcaaron]

Details

cc @roryabraham @aldo-expensify

This adds a setState() style callback to Onyx.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

[roryabraham]

LGTM 👍

I am confused why we have to do Promise.resolve().then(() => keyChanged(key, finalValue));, but that was there before so NAB.

[marcaaron]

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 Expensify/App is assuming the keyChanged() update to happen after one or more ticks of the event loop (since before we did not have caching and it happened after a Promise resolves). Rather than try and discover all the places where code might assume this it was decided to move the call inside a Promise that resolves as fast as possible (so kind of like a fancy setTimeout).

[marcaaron]

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.

[marcaaron]

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 Onyx.set() but also call Onyx.merge() after then it's possible that one will succeed before the other.

I'm not sure if this is really much different from what would happen if you mixed and matched a set() without a callback with a merge().

[marcaaron]

Gonna switch this up and start looking into a merge() with a callback instead of a set().

My reasoning is that:

• set() doesn't work with the mergeQueue and we shouldn't encourage that use for this behavior - which is modifying an existing value.
• set() should be for setting data and I think not modifying existing data.
• merge() makes more sense.

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 Onyx.set() that happens after Onyx.merge() can be overwritten and Onyx.merge() still has a problem where array values do not cleanly get merged.

Will think about it some more.

[marcaaron]

Gonna close this until we are ready to implement and come to some conclusions here