Ignore undefined values in merge queue when merging objects

[chrispader]

@tgolen @marcaaron

Moving code from this PR as it became kind of a scope creep.

Details

This PR intends to improve how we merge a batch of changes which includes nullish values and objects.

Notice: This PR does not handle nullish values in nested objects, there's another PR for this change.

In general, we have the following possible values that are passed directly to Onyx.merge:

• object: Merges the existing value with the merge changes
• arrays: Just replaces the existing value
• undefined: Also replaces the existing value
• null: Will remove the key from storage

When we call Onyx.merge multiple times with different types of data, we can eventually get a batch of changes like this:

const existingValue = {a: 1, b: 2}
const mergeQueue = [{a: 2}, undefined, {c: 3, d: 4}, {d: 5, e: 6}]
const batchedChanges = applyMerge(undefined, mergeQueue) // evaluates to {c: 3, d: 5, e: 6}
const modifiedData = applyMerge(existingValue, [batchedChanges]) // evaluates to {a: 1, b: 2, c: 3, d: 5, e: 6}

With the current implementation of applyMerge, batchedChanges would evaluate to {a: 1, b: 2, c: 3, d: 5, e: 6}. This behaviour ignores the fact, that we had an undefined change in the merge queue, which should basically reset the existing value.

Therefore, we need to check if the mergeQueue contains any nullish value, and if so, we have to ignore the existingValue in the process of merging the delta changes.

Related Issues

Automated Tests

Linked PRs

[tgolen]

an undefined change in the merge queue, which should basically reset the existing value.

Why would it reset the value? I would think it would just not do anything.

[chrispader]

an undefined change in the merge queue, which should basically reset the existing value.

Why would it reset the value? I would think it would just not do anything.

Ok, if that's the expected behaviour then we can ignore this change. Still, the current implementation isn't 100% correct then.

I created a new test, which has a lot of Onyx.merge calls including some with the value undefined:

The expected output should be {test1: 'test1', test2: 'test2', test3: 'test3', test4: 'test4'}, is that correct?

P.S. pls ignore the typo in {test4: 'test3'}

[chrispader]

The solution to this would be to simply remove all undefined values from the merge queue and instead only merge the delta changes when it's an object.

[chrispader]

an undefined change in the merge queue, which should basically reset the existing value.

Why would it reset the value? I would think it would just not do anything.

Different question. What if a batch of merge changes contains a null. Should it also be ignored, if there are other objects in the merge queue. I suppose so? Because it would be set in storage right afterwards anyway... just for clarification

[chrispader]

@tgolen @marcaaron i updated the PR according to my latest proposal

[tgolen]

The solution to this would be to simply remove all undefined values from the merge queue and instead only merge the delta changes when it's an object.

I personally like this. I don't think we should try to do anything special for handling undefined values. If the value is undefined, it's a mistake from the engineer who implemented it.

What if a batch of merge changes contains a null. Should it also be ignored, if there are other objects in the merge queue. I suppose so? Because it would be set in storage right afterwards anyway... just for clarification

null is an expected and valid value, so I think it would just need to be careful to apply everything in the proper order. If this is the set of merges: {a: 1}, {b: 2}, {b: null}, then it should result in {a: 1}.

[tgolen]

Still, the current implementation isn't 100% correct then.

I forgot to touch on this. I'm glad you found this! I definitely don't think we ever considered undefined values, so I think it's totally possible that there would be unexpected behaviors around this.

[marcaaron]

I agree with @tgolen. The test you wrote makes sense given the fact that we allow undefined values into the merge queue. But I think the implementation is wrong. And the null behavior described is what we want.

That said, I'm not sure if we really need to handle a case like this:

[{a: 1}, {b: 1}, {b: undefined}]

It's hard to say what my expectations would be here because I can't really think of why someone would do it. It seems reasonable to maybe end up with {a: 1, b: undefined}. But in practice I'd suggest using null instead of undefined because your goal (if this is intentional) is most likely to end up with {a: 1}.

[chrispader]

I agree with @tgolen. The test you wrote makes sense given the fact that we allow undefined values into the merge queue. But I think the implementation is wrong. And the null behavior described is what we want.

The implementation i wrote is only about handling top-level undefined values, which will be ignored.

For nested objects, nothing changes by this. Actually, @thienlnam recently created an issue here that claims, that null values in nested objects aren't removed from storage. This is technically not a bug, but we just never implemented it. Possible to do so though.

That said, I'm not sure if we really need to handle a case like this:

[{a: 1}, {b: 1}, {b: undefined}]

The use case my PR tries to fix is more for merge queues like this:

[{a: 1}, {b: 1}, undefined, {c: 1}]

where we would expect the undefined value to be ignored and end up with a delta of {a: 1, b: 1, c: 1}. In the current implementation, this would end up as just {c: 1}, because undefined would overwrite the previous reduced changes.

[chrispader]

null is an expected and valid value, so I think it would just need to be careful to apply everything in the proper order. If this is the set of merges: {a: 1}, {b: 2}, {b: null}, then it should result in {a: 1}.

That's already happening actually, since we ignore all top-level nullish keys in objects when merging. Not part of my changes. 👍

[chrispader]

@tgolen @marcaaron just resolved merge conflicts! 👍

I think this PR is still valid, as i explained in this comment. This one only handles nullish values that are directly passed to Onyx.merge(key, <undefined|null>).

I opened another PR, which addresses the nested nullish values as discussed on Slack.

[chrispader]

@amyevans @marcaaron @tgolen also just added an additional test which checks for null values overwriting the existing value. Therefore, i needed to add one extra line in Onyx.merge

[tgolen]

This is yours now @marcaaron

[marcaaron]

If anyone calls merge() with undefined ever we should just ignore it. We are filtering out these values, but why let them even enter the mergeQueue at all? 🤔

I think it's a bit simpler to just do:

diff --git a/lib/Onyx.js b/lib/Onyx.js
index d4d329e..c9f8311 100644
--- a/lib/Onyx.js
+++ b/lib/Onyx.js
@@ -1120,6 +1120,10 @@ function applyMerge(existingValue, changes) {
  * @returns {Promise}
  */
 function merge(key, changes) {
+    if (_.isUndefined(changes)) {
+        return Promise.resolve();
+    }

[chrispader]

If anyone calls merge() with undefined ever we should just ignore it. We are filtering out these values, but why let them even enter the mergeQueue at all? 🤔

I think it's a bit simpler to just do:

Yes, you're right. The only thing i changed is that we need to return the current mergeQueuePromise[key] instead of Promise.resolve(), in case there's already change in the merge queue

[chrispader]

Can we merge this already? 🚀

[amyevans]

@marcaaron want a final look?