[Discussion] Data updating API improvements. Should we merge(), set(), something else?

[marcaaron]

Regarding "Make sure that we are always using Onyx.merge() over Onxy.set() except for when absolutely necessary"

This is not just an Expensify/App problem, every future consumer of Onyx can have the same problem as mixing merge and set can lead to race conditions: Expensify/App#5930

In that case IMO it would be best to expose only one method for updating Onyx: Expensify/App#5930 (comment)

Originally posted by @kidroca in Expensify/App#5984 (comment)

@kidroca Thanks for those insights! I think some of those eslint rules definitely make sense for us to add.

mixing merge and set can lead to race conditions

That's a good point. I guess we can keep discussing on that issue, but sounds like the race condition issue might not change whether we should prefer set() or merge(). I think the point we want to make with that one is mostly that we should use whichever is more appropriate.

To prevent this the action should not return a promise

This gets a little tricky sometimes since when actions are referenced by other actions we sometimes need them to return promises, but I like where that idea is going. Probably in those cases we are missing opportunities to move more logic to the API layer.

Originally posted by @marcaaron in Expensify/App#5984 (comment)

Been thinking about this one a bit...

Make sure that we are always using Onyx.merge() over Onxy.set() except for when absolutely necessary

I am proposing something like this for the docs

Should I use merge() or set() or both?

• Use merge() if you want to merge partial data into an existing Array or Object
• Use set() if we are working with simple values (String, Boolean, etc), need to completely overwrite a complex property of an Object, or reset some data entirely.

I might even go so far as to say that Onyx.merge() should throw an error if you try to pass it something that is not one of these:

• Object
• Array
• null
• undefined

If you have something simple I think there is virtually no reason to use merge()? 🤔

Originally posted by @marcaaron in Expensify/App#5984 (comment)

This seems like one more reason to have a single method for updates - it can work with primitive and referential values and handle them appropriately instead of trying to explain that in the documentation where it could be missed or forgotten

Another thing that seems odd is that merge expects null or undefined. How are these values treated? - they can't be merged or at least it wouldn't change the target object. Is Onyx.merge('someKey', null) the same as Onyx.set('someKey', null) ?

Originally posted by @kidroca in Expensify/App#5984 (comment)

For the merge vs. set discussion, I haven't really weighed in because I was interested to see where it would go. I think that when there are two methods, we will always struggle with not only advising which to use, but also enforcing it. I think throwing an error based on value type is a good enforcement method, but I am not totally sold on value type being the reason to choose one over the other. I think that's a hard determination for someone to make because the same piece of data could be stored in multiple ways (ie 0, false, {something: false}, {something: 0}), so now they have to decide what the type of data is that they want to store, and then find the matching method.

I like @kidroca's proposal of a unified method because I think that makes the usages much more explicit (and also a bonus if it solves race conditions, but that's a separate discussion). It is easier for someone to pass ANYTHING as a value, and then they just need to decide the update strategy (which is really the thing that matters).

Originally posted by @tgolen in Expensify/App#5984 (comment)

[marcaaron]

@tgolen replying to this comment

when there are two methods, we will always struggle with not only advising which to use, but also enforcing it

I've definitely seen us struggle here and there. Also a bit curious to see what advising achieves. The docs are very unclear on this (which I'm working on).

the same piece of data could be stored in multiple ways

Ah I didn't think of this. Do we have any examples where a key should be false, 0, or {}? Should we allow that practice? I kind of am operating on the assumption that most data is either null or stays the same type - but that does feel restrictive.

unified method because I think that makes the usages much more explicit

Yeah it could be great, but I'm not quite on board yet. I'm concerned we might lose explicitness in creating a "super updater function" with many purposes instead of specific functions to handle specific behavior. But definitely open to it.

[tgolen]

Also a bit curious to see what advising achieves. The docs are very unclear on this (which I'm working on).

This is a good point. I was working under the assumption that we've already done a good job of telling people which to use, but that could be a terrible assumption to work under!

Do we have any examples where a key should be false, 0, or {}? Should we allow that practice?

Ah, I'm not really talking about the same key being saved with multiple data types, but rather the same key could be saved in different ways and it depends on the developer implementing it to choose how it's saved. I'm mostly thinking about the design/planning process. If person A designs a solution, they might choose to store a value in one way, and if person B were to design the solution, they might choose a different way. In practice, it ends up looking like this:

Take modal for example. Someone could have easily implemented it as isModalVisible as the key and then used a boolean for the value.

I'm concerned we might lose explicitness in creating a "super updater function" with many purposes instead of specific functions to handle specific behavior.

Yeah, I get that. A variation of this approach might be to use ONE_MEGA_FUNCTION internally in Onyx, but then expose more explicit methods from Onyx such as setShallowCopy() or setDeepCopy() (just riffing here). Even those could be more clear than trying to guess what set() or merge() is going to do.

[marcaaron]

If person A designs a solution, they might choose to store a value in one way, and if person B were to design the solution, they might choose a different way

Ok I think I understand, but still unsure what the exact concern is here. Agree that it we want to be conscious of adding too much extra overhead to the design decision. In that case, the simplest thing would be to have one method.

@roryabraham made a good point that React handles this in an interesting way where a setState() is always a "set" and a setState((prevState) => merge({}, prevState, newValues)) more explicitly handles merges. We are exploring that here, but haven't done anything yet.

A variation of this approach might be to use ONE_MEGA_FUNCTION internally in Onyx, but then expose more explicit methods from Onyx such as setShallowCopy() or setDeepCopy() (just riffing here). Even those could be more clear than trying to guess what set() or merge() is going to do.

👍

[kidroca]

I was going to bring React's setState s well - there's only one methods to update state and it always performs a shallow merge

setState() is always a "set"

No, It's a shallow merge - setState({ keyA: [1,2,3] }) is the same as setState({...this.state, keyA: [1,2,3] })

In useState (hooks) - there the method is a set.
One of the reasons for this is you can have many separate state pieces - E.g. 3 useState calls, but in a class component you have only one this.state field

My point is everyone knows or learns quickly how to use setState in react but it wouldn't have been so easy if there were more than one methods to call

To make a deep merge with setState you can use the callback version which exposes current state and gives you the most flexibility, but usually you don't have to deep merge. Onyx works with a deep merge right now and this might be something we can consider switching to shallow. This will allow to replace an array instead of adding elements to it

It seems the only time where we do need to merge more items to a list is when we add report actions to a collection - but we're not using an array there - a shallow merge would still work { 1: {}, 2: {}}

[kidroca]

IMO we might need to behave differently based on where we're updating a collection key or not, but this again can happen behind the update facade as we would know when we're updating such a key.

My reasoning behind a single update function is that we've identified that we usually prefer merge
Having a single method that works in the default way enforces that. And for the cases where the default does not work for some reason we handle in a special way - a callback parameter or a merge strategy but it stands out - a reviewer might ask "why do we have to deviate from the default update strategy here?" While with set/merge it's not that obvious when we're dealing with a special case

[marcaaron]

No, It's a shallow merge - setState({ keyA: [1,2,3] }) is the same as setState({...this.state, keyA: [1,2,3] })

I think you misunderstood me here. If you call setState() and pass a specific property that property gets overwritten and in that way is similar to a call to set().

[marcaaron]

My reasoning behind a single update function is that we've identified that we usually prefer merge

I agree it is rare to see a merge() called on simple types. And that it is common to see it used on objects or arrays. And that we have more objects than anything else.

a reviewer might ask "why do we have to deviate from the default update strategy here?"

Not necessarily a bad question to ask though if we adopted set() as the default. Then you'd only ask why are you using a callback parameter and the answer would be - "do this anytime you want to perform either a deep or shallow merge".

But I share that concern that if we deprecated merge() for set(key, value) and set(key, callback) then we would forcing our "most common" cases to use a syntax like:

Onyx.set(key, (prev) => ({
    ...prev,
    isShallow: true,
}))

Which feels uglier than

Onyx.merge(key, {isShallow: true});

On the other hand, having the default be a shallow merge causes confusion in the current state because lodash/merge leads to incorrect assumptions like arrays should be concatenated.

Anyways, I would love any solution where the expectations are very clear about how data is handled. We have to ask these same questions all the time when working with React state so my first thought is it would be convenient to explain that something "works" just like setState() and not have to offer some abstruse explanation.

[kidroca]

it would be convenient to explain that something "works" just like setState()

👍

But I share that concern that if we deprecated merge() for set(key, value) and set(key, callback) then we would forcing our "most common" cases to use a syntax like:

Onyx.set(key, (prev) => ({
    ...prev,
    isShallow: true,
}))

If we leave a single method for updates it should work like the most common case, so it should be merge.
The question then is should it be shallow like in React or deep like it's currently in Onyx

Onyx.set(key, value_to_be_shallow_merged);

Onyx.set(key => (prev) => _.merge({}, prev, value_to_be_deepMerged))

And we can extract some common strategies (that we can pass as 3rd param options) if we find we're often or only using the callback version for deep merges

The only difference compared to React is that we handle primitive values - Onyx.set('myKey', 'Non Object Value');

---

The reason I initially suggested that deep merge should be the default is because right now it is (for Onyx.merge) and something can break.
I don't think we need a deep merge most of the time, even if we do it's probably up to the 2nd level of the tree
Shallow merging like in React should be a bit more performant