Map returns an Immutable object

[ntkoopman]

I don't really understand why .map is wrapped to return an Immutable object. This seems needlessly restricting. In combination with #16, this also causes unexpected stack overflow errors.

Immutable([1,2,3]).map(function() {
   return React.createElement( "div", null );
});
// too much recursion

[rtfeldman]

This is by design. One of the invariants of the library is that once you invoke Immutable, what you get back is immutable, and continuing to use its methods will only yield further immutable results.

However, since you can pass seamless-immutable's data structures to other JS libraries, if you really want a mutable copy back, you can just pass them to other libraries that are not designed for immutability.

For example:

_.map(Immutable([1,2,3]), function() {
   return React.createElement( "div", null );
});
// no error

[ntkoopman]

I don't think this makes much sense for the map function.
For the other functions I can understand this rational, because they return parts of the array itself, which are guaranteed to be immutable, but map can return things not related to the immutable structure. To me, this is very unexpected behaviour.

You are also suggesting using a non-standard map implementation for a library that has 'backwards-compatible' in the first line of its summary.

[rtfeldman]

map does indeed return a collection of elements which may be completely unrelated to the original elements. However, map is still supposed to return the same type of collection you began with. If you call [].map you get back an Array, if you call $().map you get back a jQuery object, and if you call Immutable([]).map, you get back a seamless-immutable array. This is the standard map behavior.

I sympathize with the fact that this map implementation doesn't work with React components, because I use seamless-immutable with React and find it annoying that this doesn't Just Work. I manage this annoyance by using _.map when it finally comes time to turn my seamless-immutable arrays into React components, but naturally it would be less trouble in this particular use case if I didn't have to.

That said, I don't think it makes sense to couple this library to React, and I don't see a strong case for changing map on its own merits. When libraries start developing implicit dependencies on other libraries, unexpected consequences tend to snowball into unpleasant surprises. I value the simplicity and consistency of this library, and hope to maintain it.

As such, I appreciate your perspective, but the current design still makes the most sense to me overall. 😃

[abritinthebay]

Agree with @rtfeldman here.

To quote the description of Array.map:

The map() method creates a new array with the results of calling a provided function on every element in this array.

Well... now make it for Immutables:

The map() method creates a new Immutable with the results of calling a provided function on every element in this Immutable.

This is in fact the only logical thing to do - you're operating on a specific type and returning a similar but different type would be... a bit wacky to say the least.

[Jan-Jan]

@abritinthebay +1

[agurtovoy]

I'd like to reopen this discussion in the light that nowadays seamless-immutable treats functions, errors, dates, and React components as immutable even though they are not in order to make the library play nicely with the rest of the code out there.

Here's a problem I ran into yesterday when using seamless-immutable together with Draft.js:

const editorState = Immutable( { content: Draft.convertToRaw( ContentState.createFromText( text ) ) } );
...
const state =  Draft.convertFromRaw( editorState.content );
//             ^^^^^^^^^^^^^^^^^^^^
// TypeError: block.getKey is not a function
//    at node_modules/draft-js/lib/BlockMapBuilder.js:24:21
//    at Array.map (native)
//    at Array.<anonymous> (node_modules/seamless-immutable/src/seamless-immutable.js:132:38)
//    at Object.createFromArray (node_modules/draft-js/lib/BlockMapBuilder.js:22:30)
//    at Function.createFromBlockArray (node_modules/draft-js/lib/ContentState.js:163:36)
//    at convertFromRawToDraftState (node_modules/draft-js/lib/convertFromRawToDraftState.js:89:23)

The reason the above fails is that Draft.js' createFromBlockArray uses .map to convert an array of raw objects into an array of prototype-based objects, which seamless-immutable promptly strips down to raw immutable data, no methods, thus block.getKey is not a function.

I worked around it by running editorState.content through Immutable.asMutable( ..., { deep: true } ), but this was thoroughly unexpected and IMHO incompatible with the library's promise of "data structures which are backwards-compatible with normal Arrays and Objects."

I think it makes a lot of sense to make plain-old-js-objects returned from .map immutable, but we've gotta pass everything else through as-is.

Thoughts?

[willian]

@agurtovoy I'm having the same issue with react-select, the only way to solve it was by using .asMutable as well.
My team and I are starting to study alternatives, Immer looks really nice and I want to give it a try.