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/

[lfnavess]

maybe a extra parameter in the constructor to create a freeze object, because a freeze object can't be unfreeze

[ichernev]

@lfnavess I thought about freeze before mentioning the copy on write. The problem is that ... nobody uses it / knows about it, nor does it help when it doesn't throw an exception (in non-strict mode) -- it actually creates crazy bugs for you to track.

@timrwood I don't think I made my example clear. On the line m.year(2014) // clone here I meant that internally moment would actually make a clone (allocate more memory) and m would point to that new memory, automatically. Well that basically means that clone() should also allocate a bit of shell memory (something to point to the internal date representation), I'm just not sure how much would be gained by doing so.

Creating a half-assed version of clone, that clones only the interface, and the ability to change the underlying data (from shared storage to instance-specific) -- it really depends on how expensive Date objects are. The downside is that every function needs to do this._storage._d instead of this._d, and I'm not sure if that would overcome the benefit.

I didn't get any any comments on how to deal with migrating the existing libraries/users of moment. I don't really like any of the options I listed above.

[icambron]

Reverse-compatability is, IMO, the strongest argument against this. If we do this, we just have to accept that it's a big breaking change. If we don't want to accept that, we shouldn't do it.

It's worth mentioning re:perf that there are also some huge advantages you can gain from immutability; it's not a monotonic perf hit. For example, you can cache things at the object level, because they'll never change. I also think we should be able to optimize the living crap out of clone(); AFAIK it involves cloning a date and copying like five values; I think we should just hardcode them like newThing._offset = oldThing._offset.

Edit, arg, no - plugins add fields too (e.g. here).

[gregwebs]

given the strong desire for backwards compatibility and yet to keep thing light-weight, I think the best solution is to fork the javascript sources (either in the sources of this project or start an entirely new project). There is room for more than 1 time library for the internet.

[icambron]

Also: re: @ichernev's idea on structural sharing, one possibility is to use prototype inheritance instead of wrapping a shared state object.

[butterflyhug]

We at WhoopInc have been lurking in this discussion for quite a while. Since the discussion here seems to be going in circles, I took some time this weekend to explore what an immutable version of moment with a builder API might look like. (I have no intention of submitting a PR against moment unless invited to do so, since I'm deliberately making more strident API changes than I would expect to ever see implemented in moment itself.) Here's the result: https://github.com/WhoopInc/frozen-moment

I'm just a few hours in, so everything is really rough around the edges, but based on test results I think most of the core moment functionality is working. I'll continue to track this conversation, and I'd welcome feedback specific to our fork in our repo's issues.

I'll try to publish some updated docs on that repo tonight, but basically I just split out all the setter and mutation methods into a separate builder object. So using the API might look like frozenMoment("2014-07-21").thaw().subtract(1, "day").startOf("day").freeze().format("YYYY-MM-DD"). (Although in this particular example it would be more efficient to just start the chain with a builder instead of initializing a builder from a frozenMoment, using frozenMoment.build("2014-07-21").subtract...)

[jehoshua02]

FWIW, when I started using moment I had assumed it followed FP principles and would return the same value every time I call a function:

var now = moment();
var yesterday = now.subtract(1, 'days');
var dayBeforeYesterday = now.subtract(2, 'days');

Of course, I didn't get the results I expected. This caught me off guard as a new user.

Consider this pseudocode, which demonstrates how I would have expected the code to behave:

var now = now;
var yesterday = now - 1day;
var dayBeforeYesterday = now - 2days;

But instead it ended up working like this, which feels strange to me:

var now = now;
var yesterday = now = now - 1day;
var dayBeforeYesterday = now = now - 2days;

For now, even though it's quite tedious, I just carefully .clone() everywhere.

var now = moment();
var yesterday = now.clone().subtract(1, 'days');
var dayBeforeYesterday = now.clone().subtract(2, 'days');

IMO, Javascript is prone to subtle errors and I feel that FP principles help minimize those errors.

I sympathize that this is a hard decision to make. I appreciate your work. Moment.js is amazing.

[AlexGalays]

+1 for 100% immutability.

[fingermark]

It's honestly kind of frustrating having it not be immutable.

[nikoskalogridis]

+1 for 100% immutability in version 3

[ForNeVeR]

There definitely should be an immutable API. As a user of other date libraries (notably, .NET DateTime and Joda Time / Noda Time), I intuitively expect that the add method will not mutate the date object.

[chopachom]

+1 for immutability

[hojberg]

+1 for 100% immutability

[jehoshua02]

If the decision is made for immutability, I'd be willing to give of my time to make it happen. Perhaps pairing over a video call. I'd like to contribute more to open source but need to learn the ropes.

[maggiepint]

Executive summary of RFC with our primary questions to be answered: https://maggiepint.com/2016/07/16/immutability-its-happening/

[jbreckmckye]

@maggiepint I tried to post a comment on that article, but for some reason it was swallowed. Here's what I wrote:

---

My biggest concern with these changes is that they respond to a disproportionately vocal and expressive part of the community - avoiding the quiet bulwark of ordinary developers who aren't even aware this discussion is taking place.

GitHub's threads are not a microcosm of the wider development community. This site suffers the same participation bias as many forums on the web, and is biased towards a certain kind of developer: intellectually engaged with the theory of computing; interested in ideas rather than applications; and dare I even say socially inclined to rallying around popular trends. These developers are naturally attracted to philosophical cause célèbres like immutability and functional programming, and have closer access to you than any other group. They will be the loudest voice in the room, and they will clamour for this change - but what of the wider world?

The wider world wants Stuff That Works. It wants to know that the library it currently uses will continue receiving updates - bugfixes at least, but ideally small incremental improvements that make their lives better. But it does not say so, because it does not actively seek out these forums, and because it takes a thick skin to speak out against the trend.

If you are intent on this change, I think you will need to make a very clear case to these people why this change will make their lives better; why they should upgrade; why they needn't worry their legacy projects won't receive indirect bugfixes; and essentially - what things they will now be able to do that they could not presently.

I also think you should also be very careful to validate the importance of this change in real terms. I wonder if you should release this change as a plugin or wrapper, and monitor its uptake carefully over several months before merging it into trunk. If immutability has been over-represented as a niche concern, you will have found a way to satisfy these vocal users without changing the course of the library as a whole.

[asokani]

As a first time user I ended here after some time lost with startOf endOf. These methods are surprisingly mutating!

+1 for full immutability

[AdamHess]

Maybe another solution is to make it painfully obvious that objects in momentjs are mutable. The documentation does mention it in the cloning section, but it is NOT salient enough.

Another solution,If you want mutability use Object Oriented concepts and create object creation using the NEW keyword vs the moment() factory pattern.

[igl]

The wider world wants Stuff That Works.
...you will need to make a very clear case to these people why this change will make their lives better

Everybody i have witnessed being new to moment falls into the trap of "moments" being mutated.
Even after 3 years of using moment i still have to tell myself "oh shit you are using .startOf here .. better check twice if you need a copy".

The current behaviour is not intuitive and this is a long time overdue.
Try to make array.filter/map mutate and see how much fun that is.

Regarding...
Performance/Memory: I have never chained more than 2 funcs in moment and usually it's .set().get()
Pseudo-Immutability: It will take many many many generations until the java-pass-by-ref-gen is out.

[mull]

I like @AdamHess' idea about choosing whether you're looking for OOP or immutability.

My two cents about why we get confused is this: return values. If moment.add('1', 'days') was returning undefined, like mutable functions in JS generally do, then the confusion would go away. Returning the same object, for me at least, implies I have a new copy. Of course, that would break chainability.

[andyfleming]

I think there is a low likelihood of hitting memory use issues for more developers (except for special use cases). Moment's mutability has already bitten me though. Dates should be treated as values like a string or number.

+1 for immutable by default

[andyfleming]

This same issue exists for PHP, by the way. Do you want to be like PHP?! 😆

In PHP, they solve it by providing DateTimeImmutable (in addition to the normal DateTime).

If we don't change the default behavior to be immutable, we should at least consider a first-class alternate API like imoment/momenti (or whatever). I would literally always use that though (over the mutable API) and I would hope that any other library I'd use would be using the immutable version/API too.

[ngerritsen]

I vote for immutability too, I'm willing to help with implementing if that's the plan. I already ran into mutability issues when doing addition and substraction, what makes it more confusing even, is that these methods do return the instance because of chaining.

[ngerritsen]

By the way how about cloning durations? What's the best way of doing that, couldn't find any that did not feel hacky.

[butterflyhug]

@ngerritsen I believe the best available option is moment.duration(existingDuration).

Re: implementation, #3548 is still an active PR. Hopefully there isn't much code-level work left, but it never hurts to have more eyes to validate big changes. We also need to work on documentation and etc before we can do a major version bump, which will be necessary in order to release a change like this. If you'd like to help with any of this list, I'm sure we'd appreciate it. 😀

[fabiosussetto]

Just spent an hour trying to identify a subtle bug caused by .startOf() mutating the original date... Thanks for the hard work, momentjs did a great job showing how to build an excellent date lib for JS, but I'm switching to date-fns because of the very subtle nature of this kind of bugs and because after my introduction to some general FP concepts (mostly thanks to React, Redux and ELM) I started to appreciate the general benefits of immutability.

For what it's worth, lodash has already followed a more FP approach with lodash/fp. I'd suggest having a look at the way lodash/fp has been implemented because they wrap their existing functions instead of having to complete rewrite everything. Lodash guys are also very very concerned about performance.

I also agree with @mull, the real problem is due to the chaining API for me, which IMO has some big design flaws not just in this case but more generally (e.g. jQuery). I'd be better to me if method mutating dates would return undefined (at least this is the general rule I apply to the code I write)

[noway]

While moment.frozen namespace is in the works, I'd recommend --as previous poster suggested-- to just use date-fns.

[igl]

Just fixed another bug due to mutable moments 🎉

[sandstrom]

Tangentially related, it would be awesome if 3.0 could move to ES6 classes:

let mom1 = new Moment();
let mom2 = Moment.parse('2019-03-01T14:55');
// etc

Such a move could also guide the immutability discussion. I'd say all methods should be immutable with one exception. A method called .set('minute/hour/year/etc', 18).

[alancnet]

I just started using Moment, and was horrified that this library was not entirely immutable from the start. I am using moment-immutable until this gets fixed.

[igl]

@alancnet moment will probably never be changed. It's too much of a change and there is enough push back from users who learned to embrace the current behaviour.

Check out their new project: luxon
It's very nice, modern, immutable(!) and should perform way better than moment-immutable which just wraps everything in .clone() calls.

[stevemao]

For those who'd like to transition from momentjs to a modern approach, please check out https://github.com/you-dont-need/You-Dont-Need-Momentjs

[alamothe]

Very surprised it is mutable!

How far is luxon to being a replacement? One of the reasons I'm using Moment.js is timezone support - it is a must for my project.

[sandstrom]

@alamothe That question is clearly answered on the website: https://moment.github.io/luxon/docs/manual/moment.html

[mattjohnsonpint]

Closing this. As others have pointed out, use Luxon if you want a mostly immutable API. Thanks.