8000 BREAKING: Use === for deep merge equality. (#1391) · lisongyu/immutable-js@732a159 · GitHub
[go: up one dir, main page]
Skip to content

Commit 732a159

Browse files
leebyronleebyron
authored
BREAKING: Use === for deep merge equality. (immutable-js#1391)
This rectifies an inconsistent behavior between `x.merge(y)` and `x.mergeDeep(y)` where merge would use === on leaf values to determine return-self optimizations, while mergeDeep would use is(). Not only was this inconsistent, it was the only place in the library this behavior existed - where everwhere else `===` is used to determine return-self optimization. Also minor refactor to save a handful of gz-bytes and reduce overhead.
1 parent ce9df42 commit 732a159

File tree

6 files changed

+40
-35
lines changed

6 files changed

+40
-35
lines changed

‎__tests__/merge.ts

Lines changed: 13 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -43,12 +43,22 @@ describe('merge', () => {
4343
expect(m1.mergeDeep(m2)).toEqual(fromJS({a: {b: {c: 10, d: 2, e: 20}, f: 30}, g: 40}));
4444
});
4545

46-
it('deep merge uses is() for return-self optimization', () => {
46+
it('merge uses === for return-self optimization', () => {
4747
const date1 = new Date(1234567890000);
48+
// Value equal, but different reference.
49+
const date2 = new Date(1234567890000);
50+
const m = Map().set('a', date1);
51+
expect(m.merge({a: date2})).not.toBe(m);
52+
expect(m.merge({a: date1})).toBe(m);
53+
});
54+
55+
it('deep merge uses === for return-self optimization', () => {
56+
const date1 = new Date(1234567890000);
57+
// Value equal, but different reference.
4858
const date2 = new Date(1234567890000);
4959
const m = Map().setIn(['a', 'b', 'c'], date1);
50-
const m2 = m.mergeDeep({a: {b: {c: date2 }}});
51-
expect(m2 === m).toBe(true);
60+
expect(m.merg 8000 eDeep({a: {b: {c: date2}}})).not.toBe(m);
61+
expect(m.mergeDeep({a: {b: {c: date1}}})).toBe(m);
5262
});
5363

5464
it('deep merges raw JS', () => {

‎src/functional/merge.js

Lines changed: 15 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -10,25 +10,28 @@ import { IndexedCollection, KeyedCollection } from '../Collection';
1010
import hasOwnProperty from '../utils/hasOwnProperty';
1111
import isDataStructure from '../utils/isDataStructure';
1212
import shallowCopy from '../utils/shallowCopy';
13-
import { is } from '../is';
1413

1514
export function merge(collection, ...sources) {
16-
return mergeWithSources(undefined, collection, sources);
15+
return mergeWithSources(collection, sources);
1716
}
1817

1918
export function mergeWith(merger, collection, ...sources) {
20-
return mergeWithSources(merger, collection, sources);
19+
return mergeWithSources(collection, sources, merger);
2120
}
2221

2322
export function mergeDeep(collection, ...sources) {
24-
return mergeWithSources(deepMergerWith(alwaysNewValue), collection, sources);
23+
return mergeDeepWithSources(collection, sources);
2524
}
2625

2726
export function mergeDeepWith(merger, collection, ...sources) {
28-
return mergeWithSources(deepMergerWith(merger), collection, sources);
27+
return mergeDeepWithSources(collection, sources, merger);
2928
}
3029

31-
function mergeWithSources(merger, collection, sources) {
30+
export function mergeDeepWithSources(collection, sources, merger) {
31+
return mergeWithSources(collection, sources, deepMergerWith(merger));
32+
}
33+
34+
export function mergeWithSources(collection, sources, merger) {
3235
if (!isDataStructure(collection)) {
3336
throw new TypeError(
3437
'Cannot merge into non-data-structure value: ' + collection
@@ -51,11 +54,10 @@ function mergeWithSources(merger, collection, sources) {
5154
merged.push(value);
5255
}
5356
: (value, key) => {
57+
const hasVal = hasOwnProperty.call(merged, key);
5458
const nextVal =
55-
merger && hasOwnProperty.call(merged, key)
56-
? merger(merged[key], value, key)
57-
: value;
58-
if (!hasOwnProperty.call(merged, key) || nextVal !== merged[key]) {
59+
hasVal && merger ? merger(merged[key], value, key) : value;
60+
if (!hasVal || nextVal !== merged[key]) {
5961
// Copy on write
6062
if (merged === collection) {
6163
merged = shallowCopy(merged);
@@ -71,15 +73,9 @@ function mergeWithSources(merger, collection, sources) {
7173

7274
function deepMergerWith(merger) {
7375
function deepMerger(oldValue, newValue, key) {
74-
if (isDataStructure(oldValue) && isDataStructure(newValue)) {
75-
return mergeWithSources(deepMerger, oldValue, [newValue]);
76-
}
77-
const nextValue = merger(oldValue, newValue, key);
78-
return is(oldValue, nextValue) ? oldValue : nextValue;
76+
return isDataStructure(oldValue) && isDataStructure(newValue)
77+
? mergeWithSources(oldValue, [newValue], deepMerger)
78+
: merger ? merger(oldValue, newValue, key) : newValue;
7979
}
8080
return deepMerger;
8181
}
82-
83-
function alwaysNewValue(oldValue, newValue) {
84-
return newValue;
85-
}

‎src/methods/merge.js

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -10,14 +10,14 @@ import { NOT_SET } from '../TrieUtils';
1010
import { update } from '../functional/update';
1111

1212
export function merge(...iters) {
13-
return mergeIntoKeyedWith(this, undefined, iters);
13+
return mergeIntoKeyedWith(this, iters);
1414
}
1515

1616
export function mergeWith(merger, ...iters) {
17-
return mergeIntoKeyedWith(this, merger, iters);
17+
return mergeIntoKeyedWith(this, iters, merger);
1818
}
1919

20-
function mergeIntoKeyedWith(collection, merger, collections) {
20+
function mergeIntoKeyedWith(collection, collections, merger) {
2121
const iters = [];
2222
for (let ii = 0; ii < collections.length; ii++) {
2323
const collection = KeyedCollection(collections[ii]);

‎src/methods/mergeDeep.js

Lines changed: 3 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -5,15 +5,12 @@
55
* LICENSE file in the root directory of this source tree.
66
*/
77

8-
import {
9-
mergeDeep as _mergeDeep,
10-
mergeDeepWith as _mergeDeepWith
11-
} from '../functional/merge';
8+
import { mergeDeepWithSources } from '../functional/merge';
129

1310
export function mergeDeep(...iters) {
14-
return _mergeDeep(this, ...iters);
11+
return mergeDeepWithSources(this, iters);
1512
}
1613

1714
export function mergeDeepWith(merger, ...iters) {
18-
return _mergeDeepWith(merger, this, ...iters);
15+
return mergeDeepWithSources(this, iters, merger);
1916
}

‎src/methods/mergeDeepIn.js

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -5,10 +5,12 @@
55
* LICENSE file in the root directory of this source tree.
66
*/
77

8-
import { mergeDeep } from '../functional/merge';
8+
import { mergeDeepWithSources } from '../functional/merge';
99
import { updateIn } from '../functional/updateIn';
1010
import { emptyMap } from '../Map';
1111

1212
export function mergeDeepIn(keyPath, ...iters) {
13-
return updateIn(this, keyPath, emptyMap(), m => mergeDeep(m, ...iters));
13+
return updateIn(this, keyPath, emptyMap(), m =>
14+
mergeDeepWithSources(m, iters)
15+
);
1416
}

‎src/methods/mergeIn.js

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -5,10 +5,10 @@
55
* LICENSE file in the root directory of this source tree.
66
*/
77

8-
import { merge } from '../functional/merge';
8+
import { mergeWithSources } from '../functional/merge';
99
import { updateIn } from '../functional/updateIn';
1010
import { emptyMap } from '../Map';
1111

1212
export function mergeIn(keyPath, ...iters) {
13-
return updateIn(this, keyPath, emptyMap(), m => merge(m, ...iters));
13+
return updateIn(this, keyPath, emptyMap(), m => mergeWithSources(m, iters));
1414
}

0 commit comments

Comments
 (0)
0