make moment mostly immutable

[ichernev]

There has been a lot of discussions about this. Here's the proposal:

The following mutable methods will become immutable in 3.0.0: utc, local, zone, add, subtract, startOf, endOf, lang, also in duration: add, subtract, and lang.

As a start, all methods will be duplicated with methodNameMute variants. We also need immutable variants named methodNameImmute. From 3.0 the plain old methods will start using the immutable option by default.

What is debatable is:

• should lang be made immutable
• should all the getters/setters (including get/set) be made immutable too
• naming of the mutable and immutable versions of the methods
• and of course -- should we do the switch, or just stop at the immutable API

The good part is we can make immutable versions of the methods today and decide later on what to do. If we switch it would also mean the 2.x branch would be around for quite some time after 3.x is out.

@icambron @timrwood @gregwebs @yang @lfnavess @soswow @langalex

[icambron]

My two cents is that we should either go for full-hog immutability or not at all. Having some methods be immutable (startOf, add) and some not (year, get) is just confusing, and developers will have to keep track of which ones are which.

[gregwebs]

I would also prefer everything immutable by default. Are the getters not immutable already?

[timrwood]

Here are the big issues I see with switching to immutability.

Pseudo-Immutability

Because of the nature of javascript, we will never be able to have true immutability.

The best we can do is create a copy and then mutate and return that copy. We can carefully wrap all the public methods to make sure we are always copying and mutating rather than just mutating, but that doesn't prevent someone from doing m._d = new Date() or even m._d.setHours(1).

I agree with @icambron, if we move to immutability, it should be a complete change. Any method that could possibly change a property on a moment would instead create a copy of the moment and make a change on the copy instead.

Api Surface Area

The unfortunate thing about switching to pseudo-immutability is that many new apis need to be created if we want to still support mutability.

Before, switching from mutating a moment to cloning and mutating a moment was as simple as adding a .clone() in the right place. Now, we would have to create mutable interfaces for all setters, which adds considerably to the api surface area.

This includes the ~20 setter methods, add/subtract, local/utc/zone/tz, startOf/endOf, lang, and any other methods used in 3rd party plugins.

Memory Concerns

Because we are now creating copies every time we want to change a value, the memory usage will increase. Of course, the new moments will be garbage collected, but the additional cost associated with this change is something to keep in mind.

We would have to take great care to make sure we are not creating tons of disposable clones with methods that use other setters.

To track what methods are being called, I used this little function wrapper.

for (var method in moment.fn) {
  moment.fn[method] = (function (fn, method) {
    return function () {
      console.log(method);
      return fn.apply(this, arguments)
    }
  })(moment.fn[method], method)
}

Now, when running a method, we can see how may other methods on the prototype are used. Not all of these methods would need to clone the moment, so I added comments on the ones that would require cloning.

moment().isSame(moment(), 'year')
isSame
clone        // clone
startOf      // clone
month        // clone
date         // clone
year         // clone
date         // clone
hours        // clone
minutes      // clone
seconds      // clone
milliseconds // clone
valueOf
local        // clone
zone         // clone
startOf      // clone
month        // clone
date         // clone
year         // clone
date         // clone
hours        // clone
minutes      // clone
seconds      // clone
milliseconds // clone
valueOf

That is 21 copies that are created and then immediately discarded. Obviously we could optimize this by using some internal methods that are mutable and only expose the immutable versions, but it will significantly add to the internal complexity trying to keep a record of which moments still need cloning and which don't.

Performance Concerns

Cloning a moment is much slower than mutating a moment. I threw together a couple jsperf tests for this.

http://jsperf.com/moment-cloning

http://jsperf.com/moment-cloning-2

I think the second test is a much better representation of the performance losses when switching to pseudo-immutability. If we multiply those results by the 21 cloning instances noted above, the execution times are that much slower.

I'm sure we could optimize the path for cloning a moment, but we would need to make it 50x faster in order to have comparable performance. I'm pretty sure this is impossible.

Summary

Switching to immutability greatly increases the complexity of the internal and external apis and has major performance and memory concerns. I don't think those costs are worth the benefits that immutability would provide.

[gregwebs]

I think the performance concerns listed here are missing the point:

Generally speaking, an initial .clone() is needed to ensure correctness before performing mutation.

We can't pretend like clone() is not needed with the current API. The main case here that is different is performing multiple sequential mutations. That case is addressed by creating a builder API such that all the mutations are performed as mutations on a single clone.

Am I missing other common use cases?

[soswow]

My original issue was about startOf and endOf methods specifically. For some reason these names for me were like "get me startOf a month" not "set this moment to the startOf a month". Methods like add and subtract are perfectly ok in the way they are in sense of semantics. It's perfectly ok to Add something To an object without creating new one.
For me personally renaming methods startOf and endOf to smth like toStartOf and toEndOf (like "move this moment to start of a month") would solve the issue. IMHO

[icambron]

@gregwebs Sorry, I meant set above.

I disagree with @soswow; I think it needs to be consistent. In fact, I think that toStartOf implies immutability even more strongly, like it's providing an alternative presentation a la toISOString. More importantly, I think we need to be able to make statements like "Moment's setters mutate moments" or "Moments setters return copies", not "well, for these methods..."

On @timrwood's concerns:

That the JS objects won't be truly immutable doesn't bother me. The point is that the API provide an immutable contract. Of course the user can cheat by fiddling with the underscored properties, and cheating is generally possible even languages where immutability is the main way of doing things.

On surface area and about perf: I think we'll need to use the mutators internally to avoid using up all that CPU and memory [1], so we'll then we'll have to support them at some level. Then we might as well expose them externally, like setYear(), etc. That adds a bunch of surface area, but it doesn't really add much complexity; for non-explicit-mutators, clone externally, mutate internally.

One way to look at that is that the user has to clone in their code, so Moment might as well be doing for them. That does present a problem with chaining in perf-sensitive places, which could be battled by either a builder interface (per Greg's idea) or by letting the user just use mutators there. The builder adds a bunch complexity [2], so I think I just favor explicit mutator alternatives. I think the reality is that most of the time, Moment is not being used in perf-sensitive situations, so those situations don't have to be the most convenient API-wise. I'd rather have a nice immutable API with a perf hatch for when I need it.

[1] The cool kids in FP land solve this with structural sharing, but that's probably impractical here.

[2] Traditionally, people make builders that are separate objects, but that would be really verbose here, since you'd have to copy the whole setter API. Just spitballing, but one alternative is that .chain() creates a clone moment that just has an isBuilding flag set on it. Then internal clones are ignored, just returning the object for mutation. Then build() unsets the flag and returns that clone. The trouble is that you need your getters to scream bloody murder if the flag is set, or people will end up using the using the chained but unbuilt Moments which are suddenly mutatorish. Then you need to differentiate externally and internally called getters. Blech. Another alternative is to internally decompose the functionality needed by the builder into a mixin and use it in both the builder and in Moment, but that's probably not workable from a code-organization perspective.

[lfnavess]

what worked to me, was adding an extra parameter to the functions, a flag (i named self) to denote mutability, by default is in inmutable (return a copy or new object), and when i detect perfomance i set the flag to true

this stand point solved a lot of conflicts,
having functions with similar name executing almost the same code,
or having to change the functionname and problably the parameters when i detect performance points
in my public methods i start the code calling the functions with a copy and following calls with the flag in true
with this i also i can chain the functions

in my code i work with arrays of arrays, (like a table, array of rows)
so i have functions to filter, union, etc, that previusly retruns a new array with the reult, and i detect that to get the final result i called several times the same function, now the first call is to create a copy and not change the initial array, and teh following calls i work with the same array droping row that i don't need

an basic example that could be here:
moment.add = function(measure, ammount, self){
}

[lfnavess]

moment.add = function (measure, ammount, self) {
var $moment = self ? this : this.clone();
// the actual code
return $moment;
}

[ichernev]

Thank you everybody for their 2 cents :)

For the protocol, I agree on @icambron's last post on all points.

There are two big questions left.

The easier one is what should the new API be, two options:
1.1 differently named methods (mutable and immutable) year/setYear, startOf/setStartOf
1.2 or builder api chain().mutators().build(), which is the non-hacky version of what @lfnavess proposed.

The builder api definitely looks sexier, but care should be taken that objects don't stay in build mode for too long, which adds another source of trouble for us and the users.

Now the hard problem -- migrating to the new version. I see two options here:
2.1 devs have to rewrite their code (crazy regex might work in 1.1 and AST-level solution for 1.2 -- given nobody uses year and month as the names of their own methods). python took this approach -- we can all see the result -- a brand new language was born!
2.2 option to turn builder api always on (same as now), and a way to deactivate it for new code. This looks more evolutionary, but the amount of confusion it would cause is probably not worth it. Every moment now has a two flags: is it mutable, and in case it is -- is it strictly mutable (no getters) or transitionally mutable (getters ok). Not to mention receiving moment objects in functions -- you should check what the mode is, make sure to maintain it ... what a mess!

---

And now a crazy idea that came to me now

Copy-on-write clone

m = moment();
funcIDontTrust(m.clone());  // doesn't actually clone

function funcIDontTrust(m) {
  m.year(2005);  // perform the clone here
  console.log(m);
}

I'm not sure how much can be shaved off with this approach, given moment instances are pretty light. Also all mutators now have to perform a check.

There are a few ways to implement with varying performance in different scenarios. The good news is that its backwards compatible and we'll save us and our users a great deal of effort. And I think this is more important than reinventing the wheel.

[timrwood]

I'm not sure what we are gaining here.

Switching to immutablilty has a ton of associated costs and maybe I'm missing it, but
I don't really see comparable benefits.

The main benefits seem to be developer preference. Is this all so that developers don't have
to think about ownership of a moment when passing it around?

I don't think switching to immutability will make bugs any less frequent, it will just
change the type of bugs. @ichernev's example even shows the exact kind of bugs that will
surface, which are just as hard to track down.

m = moment();
funcIDontTrust(m.clone());  // doesn't actually clone

function funcIDontTrust(m) {
  m.year(2005);  // perform the clone here
  // m is still in 2014
  // m.year(2005) created a clone but did not assign it to anything
  // it should be `m = m.year(2005)`
  console.log(m);
}

Here is a pros/cons list between mutability and immutability. If I missed anything,
let me know and I'll edit this comment.

Immutable	Mutable
Some devs prefer it	Some other devs prefer it
Avoids bugs when passing around moments	Avoids bugs when forgetting to assign cloned moments
With a few dozen new api methods, mutability will also be supported	With the existing .clone() method, immutability is already supported
	An order of magnitude faster
	Uses significantly less memory

I do think immutability is useful, but I don't think it is a good fit in JavaScript. I think an immutable interface may make sense for a lanugage like Elm, where immutablity is expected, but for JavaScript, I think mutability is expected.

Much of the apis for typeof a === "object" built-ins are mutable. Array#push,pop,reverse,sort,shift,splice,unshift all change the contents of an array rather than returning a new array. All 16 of the Date#setX methods mutate their instance.

I think we are seeing a lot of people complaining about moments being mutable, but if we switch, I think we will have just as many people complaining. This has already happened with the eod/sod methods two years ago.

After looking at a bunch of old issues on mutability, I guess I'm probably sounding like a broken record here. On both sides, it's the same points that have been brought up for the past few years. I just wanted to make sure an argument for keeping a mutable api was represented in the discussion.

[gregwebs]

@timrwood those are good comparisons, but it is pretty clear you haven't taken the time to understand the immutable use case. We have already discussed why the performance comparisons you posted assume a poorly implemented API and do not make sense.

The bug comparison is also invalid. Because momentjs supports a chaining API, one could expect it to be immutable.

var newM = m.year(2005) // wrong, these are both the same!

So both immutable and mutable have the same problem right now. You could avoid it with the current mutable version if you got rid of the chaining API.

So the immutable API is preferable to mutable because you can safely pass a moment between functions. With the current mutable moments if I pass a moment between function I have 2 options

1. The insane buggy way (that is probably most common): investigate all the source code to make sure there aren't unwanted mutations. Write unit tests to make sure unwanted mutations don't creep in.
2. The sane way (lets instead assume everyone is doing this), defensive programming: remember to call the clone() function before mutating in my function.

With the immutable API we don't have to remember to call clone() every time. Instead, we have to remember to call the API function that lets us avoid cloning, but this is only a performance optimization, not an issue of correctness.

[timrwood]

it is pretty clear you haven't taken the time to understand the immutable use case

That's an unfair statement. My argument is that I see the benefits, but do not think they outweigh the costs.

you can safely pass a moment between functions

we don't have to remember to call clone

Is this not the use case for immutability? If there is more to it that I haven't taken the time to understand, please let me know, but this seems to be the only argument for the past few years.

[gregwebs]

@timrwood yes, that is the entire case.

But I don't see a sign from you acknowledging that your case against immutability (horrible performance, promotes a different type of bug not present in the mutable API) is not valid.

[lfnavess]

i don't know if freeze() could help
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Object/freeze

[lfnavess]

i think we should stick with the ecmascript 5 point of view, and maybe add a function that deep freeze the current object, or a global flag that automatically creates frezee objects

http://blogorama.nerdworks.in/preventextensionssealandfreeze/