Prevent mutable structures when merging Immutable objects using mergeDeep #1840

jdeniau · 2021-07-06T12:34:12Z

bdurrer · 2021-07-13T12:07:50Z

Also see conversation of immutable-js-oss#215

bdurrer · 2021-07-13T13:17:18Z

We discussed the issues of merging unmergable types a few times, and it boils down to "there is no general fixed solution that makes everyone happy".

It [probably] started at immutable-js-oss#215 and was discussed on slack a few times, the latest discussion was in june.

Problem: Merging Lists (collections without keys) and Maps (keyed Collection) almost certainly results in unwanted behaviour

A few possible options:

Deny merging them and throw a TypeError
Silently skip merging these and keep the old values.
Sliently overwrite the whole conflicting collection with the new data
Add a merger param (like mergeWith) that is called for every value, which means also for every [sub]collection
Add a conflict param that is called for every incompatible data type combination

We concluded that

mergeDeep is probably not widely used
None of us was very comfortable with mergeDeep, we think it is a dangerous function anyway
some wrote their own mergeDeep using recursive mergeWith, which matched their needs (see below)
We know of one live use-case: storing/retrieving of data from browser store, which could contain incompatible data from a previous app version. Solution 2 would be their for this

Pro / Cons of these solutions

1 - Type Error

Works well with explicit typing (TS)
Breaks with the current behavior of methods (nothing ever throws)
The user must deal with Errors by catching, which probably is an annoying behaviour in a React reducer
Doesn't solve the issue, only uncovers it

2 - keep existing collection

Predictable behavior
Works well with static typing (TS)
Bad for legit use cases where types do change, e.g. collection with mixed data types
If you don't like the behavior, you must implement your own mergeDeep (like Andrew's team did)

3 - keep new collection

Predictable behavior
Bad for static typing (TS)
If you don't like the behavior, you must implement your own mergeDeep (like Andrew's team did)

4 - merger param

(positive) you have full control
(negative) you have full control. Changing the behaviour means you have to provide a full merger function, you might be faster by implementing a custom solution with mergeWith
In contrast to mergeWith it is called for both the collection(s) and the valules of these collections. Implementing it would probably be confusing.

5 - conflict param:

control over conflicts
default could be "keep old", which is good for static typing (TS)
default could log to console to make users aware of the issue
we could make the default overwriteable, e.g. something like Immutable.conflictFunction = (a, b) => a;

Custom solution provided by Andrew Patton:

const isMergeable = (a) =>
    a && typeof a === 'object' && typeof a.mergeWith === 'function' && !List.isList(a);
export const mergeDeep = (a, b) => {
    // If b is null, it would overwrite a, even if a is mergeable
    if (isMergeable(a) && b !== null) {
        return a.mergeWith(mergeDeep, b);
    }
    if (!List.isList(a) || !List.isList(b)) {
        return b;
    }
    return b.reduce((acc, nextItem, index) => {
        const existingItem = acc.get(index);
        if (isMergeable(existingItem)) {
            return acc.set(index, existingItem.mergeWith(mergeDeep, nextItem));
        }
        return acc.set(index, nextItem);
    }, a);
};

jdeniau · 2021-07-19T18:52:13Z

My preferred option according to the current API of immutable goes to 3 : I don't think it's bad in strictly typed land. In fact, you probably don't want to go that in TS/flow as it is really an opposite to strict typing.
If TS/flow does throw an error here, I think it is fine.

In JS-land, I find more logical that :

Map({ l: List(['foo']) })
  .mergeDeep({
    l:  Map({foo: bar }
  })

// { l: {foo:'bar'} }

Without the current API of immutable, I think that every weird comportment should throw instead of behing silent. Explicit is better that implicit. And in this case I do prefer option 1.

leebyron · 2021-07-23T18:53:27Z

I agree with all the analysis here. mergeDeep is not always predictable and while some iterations on improving that behavior should be welcomed, we should probably be careful not to change its existing semantics too much. Better behavior might be best reserved for a new function (if there's demand)

3 - keep new collection

This seems like the most reasonable choice to me. mergeDeep is already a not very explicit type friendly operation in the first place, so I don't put too much weight on that. The key semantic of "merge" is "set multiple times" and "mergeDeep" is "set multiple times, in potentially deep positions in a data structure" - I can't reconcile that semantic with a behavior that keeps an old value (does not set) or throws an error for something that could otherwise be merged, albeit in a non-type-safe way.

At a high level I think a mergeDeep algorithm should be:

If a and b are mergeable:
- Let ab have all (k, v) entries of a
- For each entry (k, v) of b:
  - If k exists in a: let ab[k] be mergeDeep(a[k], v)
  - Otherwise let ab[k] be v
- Return ab
Otherwise return b

leebyron

I suspect the correct fix will require changing the mergeDeep behavior of determining if two deep values are mergeable rather than changing the existing behavior of merge and concat (though it does seem like concat needs behavior refinement)

leebyron · 2021-07-23T18:55:10Z

src/List.js

@@ -148,10 +150,16 @@ export class List extends IndexedCollection {
    const seqs = [];
    for (let i = 0; i < arguments.length; i++) {
      const argument = arguments[i];
+      const argumentHasIterator =


NB: there's an existing concept for this we may want to use https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Symbol/isConcatSpreadable in the future

leebyron · 2021-07-23T18:59:25Z

src/List.js

+        isKeyed(argument) ||
+        (!argumentHasIterator && isPlainObject(argument))
+      ) {
+        continue;


I might be reading this wrong, but this looks like if you provide an object or keyed collection to concat() that it will simply omit it?

Can you add a test that List().concat(Map()) returns List [ Map {} ] instead of List []?

leebyron · 2021-07-23T19:00:01Z

src/functional/merge.js

+      (isArray && (isPlainObject(source) || isKeyed(source))) ||
+      (isObject && (Array.isArray(source) || isIndexed(source)))
+    ) {
+      continue;


Similar here - is data being lost?

Methuselah96 · 2021-09-06T22:22:57Z

Thanks for the helpful feedback. Some of the shortcomings mentioned in the review were intentional since this was just a proof of concept to get the conversation started.

There's one class of cases I'm still not sure about. How should the following behave:

Map({ foo: 'bar' }).merge(List(['baz']))
merge({ foo: 'bar' }, ['baz'])
Map({ foo: 'bar' }).mergeDeep(List(['baz']))
mergeDeep({ foo: 'bar' }, ['baz'])

Should these return the Map/object or the List/array? Presumably if this were nested in a mergeDeep scenario it should return the List/array, but do we want to make an exception for the top level for either/both merge or/and mergeDeep so that it's still a Map/object or do we want to go for more across-the-board consistency and have these return the List/array?

bdurrer · 2021-09-07T14:36:06Z

There's one class of cases I'm still not sure about. How should the following behave:
Map({ foo: 'bar' }).merge(List(['baz']))
merge({ foo: 'bar' }, ['baz'])
Map({ foo: 'bar' }).mergeDeep(List(['baz']))
mergeDeep({ foo: 'bar' }, ['baz'])
Should these return the Map/object or the List/array? Presumably if this were nested in a mergeDeep scenario it should return the List/array, but do we want to make an exception for the top level for either/both merge or/and mergeDeep so that it's still a Map/object or do we want to go for more across-the-board consistency and have these return the List/array?

Interesting question. keeping the toplevel makes sense, but it adds a special case that might be confusing too.

I just realized that Object.assign solves this in an clever way:

var obj = { hello: 'world' };
var arr = ['ooh'];
 Object.assign(obj, arr);
// Object { 0: "ooh", hello: "world" }

Object.assign(arr, obj);
// Array [ "ooh" ]

Maybe we also could just treat indexes as keys?
Map({ foo: 'bar' }).merge(List(['baz'])) would produce Map({ foo: 'bar', 0: List(['baz']) })?

Methuselah96 · 2021-09-07T14:52:51Z

Nice observation, I'd be fine with that behavior especially since it has the added benefit of being consistent with Object.assign although that would change the behavior for nested collections since it would no longer replace the Map with the List.

@leebyron What's your opinion of how it should behave?

leebyron · 2021-09-07T15:21:37Z

I like the appeal to consistency with core JS API, and this behavior makes sense.

For documentation and our own sanity sake, do we have a high level algorithm for what the final behavior should be?

Seems like this special case is: if the merge destination is a keyed collection, convert the merge source to a keyed sequence before merging?

leebyron · 2021-09-07T15:30:09Z

My one concern though is whether this is the behavior most will want. It makes sense for a shallow merge where you need to imply some reasonable behavior. Which is what Object.assign is doing. But when merging deeply, is this what most would expect?

My thought was the main use for mergeDeep was to apply an update to a complex data structure with a known shape. It's not too uncommon to have Thing | Array<Thing> in that shape, and if your intent with a deep merge was to replace a Thing with a Thing[] then this new behavior would be frustrating

Methuselah96 · 2021-09-07T15:35:00Z

Yeah, I share that concern. I think my slight leaning is that merge and the top level of mergeDeep should mimic Object.assign (i.e., merge the List into the Map as if it were a keyed collection) and then mergeDeep should replace the data structure if there's a type conflict below the top level.

bdurrer · 2021-09-07T15:47:11Z

I think Methuselah96's proposal (object.assign on top level, replace behavior for rest) is the best we can do, considering that there is no golden bullet.
Letting the user decide could be the next step, but I believe it would not be so implement for both library and user - and as Lee suggested, might/should be an entirely different function.

The only non-derpy use-case we heard of was storing and retrieving data into session storage (oder some other serialized form), where you can't be entirely sure the data still matches the current app's data structure.
I believe the proposal should work with that use case (maybe?).

Whatever we do, the mergeDeep documentation needs more samples and explanation!

leebyron · 2021-09-07T18:04:57Z

I think the Object.assign behavior at the top level makes sense. However for the sake of keeping this specific PR/issue focused, I might suggest limiting changes to the top level merge strategy that aren't directly solving this bad behavior.

Another thing to consider is that the top level merge behavior should be as similar as possible to merge() behavior and we should avoid breaking changes.

Right now at top level this just throws a TypeError in many cases, but there is existing support for Lists of tuples we should avoid breaking (at least until a future major version)

Immutable.Map({a:'X'}).merge(Immutable.List(['Y']))
> Uncaught TypeError: Expected [K, V] tuple: Y
Immutable.Map({a:'X'}).merge(Immutable.fromJS([['b', 'Y']]))
> Map { "a": "X", "b": "Y" }

Methuselah96 · 2021-09-08T03:47:27Z

@bdurrer @leebyron Alright I've redone it; let me know what you think. It's a much more localized change now that will only affect mergeDeep.

Methuselah96 · 2021-09-08T04:39:44Z

Note that this only overwrites the value when we're merging a list into a map (as in #1475) and not vice versa (as in #1719). I'm not sure what to do if we're merging a list into a map because we need to continue to support merging a list of tuples into a map and I can't think of any obvious way to know whether a list is intended to be a list of tuples or not. It seems like the current approach is inconsistent if we leave it like it is. Any thoughts of how to overcome this?

Methuselah96 · 2021-09-08T04:59:11Z

I guess we could determine if it's a list of tuples by checking to see if each element in the list has a length of two. But I'm not sure if that's getting too complex for determining whether to try to merge. That would also mean that we could end up iterating over that list twice (once to check it and the other when merging it).

leebyron · 2021-09-08T05:12:20Z

Yeah I agree with your concern, I don't think sniffing for tuples is sustainable. Considering that the root concern here is that this behavior is too complicated then we should probably bias to a simpler solution that is more explainable, even if that solution covers less useful surface area as a result.

We might even want to make the behavior for "list into map" and "map into list" the same, total replacement. What's the cost of that?

leebyron · 2021-09-08T05:21:59Z

Very nice and focused change in this update. Great work

bdurrer · 2021-09-08T06:47:56Z

Just realized I can't review, I probably don't have any rights 🤷

Since we are discussing it all the time, I suggest to also add test case(s) for partial and absolute conflicts:

e.g. (untested):

    const a = fromJS({
      ch: [
        {
          code: 8,
        },
      ],
      banana: 'good',
    }) as Map<unknown, unknown>;
    const b = fromJS({
      ch: {
        code: 8,
      },
      apple: 'anti-doctor',
    });
    expect(a.mergeDeep(b).equals({
      ch: {
        code: 8,
      },
      apple: 'anti-doctor',
      banana: 'good',
    })).toBe(true);

and absolute conflict:

    const a = Map({
      banana: 'good',
    }) as Map<unknown, unknown>;
    const b = List(['banana']);
    expect(a.mergeDeep(b).equals(b)).toBe(true);

And please, could we stop nesting ternary conditions and just write if statements? That stuff is gonna blow up in our face on day 😅

Methuselah96 · 2021-09-08T14:37:32Z

We might even want to make the behavior for "list into map" and "map into list" the same, total replacement. What's the cost of that?

Looks like the only failing test with that change would be this test checking that you can merge tuples of symbols into a Map using mergeDeep. Not sure what to think of that. Maybe the best compromise we can make is to just say you can't merge tuple lists into a map using mergeDeep? The other alternative (i.e., only replacing lists with maps and not maps with lists) seem like too much of an edge-case and this new behavior seems more consistent/useful.

Methuselah96 · 2021-09-08T14:39:24Z

Just realized I can't review, I probably don't have any rights 🤷

@leebyron Can we add @bdurrer as a maintainer? @bdurrer's been extremely helpful since the beginning.

Methuselah96 · 2021-09-08T14:41:25Z

And please, could we stop nesting ternary conditions and just write if statements? That stuff is gonna blow up in our face on day 😅

Personally I find nested ternaries a lot easier to read and much more succinct in many situations. Now if only Prettier formatted them without flattening them... (although I am getting used to the flattening)

Methuselah96 · 2021-09-08T14:57:58Z

and absolute conflict:

    const a = Map({
      banana: 'good',
    }) as Map<unknown, unknown>;
    const b = List(['banana']);
    expect(a.mergeDeep(b).equals(b)).toBe(true);

This test currently throws with:

  ● merge › mergeDeep merges absolute conflicts

    TypeError: Expected [K, V] tuple: banana

      839 | function validateEntry(entry) {
      840 |   if (entry !== Object(entry)) {
    > 841 |     throw new TypeError('Expected [K, V] tuple: ' + entry);
          |           ^
      842 |   }
      843 | }
      844 |

I would be inclined to think that throwing an error for this situation is fine. The impetus for this PR and for the attached issues is specifically that people expect mergeDeep to replace nested collections if the collections aren't mergeable. I think it's reasonable for the top-level of mergeDeep to behave identically to merge and it doesn't seem like people are bothered by the fact that merging a List into a Map at the top level results in a TypeError if it's not a list of tuples.

leebyron · 2021-09-10T18:28:18Z

src/functional/merge.js

+  return (
+    isIndexed(oldSeq) === isIndexed(newSeq) &&
+    isKeyed(oldSeq) === isKeyed(newSeq) &&
+    isSet(oldSeq) === isSet(newSeq)


You may need some test coverage to cover this, but I believe isSet only checks if something is actually Set or OrderedSet and doesn't include SetSeq. We don't have a great first class function for this, something is set-like if it is both not indexed and not keyed.

Suggested change

isSet(oldSeq) === isSet(newSeq)

I wondered about that, I should've looked into that more, makes sense.

leebyron · 2021-09-10T18:57:37Z

This is looking great! Any additional tests you want to add? Documentation improvements part of this PR or a follow up?

What do we need to do to unblock the unit test CI spurious failure?

Methuselah96 · 2021-09-10T19:17:56Z

I'll probably add some more tests to make sure the overriding is working as expected for more cases.

I'm willing to add more documentation. Any ideas of what would be most helpful? Would some examples be helpful or maybe expanded clarification on what we consider mergeable?

The build will be fixed once nodejs/node#40030 is released. We could either:

Wait until that is released in a patch release to merge this PR.
Set the Node version in the CI to 16.8 and then revert the change once a patch is released for 16.9.
Merge this without the CI passing since we know it passes locally and just wait for the patch to be released.

Methuselah96 · 2021-09-10T19:22:14Z

Looks like the release of the Node fix is imminent, so we can probably just wait it out since I still have some more tests and possibly documentation to add.

leebyron · 2021-09-10T19:53:26Z

Sounds good. Seems like the build is going to be fixed upstream today.

For documentation, given it's been mentioned a few times that this function's behavior can be confusing, I think it's worth adding:

An explanation of when mergeDeep will recurse vs when it will consider something a "conflict" (ie the collections have to be of similar type - indexed (List)/keyed (Map)/set-like (Set)
How mergeDeep will treat conflicts by always taking the merged in value and that this behavior can be replaced by mergeDeepWith
How Lists and Sets are "merged" via concat/union and therefore do not recurse

I think it would also be helpful to do a scan for some consistency across the few places we document these both as instance methods and as the top level function

For tests, it would be great to explicitly include a test for a scenario that looks like a deep-edit but is actually another example of #1475

a = Map({ a: List([ Map({ x: 1 }) ]))
b = Map({ a: Map([[ 0, Map({ y: 2 }) ]]) })
a.mergeDeep(b)

leebyron · 2021-09-10T19:57:04Z

We should probably also add a test for the functional case:

a = { a: [ { x: 1 } ] }
b = Map({ a: Map([[ 0, Map({ y: 2 }) ]]) })
mergeDeep(a, b)

Methuselah96 · 2021-09-13T15:13:02Z

Added some more tests and updated the documentation.

leebyron

Excellent work. Great tests and docs improvements

leebyron · 2021-09-13T19:46:16Z

src/functional/merge.js

      ? mergeWithSources(oldValue, [newValue], deepMerger)
      : merger
      ? merger(oldValue, newValue, key)
      : newValue;
  }
  return deepMerger;
 }
+
+function areMergeable(oldDataStructure, newDataStructure) {


Nit: it would still be nice to include a comment block above this function explaining why this implementation determines things are considered mergeable

Done. Let me know if that's what you were thinking or if you were asking for a different explanation.

Yeah this looks good to me. It's the connection between the collection categorization and this somewhat nuanced boolean logic that will be helpful when we look at this function again after our collective memory is lost

leebyron · 2021-09-13T20:26:43Z

One small note before this squash-merge lands:

We should make sure the commit message includes a note about the aspect of this PR which is a breaking change so we can clearly communicate that in the release notes for the next RC.

I think that's just the behavior related to the one removed unit test (eg. lists of tuples no longer deep merge into maps)?

Methuselah96 · 2021-09-13T20:28:50Z

Yeah, that sounds like the only "expected" behavior that has been changed in this PR.

686: Update dependency immutable to v4.0.0-rc.15 r=elliotcourant a=renovate[bot] [![WhiteSource Renovate](https://app.renovatebot.com/images/banner.svg)](https://renovatebot.com) This PR contains the following updates: | Package | Change | Age | Adoption | Passing | Confidence | |---|---|---|---|---|---| | [immutable](https://immutable-js.com) ([source](https://togithub.com/immutable-js/immutable-js)) | [`4.0.0-rc.14` -> `4.0.0-rc.15`](https://renovatebot.com/diffs/npm/immutable/4.0.0-rc.14/4.0.0-rc.15) | [![age](https://badges.renovateapi.com/packages/npm/immutable/4.0.0-rc.15/age-slim)](https://docs.renovatebot.com/merge-confidence/) | [![adoption](https://badges.renovateapi.com/packages/npm/immutable/4.0.0-rc.15/adoption-slim)](https://docs.renovatebot.com/merge-confidence/) | [![passing](https://badges.renovateapi.com/packages/npm/immutable/4.0.0-rc.15/compatibility-slim/4.0.0-rc.14)](https://docs.renovatebot.com/merge-confidence/) | [![confidence](https://badges.renovateapi.com/packages/npm/immutable/4.0.0-rc.15/confidence-slim/4.0.0-rc.14)](https://docs.renovatebot.com/merge-confidence/) | --- ### Release Notes <details> <summary>immutable-js/immutable-js</summary> ### [`v4.0.0-rc.15`](https://togithub.com/immutable-js/immutable-js/releases/v4.0.0-rc.15) [Compare Source](https://togithub.com/immutable-js/immutable-js/compare/v4.0.0-rc.14...v4.0.0-rc.15) This is the last planned RC release before releasing a stable 4.0!! 🎉 🎉 🎉 **BREAKING:** - Replace incompatible collections when merging nested data with `mergeDeep()` ([#1840](https://togithub.com/immutable-js/immutable-js/issues/1840)) - This means that `mergeDeep()` will no longer merge lists of tuples into maps. For more information see [immutable-js/immutable-js#1840 and the updated `mergeDeep()` documentation. **New:** - Add "sideEffects: false" to package.json ([#1661](https://togithub.com/immutable-js/immutable-js/issues/1661)) - Update Flow to latest version ([#1863](https://togithub.com/immutable-js/immutable-js/issues/1863)) - Use ES standard for iterator method reuse ([#1867](https://togithub.com/immutable-js/immutable-js/issues/1867)) - Generalize fromJS() and Seq() to support Sets ([#1865](https://togithub.com/immutable-js/immutable-js/issues/1865)) **Fixes:** - Fix some TS type defs ([#1847](https://togithub.com/immutable-js/immutable-js/issues/1847)) - Adds `ArrayLike<T>` as option to type factory functions and `fromJS` now returns `Collection<unknown>` instead of just `unknown`. - Fix issue with IE11 and missing Symbol.iterator ([#1850](https://togithub.com/immutable-js/immutable-js/issues/1850)) - Simplify typescript definition files to support all UMD use cases ([#1854](https://togithub.com/immutable-js/immutable-js/issues/1854)) </details> --- ### Configuration 📅 **Schedule**: At any time (no schedule defined). 🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied. ♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox. 🔕 **Ignore**: Close this PR and you won't be reminded about this update again. --- - [ ] If you want to rebase/retry this PR, check this box. --- This PR has been generated by [WhiteSource Renovate](https://renovate.whitesourcesoftware.com). View repository job log [here](https://app.renovatebot.com/dashboard#github/monetr/web-ui). 687: Update dependency sass to v1.41.1 r=elliotcourant a=renovate[bot] [![WhiteSource Renovate](https://app.renovatebot.com/images/banner.svg)](https://renovatebot.com) This PR contains the following updates: | Package | Change | Age | Adoption | Passing | Confidence | |---|---|---|---|---|---| | [sass](https://togithub.com/sass/dart-sass) | [`1.41.0` -> `1.41.1`](https://renovatebot.com/diffs/npm/sass/1.41.0/1.41.1) | [![age](https://badges.renovateapi.com/packages/npm/sass/1.41.1/age-slim)](https://docs.renovatebot.com/merge-confidence/) | [![adoption](https://badges.renovateapi.com/packages/npm/sass/1.41.1/adoption-slim)](https://docs.renovatebot.com/merge-confidence/) | [![passing](https://badges.renovateapi.com/packages/npm/sass/1.41.1/compatibility-slim/1.41.0)](https://docs.renovatebot.com/merge-confidence/) | [![confidence](https://badges.renovateapi.com/packages/npm/sass/1.41.1/confidence-slim/1.41.0)](https://docs.renovatebot.com/merge-confidence/) | --- ### Release Notes <details> <summary>sass/dart-sass</summary> ### [`v1.41.1`](https://togithub.com/sass/dart-sass/blob/master/CHANGELOG.md#1411) [Compare Source](https://togithub.com/sass/dart-sass/compare/1.41.0...1.41.1) - Preserve parentheses around `var()` functions in calculations, because they could potentially be replaced with sub-expressions that might need to be parenthesized. </details> --- ### Configuration 📅 **Schedule**: At any time (no schedule defined). 🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied. ♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox. 🔕 **Ignore**: Close this PR and you won't be reminded about this update again. --- - [ ] If you want to rebase/retry this PR, check this box. --- This PR has been generated by [WhiteSource Renovate](https://renovate.whitesourcesoftware.com). View repository job log [here](https://app.renovatebot.com/dashboard#github/monetr/web-ui). Co-authored-by: Renovate Bot <bot@renovateapp.com>

jdeniau assigned Methuselah96 Jul 6, 2021

jdeniau mentioned this pull request Jul 6, 2021

Prevent mutable structures when merging Immutable objects immutable-js-oss/immutable-js#220

Closed

rgbkrk approved these changes Jul 11, 2021

View reviewed changes

jdeniau linked an issue Jul 19, 2021 that may be closed by this pull request

Deep Merge with same key of different value type (Map vs List) does not merge as expected #1719

Closed

jdeniau linked an issue Jul 19, 2021 that may be closed by this pull request

mergeIn throws Cannot read property 'merge' of null when mergeIn on keyPath that has a null value #1807

Open

jdeniau mentioned this pull request Jul 19, 2021

mergeIn throws Cannot read property 'merge' of null when mergeIn on keyPath that has a null value #1807

Open

leebyron requested changes Jul 23, 2021

View reviewed changes

Methuselah96 force-pushed the fix-undefined-merge-cases branch from 1be909d to 0f4bab4 Compare September 8, 2021 03:25

Methuselah96 added 3 commits September 9, 2021 19:52

Be more strict about mergeability

34ca015

Update documentation

ab51c3e

Change wording

48183f1

leebyron reviewed Sep 10, 2021

View reviewed changes

Methuselah96 added 2 commits September 10, 2021 14:30

Remove isSet check

3e9c703

Remove unused import

050dfd0

Methuselah96 added 10 commits September 12, 2021 16:25

Add more test cases

e912a1b

Add more test cases

cbaa071

Update documentation

25896fa

Update documentation

b526da3

Update documentation

63f4b80

Update documentation

1ab0cd9

Update documentation

31e46bd

Update documentation

6e3be74

Update documentation

5b43972

Update documentation

2bd33cb

leebyron approved these changes Sep 13, 2021

View reviewed changes

Add comments

be3e91f

Methuselah96 merged commit ad90f2e into immutable-js:main Sep 13, 2021

Methuselah96 deleted the fix-undefined-merge-cases branch September 16, 2021 15:06

sync-by-unito bot mentioned this pull request Oct 1, 2021

build(deps): bump immutable from 4.0.0-rc.14 to 4.0.0-rc.15 numbersprotocol/capture-lite#922

Closed

Prevent mutable structures when merging Immutable objects using mergeDeep #1840

Prevent mutable structures when merging Immutable objects using mergeDeep #1840

Conversation

jdeniau commented Jul 6, 2021

bdurrer commented Jul 13, 2021

bdurrer commented Jul 13, 2021

A few possible options:

We concluded that

Pro / Cons of these solutions

1 - Type Error

2 - keep existing collection

3 - keep new collection

4 - merger param

5 - conflict param:

jdeniau commented Jul 19, 2021

leebyron commented Jul 23, 2021

leebyron left a comment

Choose a reason for hiding this comment

leebyron Jul 23, 2021

Choose a reason for hiding this comment

leebyron Jul 23, 2021

Choose a reason for hiding this comment

leebyron Jul 23, 2021

Choose a reason for hiding this comment

Methuselah96 commented Sep 6, 2021 • edited

bdurrer commented Sep 7, 2021

Methuselah96 commented Sep 7, 2021 • edited

leebyron commented Sep 7, 2021

leebyron commented Sep 7, 2021

Methuselah96 commented Sep 7, 2021 • edited

bdurrer commented Sep 7, 2021 • edited

leebyron commented Sep 7, 2021

Methuselah96 commented Sep 8, 2021 • edited

Methuselah96 commented Sep 8, 2021

Methuselah96 commented Sep 8, 2021 • edited

leebyron commented Sep 8, 2021

leebyron commented Sep 8, 2021

bdurrer commented Sep 8, 2021 • edited

Methuselah96 commented Sep 8, 2021 • edited

Methuselah96 commented Sep 8, 2021

Methuselah96 commented Sep 8, 2021

Methuselah96 commented Sep 8, 2021

leebyron Sep 10, 2021 • edited

Choose a reason for hiding this comment

Methuselah96 Sep 10, 2021

Choose a reason for hiding this comment

leebyron commented Sep 10, 2021

Methuselah96 commented Sep 10, 2021 • edited

Methuselah96 commented Sep 10, 2021 • edited

leebyron commented Sep 10, 2021 • edited

leebyron commented Sep 10, 2021

Methuselah96 commented Sep 13, 2021 • edited

leebyron left a comment

Choose a reason for hiding this comment

leebyron Sep 13, 2021

Choose a reason for hiding this comment

Methuselah96 Sep 13, 2021

Choose a reason for hiding this comment

leebyron Sep 13, 2021

Choose a reason for hiding this comment

leebyron commented Sep 13, 2021

Methuselah96 commented Sep 13, 2021

Methuselah96 commented Sep 6, 2021 •

edited

Methuselah96 commented Sep 7, 2021 •

edited

Methuselah96 commented Sep 7, 2021 •

edited

bdurrer commented Sep 7, 2021 •

edited

Methuselah96 commented Sep 8, 2021 •

edited

Methuselah96 commented Sep 8, 2021 •

edited

bdurrer commented Sep 8, 2021 •

edited

Methuselah96 commented Sep 8, 2021 •

edited

leebyron Sep 10, 2021 •

edited

Methuselah96 commented Sep 10, 2021 •

edited

Methuselah96 commented Sep 10, 2021 •

edited

leebyron commented Sep 10, 2021 •

edited

Methuselah96 commented Sep 13, 2021 •

edited