Is there any interest in allowing mutable methods to return copies?

[danprince]

I've moved some bits around in a local clone of the repo so that rather than throwing an exception when any of the banned mutable methods are called, a quick copy is created and is mutated then returned as immutable.

Most of this logic happens in a new function.

function proxyMutableMethod(obj, methodName) {                                                                                                   
   var methodRef = obj[methodName];                                                                                                               

  addPropertyTo(obj, methodName, function() {                                                                                                    
    var host = obj instanceof Array ? [] : {},                                                                                                   
         copy = quickCopy(obj, host);

    methodRef.apply(copy, arguments);
    return Immutable(copy);
  });
}

Rather than calling banProperty for each of the banned methods, proxyMutableMethod is called instead.

Now aside from the obvious performance implications (all these operations become at least O(n)*) is there a reason why this isn't already a feature of the library? I understand that the performance itself may be enough of a reason, but it makes it more friendly for people coming from Clojure or Immutable.js. Just want to know whether I'm missing something before I crack on with writing the tests and amending the docs so I can send a pull request.

* It wouldn't be too much work to add an interface for defining structural sharing overrides for some of the mutable methods though.

[dallonf]

That sounds like it would cause some very odd behavior and differences from mutable counterparts:

var mutableArray = ["a", "b", "c"];
console.log(mutableArray.push("d")); // Prints 4, the new length of the array

var immutableArray = Immutable(["a", "b", "c"]);
console.log(mutableArray.push("d")); // Prints ["a", "b", "c", "d"], the copy of the array

It's pretty rare that you would actually use the return value of push(), for example, but if you did, this would be a major headache and it'd be hard to find out what was going on.

Although maybe if you rename the proxy method (pushImmutable or pushCopy or something), this could be very useful!

[danprince]

Yeah, that's a really good point. It would of course be possible to rename the proxied method, but fundamentally it'd be confusing in the end. I often forget that some of those mutable methods return slightly odd things.

Method	Returns
push	Length of new array
pop	Removed last item
sort	Sorted array
splice	Array containing deleted items
shift	Removed first item
unshift	Length of new array
reverse	Reversed array

Only sort and reverse would keep consistent behavior between both versions. It would be possible to tag these as proxyableMethods and only apply this proxy to them?

Or rename to Ipush/pushImmutable/whatever then make that explicit within the immutable error for the banned methods. Then update the docs to make it explicit that these copying methods return the state of the object/array after applying the mutable change to a copy, so that there's no confusion as to what they return.

I agree, it would be very useful to have these methods, but not at the cost of adding unnecessary complexity, which might be the long and short of hiding them behind suffixed names? I'd be interested to hear what other people think about that suggestion?

[danprince]

What about prefixing them with after? That way the semantics of the operation can't be confused and it can be made clear that these methods return the state of the array after the operation, rather than whatever they usually return. This way they will still work whilst method chaining.

Immutable([1, 2, 3]).afterPush(4); // [1, 2, 3, 4]
Immutable([3, 2, 1]).afterSort(); // [1, 2, 3]
Immutable(['a', 'b', 'c']).afterReverse(); // ['c', 'b', 'a']

var ns = Immutable([1, 2, 3, 4, 5])
  .slice(2)
  .map(n => 2 * 2)
  .afterPush(3)
  .afterSort();

// [3, 6, 8, 10]

[jokeyrhyme]

I've just started using this library, having played with Facebook's immutable.js previously.

I have an Immutable Array, and I want to get a new Immutable Array that includes the contents of the previous one, with the addition of a new item at the end. The first thing I tried is #push() which throws an error and now I am here.

To achieve this, should I do something like this?

var array = Immutable([1, 2, 3]);
array = Immutable(array.asMutable().concat([4]));

[sudhakar]

:+1: Started with seamless-immutable & after failing to easily splice, tried porting it to immutable-js. Now my app has become even more complex with all that get, setIn etc, I am now back here.

How about ruby sort of suffix for immutable methods like splice$ or splice_ ?

[jedwards1211]

Let's back up a moment here: seamless-immutable has already changed the behavior of these standard JS methods. It has to; they can't perform any mutations. It tries valiantly to make immutables look and feel as much like mutables as it can, but it could never go all the way. Therefore if users call any of those methods expecting usual JS behavior, they'll be surprised anyway.

So seamless-immutable might as well change the API however it wants. If push, pop, etc. return a new immutable, it won't be more surprising to the user than throwing an error, and I think users will get used to it easily, since plenty of people have gotten used to it being the standard behavior of Immutable.js.

[wesleytodd]

FWIW, I agree with @jedwards1211. The point of this library IS the changes, so I think this kind of change is a good thing.

Also, from a new users perspective, this is a big deal. Having to call .asMutable or wrap in a try/catch before passing it to any component that is un-trusted all the time is not great. And a throw in production if something calls one of these methods is really bad.

EDIT: an hour more in and I realized that I am doing alot of a = Immutable(a.asMutable().push(val)) type stuff. Which, IMHO, is that this library should be doing for me. Really what I want is a = a.push(val), as per this issue, otherwise what is the point of wrapping my arrays in this lib?

EDIT 2: Realized after some sleep that the throw wouldn't happen in production.

[planttheidea]

I also want to chime in, because taking the "people have gotten used to immutable.js" comment one step further, I believe returning the new object is (if you look at it without your JavaScript-trained glasses on) the expected behavior.

const foo = [];
const bar = foo.push('foo');

No developer that didn't know of the mutable quirks of JavaScript would look at that and say that bar is obviously equal to 0. The same is true for splice, reverse, sort, etc. Also, in the larger context of this being an immutable library, the fact that with the current behavior you can (and do) perform a mutation in production seems counter to its inherent goal.

There are easy-to-implement immutable versions of all the functions you call out, made especially easy with ES2015. It seems to be a big miss that a library dedicated to immutability is only propagating it in the most literal sense of the word, and not in the larger context of developing with the methodology.

[rtfeldman]

@wesleytodd Instead of doing a = Immutable(a.asMutable().push(val)), I would do a variant of what @jokeyrhyme suggested:

a = Immutable(a).concat([4])

The main reason I went with the “explode when you try to mutate” design was to avoid pernicious bugs when changing over previously mutable code to use Immutable. This way if you ever forget to change something over from the old style to the new style, you'll at least get an error!

As far as adding a separate set of methods to add this functionality, I like keeping the API intentionally simple, and I don't think something like Immutable(a).afterPush(4) is enough added convenience over Immutable(a).concat([4]) to justify adding it.

Thanks for the spirited discussion! You folks are excellent. :smiley_cat: