Inconsistency between merge & mergeCollection when merging a deeply nested null value

[paultsimura]

There are 2 ways to merge a specific item that belongs to a collection:

• Calling Onyx.merge() directly;
• Calling Onyx.mergeCollection() for the collection holding this item.

These 2 operations must be interchangeable, meaning the same result should be obtained by calling either operation.
However, they handle a deeply nested null differently: merge removes the value completely, while mergeCollection sets the value to null and returns it to the subscriber.

Here's the Unit test for the same operation called via merge and mergeCollection:

Expand for code

    describe('merge', () => {
        it('should remove a deeply nested null when merging an existing key', () => {
            let result;

            const routineRoute = `${ONYX_KEYS.COLLECTION.ROUTES}routine`;

            connectionID = Onyx.connect({
                key: routineRoute,
                initWithStoredValues: false,
                callback: (value) => result = value,
            });

            const initialValue = {
                waypoints: {
                    1: 'Home',
                    2: 'Work',
                    3: 'Gym',
                },
            };

            return Onyx.set(routineRoute, initialValue)
                .then(() => {
                    expect(result).toEqual(initialValue);
                    Onyx.merge(routineRoute, {
                        waypoints: {
                            1: 'Home',
                            2: 'Work',
                            3: null,
                        },
                    });
                    return waitForPromisesToResolve();
                })
                .then(() => {
                    expect(result).toEqual({
                        waypoints: {
                            1: 'Home',
                            2: 'Work',
                        }
                    });
                });
        });
    });

    describe('mergeCollection', () => {
        it('should remove a deeply nested null when merging an existing collection item', () => {
            let result;

            const routineRoute = `${ONYX_KEYS.COLLECTION.ROUTES}routine`;

            connectionID = Onyx.connect({
                key: ONYX_KEYS.COLLECTION.ROUTES,
                initWithStoredValues: false,
                waitForCollectionCallback: true,
                callback: (value) => result = value,
            });

            const initialValue = {
                waypoints: {
                    1: 'Home',
                    2: 'Work',
                    3: 'Gym',
                },
            };

            return Onyx.set(routineRoute, initialValue)
                .then(() => {
                    expect(result).toEqual({[routineRoute]: initialValue});
                    Onyx.mergeCollection(ONYX_KEYS.COLLECTION.ROUTES, {
                        [routineRoute]: {
                            waypoints: {
                                1: 'Home',
                                2: 'Work',
                                3: null,
                            },
                        }
                    });
                    return waitForPromisesToResolve();
                })
                .then(() => {
                    expect(result).toEqual({
                        [routineRoute]: {
                            waypoints: {
                                1: 'Home',
                                2: 'Work',
                            },
                        },
                    });
                });
        });
    });

https://github.com/paultsimura/react-native-onyx/blob/8c0f4f2e38e76e7b4193b525869ab32a92c6c4ee/tests/unit/onyxTest.js#L1049-L1137

The merge test passes, while the mergeCollection test fails because the actual result is:

{
  "routes_routine": {
    "waypoints": {
      "1": "Home",
      "2": "Work",
      "3": null
    }
  }
}

[chrispader]

Working on fixing the inconsistency between merge and multiMerge in #519

Before working on this, we should merge #518 first