Skip to content

Commit 2ed4799

Browse files
marcaaronmarcaaron
authored
Merge pull request #173 from Expensify/ionatan_change_mergearray
Change behavior of merging arrays
2 parents 5f85baf + ef82188 commit 2ed4799

9 files changed

Lines changed: 117 additions & 15 deletions

File tree

‎README.md‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -63,7 +63,7 @@ The data will then be cached and stored via [`AsyncStorage`](https://github.com/
6363

6464
We can also use `Onyx.merge()` to merge new `Object` or `Array` data in with existing data.
6565

66-
For `Array` the default behavior is to concatenate new items.
66+
For `Array` the default behavior is to concatenate replace it fully, effectively making it equivalent to set:
6767

6868
```javascript
6969
Onyx.merge(ONYXKEYS.EMPLOYEE_LIST, ['Joe']); // -> ['Joe']
@@ -77,10 +77,10 @@ Onyx.merge(ONYXKEYS.POLICY, {id: 1}); // -> {id: 1}
7777
Onyx.merge(ONYXKEYS.POLICY, {name: 'My Workspace'}); // -> {id: 1, name: 'My Workspace'}
7878
```
7979

80-
One caveat to be aware of is that `lodash/merge` [follows the behavior of jQuery's deep extend](https://github.com/lodash/lodash/issues/2872) and will not concatenate nested arrays in objects. It might seem like this code would concat these arrays, but it does not.
80+
Arrays inside objects will NOT be concatenated and instead will be replaced fully, same as arrays not inside objects:
8181

8282
```javascript
83-
Onyx.merge(ONYXKEYS.POLICY, {employeeList: ['Joe']}); // -> {employeeList: ['Joe']}
83+
Onyx.merge(ONYXKEYS.POLICY, {employeeList: ['Joe', 'Jack']}); // -> {employeeList: ['Joe', 'Jack']}
8484
Onyx.merge(ONYXKEYS.POLICY, {employeeList: ['Jack']}); // -> {employeeList: ['Jack']}
8585
```
8686

‎lib/Onyx.js‎

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,11 +1,13 @@
11
import _ from 'underscore';
22
import Str from 'expensify-common/lib/str';
33
import lodashMerge from 'lodash/merge';
4+
import lodashMergeWith from 'lodash/mergeWith';
45
import lodashGet from 'lodash/get';
56
import Storage from './storage';
67
import * as Logger from './Logger';
78
import cache from './OnyxCache';
89
import createDeferredTask from './createDeferredTask';
10+
import customizerForMergeWith from './customizerForMergeWith';
911

1012
// Keeps track of the last connectionID that was used so we can keep incrementing it
1113
let lastConnectionID = 0;
@@ -661,7 +663,7 @@ function applyMerge(key, data) {
661663
if (_.isObject(data) || _.every(mergeValues, _.isObject)) {
662664
// Object values are merged one after the other
663665
return _.reduce(mergeValues, (modifiedData, mergeValue) => {
664-
const newData = lodashMerge({}, modifiedData, mergeValue);
666+
const newData = lodashMergeWith({}, modifiedData, mergeValue, customizerForMergeWith);
665667

666668
// We will also delete any object keys that are undefined or null.
667669
// Deleting keys is not supported by AsyncStorage so we do it this way.
@@ -732,7 +734,7 @@ function initializeWithDefaultKeyStates() {
732734
.then((pairs) => {
733735
const asObject = _.object(pairs);
734736

735-
const merged = lodashMerge(asObject, defaultKeyStates);
737+
const merged = lodashMergeWith(asObject, defaultKeyStates, customizerForMergeWith);
736738
cache.merge(merged);
737739
_.each(merged, (val, key) => keyChanged(key, val));
738740
});

‎lib/OnyxCache.js‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,6 @@
11
import _ from 'underscore';
2-
import lodashMerge from 'lodash/merge';
2+
import lodashMergeWith from 'lodash/mergeWith';
3+
import customizerForMergeWith from './customizerForMergeWith';
34

45
const isDefined = _.negate(_.isUndefined);
56

@@ -110,7 +111,7 @@ class OnyxCache {
110111
* @param {Record<string, *>} data - a map of (cache) key - values
111112
*/
112113
merge(data) {
113-
this.storageMap = lodashMerge({}, this.storageMap, data);
114+
this.storageMap = lodashMergeWith({}, this.storageMap, data, customizerForMergeWith);
114115

115116
const storageKeys = this.getAllKeys();
116117
const mergedKeys = _.keys(data);

‎lib/customizerForMergeWith.js‎

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,21 @@
1+
import _ from 'underscore';
2+
3+
/**
4+
* When merging 2 objects into onyx that contain an array, we want to completely replace the array instead of the default
5+
* behavior which is to merge each item by its index.
6+
* ie:
7+
* merge({a: [1]}, {a: [2,3]}):
8+
* - In the default implementation would produce {a:[1,3]}
9+
* - With this function would produce: {a: [2,3]}
10+
* @param {*} objValue
11+
* @param {*} srcValue
12+
* @return {*}
13+
*/
14+
// eslint-disable-next-line rulesdir/prefer-early-return
15+
function customizerForMergeWith(objValue, srcValue) {
16+
if (_.isArray(objValue)) {
17+
return srcValue;
18+
}
19+
}
20+
21+
export default customizerForMergeWith;

‎lib/storage/providers/LocalForage.js‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -6,8 +6,9 @@
66

77
import localforage from 'localforage';
88
import _ from 'underscore';
9-
import lodashMerge from 'lodash/merge';
9+
import lodashMergeWith from 'lodash/mergeWith';
1010
import SyncQueue from '../../SyncQueue';
11+
import customizerForMergeWith from '../../customizerForMergeWith';
1112

1213
localforage.config({
1314
name: 'OnyxDB',
@@ -24,7 +25,7 @@ const provider = {
2425
return localforage.getItem(key)
2526
.then((existingValue) => {
2627
const newValue = _.isObject(existingValue)
27-
? lodashMerge({}, existingValue, value)
28+
? lodashMergeWith({}, existingValue, value, customizerForMergeWith)
2829
: value;
2930
return localforage.setItem(key, newValue);
3031
});

‎tests/unit/onyxCacheTest.js‎

Lines changed: 16 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -236,7 +236,7 @@ describe('Onyx', () => {
236236

237237
expect(cache.getValue('mockKey2')).toEqual({
238238
other: 'overwrittenMockValue',
239-
items: [1, 2, 5],
239+
items: [1, 2],
240240
mock: 'mock',
241241
});
242242
});
@@ -267,12 +267,25 @@ describe('Onyx', () => {
267267
mockKey: [{ID: 3}, {added: 'field'}, {}, {ID: 1000}],
268268
});
269269

270-
// THEN the arrays should be merged as expected
270+
// THEN the arrays should be replaced as expected
271271
expect(cache.getValue('mockKey')).toEqual([
272-
{ID: 3}, {ID: 2, added: 'field'}, {ID: 3}, {ID: 1000},
272+
{ID: 3}, {added: 'field'}, {}, {ID: 1000},
273273
]);
274274
});
275275

276+
it('Should merge arrays inside objects correctly', () => {
277+
// GIVEN cache with existing array data
278+
cache.set('mockKey', {ID: [1]});
279+
280+
// WHEN merge is called with an array
281+
cache.merge({
282+
mockKey: {ID: [2]},
283+
});
284+
285+
// THEN the arrays should be merged as expected
286+
expect(cache.getValue('mockKey')).toEqual({ID: [2]});
287+
});
288+
276289
it('Should work with primitive values', () => {
277290
// GIVEN cache with existing data
278291
cache.set('mockKey', {});

‎tests/unit/onyxMultiMergeWebStorageTest.js‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -47,15 +47,15 @@ describe('Onyx.mergeCollection() amd WebStorage', () => {
4747
expect(OnyxCache.getValue('test_3')).not.toBeDefined();
4848

4949
// When we merge additional data
50-
const additionalDataOne = {b: 'b', c: 'c'};
50+
const additionalDataOne = {b: 'b', c: 'c', e: [1, 2]};
5151
Onyx.mergeCollection(ONYX_KEYS.COLLECTION.TEST_KEY, {
5252
test_1: additionalDataOne,
5353
test_2: additionalDataOne,
5454
test_3: additionalDataOne,
5555
});
5656

5757
// And call again consecutively with different data
58-
const additionalDataTwo = {d: 'd'};
58+
const additionalDataTwo = {d: 'd', e: [2]};
5959
Onyx.mergeCollection(ONYX_KEYS.COLLECTION.TEST_KEY, {
6060
test_1: additionalDataTwo,
6161
test_2: additionalDataTwo,
@@ -65,7 +65,7 @@ describe('Onyx.mergeCollection() amd WebStorage', () => {
6565
return waitForPromisesToResolve()
6666
.then(() => {
6767
const finalObject = {
68-
a: 'a', b: 'b', c: 'c', d: 'd',
68+
a: 'a', b: 'b', c: 'c', d: 'd', e: [2],
6969
};
7070

7171
// Then our new data should merge with the existing data in the cache

‎tests/unit/onyxTest.js‎

Lines changed: 36 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -178,6 +178,42 @@ describe('Onyx', () => {
178178
});
179179
});
180180

181+
it('should overwrite an array key nested inside an object', () => {
182+
let testKeyValue;
183+
connectionID = Onyx.connect({
184+
key: ONYX_KEYS.TEST_KEY,
185+
initWithStoredValues: false,
186+
callback: (value) => {
187+
testKeyValue = value;
188+
},
189+
});
190+
191+
Onyx.merge(ONYX_KEYS.TEST_KEY, {something: [1, 2, 3]});
192+
Onyx.merge(ONYX_KEYS.TEST_KEY, {something: [4]});
193+
return waitForPromisesToResolve()
194+
.then(() => {
195+
expect(testKeyValue).toEqual({something: [4]});
196+
});
197+
});
198+
199+
it('should overwrite an array key nested inside an object when using merge', () => {
200+
let testKeyValue;
201+
connectionID = Onyx.connect({
202+
key: ONYX_KEYS.COLLECTION.TEST_KEY,
203+
initWithStoredValues: false,
204+
callback: (value) => {
205+
testKeyValue = value;
206+
},
207+
});
208+
209+
Onyx.merge(ONYX_KEYS.COLLECTION.TEST_KEY, {test_1: {something: [1, 2, 3]}});
210+
Onyx.merge(ONYX_KEYS.COLLECTION.TEST_KEY, {test_1: {something: [4]}});
211+
return waitForPromisesToResolve()
212+
.then(() => {
213+
expect(testKeyValue).toEqual({test_1: {something: [4]}});
214+
});
215+
});
216+
181217
it('should properly set and merge when using mergeCollection', () => {
182218
const valuesReceived = {};
183219
const mockCallback = jest.fn(data => valuesReceived[data.ID] = data.value);

‎tests/unit/withOnyxTest.js‎

Lines changed: 28 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -101,6 +101,34 @@ describe('withOnyx', () => {
101101
});
102102
});
103103

104+
it('should replace arrays inside objects with withOnyx subscribing to individual key if mergeCollection is used', () => {
105+
const collectionItemID = 1;
106+
const TestComponentWithOnyx = withOnyx({
107+
text: {
108+
key: `${ONYX_KEYS.COLLECTION.TEST_KEY}${collectionItemID}`,
109+
},
110+
})(ViewWithCollections);
111+
const onRender = jest.fn();
112+
render(<TestComponentWithOnyx onRender={onRender} />);
113+
return waitForPromisesToResolve()
114+
.then(() => {
115+
Onyx.mergeCollection(ONYX_KEYS.COLLECTION.TEST_KEY, {
116+
test_1: {list: [1, 2]},
117+
});
118+
return waitForPromisesToResolve();
119+
})
120+
.then(() => {
121+
Onyx.mergeCollection(ONYX_KEYS.COLLECTION.TEST_KEY, {
122+
test_1: {list: [7]},
123+
});
124+
return waitForPromisesToResolve();
125+
})
126+
.then(() => {
127+
expect(onRender.mock.calls.length).toBe(3);
128+
expect(onRender.mock.instances[2].text).toEqual({list: [7]});
129+
});
130+
});
131+
104132
it('should update withOnyx subscribing to individual key with merged value if mergeCollection is used', () => {
105133
const collectionItemID = 4;
106134
const TestComponentWithOnyx = withOnyx({

0 commit comments

Comments
 (0)