diff --git a/__tests__/issues.ts b/__tests__/issues.ts index c57d34d99..7083a235e 100644 --- a/__tests__/issues.ts +++ b/__tests__/issues.ts @@ -1,4 +1,13 @@ -import { List, OrderedMap, OrderedSet, Record, Seq, Set } from 'immutable'; +import { + fromJS, + List, + Map, + OrderedMap, + OrderedSet, + Record, + Seq, + Set, +} from 'immutable'; describe('Issue #1175', () => { it('invalid hashCode() response should not infinitly recurse', () => { @@ -121,3 +130,51 @@ describe('Issue #1785', () => { expect(emptyRecord.merge({ id: 1 })).toBe(emptyRecord); }); + +describe('Issue #1475', () => { + it('complex case should return first value on mergeDeep when types are incompatible', () => { + const a = fromJS({ + ch: [ + { + code: 8, + }, + ], + }) as Map; + const b = fromJS({ + ch: { + code: 8, + }, + }); + expect(a.mergeDeep(b).equals(b)).toBe(true); + }); + + it('simple case should return first value on mergeDeep when types are incompatible', () => { + const a = fromJS({ + ch: [], + }) as Map; + const b = fromJS({ + ch: { code: 8 }, + }); + expect(a.merge(b).equals(b)).toBe(true); + }); +}); + +describe('Issue #1719', () => { + it('mergeDeep() should overwrite when types conflict', () => { + const objWithObj = fromJS({ + items: { + '1': { + id: '1', + }, + }, + }) as Map; + const objWithArray = fromJS({ + items: [ + { + id: '1', + }, + ], + }); + expect(objWithObj.mergeDeep(objWithArray).equals(objWithArray)).toBe(true); + }); +}); diff --git a/__tests__/merge.ts b/__tests__/merge.ts index cddb3aca0..1c0ec5cac 100644 --- a/__tests__/merge.ts +++ b/__tests__/merge.ts @@ -210,73 +210,124 @@ describe('merge', () => { expect(merge(a, [], [])).toBe(a); }); - it('mergeDeep with tuple Symbol keys', () => { - const a = Symbol('a'); - const b = Symbol('b'); - const c = Symbol('c'); - const d = Symbol('d'); - const e = Symbol('e'); - const f = Symbol('f'); - const g = Symbol('g'); - - // Note the use of nested Map constructors, Map() does not do a deep conversion! - const m1 = Map([ - [ - a, - Map([ - [ - b, - Map([ - [c, 1], - [d, 2], - ]), - ], - ]), - ], - ]); - - // mergeDeep can be directly given a nested set of `Iterable<[K, V]>` - const merged = m1.mergeDeep([ - // @ts-ignore (Type definition limitation) - [ - a, - [ - [ - b, - [ - [c, 10], - [e, 20], - [f, 30], - [g, 40], - ], - ], - ], - ], - ]); - - expect(merged).toEqual( - Map([ - [ - a, - Map([ - [ - b, - Map([ - [c, 10], - [d, 2], - [e, 20], - [f, 30], - [g, 40], - ]), - ], - ]), - ], - ]) - ); - }); - it('merges records with a size property set to 0', () => { const Sizable = Record({ size: 0 }); expect(Sizable().merge({ size: 123 }).size).toBe(123); }); + + it('mergeDeep merges partial conflicts', () => { + const a = fromJS({ + ch: [ + { + code: 8, + }, + ], + banana: 'good', + }) as Map; + const b = fromJS({ + ch: { + code: 8, + }, + apple: 'anti-doctor', + }); + expect( + a.mergeDeep(b).equals( + fromJS({ + ch: { + code: 8, + }, + apple: 'anti-doctor', + banana: 'good', + }) + ) + ).toBe(true); + }); + + const map = { type: 'Map', value: Map({ b: 5, c: 9 }) }; + const object = { type: 'object', value: { b: 7, d: 12 } }; + const RecordFactory = Record({ a: 1, b: 2 }); + const record = { type: 'Record', value: RecordFactory({ b: 3 }) }; + const list = { type: 'List', value: List(['5']) }; + const array = { type: 'array', value: ['9'] }; + const set = { type: 'Set', value: Set('3') }; + + const incompatibleTypes = [ + [map, list], + [map, array], + [map, set], + [object, list], + [object, array], + [object, set], + [record, list], + [record, array], + [record, set], + [list, set], + ]; + + for (const [ + { type: type1, value: value1 }, + { type: type2, value: value2 }, + ] of incompatibleTypes) { + it(`mergeDeep and Map#mergeDeep replaces ${type1} and ${type2} with each other`, () => { + const aObject = { a: value1 }; + const bObject = { a: value2 }; + expect(mergeDeep(aObject, bObject)).toEqual(bObject); + expect(mergeDeep(bObject, aObject)).toEqual(aObject); + + const aMap = Map({ a: value1 }) as Map; + const bMap = Map({ a: value2 }) as Map; + expect(aMap.mergeDeep(bMap).equals(bMap)).toBe(true); + expect(bMap.mergeDeep(aMap).equals(aMap)).toBe(true); + }); + } + + const compatibleTypesAndResult = [ + [map, object, Map({ b: 7, c: 9, d: 12 })], + [map, record, Map({ a: 1, b: 3, c: 9 })], + [object, map, { b: 5, c: 9, d: 12 }], + [object, record, { a: 1, b: 3, d: 12 }], + [record, map, RecordFactory({ b: 5 })], + [record, object, RecordFactory({ b: 7 })], + [list, array, List(['5', '9'])], + [array, list, ['9', '5']], + [map, { type: 'Map', value: Map({ b: 7 }) }, Map({ b: 7, c: 9 })], + [object, { type: 'object', value: { d: 3 } }, { b: 7, d: 3 }], + [ + record, + { type: 'Record', value: RecordFactory({ a: 3 }) }, + RecordFactory({ a: 3, b: 2 }), + ], + [list, { type: 'List', value: List(['12']) }, List(['5', '12'])], + [array, { type: 'array', value: ['3'] }, ['9', '3']], + [set, { type: 'Set', value: Set(['3', '5']) }, Set(['3', '5'])], + ] as const; + + for (const [ + { type: type1, value: value1 }, + { type: type2, value: value2 }, + result, + ] of compatibleTypesAndResult) { + it(`mergeDeep and Map#mergeDeep merges ${type1} and ${type2}`, () => { + const aObject = { a: value1 }; + const bObject = { a: value2 }; + expect(mergeDeep(aObject, bObject)).toEqual({ a: result }); + + const aMap = Map({ a: value1 }) as Map; + const bMap = Map({ a: value2 }); + expect(aMap.mergeDeep(bMap)).toEqual(Map({ a: result })); + }); + } + + it('Map#mergeDeep replaces nested List with Map and Map with List', () => { + const a = Map({ a: List([Map({ x: 1 })]) }) as Map; + const b = Map({ a: Map([[0, Map({ y: 2 })]]) }) as Map; + expect(a.mergeDeep(b).equals(b)).toBe(true); + expect(b.mergeDeep(a).equals(a)).toBe(true); + }); + + it('functional mergeDeep replaces nested array with Map', () => { + const a = { a: [{ x: 1 }] }; + const b = Map({ a: Map([[0, Map({ y: 2 })]]) }); + expect(mergeDeep(a, b)).toEqual({ a: Map([[0, Map({ y: 2 })]]) }); + }); }); diff --git a/src/functional/merge.js b/src/functional/merge.js index c9f364d6d..21fbd471b 100644 --- a/src/functional/merge.js +++ b/src/functional/merge.js @@ -1,5 +1,8 @@ import { isImmutable } from '../predicates/isImmutable'; +import { isIndexed } from '../predicates/isIndexed'; +import { isKeyed } from '../predicates/isKeyed'; import { IndexedCollection, KeyedCollection } from '../Collection'; +import { Seq } from '../Seq'; import hasOwnProperty from '../utils/hasOwnProperty'; import isDataStructure from '../utils/isDataStructure'; import shallowCopy from '../utils/shallowCopy'; @@ -68,7 +71,9 @@ export function mergeWithSources(collection, sources, merger) { function deepMergerWith(merger) { function deepMerger(oldValue, newValue, key) { - return isDataStructure(oldValue) && isDataStructure(newValue) + return isDataStructure(oldValue) && + isDataStructure(newValue) && + areMergeable(oldValue, newValue) ? mergeWithSources(oldValue, [newValue], deepMerger) : merger ? merger(oldValue, newValue, key) @@ -76,3 +81,19 @@ function deepMergerWith(merger) { } return deepMerger; } + +/** + * It's unclear what the desired behavior is for merging two collections that + * fall into separate categories between keyed, indexed, or set-like, so we only + * consider them mergeable if they fall into the same category. + */ +function areMergeable(oldDataStructure, newDataStructure) { + const oldSeq = Seq(oldDataStructure); + const newSeq = Seq(newDataStructure); + // This logic assumes that a sequence can only fall into one of the three + // categories mentioned above (since there's no `isSetLike()` method). + return ( + isIndexed(oldSeq) === isIndexed(newSeq) && + isKeyed(oldSeq) === isKeyed(newSeq) + ); +} diff --git a/type-definitions/immutable.d.ts b/type-definitions/immutable.d.ts index 456f2bc81..187e3d41b 100644 --- a/type-definitions/immutable.d.ts +++ b/type-definitions/immutable.d.ts @@ -1015,12 +1015,18 @@ declare namespace Immutable { ): this; /** - * Like `merge()`, but when two Collections conflict, it merges them as well, - * recursing deeply through the nested data. - * - * Note: Values provided to `merge` are shallowly converted before being - * merged. No nested values are altered unless they will also be merged at - * a deeper level. + * Like `merge()`, but when two compatible collections are encountered with + * the same key, it merges them as well, recursing deeply through the nested + * data. Two collections are considered to be compatible (and thus will be + * merged together) if they both fall into one of three categories: keyed + * (e.g., `Map`s, `Record`s, and objects), indexed (e.g., `List`s and + * arrays), or set-like (e.g., `Set`s). If they fall into separate + * categories, `mergeDeep` will replace the existing collection with the + * collection being merged in. This behavior can be customized by using + * `mergeDeepWith()`. + * + * Note: Indexed and set-like collections are merged using + * `concat()`/`union()` and therefore do not recurse. * * * ```js @@ -1042,8 +1048,11 @@ declare namespace Immutable { ): this; /** - * Like `mergeDeep()`, but when two non-Collections conflict, it uses the - * `merger` function to determine the resulting value. + * Like `mergeDeep()`, but when two non-collections or incompatible + * collections are encountered at the same key, it uses the `merger` + * function to determine the resulting value. Collections are considered + * incompatible if they fall into separate categories between keyed, + * indexed, and set-like. * * * ```js @@ -5534,8 +5543,18 @@ declare namespace Immutable { ): C; /** - * Returns a copy of the collection with the remaining collections merged in - * deeply (recursively). + * Like `merge()`, but when two compatible collections are encountered with + * the same key, it merges them as well, recursing deeply through the nested + * data. Two collections are considered to be compatible (and thus will be + * merged together) if they both fall into one of three categories: keyed + * (e.g., `Map`s, `Record`s, and objects), indexed (e.g., `List`s and + * arrays), or set-like (e.g., `Set`s). If they fall into separate + * categories, `mergeDeep` will replace the existing collection with the + * collection being merged in. This behavior can be customized by using + * `mergeDeepWith()`. + * + * Note: Indexed and set-like collections are merged using + * `concat()`/`union()` and therefore do not recurse. * * A functional alternative to `collection.mergeDeep()` which will also work * with plain Objects and Arrays. @@ -5558,9 +5577,10 @@ declare namespace Immutable { ): C; /** - * Returns a copy of the collection with the remaining collections merged in - * deeply (recursively), calling the `merger` function whenever an existing - * value is encountered. + * Like `mergeDeep()`, but when two non-collections or incompatible + * collections are encountered at the same key, it uses the `merger` function + * to determine the resulting value. Collections are considered incompatible + * if they fall into separate categories between keyed, indexed, and set-like. * * A functional alternative to `collection.mergeDeepWith()` which will also * work with plain Objects and Arrays.