Expensify / react-native-onyx

Persistent, offline-first state management solution for React Native. Easy to use with minimal config and boilerplate.
MIT License
162 stars 73 forks source link

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

Open paultsimura opened 8 months ago

paultsimura commented 8 months ago

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

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

```js 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 commented 7 months ago

Working on fixing the inconsistency between merge and multiMerge in https://github.com/Expensify/react-native-onyx/pull/519

Before working on this, we should merge https://github.com/Expensify/react-native-onyx/pull/518 first