Support a pseudo-immutable API (first-party Frozen Moment)

[butterflyhug]

Rendered

[butterflyhug]

As I discuss in this draft text, there's still a few major unanswered questions in my mind. Mostly, how do we want to package this for distribution? Which API do we want to encourage new users to adopt? Those two questions have a lot of flow-through effects; I've tried to describe the tradeoffs as completely as possible throughout the RFC, but I'm sure I may have missed some things. Overall the text should get a bit more readable once we resolve those questions, because then I can strip out a bunch of detail from the path we decided against.

Feedback is also more than welcome on the rest of the doc, of course. And I'd generally happy to work on implementing this RFC once we agree on the proposal. 😉

[butterflyhug]

Sounds like we probably want to steer new users pretty firmly toward the new Frozen APIs, so I've made a few quick edits accordingly.

The existing Mutable APIs still aren't going anywhere, of course.

[maggiepint]

One thought on this. We could distribute a build that automatically aliases the frozen namespace AS moment. This could actually be the preferred build (the one that we name moment on CDNs, etc). Then we could distribute a moment-legacy-compat build that had the old moment namespace and the frozen namespace. That would certainly be the best way to push new users onto immutable Moment.

[ichernev]

Very well written document, thank you @butterflyhug. Now that I think more about it, having both mutables and frozens in a single project will be very confusing.

So I say lets keep the implementation details, but change the client perspective.

If we introduce a global flag to switch mutable/immutable -- this will make it easy for eager adopters to just flip it, and fix their code bases (this part requires non-trivial effort). For plugin authors and library maintainers who use moment -- they can make their code work as if its immutable (so it works for both), and also easily test against it (by running tests with or without the flag).

We can release that as 3.0, and say plugin authors need to use the registration API, and library authors need to make it work as if its immutable. The good part is 2.x and 3.x will be released together (they'll have the same API), so if a plugin is compatible with 3.x.y it will work with 2.x.y, for people using the mutable moments, because it should be tested with the flag flipped on/off. Then after the dust settles we'll see number of downloads and such and we can choose to release 4.0 that is immutable-only, having most of the plugins/libraries already onboard.

[icambron]

@butterflyhug and I had a long chat yesterday and we agreed on some important changes to the plugin side of things. He is updating the document to reflect those discussions.

[icambron]

@ichernev A few thoughts re:plugins:

In the current plan, plugin authors actually write their plugin to work with mutables because it's easier for the library to convert that to immutable. @butterflyhug is considering flipping that, though. It would certainly make an eventual conversion to immutable-only simpler. We could provide mutable->immutable, immutable->mutable, or both, depending on how much rewrite we want to demand now vs later.

We discussed some of the challenges the registration API creates for plugin authors, especially Twix. Specifically, the plugin may care whether the user is applying the plugin to an immutable or mutable moment, and the registration hides that from the plugin. We came up with a simple scheme that discards the registration API and instead has a wrapping helper function that allows plugin authors to provide mutable implementation based on their immutable implementation or vice versa. That gives plugins more flexibility and about the same level of convenience.

I'll let Lucas write up the details.

[ichernev]

In the current plan, plugin authors actually write their plugin to work with mutables because it's easier for the library to convert that to immutable

I didn't get that. If you write a piece of code that works with moment (library), you can not just treat the moment objects as mutable or immutable. Well, if you treat them as immutable and you're given a mutable it should work, but not vice versa.

So if we ship 3.0 that is immutable by default but has a global flag, it will enable people to test against both immutable and mutable, it will also make releasing things simpler (because of only one "core" code). Also people can switch to 3.0 with mutability and start working towards immutability by flipping the flag and porting parts of the code.

I agree that plugins might prefer to know if the moment is mutable or immutable and act accordingly, and register functions to both interfaces separately. It makes sense to add a method "isFrozen()" in 3.x so a check obj.isFrozen !== null /* 2.x */ && obj.isFrozen() /* 3.x */ is necessary so code works both for 2.x and 3.x

[maggiepint]

Well, if you treat them as immutable and you're given a mutable it should work, but not vice versa.

What if plugin authors were instructed to treat all objects as immutable in v3? Then all users can assume all plugins return a copy if they are using moment 3. Seems like that would cut out a lot of pain, and as long as we assume the immutable API is always available, it would work.

[ichernev]

@maggiepint well, yes. But if the plugin also works for 2.x and has behavior for it, making it work the same for 3.x mutable version should also work. Returning a new one should not fail either, but that's up to the implementation to decide.

Lets say you write a simple method like:

function (m) { return m.add(5, 'seconds'); }

Clearly that would work well for 2.x and 3.x mutable/immutable moments. In 2.x and 3.x mutable it will return the input moment, modified.

[icambron]

Well, if you treat them as immutable and you're given a mutable it should work, but not vice versa.

It's the other way around, right? If you expect to return an immutable, you can just freeze the incoming object, and it won't matter if it was mutable or not, as long as it gets refrozen in the end. But if you are expected to mutate a passed-in mutable instance, you can't just fiddle with the cloned instance; you have to mutate it. So in the current version of the RFC, the library does that for you: if it's immutable, pass in a thawed instance, let the plugin do its work, and then freeze the result. If it's mutable, let the plugin mutate it and do nothing. The opposite works too (i.e. always provide plugins with immutable instances), but if the lib automagically freezes the mutable for you, it has to know to update the original vis-a-vis the returned immutable instance, and that's what's (somewhat) harder. That whole dance is, as I understand it, the reason why there's registration in the first place; it provides a hook to intercept passed and returned Moments and turn them into the right API.

Edit: Oh, I see what you're saying. You're saying have the plugin be written in a way that it doesn't care. I still think that means assuming it's mutable by adding in clone() calls everywhere that no-op for immutable Moments. But in any case, I think it's easier if we enforce (somehow) that the plugin get a certain kind of Moment and dance around it as necessary. That also means no global flag.

And to reiterate, I think there are easier ways to provide this kind of API-conversion than a registration system.

all users can assume all plugins return a copy if they are using moment 3

I think that's the assumption we're trying to avoid. If users are cool with everything returning copies, we should just make Moment immutable across the board and go home.

[maggiepint]

If users are cool with everything returning copies, we should just make Moment immutable across the board and go home.

The more I watch this discussion go around in circles, the more I wonder if this actually isn't a half bad idea.

We're adding a lot of complexity with the dual APIs. Is this really any less complex than maintaining a 2.0 fork for a while would be?

[icambron]

Here's what I think a plugin in v3 could look like. This is along the lines of the current proposal wherein plugins are compatible with old versions of Moment and mutable Moments are the default, but minus registration.

function addTwoDays(){
  return this.add(2, 'days');
}

if (moment.mutable.fn){
  moment.mutable.fn.addTwoDays = addTwoDays;
}
else {
  //2.0, I guess
  moment.fn.addTwoDays = addTwoDays;
}

if (moment.immutable.fn){
  moment.immutable.fn.addTwoDays = moment.wrapToImmutable(addTwodays);
}

//wrapToImmutable, provided by Moment, looks something like this:
function wrapToImmutable(f){
  return function(){
     return moment.immutable(f.call(moment.mutable(this));
  }
};

Alternatives:

1. Wrap the other direction, i.e. encourage plugins to demand and return immutable instances and apply their state to the input Moment. (I prefer this somewhat. More work for plugin maintainers now, less pain if we drop mutability later. But does make it hard to support 2.0 and 3.0 with the same plugin, since the wrapper won't be available in Moment 2.0)
2. Do that magic inside the registration wrappers (Twix would hate this.)
3. Fuck it all, just make Moments immutable all of the time (100% fine by me)

[maggiepint]

Expanding a bit further, when I first started pushing for this plugin to come in a month ago, I had in my head that it would be a plugin - truly separate code that users could elect to bring in. I had in mind that other plugins would be able to elect to support the immutable plugin or not on their own, and would deal with handling how that worked on their own. Basically, I had a much simpler thing in mind than what we have here. A way to make the immutability crowd happy with minimal impact on the library.

This has become a lot more than that, which actually I think is good. At this point we aren't just 'making the immutability people happy'. Instead, we're tangibly improving the library. That's awesome! But if that's the direction were going in, I guess I feel there's good reason to just go all in and make the thing immutable.

I realize there's a manpower issue with doing this from a support perspective. I will point out though that eight months ago this library didn't have me or Lucas helping. Perhaps the circumstances have changed sufficiently to make it an option?

[ichernev]

@icambron what do you mean by "all the magic in the registration wrappers" -- the original proposal by @butterflyhug (have options that tell moment core how to wrap)? I guess even your last example of wrapToImmutable assumes you're returning a mutable moment, so there are going to be a few variants of that too.

I think that's the assumption we're trying to avoid. If users are cool with everything returning copies, we should just make Moment immutable across the board and go home.

We're pretty much doing that, but let people keep compatibility with both (for libraries and plugins), and ease development for us. Only functions that return one thing and mutate a moment passed as argument will be a bit tough to port

[butterflyhug]

RFC updated, with lots of rewriting and simplifying to reflect my conversation with @icambron yesterday. I also moved the plugin authors section below the user-facing APIs section, because I think that flows better given the content changes.

Rendered diff here, in case it helps anyone who already read the earlier draft.

[icambron]

@butterflyhug This looks great. A couple of minor notes:

One. This should be altered to refer to wrappers instead of registration:

Note that this includes the new plugin registration methods described in the previous section.

Two

Do static "configuration" methods get copied into moment.mutable and moment.immutable, or do they only exist on the top-level moment?

If the top-level moment is an alias for one of the two options, won't this just work itself out?

Three

We know we want to strongly encourage users — especially new users — to adopt the Immutable APIs as much and as quickly as possible. But, how quickly and strongly do we want to deprecate and remove the traditional Mutable APIs?

I think this is the central question. Here's how I think about the consequences of the different ways we could push this:

1. Not at all. Support both as first class citizens, make sure everything works for plugins and it's equally easy for users to pick either.
2. Just a bit. Make the immutable API the default binding for moment.
3. Kinda. Includes 2, but also do less work for plugins. They consume either, but if they return one, it's immutable.
4. Strongly. Includes 3, but the mutable one is a separate package.
5. With vigor. Like 4, except no one bothers to write the separate package.

I don't know what the right answer is, but I do think that's the shape of it; e.g. would painful to try 4 without 3. The proposal here looks most like 1, which is good because it let's us see how complicated it is. 3 seems like a big simplification with some non-trivial support costs. 4 would be a great way to smooth an eventual transition to immutable-only.

[maggiepint]

I like mutable as a separate package - I think this will tend to reduce our support burden. People can be assumed to be using an immutable API unless they have explicitly included something else. People forget what is in their configuration code all the time, but "is moment-mutable in your package.json file?" is an easy question with an easy answer.

[icambron]

I suspect the costs of user confusion will be low too: it will be easy enough to decern the cases, and most of the documentation is indifferent to mutability. My worry is mostly about keeping the plugin feature-complete WRT the core. I haven't thought it through and maybe it's easy enough to do automagically.

[maggiepint]

Something that will need to be considered that isn't covered here - how do we want to handle test coverage? We now basically have three of everything - the base function that implements the functionality, the mutable wrapper, and the immutable wrapper. Do we leave all tests as-is and use them to test the base function, and then write yet more tests to test just the mutability or immutability of outputs? Do we simply assert that the code generating the mutable and immutable APIs works?

Also, how do we handle non-factory functions that create moments based on input parameters (like diff for example - any of the functions that take a moment-ish input)? Do any of those return that input as a value that would need to be mutable or immutable based on the user's preference? I don't think so, but I'm not sure. Also, if a date or Moment is passed to any of those, do we need to ensure that the date or moment is not mutated if the user is using the immutable API?

[butterflyhug]

Yeah, I've been deliberately trying to keep the RFC on the "most-compatible" end of our discussion because it's way easier to remove compatibility stuff later than to add it back in.

Tests: oops, forgot to write about that. This is why I ended up proposing that moment is literally the same object as either moment.mutable or moment.immutable (even though I went back and forth about that while drafting, because I don't love that circular reference): we test the two namespaces, and then we just have to test object equality between moment and the appropriate namespace.

Not sure how to test both namespaces efficiently: just testing the base implementations and the wrapping functions seems pretty attractive.

Things like moment.diff weren't really on my radar yet, good questions there.

[ichernev]

Provide immutable moments to ES6/Node users via import moment from 'moment/immutable' or similar.

The problem with this is that all functions that deal with moments will not know what kind of moment is passed through. Also it means it can be mixed in the same project. So when people copy paste code (that is how most code is written anyway) it depends on the source file you copied from.

I dont see how a global setting is confusing, because its either one or the other, the people wining about immutability can use it and the rest not -- even if you accidently end with the wrong version/config, you just change one line, not every import in every file.

[ichernev]

And lodash is utulity library that is used on the spot. I dont think its too popular to pass lodash objects around and think about which one is witch, in the case of fp.

[schmod]

A major version number bump means that we CAN make breaking changes. It says nothing about whether or not that's a good thing for the users of our library.

"Stay on 2.x" is going to give a subset of our users the feeling that they're being left behind. If mutable moments are being left in the library, I do not see any benefit of making the upgrade process more difficult for users who want to continue using them.

[butterflyhug]

Re: "stay on 2.x": if the release is perfectly backward-compatible, then why wouldn't we release this change with a 2.x version number? I think the options are to do a purely-additive change in a 2.x release (aka @icambron's option 1), or to do a 3.x release that changes the behavior of moment. (Or both.)

I increasingly think that options 1 and 5 are the only tenable long-term options. Option 1 is "do this in 2.x". Option 5 is "do a clean break and call it 3.x". Option 4 is basically "indefinitely build 2.x-compatible releases from the 3.x codebase". I think option 4 is superior to options 2 or 3 because IMO it's easier to explain (and build) option 4 as a time-limited transitional strategy -- I do think @schmod is right that the best reason to break compatibility at all is to (eventually?) stop supporting the old API altogether.

All of this said, I think option 5 would either (1) be a much smaller issue than we're worried about, or (2) eventually turn into a de-facto option 4. If we drop the mutable API and that migration is tough for a reasonable number of folks, I expect someone will eventually find that their simplest strategy is to write a "mutable moment" plugin.

[maggiepint]

I personally am at this point 95% for just busting it and making it fully immutable. If we have to support a 2.* fork for a while, so be it.

[maggiepint]

Also worth noting - Moment 2.* is extremely stable. If any users feel that they cannot upgrade to 3.*, the license certainly allows for forking. Feel free.

[butterflyhug]

So if we did decide to just go fully immutable, with no officially-supported mutable API, what would that mean for moment-timezone users? tzdb data keeps changing, and most people just use bundled builds, so that could make it harder for some folks to "just stick with 2.x" (until they can hopefully eventually update their code) if future releases of moment-timezone don't support a mutable version of moment.

[ichernev]

Even if 3.0 is fully immutable I dont see why we dont keep 2.x around, esp if its built from the same code. I can see how this creates a problem for plugin authors that have to maintain 2 versions of the code, or follow best practices and try to have one codebase.

[icambron]

@ichernev For what it's worth, with my plugin author hat on, I wouldn't support Moment 2.x in Twix going forward. It doesn't release often enough for it not to acceptable to just say "use this older version of Twix if you're still on 2.x". Other plugins are even less active. I think the multi-version-compatible plugins might not be a real issue.

[ichernev]

@icambron but this assumes the new version is going to catch on.

[maggiepint]

@ichernev I think that our community trusts us enough to upgrade to a new version. Moment is the longest running, best tested, fullest featured date library for JavaScript. I think over the years we (well, mostly not me, mostly you guys) have delivered great value to consumers. I think that people with a solid understanding of software engineering understand that dates should be immutable, and many people have stuck to our library IN SPITE of it's mutability because of the reliability and track record previously mentioned. If we take the library, and make it immutable, all we have done is improve something with an already solid track record. Why wouldn't people adopt it?
It will catch on.

Worth noting, there is no competing mutable datetime library. Everyone else is immutable. Because dates should be immutable.

[gilmoreorless]

Hi, adding my thoughts as someone who has long been bitten by the immutable-looking-API-that-is-actually-mutable. I’d refrained from adding more noise to moment/moment#1754, but I’m posting here because @maggiepint asked for feedback.

There are only two real options that I see for Moment.

1. Hard break, make 3.0 immutable-only

As a consumer of SemVer-adherent libraries, I know that any bump to the first number means “there are breaking changes, do not just drop in the new version without checking the documentation”.

The question becomes how much of a breaking change do you want it to be? Not all major version bumps are equal, and can range from “we’ve removed some deprecated methods that no-one used anyway” to “we’ve rewritten the entire API and it’s effectively a brand new library”.

I think the best way to guarantee this goes smoothly is by making sure that immutability is the only breaking change (or as close to that aim as possible). The upgrade path for consumers is a lot easier if they know that the only thing to change is mutable -> immutable, and there aren’t any other big API changes mixed in with it. (On the flip side, if some people end up wanting just the other API changes, they can get annoyed with having it bundled in with the immutability change.)

Are there any other breaking changes scheduled for 3.0 that might cause entanglement issues? The docs say that a few things are deprecated and will be removed in the next major version (global export and some older method signatures that have direct replacements). I can’t see the removal of long-deprecated methods getting in the way of upgrade rates, but I’m not familiar enough with the changes to tell.

2. Phased switch-over

This would mean:

• 3.0 is mutable by default, with immutable opt-in
• 4.0 is immutable by default, with opt-out to mutability
• 5.0 is immutable only

(The same approach is detailed in a comment on the original issue)

This guarantees the smoothest possible path for upgrades, but it also increases maintenance/support burden and potentially causes more confusion. There’s also no guarantee of people converting their code during the dual-API phase. The choice generally comes down to pro-actively upgrading to a new API in spare time, or putting it off until the very last minute. In my experience, it almost always gets left until too late (especially when there are commercial and/or time pressures involved). At which point, it's effectively become the hard-break option anyway.

---

My preference is for a hard break straight to immutable-only. Admittedly, I have no idea how that would affect plugins. I also am no longer working at the company I was at when I hit some mutability bugs, so my vested interest in this has diminished a little. I’ve asked some of my former colleagues to take a look at the RFC though, as they’ll have a better perspective on what’s required to upgrade to an immutable moment within a large legacy codebase.

[jblanchette]

Great stuff here @butterflyhug as always.

I'm strongly in favor of a hard break to immutable only 3.0

Adopting the 3.0 code would also mean waiting for the excellent Twix.js library to also be upgraded / compatible - since it's crucial for our build

[icambron]

After thinking through these options, I'm back in the full-break camp too. There's a lot of complexity to supporting both, and any phasing requires a lot of coordination, documentation, and options. I think it's more straightforward and fair to everyone to just break everything once and unambiguously.

@jblanchette I'm pretty confident that under the full break scenario, I'd have Twix 3.0-compatible in a few hours. Twix is already itself immutable and I'll just have to make the iterators not mutate things internally. Might be a bit more complicated under the other options.

[butterflyhug]

Based on the feedback so far, I'll aim to revise the RFC text this weekend so we can see more precisely what a hard break looks like.

[butterflyhug]

Wow, that really is a much simpler story -- not only for our code-level implementation efforts, but also for explaining what's going on to users. I think we'd have to end up with quite a lot of extra ecosystem and support work for anything other than a clean break to make sense here, especially since 2.x has been so rock solid for folks that it'll remain a viable solution for years to come.

I still think that we'll want some sort of easy compatibility story for Moment Timezone to get future tzdb data in 2.x environments. Maybe it's not a big deal to keep Moment Timezone backward-compatible, and/or to do builds with the old 2.x codebase and new data files for a while?

I agree with @icambron that other plugins can probably just make a clean break along with us. Moment Timezone is special here because its most popular builds have frequently-changing data baked into the distribution. (Or at least I assume the data-included builds are most popular -- I've personally never seen anyone bother to use the raw library with a separate/customized data file.)

[schmod]

I think I'm gradually getting on board with the idea of a clean break. This really is a lot simpler.

---

Would there be a use-case .isImmutable() method, so plugin/library authors have a straightforward way to sniff to see if they're working with a 2.x moment or a 3.x moment?

---

Immutability or not, I think there's a reasonable case to be made for decoupling moment-timezone from the accompanying IANA timezone data (as a separate repo/NPM module), but that's probably a separate conversation.

[butterflyhug]

Yeah, it seems useful to have some clean way of distinguishing 2.x from 3.x at runtime. My instinct would be to add a momentVersion property (containing the version number as a string) to the Moment and Duration prototypes.

[icambron]

@butterflyhug already exists:

> moment.version
"2.14.1"

[not-an-aardvark]

@icambron That works when one has access to the moment constructor, but I think the suggestion was to be able to determine the immutability of an individual moment instance without the constructor.

[butterflyhug]

Yep, exactly. We'll never be encouraging people to have more than one copy of Moment in a single project, but I do expect that it'll happen in some larger codebases -- hopefully as a temporary transition thing, but who knows. Also, some plugins might want to do a runtime check to enforce that they never get handed a 2.x Moment or whatever -- I know that might seem like overkill, but some folks like overkill. Anyway, I feel like it's trivial to copy that version string onto moment.fn (and moment.duration.fn) and so we might as well do it to help out those folks.

On the other hand, I could probably be persuaded that prototype-level versioning is a bad idea because it encourages users to do stupid things, and plugins should probably just look at moment.version when they initialize themselves instead of fussing over every single instance.

[butterflyhug]

My current thoughts on the unresolved questions in current RFC:

1. We should probably keep #clone(), and continue to have it return a new instance in 3.x, but mark it as deprecated for future removal in 4.0.
2. We should probably do at least one "preview" (or "beta") release of 3.0 before releasing 3.0 final. The preview release would happen as soon as moment core code is ready, to get the word out about these changes throughout our community ASAP. Moment-timezone and other plugin updates would happen between preview and final. I'm not sure how we'd want to handle the preview release logistically, though -- should we rule out the possibility of further 2.x releases as soon as 3.0 preview is ready, or will the 3.0 preview be a separate branch until we're ready to release 3.0 final, or...?
3. Documentation plan -- I think the big question for now is whether we want a separate docs page for 2.x vs 3.x, or whether we pick a date for a hard cutover to describing the immutable behavior of 3.x. My sense is that we do a hard cutover of everything on momentjs.com when we do a final release of 3.0. Until then we'd just have the Github release and/or blog announcement with a link to this RFC for the gory details.

I don't feel super strongly about any of this, but it's what I'll default to implementing in the near future if other folks don't disagree. 😉

[maggiepint]

@butterflyhug I think we're all on the same page. Sorry that I've been slammed with moving stuff the last month, but there's no reason why we can't proceed with getting some code for an alpha release out there.

[icambron]

I agree we should keep #clone to ease transition, but shouldn't it just return this rather than a new instance?

[butterflyhug]

@icambron -- okay, I'll have #clone return this on the first pass and we'll see if there's any pushback. I was probably too paranoid about people doing stupid things. 😀