Onyx.merge does not remove a nested object key

[thienlnam]

If you set an object key to null, Onyx.merge will remove it

Object: {
	transactionID: 12312312
	comment: {
			waypoints: {
				waypoint0: {}
			}
     }
}

This works for first level keys
// Remove all first level keys that are explicitly set to null.

Onyx.merge(key, {
	transactionID: null
}

Corresponding object:

{
	comment: {
			waypoints: {
				waypoint0: {}
			}
     }
}

But if you try to remove a nested key

Onyx.merge(key, {
	comment: {
		waypoints: {
			waypoint0: null
		}
	}
}

It will just set the value to null

{
	comment: {
			waypoints: {
				waypoint0: null
			}
     }
}

[neil-marcellini]

following

[tgolen]

This is very interesting! I didn't believe it at first, but I did a quick test and confirmed exactly what you report. @chrispader you might be interested in checking this one out (our resident expert on Onyx.merge() 😁 ).

[tgolen]

@thienlnam Could you give a little more context into the underlying thing you're trying to do and why it's preferrable to remove the nested key as opposed to having it remain as null?

I could see "consistency" as one argument (and a good one at that), but I kind of feel there needs to be a better reason to change it since it's changing some very low-lying parts of the lib.

[chrispader]

Yes this is indeed a bit inconsistent, but until now this was actually the intended output.

I'm open to changing this so all null values will be removed when merging

[neil-marcellini]

A PR was raised to fix this #350. (edit: oh I think that's just for the type declarations). If top level keys are removed when set to null then we should do the same for nested keys IMO.

[thienlnam]

@thienlnam Could you give a little more context into the underlying thing you're trying to do and why it's preferrable to remove the nested key as opposed to having it remain as null?

Yeah, mostly just an expectation on how merging a null value would work and specifically for waypoints we had the issue because we keyed them like this, and used that to determine the order of the stops / when to add new ones

...
{
	waypoint1: {}
    waypoint2: {}
    waypoint3: {}
}

It just becomes difficult to determine when to add a new waypoint. So for example, if I remove a waypoint it should be

...
{
	waypoint1: {}
    waypoint2: {}
}

But since it doesn't null out, it would be

...
{
	waypoint1: {}
    waypoint2: {}
    waypoint3: null
}

And let's say I wanted to add a new waypoint - I'd have to find the first null waypoint and merge an empty object into it.

And if I deleted multiple stops, it becomes difficult to keep track of - we'd reorder the waypoints and leave all the empty waypoints as waypointX?

...
{
	waypoint1: {}
    waypoint2: null
    waypoint3: {}
	waypoint4: null
	waypoint5: {}
	waypoint6: null
}

In that specific scenario I ran into, it would be nice if we could remove unused nested object keys and I'm sure there are a few other cases that cleaning up the data object would be nice

[chrispader]

I just created a PR for this here. Should be done really soon 👍

[chrispader]

Finished implementation of this feature/bug. 👍

[thienlnam]

Merged!

[thienlnam]

PR was reverted

[chrispader]

After the revert, another PR fixing this issue was merged: #380

I think we can close this now? :) @thienlnam @tgolen

[thienlnam]

Yup looks good, thanks!

[paultsimura]

Hi folks, while working on my PR, I think I've run into a similar issue again.

Let's take this test case:

1. There is no existing value of a given key in Onyx;
2. We call merge with a nested null;

Should the null be ignored here (since if there was an existing value, the null will remove it)?

This is relevant for both merge and mergeCollection (but I believe, both stem from the incorrect fastMerge implementation when the existing value is undefined).

Onyx.merge():

Unit test:

    it('should merge a non-existing key with a nested null removed', () => {
        let testKeyValue;

        connectionID = Onyx.connect({
            key: ONYX_KEYS.TEST_KEY,
            initWithStoredValues: false,
            callback: (value) => {
                testKeyValue = value;
            },
        });

        return Onyx.merge(ONYX_KEYS.TEST_KEY, {
            waypoints: {
                1: 'Home',
                2: 'Work',
                3: null,
            },
        }).then(() => {
            expect(testKeyValue).toEqual({
                waypoints: {
                    1: 'Home',
                    2: 'Work',
                },
            });
        });
    });

The test fails because the persisted value is:

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

Onyx.mergeCollection():

Unit test:

    describe('Onyx.mergeCollection', () => {
        it('should omit nested null values', () => {
            let result;

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

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

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

The test fails because the persisted value is:

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

cc: @tgolen @thienlnam

[paultsimura]

I've narrowed the issue down a little.

Here's a failing unit test of the fastMerge function that I believe shouldn't be failing. Please correct me if I'm wrong:

Given:

react-native-onyx/tests/unit/fastMergeTest.js

Lines 15 to 34 in db7007f

	const testObjectWithNullishValues = {
	a: undefined,
	b: {
	c: {
	h: 'h',
	},
	d: {
	e: null,
	},
	},
	};
	const testObjectWithNullValuesRemoved = {
	b: {
	c: {
	h: 'h',
	},
	d: {},
	},
	};

This test fails:

    it('should merge an object with an empty object and remove deeply nested null values', () => {
        const result = utils.fastMerge({}, testObjectWithNullishValues);
        expect(result).toEqual(testObjectWithNullValuesRemoved);
    });

The actual result is:

{
  "b": {
    "c": {
      "h": "h"
    },
    "d": {
      "e": null
    }
  }
}

The top-level nullish values are removed (the a: undefined was removed, the same happens if I change undefined to null), but the nested ones are retained.