8000 BREAKING: concat Lists when merging deeply (#1344) · immutable-js/immutable-js@5fbce6a · GitHub
[go: up one dir, main page]
Skip to content

Commit 5fbce6a

Browse files
leebyronleebyron
authored
BREAKING: concat Lists when merging deeply (#1344)
Fixes a long standing issue where "merge" did not treat List as an expected monoid form, overwriting entries as if it were a Map instead. This *changes* `merge` on List to be an alias of `concat` (much like `merge` is an alias for `union` on Set). Other functions like `mergeDeep` and mergeWith` have been *removed* from List, since they no longer make sense with this behavior. When `mergeDeep` is called on a Map, nested Lists are now concatenated instead of overwritten. Fixes #406
1 parent d093223 commit 5fbce6a

File tree

8 files changed

+35
-186
lines changed

8 files changed

+35
-186
lines changed

‎__tests__/merge.ts

Lines changed: 14 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,7 @@
77

88
///<reference path='../resources/jest.d.ts'/>
99

10-
import { fromJS, is, List, Map } from '../';
10+
import { fromJS, is, List, Map, Set } from '../';
1111

1212
describe('merge', () => {
1313
it('merges two maps', () => {
@@ -108,7 +108,15 @@ describe('merge', () => {
108108
expect(m1.mergeDeep(m2).get('a')).toEqual(Map({x: 1, y: 1, z: 10}));
109109
});
110110

111-
it('merges map entries with Vector values', () => {
111+
it('merges map enties with List and Set values', () => {
112+
const initial = Map({a: Map({x: 10, y: 20}), b: List([1, 2, 3]), c: Set([1, 2, 3])});
113+
const additions = Map({a: Map({y: 50, z: 100}), b: List([4, 5, 6]), c: Set([4, 5, 6])});
114+
expect(initial.mergeDeep(additions)).toEqual(
115+
Map({a: Map({x: 10, y: 50, z: 100}), b: List([1, 2, 3, 4, 5, 6]), c: Set([1, 2, 3, 4, 5, 6])}),
116+
);
117+
});
118+
119+
it('merges map entries with new values', () => {
112120
const initial = Map({a: List([1])});
113121

114122
// Note: merge and mergeDeep do not deeply coerce values, they only merge
@@ -126,14 +134,14 @@ describe('merge', () => {
126134
});
127135

128136
it('maintains JS values inside immutable collections', () => {
129-
let m1 = fromJS({a: {b: [{imm: 'map'}]}});
137+
let m1 = fromJS({a: {b: {imm: 'map'}}});
130138
let m2 = m1.mergeDeep(
131-
Map({a: Map({b: List.of( {plain: 'obj'} )})}),
139+
Map({a: Map({b: {plain: 'obj'} })}),
132140
);
133141

134-
expect(m1.getIn(['a', 'b', 0])).toEqual(Map([['imm', 'map']]));
142+
expect(m1.getIn(['a', 'b'])).toEqual(Map([['imm', 'map']]));
135143
// However mergeDeep will merge that value into the inner Map
136-
expect(m2.getIn(['a', 'b', 0])).toEqual(Map({imm: 'map', plain: 'obj'}));
144+
expect(m2.getIn(['a', 'b'])).toEqual(Map({imm: 'map', plain: 'obj'}));
137145
});
138146

139147
});

‎src/List.js

Lines changed: 3 additions & 36 deletions
Original file line numberDiff line numberDiff line change
@@ -20,12 +20,7 @@ import {
2020
resolveEnd
2121
} from './TrieUtils';
2222
import { IndexedCollection } from './Collection';
23-
import {
24-
MapPrototype,
25-
mergeIntoCollectionWith,
26-
deepMerger,
27-
deepMergerWith
28-
} from './Map';
23+
import { MapPrototype } from './Map';
2924
import { Iterator, iteratorValue, iteratorDone } from './Iterator';
3025

< F438 /div>
3126
import assertNotInfinite from './utils/assertNotInfinite';
@@ -140,20 +135,8 @@ export class List extends IndexedCollection {
140135

141136
// @pragma Composition
142137

143-
merge(/*...iters*/) {
144-
return mergeIntoListWith(this, undefined, arguments);
145-
}
146-
147-
mergeWith(merger, ...iters) {
148-
return mergeIntoListWith(this, merger, iters);
149-
}
150-
151-
mergeDeep(/*...iters*/) {
152-
return mergeIntoListWith(this, deepMerger, arguments);
153-
}
154-
155-
mergeDeepWith(merger, ...iters) {
156-
return mergeIntoListWith(this, deepMergerWith(merger), iters);
138+
merge(/*...collections*/) {
139+
return this.concat.apply(this, arguments);
157140
}
158141

159142
setSize(size) {
@@ -652,22 +635,6 @@ function setListBounds(list, begin, end) {
652635
return makeList(newOrigin, newCapacity, newLevel, newRoot, newTail);
653636
}
654637

655-
function mergeIntoListWith(list, merger, collections) {
656-
const iters = [];
657-
let maxSize = 0;
658-
for (let ii = 0; ii < collections.length; ii++) {
659-
const iter = IndexedCollection(collections[ii]);
660-
if (iter.size > maxSize) {
661-
maxSize = iter.size;
662-
}
663-
iters.push(iter);
664-
}
665-
if (maxSize > list.size) {
666-
list = list.setSize(maxSize);
667-
}
668-
return mergeIntoCollectionWith(list, merger, iters);
669-
}
670-
671638
function getTailOffset(size) {
672639
return size < SIZE ? 0 : ((size - 1) >>> SHIFT) << SHIFT;
673640
}

‎src/Map.js

Lines changed: 11 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -159,7 +159,7 @@ export class Map extends KeyedCollection {
159159
}
160160

161161
mergeDeep(/*...iters*/) {
162-
return mergeIntoMapWith(this, deepMerger, arguments);
162+
return mergeIntoMapWith(this, deepMergerWith(alwaysNewVal), arguments);
163163
}
164164

165165
mergeDeepWith(merger, ...iters) {
@@ -839,21 +839,19 @@ function mergeIntoMapWith(map, merger, collections) {
839839
return mergeIntoCollectionWith(map, merger, iters);
840840
}
841841

842-
export function deepMerger(oldVal, newVal) {
843-
return newVal && typeof newVal === 'object' && oldVal && oldVal.mergeDeep
844-
? oldVal.mergeDeep(newVal)
845-
: is(oldVal, newVal) ? oldVal : newVal;
842+
function alwaysNewVal(oldVal, newVal) {
843+
return newVal;
846844
}
847845

848846
export function deepMergerWith(merger) {
849-
return (oldVal, newVal, key) => {
850-
if (
851-
newVal &&
852-
typeof newVal === 'object' &&
853-
oldVal &&
854-
oldVal.mergeDeepWith
855-
) {
856-
return oldVal.mergeDeepWith(merger, newVal);
847+
return function(oldVal, newVal, key) {
848+
if (oldVal && newVal && typeof newVal === 'object') {
849+
if (oldVal.mergeDeepWith) {
850+
return oldVal.mergeDeepWith(merger, newVal);
851+
}
852+
if (oldVal.merge) {
853+
return oldVal.merge(newVal);
854+
}
857855
}
858856
const nextValue = merger(oldVal, newVal, key);
859857
return is(oldVal, nextValue) ? oldVal : nextValue;

‎src/Set.js

Lines changed: 1 addition & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -128,14 +128,6 @@ export class Set extends SetCollection {
128128
});
129129
}
130130

131-
merge() {
132-
return this.union.apply(this, arguments);
133-
}
134-
135-
mergeWith(merger, ...iters) {
136-
return this.union.apply(this, iters);
137-
}
138-
139131
sort(comparator) {
140132
// Late binding
141133
return OrderedSet(sortFactory(this, comparator));
@@ -186,8 +178,7 @@ const IS_SET_SENTINEL = '@@__IMMUTABLE_SET__@@';
186178
const SetPrototype = Set.prototype;
187179
SetPrototype[IS_SET_SENTINEL] = true;
188180
SetPrototype[DELETE] = SetPrototype.remove;
189-
SetPrototype.mergeDeep = SetPrototype.merge;
190-
SetPrototype.mergeDeepWith = SetPrototype.mergeWith;
181+
SetPrototype.merge = SetPrototype.union;
191182
SetPrototype.withMutations = MapPrototype.withMutations;
192183
SetPrototype.asMutable = MapPrototype.asMutable;
193184
SetPrototype.asImmutable = MapPrototype.asImmutable;

‎type-definitions/Immutable.d.ts

Lines changed: 5 additions & 33 deletions
Original file line numberDiff line numberDiff line change
@@ -675,39 +675,6 @@ declare module Immutable {
675675
update(index: number, updater: (value: T) => T): this;
676676
update<R>(updater: (value: this) => R): R;
677677

678-
/**
679-
* Note: `merge` can be used in `withMutations`.
680-
*
681-
* @see `Map#merge`
682-
*/
683-
merge(...collections: Array<Iterable<T>>): this;
684-
685-
/**
686-
* Note: `mergeWith` can be used in `withMutations`.
687-
*
688-
* @see `Map#mergeWith`
689-
*/
690-
mergeWith(
691-
merger: (oldVal: T, newVal: T, key: number) => T,
692-
...collections: Array<Iterable<T>>
693-
): this;
694-
695-
/**
696-
* Note: `mergeDeep` can be used in `withMutations`.
697-
*
698-
* @see `Map#mergeDeep`
699-
*/
700-
mergeDeep(...collections: Array<Iterable<T>>): this;
701-
702-
/**
703-
* Note: `mergeDeepWith` can be used in `withMutations`.
704-
* @see `Map#mergeDeepWith`
705-
*/
706-
mergeDeepWith(
707-
merger: (oldVal: T, newVal: T, key: number) => T,
708-
...collections: Array<Iterable<T>>
709-
): this;
710-
711678
/**
712679
* Returns a new List with size `size`. If `size` is less than this
713680
* List's size, the new List will exclude values at the higher indices.
@@ -819,8 +786,13 @@ declare module Immutable {
819786

820787
/**
821788
* Returns a new List with other values or collections concatenated to this one.
789+
*
790+
* Note: `concat` *cannot* be safely used in `withMutations`.
791+
*
792+
* @alias merge
822793
*/
823794
concat<C>(...valuesOrCollections: Array<Iterable<C> | C>): List<T | C>;
795+
merge<C>(...collections: Array<Iterable<C>): List<T | C>;
824796

825797
/**
826798
* Returns a new List with values passed through a

‎type-definitions/immutable.js.flow

Lines changed: 0 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -636,18 +636,6 @@ declare class List<+T> extends IndexedCollection<T> {
636636

637637
merge<U>(...collections: Iterable<U>[]): List<T | U>;
638638

639-
mergeWith<U, V>(
640-
merger: (oldVal: T, newVal: U, key: number) => V,
641-
...collections: Iterable<U>[]
642-
): List<T | U | V>;
643-
644-
mergeDeep<U>(...collections: Iterable<U>[]): List<T | U>;
645-
646-
mergeDeepWith<U, V>(
647-
merger: (oldVal: T, newVal: U, key: number) => V,
648-
...collections: Iterable<U>[]
649-
): List<T | U | V>;
650-
651639
setSize(size: number): this;
652640
setIn(keyPath: Iterable<mixed>, value: mixed): this;
653641
deleteIn(keyPath: Iterable<mixed>, value: mixed): this;

‎type-definitions/tests/immutable-flow.js

Lines changed: 0 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -163,18 +163,6 @@ numberOrStringList = List.of('a').merge(List.of(1))
163163
// $ExpectError
164164
numberList = List.of('a').merge(List.of(1))
165165

166-
numberList = List.of(1).mergeWith((previous, next, key) => 1, [1])
167-
// $ExpectError
168-
numberList = List.of(1).mergeWith((previous, next, key) => previous + next, ['a'])
169-
170-
numberOrStringList = List.of(1).mergeDeep(['a'])
171-
// $ExpectError
172-
numberList = List.of(1).mergeDeep(['a'])
173-
174-
numberList = List.of(1).mergeDeepWith((previous, next, key) => 1, [1])
175-
// $ExpectError
176-
numberList = List.of(1).mergeDeepWith((previous, next, key) => previous + next, ['a'])
177-
178166
nullableNumberList = List.of(1).setSize(2)
179167

180168
numberList = List.of(1).setIn([], 0)

‎type-definitions/ts-tests/list.ts

Lines changed: 1 addition & 64 deletions
Original file line numberDiff line numberDiff line change
@@ -288,7 +288,7 @@ import { List } from '../../';
288288
// $ExpectType List<number>
289289
List<number>().merge(List<number>());
290290

291-
// $ExpectError
291+
// $ExpectType List<string | number>
292292
List<number>().merge(List<string>());
293293

294294
// $ExpectType List<string | number>
@@ -304,75 +304,12 @@ import { List } from '../../';
304304
List<number>().mergeIn([], []);
305305
}
306306

307-
{ // #mergeWith
308-
309-
// $ExpectType List<number>
310-
List<number>().mergeWith((prev: number, next: number, key: number) => 1, List<number>());
311-
312-
// $ExpectError
313-
List<number>().mergeWith((prev: string, next: number, key: number) => 1, List<number>());
314-
315-
// $ExpectError
316-
List<number>().mergeWith((prev: number, next: string, key: number) => 1, List<number>());
317-
318-
// $ExpectError
319-
List<number>().mergeWith((prev: number, next: number, key: string) => 1, List<number>());
320-
321-
// $ExpectError
322-
List<number>().mergeWith((prev: number, next: number, key: number) => 'a', List<number>());
323-
324-
// $ExpectError
325-
List<number>().mergeWith((prev: number, next: number, key: number) => 1, List<string>());
326-
327-
// $ExpectType List<string | number>
328-
List<number | string>().mergeWith((prev: number, next: string, key: number) => 1, List<string>());
329-
}
330-
331-
{ // #mergeDeep
332-
333-
// $ExpectType List<number>
334-
List<number>().mergeDeep(List<number>());
335-
336-
// $ExpectError
337-
List<number>().mergeDeep(List<string>());
338-
339-
// $ExpectType List<string | number>
340-
List<number | string>().mergeDeep(List<string>());
341-
342-
// $ExpectType List<string | number>
343-
List<number | string>().mergeDeep(List<number>());
344-
}
345-
346307
{ // #mergeDeepIn
347308

348309
// $ExpectType List<number>
349310
List<number>().mergeDeepIn([], []);
350311
}
351312

352-
{ // #mergeDeepWith
353-
354-
// $ExpectType List<number>
355-
List<number>().mergeDeepWith((prev: number, next: number, key: number) => 1, List<number>());
356-
357-
// $ExpectError
358-
List<number>().mergeDeepWith((prev: string, next: number, key: number) => 1, List<number>());
359-
360-
// $ExpectError
361-
List<number>().mergeDeepWith((prev: number, next: string, key: number) => 1, List<number>());
362-
363-
// $ExpectError
364-
List<number>().mergeDeepWith((prev: number, next: number, key: string) => 1, List<number>());
365-
366-
// $ExpectError
367-
List<number>().mergeDeepWith((prev: number, next: number, key: number) => 'a', List<number>());
368-
369-
// $ExpectError
370-
List<number>().mergeDeepWith((prev: number, next: number, key: number) => 1, List<string>());
371-
372-
// $ExpectType List<string | number>
373-
List<number | string>().mergeDeepWith((prev: number, next: string, key: number) => 1, List<string>());
374-
}
375-
376313
{ // #flatten
377314

378315
// $ExpectType Collection<any, any>

0 commit comments

Comments
 (0)
0