Throw TypeError on merge() if types are incompatible

[Methuselah96]

Resolves https://github.com/immutable-js-oss/immutable-js/issues/70.

[acusti]

@Methuselah96 it’s awesome that this last issue for 4.0 is resolved!

i’m late to the party, but i think making a .merge() of two immutable collections unsafe (i.e requiring a try / catch if there is any chance the two collections may have differing internal structures) goes against the existing APIs, and potentially problematic. part of the beauty of the immutable getter/setter APIs is that they are safe and fail predictably and without throwing. to illustrate what i mean, i made a runkit: https://runkit.com/acusti/600b642ac3b34d001bfb925a

that example shows the result of a couple of “valid” setIn operations and then the result of an “invalid” setIn operation (where the structure implicitly defined by the keyPath doesn’t match the internal structure of the collection). for setIn the operation is a noop. if setIn threw a TypeError, i think it would make sense for merge() to follow suit. as it is, i can’t think of any other part of the immutable.js API that feels consistent with this new behavior.

i think the most consistent behavior with the existing API would be to actually imitate setIn in this case and preserve the “lefthand” value over the incompatible righthand value, so maybe that’s correct. intuitively, however, i would expect the righthand value to overwrite the original lefthand value, because values are applied from right to left in the case of matching internal structures, as was suggested in the original issue: https://github.com/immutable-js-oss/immutable-js/issues/70#issuecomment-731893548

either behavior is defensible, so either, i feel, is acceptable. but making the function throw seems wrong to me.

[Methuselah96]

@acusti Thanks for your comment. I generally agree with your sentiment that immutable is very permissive and doesn't do much validation and falls back on a no-op in some ambiguous situations.

That being said these cases were already throwing an error in immutable@3.x:

• Map({ b: 'B' }).merge([5])
• Map({ b: 'B' }).merge(List([5]))
• List(['a']).merge({ b: 'B' })

So you would already need a try / catch for those situations. I understand the argument for making these a no-op for sake of consistency with the rest of the library. Maybe I just need to shift my mindset for this project, but I have trouble understanding why a developer would want it to fail silently in these situations since this is almost certainly not the intent of the developer.

I think we can all agree that the issue as reported is the worst outcome because we end up with non-Immutable structures after merging two Immutable structures. I'll continue thinking about this.

@conartist6 Any thoughts from your end?

[acusti]

i didn’t know about the TypeError for merging different collection types (e.g. trying to merge a Map into a List or vice versa, as in your examples), though i see the logic for it. that said, i’m not sure i’m convinced that we should extend that behavior to deeply nested collection incompatibilities.

the obvious use case for a more permissive merge in my mind is unsafe data that comes from a user agent, e.g. localStorage or the like. you fromJS it, make sure it’s isCollection, then merge it over top of something else, or something else over it, and you assume that operation is safe because you already verified that it was a collection.

[bdurrer]

I just had to check, but at least in redux-saga reducers a thrown error does not make the application explode... but in does break an app when an uncaught expression occurs in a middleware generator. Of course it's the devs fault (or redux-saga), but I agree that it's preferrable to "not work" instead of explode when a irrelevant side effect occurs. And also that the API should be consistent.

But it makes it easy to miss bugs, so maybe it would be useful to introduce a logger to immutable? It could be configurable and even be no-op per default

[conartist6]

I agree that a deep merge could and probably should be more permissive than an explicitly specified one. I think consuming unvalidated data from local-storage or such is a footgun waiting to go off no matter what the behavior of merge is. A good dev would use something like joi to define a schema and validate against it, but sadly joi doesn't include any way to use only as much of the input as matches the schema. Anyway, that said I'm sure plenty of devs would just toss the input into a merge and live with the result. I think overwriting is fine and probably the most sensible, but I'm curious about how some other mechanics work. E.g. what happens when trying to merge a record into an object?

[conartist6]

Oh yeah last time I had to do this I think I used ajv.

[Methuselah96]

I think for consistency between shallow merges and deep merges it might make sense to keep the original value if the types differ.

// Shallow
Map({ b: 'B' }).merge([5])
=> Map({ b: 'B' })

// Deep
merge({ a: Map({ b: 'B' }) }, { a: [5] })
=> { a: Map({ b: 'B' }) }

Preserving the type should take precedence over overwriting the value. I think this would be consistent with what would be desired if merging data in from localstorage as well.

Thoughts?

[Methuselah96]

Also, this shouldn't necessarily be the deciding factor, but typing merge would be impossible if the types could change when merging and could definitely lead to unexpected bugs.

[bdurrer]

I remember that we had issues one or two years back because merge was used too often. Add a random prop to a Record type and you might overwrite a whole sub-state. The only use case it still has [in our code] is as convenience function for setting multiple properties at once:

state.merge({
  a: 'one',
  b: 'two',
}

I like the idea to only merge things where the original type can be retained and falling back to no-op. Keeping the "new" value would work too, but I agree with conartist that it is a greater security hazard.

The third option would be to always prefer immutable types. So in the follwing sample both times a would become a List.

Map({ a: List([5]) })
.merge({ a: [3,4] })

Map({ a: [5] })
.merge({ a: List([3,4]) })

But then again, if you ever end up with code where changing types are okay, your code sucks. It should be a way to increase consistency in both the API and the data, not a magic randomizer.

PS: I would also suggest to bring mergeWith and merge together. That would make it consistent with update and fromJS and IMO would make it obvious how to deal with "merge weird types" works: by providing a custom merger [reviver/updater/...] function.

[acusti]

I think for consistency between shallow merges and deep merges it might make sense to keep the original value if the types differ. … Preserving the type should take precedence over overwriting the value. I think this would be consistent with what would be desired if merging data in from localstorage as well.

that’s a good observation that i hadn’t really considered. that point, combined with the consistency with setIn behavior, makes me now agree that keeping the original value if the types differ is the best option.