rtfeldman / seamless-immutable

Immutable data structures for JavaScript which are backwards-compatible with normal JS Arrays and Objects.
BSD 3-Clause "New" or "Revised" License
5.37k stars 194 forks source link

.slice() not returning a mutable copy #175

Closed marbemac closed 7 years ago

marbemac commented 7 years ago

Should it? We just ran into an issue because we use seamless-immutable + redux-form, and they recently introduced this change, which breaks setIn w arrays:

https://github.com/erikras/redux-form/pull/2175/files#diff-ba79b920ecda0b13b45785ad078f6958L14

Basically, they moved to arr.slice() instead of [...arr] to produce a shallow mutable copy of the array.

As you can see in the image below, immutableArr.slice() does not return a mutable copy. Note the 4th command, that outputs y - it is still "foo", when we would expect it to be mutated to "bar". This is confusing, because the tests seem to check for this:

https://github.com/rtfeldman/seamless-immutable/blob/8a75d7fc6a4e892b857e5afdc714c1cc9a95fc2a/test/ImmutableArray/test-set.js#L38-L48

screen shot 2016-12-01 at 1 07 11 am
huan086 commented 7 years ago

Seems to be intentional, looking at the following example

Immutable([1, 2, 3]).concat([10, 9, 8]).sort()
// This will also throw ImmutableError, because an Immutable Array's methods
// (including concat()) are guaranteed to return other immutable values.

[1, 2, 3].concat(Immutable([6, 5, 4])).sort()
// This will succeed, and will yield a sorted mutable array containing
// [1, 2, 3, 4, 5, 6], because a vanilla array's concat() method has
// no knowledge of Immutable.

Needs to be fixed on the redux-form side.

marbemac commented 7 years ago

Agreed - I missed that - thanks @huan086.