optional immutability - #390

+1

Actually, you know why this might be a bad idea? I bet it breaks the plugins if you actually use it.

I'll leave up the PR anyway.

It really does not break the plugins, because the plugins will continue using the mutable version of "moment".

I just wasted a lot of debugging time because I assumed utc() returned a new time. Without this patch I need to do defensive copying any time a time value is exposed. Great patch, please pull! Perhaps in 2.0 moment can be immutable by default.

I just tried this out in my browser and I can't seem to get it to work at all.

moment().day() // 1
moment().add('days', 3).day() // 1

If I set immutable to false it works as before.

Thanks @gregwebs. I took a look, and yeah, it's just busted. I was a bit circumspect with my testing since it was just an experiment to get some feedback. In this case, the issue is that moment uses some of these functions internally and expects them to mutate, so it isn't working right with immutability on. I need to think about how to solve that without totally destroying the moment codebase.

This pull request passes (merged 3904b06 into cc61ce8).

There, that fixes and tests add, startOf, and endOf for immutable = true. There may be other issues lurking.

The scenario below is one of the worries I have with this.

var a = moment();
a.add('days', 1);
console.log(a.fromNow()); // seconds ago ???
// oh wait, I have to assign the result of a.add() to a
a = a.add('days', 1);
console.log(a.fromNow()); // a day ago

While it is opt in, I feel like it may cause confusion on projects with multiple developers.

@timrwood Hmm, not being able to alter a day without an explicit assignment is kinda the point. It's like strings:

var a = "stuffs"
a.concat(" and things")
console.log(a); // "stuffs"? Where did my things go?

In practice, someone who wanted to use an immutable library would do something more like:

var a = moment();
var b = a.add('days', 1);
console.log(b.fromNow());

Note also that it's a global setting, so the whole project would use Moment in either mutable or immutable mode and not in some weird hybrid. So unless the developers were switching between different projects with different Moment mutability modes, I don't think that's an issue.

I also don't think immutability is fundamentally more confusing; I generally expect most methods to return mutated copies, not do in-place manipulation. In fact, I have been using Moment for a while now and am still sometimes momentarily (haha) confused:

var now = moment();
var tomorrow = now.add('days', 1);
tomorrow.from(now);  //a few seconds ago? what the crap?

//oh wait, I have to clone now first...

I think there are plenty of reasons why you might not want this feature, but dev confusion doesn't seem like it would be one of them.

The very long and overly-contentious discussion on this StackOverflow answer about date immutability (and immutability generally) might be useful context on where I'm coming from on this. While the discussion is ridiculous, I think it illustrates reasonably well that there are just two different ways people think about this stuff, and how frustrating it can be when the software works the way you don't.

@timrwood the current API is confusing: no where is it stated that it is based entirely on mutability. I really didn't expect utc to mutate the existing time. This can be remedied by some documentation. But @icambron's comments are exactly right. Immutability is important for larger teams and projects so they don't need to defensively copy (and so their code is thread-safe, not really a concern in practice in JS).

I feel weird about having a flag that switches behavior. I think that @timrwood's comment about problems with multiple developers is valid. You wouldn't be able to just know whether or not a moment object was immutable. I don't think we can have a system that is set up to do both immutable and mutable moments. With switches like these, when some code relies on mutability and some code relies on immutability, it could be very messy, especially in JavaScript where a lot of code is asynchronous. We don't want the immutability switch to be toggled during the execution of other code.

I think that immutability is better that mutability usually. As you may have observed, I like following what Python does because I think it is a well-designed language. In Python, most values, even datetimes, are immutable. @gregwebs makes a good point about not needing to defensively copy. With things like CSRF protection or SQL injection attacks, the best protections are the ones that are built into the system that the developer doesn't need to remember every time. If we have a system where we don't need to defensively copy every time, then the time when we forget to copy, there won't be a problem.

However, I think that if we switched from mutability to immutability, it would break most applications built on top of moment. Additionally, like @timrwood mentioned in #132, if we switch to immutability, we would get a performance hit as well as there being no way to use the mutable codepath.

If we switch to immutability, that would be a major version switch and I think it would break a lot of code.

I believe that in the near future should have an immutable api. And I agree with @gregwebs, current mutable API is somewhat confusing.

@rockymeza +1

After reading the SO thread @icambron linked to, it seems mutability is more of a developer preference than anything.

There are a lot of examples on each side. As @icambron mentioned, String objects are immutable. However, most of the Array methods are mutable, with the exception of slice. Date objects are mutable with all the getter methods.

I don't think there is really a "correct" solution to the problem of mutability vs immutability. Both have historical precedence, and both carry some developer confusion.

As @rockymeza mentioned, the reason I originally went with mutability was for the slight performance improvement. Also, I personally prefer mutability. (I hate doing str = str.replace(a, b))

While I definitely see the benefit of immutability, I agree with @rockymeza that this would probably break a lot of code and add to the confusion.

Should this not get pulled in, I'll definitely add to the docs to make it abundantly clear that the manipulation methods mutate the original moment, and hopefully this will ease future developer pain.

If the concern is a runtime change of the flag, we could simply prevent it from being changed at runtime other than allowing an initial setting of the flag. Alternatively we could provide a build of the library called moment-immutable.js: pick your build and there are no flags to set. So there is no reason why any code need be broken.

immutability is in fact the "correct" choice in terms of a program being "correct", because again, without immutability you cannot guarantee your program is correct unless you defensively copy every mutable value you expose. I do agree that it is nice to have a mutable version for performance tuning.

the problem with str = str.replace in general is the assumption that the result need be assigned to a variable. The result of the replace should just be chained or used as an argument to a function most of the time. I guess this is part of why I use CoffeeScript instead of Javascript though, its a little harder in plain JS.

While I fall solidly in the immutability camp for all the standard reasons people cite, I agree it's really a matter of preference, and there's not much point in arguing about which is better (seriously, check out the SO discussion if you haven't). So I'm taking for granted that the next version of moment can't simply switch to being immutable across-the-board; it has to be a mode or separate build or something. Otherwise, you'll make everyone who wants mutability unhappy (including the guy who owns the Moment project) and break a bunch of code. IMO, the two realistic options are (a) keep it as it is and (b) support both in some fashion.

Some additional thoughts:

Pro:

• I don't understand @rockymeza's worry about the flag getting switched in the middle of execution. The idea here is that you have an application that you've written against immutable Moment or mutable Moment, and you just set it once at the beginning and that's it. All the user's code would assume immutability or mutability. The user could switch it at some random time, but that would be silly and I don't think we need to protect them from that. That's all to say that from the user's perspective, the flag seems fine.
• I think @gregwebs' idea about separate builds is pretty cool, if the issue is still the flag per se. Would have to think on how you'd implement it without using a bunch of awkward preprocessors. It might just end up being cleaner overall.

Con:

• While I'm convinced we could avoid breaking any user code, I don't think @niwibe is correct about the plugins. In either a browser or CommonJS, you only get one instance of a required library, so if the user sets moment to immutable (or uses an immutable build of moment), all of those plugins have to behave correctly in that context. So a bunch of plugins would break or have to have a kind of ridiculous "not compatible with immutable moment" warning. Even if you can make the plugins use a different kind of moment than the user's code, that would be really confusing. So the plugins will have to be neutral on that subject, which is annoying.
• One big thing that isn't being considered so far is the cost of maintaining separate modes. Two modes means two paths through moment's code, which means that every time you make a change, you have to think "oh wait, that's going to work differently if it's (im)mutable". Similar logic applies to having two builds. And we've already seen that there's some extra testing cost, which I can see growing much larger. So supporting both, in any form, is a real tax on development.

I think this is a great discussion. I never expected for this change set to get pulled in; it was more of a "hey, guys, wouldn't this be cool?" kinda thing. But I do think it's worth thinking about.

You're right, I opined with little information. Check the code slightly and found it did not affect much, but as I said, you're right, I was wrong.

Very good observations @icambron !

Due to the backwards compatibility issues with this, I'm not going to merge it in, but I did add some more explicit documentation on the (im)mutable issue.

timrwood/momentjs.com@ae5c8aa

Hopefully this will help developer confusion in the future, and should the issue come up again, this is a great conversation to reference.

@timrwood thanks for the docs
@icambron do you want to maintain an immutable fork?

@gregwebs I don't. I feel like that would be a sizeable time commitment, given how much the internals of this library are changing/will change. I def don't have the time/energy to take that on and still make it functionally equivalent. And while I very much prefer immutable date objects, it's not quite a big enough deal to me to maintain a fork.

@timrwood Mutation leads to surprising results in composability and expressions. See also:

https://twitter.com/search?q=moment.js%20mutability%20OR%20mutable%20OR%20immutable%20OR%20immutability%20OR%20mutation%20OR%20mutates%20OR%20mutate&src=typd

http://lucumr.pocoo.org/2013/12/9/stop-being-clever/

https://stackoverflow.com/questions/17333425/add-a-duration-to-a-moment-moment-js

In terms of specific constructive recommendations: it would be great to have a second immutable interface (same library, not a fork, not a separate build, not a mode/switch) that just wraps all methods with an additional clone call.

@yang, I don't think all methods would necessarily need to be wrapped with a .clone(). The confusion and frustration around mutability seems to be universally around moment#add and moment#subtract. I haven't seen any examples of devs frustrated that moment#year(2010) mutates the original moment by setting the year, rather than providing a cloned moment with a different year.

@ichernev has actually taken over maintenance of moment.js, so I would defer to him, but one option could be changing moment#add to be immutable in 3.0.0 and moving the immutable version to another name.

moment.fn.addInPlace = moment.fn.add;
moment.fn.add = function (num, str) {
  return this.clone().addInPlace(num, str);
};

I don't think adding something like moment.fn.addImmutable would solve the problem, because by the time you think to use addImmutable instead of add, you could have as easily done .clone().add().

@timrwood I can see that year(2010) is also acceptable in a destructive fluent style, but I've witnessed frustration with e.g. startOf(), local(), and other methods.