Skip to content

Breaking change between 3.x and 4.x #1504

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

Closed
richtera opened this issue Apr 3, 2018 · 7 comments
Closed

Breaking change between 3.x and 4.x #1504

richtera opened this issue Apr 3, 2018 · 7 comments
Milestone

Comments

@richtera
Copy link

richtera commented Apr 3, 2018

What happened

map.toArray() changed to return elements using [key, value] instead of just returning value.

const vals = Map({
   a: 1,
   b: 2
}).toArray();
console.log(vals);
// 3.x
[1, 2]
// 4.x
[['a', 1], ['b', 2]]

This causes fatal breaks in most of my react code. I think toArray should stay and maybe add a toArrayElements() or something.

How to reproduce

@richtera
Copy link
Author

richtera commented Apr 11, 2018

Is anyone else concerned about .toArray() working differently in the new version? I would need to change 100s of places to call .toList().toArray() instead of just .toArray() like in 3.x.

@acusti
Copy link
Contributor

acusti commented May 3, 2018

#1340 indicates that this is likely to be a change that sticks. The justification is to more generally make the behavior of Immutable.Map more closely match that of native JS Map, where tuples are the norm for how to represent key value pairs. For example:

const nativeMap = new Map([['key', 'val']]);
// Map(1) {"key" => "val"}
Array.from(nativeMap);
// [["key", "val"]]

From that PR, the recommended code transform is x.toArray() -> x.valueSeq().toArray() (which will be more efficient than x.toList().toArray()).

This is definitely a big breaking change and should be loudly called out in the final release notes for 4 stable.

@richtera
Copy link
Author

richtera commented May 3, 2018

Ok :-( I suppose if React accepted the results of a call like

<div>
  {Map({
    name: 'Andy'
  }).map((item, key) => <div key={key}>Something</div>)}
</div>

Then it would be no problem. We could just remove the code :)

@acusti acusti mentioned this issue May 3, 2018
@leebyron leebyron added this to the 4.0 milestone May 17, 2018
@zhujinxuan
Copy link

About the toArray in the first post, I think it is caused by the following code in src/CollectionImpl.js:

mixin(Collection, {
  // ### Conversion to other types

  toArray() {
    assertNotInfinite(this.size);
    const array = new Array(this.size || 0);
    const useTuples = isKeyed(this);
    let i = 0;
    this.__iterate((v, k) => {
      // Keyed collections produce an array of tuples.
      array[i++] = useTuples ? [k, v] : v;
    });
    return array;
  },

The question is that shall we fix that part of code? Personally I think the immutable v-4's result seems more sensible. @acusti

@venikx
Copy link

venikx commented Jun 12, 2018

The change from Map.toArray => [...values] to [...[key, value]] is more familiar to how ES6 Map works and I think it's change that should stay.

The case of changing toArray() to toList().toArray(), could be fixed via some sort of codemod?

@richtera
Copy link
Author

Sounds good. Should I close this?

@Brantron
Copy link

@richtera yes, please close this

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

6 participants