-
-
Notifications
You must be signed in to change notification settings - Fork 1.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Prevent mutable structures when merging Immutable objects using mergeDeep #1840
Prevent mutable structures when merging Immutable objects using mergeDeep #1840
Conversation
Also see conversation of immutable-js-oss#215 |
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:
We concluded that
Pro / Cons of these solutions1 - Type Error
2 - keep existing collection
3 - keep new collection
4 - merger param
5 - conflict param:
Custom solution provided by Andrew Patton:
|
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. 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. |
I agree with all the analysis here.
This seems like the most reasonable choice to me. At a high level I think a mergeDeep algorithm should be:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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)
src/List.js
Outdated
@@ -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 = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
src/List.js
Outdated
isKeyed(argument) || | ||
(!argumentHasIterator && isPlainObject(argument)) | ||
) { | ||
continue; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 []
?
src/functional/merge.js
Outdated
(isArray && (isPlainObject(source) || isKeyed(source))) || | ||
(isObject && (Array.isArray(source) || isIndexed(source))) | ||
) { | ||
continue; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similar here - is data being lost?
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:
Should these return the |
Interesting question. keeping the toplevel makes sense, but it adds a special case that might be confusing too. I just realized that
Maybe we also could just treat indexes as keys? |
Nice observation, I'd be fine with that behavior especially since it has the added benefit of being consistent with @leebyron What's your opinion of how it should behave? |
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? |
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 |
Yeah, I share that concern. I think my slight leaning is that |
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. 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. Whatever we do, the mergeDeep documentation needs more samples and explanation! |
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 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)
|
1be909d
to
0f4bab4
Compare
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? |
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). |
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? |
Very nice and focused change in this update. Great work |
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):
and absolute conflict:
And please, could we stop nesting ternary conditions and just write if statements? That stuff is gonna blow up in our face on day 😅 |
Looks like the only failing test with that change would be this test checking that you can merge tuples of symbols into a |
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) |
This test currently throws with:
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 |
src/functional/merge.js
Outdated
return ( | ||
isIndexed(oldSeq) === isIndexed(newSeq) && | ||
isKeyed(oldSeq) === isKeyed(newSeq) && | ||
isSet(oldSeq) === isSet(newSeq) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
isSet(oldSeq) === isSet(newSeq) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wondered about that, I should've looked into that more, makes sense.
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? |
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:
|
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. |
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:
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) |
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) |
Added some more tests and updated the documentation. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Excellent work. Great tests and docs improvements
? mergeWithSources(oldValue, [newValue], deepMerger) | ||
: merger | ||
? merger(oldValue, newValue, key) | ||
: newValue; | ||
} | ||
return deepMerger; | ||
} | ||
|
||
function areMergeable(oldDataStructure, newDataStructure) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: it would still be nice to include a comment block above this function explaining why this implementation determines things are considered mergeable
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done. Let me know if that's what you were thinking or if you were asking for a different explanation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
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)? |
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. --- - [ ] <!-- rebase-check -->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. --- - [ ] <!-- rebase-check -->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>
Migrate immutable-js-oss#220
Resolves #1475 and immutable-js-oss#70.