Skip to content
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

BREAKING: no longer deeply coerce argument to merge() #1339

Merged
merged 2 commits into from
Oct 5, 2017
Merged

Conversation

leebyron
Copy link
Collaborator

@leebyron leebyron commented Oct 4, 2017

The methods merge() and mergeDeep() were the last places internally to call fromJS(), a method that is expensive, and limits the ability to use plain object and array values within immutable collections.

This PR removes fromJS, not doing any deep coercion at all for merge() and simply passing object (incl array) values into the merge() function recursively for mergeDeep() operations, so only coercing those values that expect to be - leaving others alone.

This is breaking because calls to merge() previously were implicitly fromJS()'d to get deeply immutable collections, however that is no longer the case.

To update code which relied on this behavior, convert collection.merge(x) to collection.merge(fromJS(x)).

Fixes #1293

The methods `merge()` and `mergeDeep()` were the last places internally to call `fromJS()`, a method that is expensive, and limits the ability to use plain object and array values within immutable collections.

This PR removes fromJS, not doing any deep coercion at all for `merge()` and simply passing object (incl array) values into the `merge()` function recursively for `mergeDeep()` operations, so only coercing those values that expect to be - leaving others alone.

This is breaking because calls to `merge()` previously were implicitly `fromJS()`'d to get deeply immutable collections, however that is no longer the case.

To update code which relied on this behavior, convert `collection.merge(x)` to `collection.merge(fromJS(x))`.

Fixes #1293
@leebyron leebyron merged commit 243bb3a into master Oct 5, 2017
@leebyron leebyron deleted the fix-1293 branch October 5, 2017 00:44
YannickDa pushed a commit to YannickDa/redux-localstorage-immutable that referenced this pull request Oct 4, 2019
…le-js/immutable-js/releases/tag/v4.0.0-rc.5) :

No longer deeply coerce argument to merge() ([#1339](immutable-js/immutable-js#1339]))

Previously, the argument provided to merge() was deeply converted to Immutable collections via fromJS(). This was the only function in the library which calls fromJS() indirectly directly, and it was surprising and made it difficult to understand what the result of merge() would be. Now, the value provided to merge() is only shallowly converted to an Immutable collection, similar to related methods in the library. This may change the behavior of your calls to merge().
@renanbandeira
Copy link

Hi, I've been using Immutable for years and I have about 500 merge calls in my project that I used to prevent transformin plain objects in immutable before an update or set. Is there any way I could pre process those merge and mergeDeep, mergeDeepIn, mergeIn, mergeDeepWith converting the argument in immutable without doing by hand?

Thanks

@leebyron
Copy link
Collaborator Author

Yes you can call fromJS() on any deep object tree, this is what merge was doing implicitly in v3, but v4 allows the use of plain JS structures with functional immutable methods

@renanbandeira
Copy link

Ok, but is there any script to make this change automatically or I should change every one of the 500 merge usages in my code?

@leebyron
Copy link
Collaborator Author

I’m not aware of a script, but if you’d like to contribute one, that would be a great positive impact for the community! Jscodeshift could be a good place to start

@renanbandeira
Copy link

That sounds great! I will take a look at Jscodeshift in order to contribute. Thanks!

@renanbandeira
Copy link

renanbandeira commented Apr 19, 2020

Hi @leebyron, I just created with one: https://github.com/renanbandeira/jscodeshift-upgrade-immutable

Thanks for the tip.

acusti added a commit to acusti/draft-js that referenced this pull request May 19, 2021
future-proof for immutable v4.x, which no longer calls fromJS on the argument passed to merge() and mergeDeep(): immutable-js/immutable-js#1339
acusti added a commit to acusti/draft-js that referenced this pull request May 21, 2021
future-proof for immutable v4.x, which no longer calls fromJS on the argument passed to merge() and mergeDeep(): immutable-js/immutable-js#1339
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Record#merge() converts argument to Immutable structure
3 participants