From 2841b15e9ae14010c5923f7758021f53ab4263f2 Mon Sep 17 00:00:00 2001 From: Lucas Sanders Date: Thu, 7 Jul 2016 09:50:49 -0400 Subject: [PATCH 1/5] First full draft of immutability RFC (first-party Frozen Moment) --- 0000-immutability.md | 480 +++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 480 insertions(+) create mode 100644 0000-immutability.md diff --git a/0000-immutability.md b/0000-immutability.md new file mode 100644 index 0000000..d7b97d9 --- /dev/null +++ b/0000-immutability.md @@ -0,0 +1,480 @@ +- Start Date: 2016-07-07 +- RFC PR: (leave this empty) +- Moment Issue: [#1754][] + +# Summary + +Add a pseudo-immutable object API to the core Moment library, replicating +the full functionality of our current API. + +# Motivation + +[Many][#1754] new-to-Moment users expect that Moment will return a new +object from various methods that actually change the value of the original +object. This expectation is consistent with the observation that Moment +instances are [Value Objects][ValueObject]. Notably, this expectation +seems to be strengthening over time, correlated with the rise of web +application and UI frameworks that rely on reference equality checks to +performantly determine whether a data structure has changed. + +At the same time, Moment has been used to build millions of existing +applications, all of which rely on the library's current API design. +Many groups with existing codebases, especially those who already use Moment +pervasively, strongly value the API stability that Moment has maintained over +the years to date. In addition, many developers coming to Moment from the +native Javascript Date API tend to expect an API where a single object's +value can change over time. + +There is a plugin, [Frozen Moment][], that has attempted to address these +issues. Unfortunately, as a third-party plugin it has limited visibility and +less certainty about long-term support, so it's not a satisfactory solution +for everyone wanting an "immutable" API. More significantly, there isn't a +great way for a plugin to find out about the other Moment plugins that might +be in use in any particular environment, which makes it difficult to use +plugins while also using the frozen APIs. + +To address these concerns, we could add a plugin registration system to the +core Moment library, coupled with a `moment.frozen` namespace that would +serve as a fully equivalent immutable API alternative to the primary `moment` +namespace and its various properties and constructors. + +# Detailed design + +There are two pieces to this proposal: plugin registration and a "Frozen" +pseudo-immutable API that wraps the core library and its registered plugins. + +For brevity, I'll refer to moment objects that use the current API as +**Mutables** throughout this section, and objects using the proposed +pseudo-immutable API will be called **Frozens**. I'll also refer to the +prototype for Frozens as `moment.frozen.fn`, even though we probably don't +want to publish that prototype for third-party use. + +## Plugin (Method and Constructor) Registration + +A Frozen API can be implemented much more robustly if we ask plugins to +explicitly opt in to compatibility with Frozens. Without this, Frozens can +only support the core library's API methods, or we need to somehow guess at +the behavior of other plugins so we can provide their methods on Frozens. + +### New APIs for Plugin Authors + +There are four types of functions that the Frozen API might need to know about: + +1. factory functions (colloquially, "constructor" — e.g. `moment.tz()`) +2. mutation methods on the `moment.fn` prototype +3. mutation methods on the `moment.duration.fn` prototype +4. methods on the `moment.fn` prototype that return new non-Moment objects, + and those new objects use Moment's APIs (e.g. moment-range, Twix.js) + +In order to achieve this, we might ask (or require) plugin authors to use +Moment-provided features to add new prototype methods and factory functions +(rather than simply assigning functions to the `moment`, `moment.fn`, and +`moment.duration.fn` objects). This could be achieved with three new static +methods: + +#### `moment.addFactory(name, func, options)` + +Example usage: + +```js +moment.addFactory('tz', () => { return moment() }); +moment.addFactory('twix', (m1, m2) => { return Twix(m1, m2) }, { returnsMoment: false }); +``` + +This function assigns `func` to `moment` using the key `name`, so that future +calls to `moment.name()` will invoke `func`. + +- If `options.returnsMoment === false`, we will also assign `func` to `moment.frozen`. +- Otherwise, we'll wrap `func` to construct and return a Frozen based on the + Mutable created by `func`, and we'll assign that wrapped function to `moment.frozen` + for the key `name`. + +#### `moment.addMethod(name, func, options)` + +Example usage: + +```js +moment.addMethod('addSix', function() { return this.add(6) }); +moment.addMethod('getSetMinutes', moment().minutes, { getterSetter: true }); +moment.addMethod('twix', function(end) { return Twix(this, end) }, { returnsMoment: false }); +``` + +This function assigns `func` to `moment.fn` using the key `name`, so that future +calls to `moment().name()` will invoke `func`. + +- If `options.returnsMoment === false`, we will also assign `func` to `moment.frozen.fn`. +- If `options.getterSetter === true`, we'll wrap `func` with a function that + clones `this` before invoking `func` if (and only if) `arguments.length > 0`. + That wrapped function will be assigned to `moment.frozen.fn` for the key `name`. +- Otherwise, we'll wrap `func` with a function that always clones `this` before + invoking `func`, and assign that function to `moment.frozen.fn` for the key `name`. + +Thus, we assume that any new prototype method returns a mutated Mutable, +unless the plugin tells us otherwise by setting an appropriate flag in the +optional `options` object. + +#### `moment.duration.addMethod(name, func, options)` + +```js +moment.duration.addMethod('addSix', function() { return this.add(6) }); +moment.duration.addMethod('getSetMinutes', moment.duration().minutes, { getterSetter: true }); +moment.duration.addMethod('toInteger', function() { return this.asMilliseconds() }, { returnsMoment: false }); +``` + +This is the same as `moment.addMethod()`, except now we're assigning to +`moment.duration.fn` and `moment.frozen.duration.fn`. In loquacious detail: + +This function assigns `func` to `moment.duration.fn` using the key `name`, so that future +calls to `moment.duration().name()` will invoke `func`. + +- If `options.returnsMoment === false`, we will also assign `func` to `moment.frozen.duration.fn`. +- If `options.getterSetter === true`, we'll wrap `func` with a function that + clones `this` before invoking `func` if (and only if) `arguments.length > 0`. + That wrapped function will be assigned to `moment.frozen.duration.fn` for the key `name`. +- Otherwise, we'll wrap `func` with a function that always clones `this` before + invoking `func`, and assign that function to `moment.frozen.duration.fn` for the key `name`. + +Thus, we assume that any new prototype method returns a mutated Mutable, +unless the plugin tells us otherwise by setting an appropriate flag in the +optional `options` object. + +#### Implementation Notes + +Ideally these methods would also be used internally to register Moment's core +factories and methods with the Frozen prototype. + +All plugins will be entirely compatible with the Frozen API proposal if they +correctly register all of their functionality with Moment using these three +methods, provided that all of their new factories and methods return Mutables. +This describes the vast majority of the existing Moment plugin ecosystem. + +Plugins that add factories or methods that return custom objects (*not* Mutables, +e.g. the range plugins) will need to do some additional work to function +correctly when called from Frozens. [1](#fn1) + +We would need to add a new section for "Plugin Authors" to the documentation to +describe these methods and their intended usage. + +If we decide want to actively push new users toward using Frozens instead of +Mutables, then I think we should probably try to simplify the user-facing +plugin compatibility story as much as possible by requiring plugins to use +these APIs if they want to work with Moment 3.0+. This could be easily +achieved for methods if we stop publicly exposing the Moment and Duration +prototypes as `moment.fn` and `moment.duration.fn`, respectively. [2](#fn2) +It's a bit harder to enforce for factories, so it still might not be worth +enforcing that case, but one strategy could be to construct the `moment` +object with a prototype, and then apply `Object.freeze()` to the moment +object itself. Then our implementation of `addFactory()` could assign +functions to the `moment` object's prototype to make them publicly available. + +Depending on how hard we push this API on plugin authors, many plugins might +not proactively adopt this registration API even though it should be very easy +for most plugins to adopt. We might want to send PRs to some high-profile +plugins to make this transition easy for their authors. + +### Plugin vs Core Feature + +I think it might be somewhat confusing for users and plugin authors if we +implement these plugin registration functions as a feature that appears in +some environments but not others — which points toward including this piece +as a feature of the core library rather than building it in a plugin. + +If plugin registration is part of a first-party Frozen Moment plugin and not +part of the core library, then the `addMethod` and `addFactory` APIs would +only be available in the `moment.frozen` namespace (and not the top-level +`moment` namespace). Registered methods would simply be added to the Frozen +API — plugins would continue to extend `moment` and `moment.fn` directly +for the Mutable API. + +This system would certainly ensure backward compatibility with existing plugins +— although we don't **have** to break the way existing plugins currently work, +even if we do build plugin registration APIs into the core library and +encourage plugin authors to use these new APIs. On the other hand, I suspect +that having entirely different systems for plugins to work with Mutable vs +Frozen APIs would ultimately reduce the number of community plugins that choose +to support the Frozen API, assuming that both approaches would be accompanied +by similar levels of support and outreach effort from the Moment maintainers. + +If this registration system is part of an optional plugin, it's essential for +users to load Frozen Moment before any other plugins that they want to use with +the Frozen API, since the plugin registration system won't exist at all until +the Frozen plugin has been loaded. [3](#fn3) If that +optional plugin is a separate file, then some number of users will get the +bundling order wrong and file support requests. On the other hand, if we +distribute a bundled build where the core library and the Frozen plugin appear +in the same file, then we need to decide which is the default build for the big +download button at momentjs.com, and we also need to figure out how to +communicate the distinctions between different builds now that we'd (presumably) +have twice as many different pre-built packages available. + +I'm tempted to say we should sidestep all this build + communication complexity +by building everything from this RFC into the core library, but I'm not 100% +confident that's the correct tradeoff. We probably need some community feedback +(particularly from plugin authors) before locking down the plugin vs core decision. + +## Frozen API + +### New APIs for Moment Users + +This proposal adds `moment.frozen` as a new API namespace. + +Users who want to work exclusively with an immutable instance API (the +"Frozen API") would be able to ignore all Moment features that are not part +of the `moment.frozen` namespace. If they're not using third-party code, +such users might want to simply overwrite the `moment` global in a traditional +web environment (`moment = moment.frozen`), or import just the frozen APIs +while discarding the traditional mutable APIs when working in a CommonJS +environment (`var moment = require('moment').frozen`), etc. + +Users who want to work exclusively with the current mutable instance API +(the "Mutable API") would be able to ignore the new `moment.frozen` +namespace without consequence. + +Users would also be able to mix and match APIs within the same application, +allowing existing applications to incrementally migrate from one API to the +other by using both APIs as appropriate, and then parsing Mutables from +Frozens (or vice-versa) at the boundaries to the modified code, as +appropriate. + +So, from a user's perspective: + +- All `moment.*` static configuration methods will be shamelessly copied + as-is into the `moment.frozen.*` namespace, using the same method names + in both namespaces. Note that this includes the new plugin registration + methods described in the previous section. + +- All factory methods in the `moment` namespace will be wrapped for use + (under the same method names) in the `moment.frozen` namespace. These + wrapped factories will return instances that use the Frozen API. For + example, the wrapped equivalent of `moment.parseZone()` would be + `moment.frozen.parseZone()`. + + When we say "all factory methods", this includes factory methods from + the core library as well as factory methods from plugins that have been + registered using the `moment.addFactory()` API described earlier. + Note that `moment.frozen()` must itself be a wrapped version of the + core library's `moment()` factory method, in additon to wrapping + factories that are static methods of `moment` (e.g. `moment.utc`, + `moment.tz`, etc). + +- Frozens created from factory methods in the `moment.frozen` namespace + will be exactly the same as Mutables created from the existing factories, + except that they will be backed by the `moment.frozen.fn` prototype + instead of the `moment.fn` prototype (or `moment.frozen.duration.fn` + instead of `moment.duration.fn`, for Durations). + +- Instance methods in the Frozen API (from `moment.frozen.fn` and + `moment.frozen.duration.fn`) are wrapped versions of the corresponding + methods from the Mutable API. The wrappers simply create a copy of the + object (when appropriate) before delegating to the Mutable API + implementation of that function name. + + All core methods will be available on Frozens, as well as all + plugin-created methods that were registered using the `addMethod` APIs + described above. + +### Implementation Plan + +A proposal for bootstrapping the `moment.frozen` namespace as described above: + +1. First, create `moment.frozen` as a wrapped version of the `moment` + factory function. We can't use `moment.addFactory()` for this, so it + will be hardcoded. If everything is in core, we create `moment.frozen` + immediately after creating `moment`, and before registering all the + core methods onto the Mutable prototype. If this RFC is a separate + plugin, then this is the first thing that happens in the plugin's + implementation. +2. Ditto for Durations: we need hardcoded logic to create + `moment.frozen.duration` as a wrapped version of the + `moment.duration` factory, and we need that code to run as early + as possible. +3. Create `addFactory` as a method of `moment` (if this is part of core) + and `moment.frozen`. Ditto for `moment.addMethod` and + `moment.duration.addMethod`. +4. Use `moment.frozen.addFactory()` to register all factory functions + and static config methods from the core library. (Static config + methods would be registered with `returnsMoment: false`.) + `moment.frozen.addFactory` is responsible for wrapping and copying + those methods into the `moment.frozen` namespace. +5. Use `moment.frozen.addMethod()` and `moment.frozen.duration.addMethod()` + to register all Mutable instance methods from the core library. + The `addMethod` methods are responsible for wrapping and copying + those methods into the appropriate Frozen namespace, same as described + below for plugin methods. + +The Frozen API would be implemented with a separate prototype +(`moment.frozen.fn` vs `moment.fn`) for Frozens than for Mutables. +Similarly, there would be a separate prototype for Frozen Durations +(`moment.frozen.duration.fn`) than for Mutable Durations +(`moment.duration.fn`). + +Each time a plugin calls `moment.addMethod`, the provided function +will be wrapped (if appropriate), and then the wrapped copy will be +assigned for the specified function name on `moment.frozen.fn`. +[4](#fn4) + +Ditto for `moment.duration.addMethod`: +Each time a plugin calls `moment.duration.addMethod`, the provided function +will be wrapped (if appropriate), and then the wrapped copy will be +assigned for the specified function name on `moment.frozen.duration.fn`. + +### Plugin vs Core Feature + +This could be implemented directly in core, or with a first-party plugin +(which could be bundled into certain distribution builds for convenience). + +The Frozen API implementation will need to hook into the behavior of the +`addFactory` and `addMethod` APIs. This is trivial if everything is in core +(or if everything is in a plugin). If the plugin registration system is in core +but the Frozen API is not, then we'll want some mechanism for the plugin to +register a callback so it can do the right thing when `addFactory` and `addMethod` +get called. We'd also need to hardcode wrappers for core methods, and/or build a +way for the plugin to get a list of previously-registered methods at the same +time as the plugin sets up these callbacks. + +That said, it might make sense to set a maximum file size increase that we're +willing for these features to impose on the core library, and use that cutoff to +determine which path is best (after writing a rough initial implementation in core). + +# How We Teach This + +We probably need a dedicated page at momentjs.com introducing users to the +semantic distinctions between the Mutable and Frozen APIs. + +We also need to describe the `addFactory` and `addMethod` functions for the +API reference documentation at momentjs.org/docs. + +It would feel pretty heavy to just append (largely duplicative) documentation +for all the `moment.frozen.*` APIs to the existing documentation page. +If these APIs are part of a plugin then maybe we need a dedicated subsite +for the plugin (similar to Moment Timezone) with its own copy of the docs +(although hopefully the docs could be automatically generated for both sites +from the same source material, since most of the info would be the same). +If it's all part of core, and/or if we're distributing bundled builds with +core + plugin in a single file, then maybe there could be a toggle switch +to convert between Mutable and Frozen API docs (similar to how some +Coffeescript libraries let you toggle between .js and .coffee syntax in +their documentation's code examples). + +Finally, we'll need some targeted documentation explaining how and +(especially) why plugin authors should register their methods for use with +the Frozen API. We might consider submitting PRs to a few popular plugins +to help them adopt the new registration APIs, and then link to those PRs +from the documentation as examples of how other plugin authors can update +their implementations. + +# Drawbacks + +1. Doubling the number of exposed API methods (for "frozen" and traditional + versions of everything) will inevitably increase our documentation and + support burdens, even if almost all of the code is the same. It's also + likely to increase the testing burden for new feature additions from + maintainers and external contributors alike, although hopefully the + impact on *implementing* new features will be negligible. +2. By giving developers a choice of frozen/mutating APIs, new users of + Moment will have more friction getting started with the library because + they'll need to choose an API before they can do anything useful. +3. This feature has often been described as an "immutable" API, but we're + not actually making the objects immutable — developers can assign and + modify custom properties on these objects as much as they want. Indeed, + "frozen" instances would still be mutating under the hood in certain + situations (because some internal properties are lazily computed). This + may be confusing for some developers and we will likely get questions + about it. +4. We expect these changes will increase the library's file size somewhat, + even for users who don't use any of the new APIs, simply because we're + adding code to the library. +5. Because plugin methods and factories are created with string names, + minifiers won't be able to rename those methods, possibly leading to an + additional slight increase in file sizes for folks who minify their + dependencies together with their custom code. +6. The new plugin registation system will slightly increase the time + required for Moment and its plugins to initialize themselves for use. + I don't expect this to be a noticeable issue, though. + +# Alternatives + +As hinted in the Motivation section, if we don't do this (or something +similar), then we're effectively asking the folks who keep +1'ing [#1754][] +to build their own community around the existing third-party plugin, or to +find or build another library instead of using Moment. This would not be the +end of the world, but it would be a little frustrating since we agree that +date-time objects should ideally be an immutable type. + +To date, the current third-party Frozen Moment plugin has had much less reach +than we might expect for a first-party solution. We might be able to close +that gap somewhat by promoting the third-party plugin more heavily on +momentjs.com, since it doesn't appear to be mentioned anywhere in the official +documentation right now. This sort of cross-promotion scheme could achieve +some of the same usage benefits while delegating much of the communication +and maintenance burden for this functionality outside the core Moment team — +at the cost of continuing with some additional perceived and actual risk +(relative to a first-party solution) as to whether that plugin would continue +to be maintained appropriately over the long term. + +There is no technical reason why the current third-party plugin could not grow +to do everything that a first-party plugin would do under this RFC, including +plugin registration and distributing bundled builds. In fact, I will likely +work to make that happen if this RFC is not adopted. But any third-party solution +will be handicapped by the fact that users don't expect plugins to be able to +override core API decisions like this, so they're less likely to find and adopt +a third-party plugin for this purpose (even if that plugin was well-publicized, +etc). + +I also hope that it will become easier to get other plugin authors to write +compatible plugins (by using the registration system, etc) if Frozen Moment is +not itself "just" another third-party plugin. + +# Unresolved questions + +1. How much of this functionality should be implemented in the core library + vs a first-party plugin? Will we want to offer additional pre-compiled + builds for users to download? If so, which is the default "recommended" + build publicized most heavily at momentjs.com, and how do we describe the + differences for our alternative builds? +2. Will we ever want to proactively encourage new users to use the frozen + APIs instead of the traditional APIs? If so, when and why? If not, how + do we make sure that new users approaching our documentation will + understand their options without first writing a bunch of code against the + wrong API and getting stuck and coming to us for help? +3. Do we want to avoid publishing our prototypes in 3.0 and force plugin + authors to adopt the new registration API, or should we just strongly + encourage plugins to update without breaking current plugins? +4. The documentation strategy needs to be fleshed out further, as we get a + better sense of our answers to the other unresolved questions. + +# Footnotes + +1. Thankfully I don't think moment-range uses mutator methods + much, if at all, so it should be easy to update. It looks like most of + Twix's functionality should also be trivial to update, although its range + iterators would need to be reimplemented. [↩](#a1) + +2. Technically, third-party code could still sidestep our + registration API by messing with `Object.getPrototypeOf(moment())`. + At that point, though, it's easier to just use our APIs instead, so I don't + think any plugin authors would actually do that. It's not like we could + possibly stop someone who is **that** motivated to muck with our internals, + anyway. [↩](#a2) + +3. If the registration system is in core but the + APIs are not, then the registration system could keep track of metadata about + all the plugin methods that have already been registered. Then any plugins + that want to hook into that system could also retrieve the metadata for all + the methods that were registered before their plugin was loaded. [↩](#a3) + +4. Thus, unlike the existing third-party Frozen Moment + plugin, `moment.frozen.fn` will **not** have `moment.fn` as its prototype. + This means that all Frozen API methods must explicitly appear on the Frozen + prototype, instead of simply falling through to the Mutable prototype for + methods that are not expected to mutate the object. This change is made to + ensure correctness; this way, unregistered plugin methods will not appear on + Frozens so that they cannot accidentally mutate the value of a Frozen. + (The same is true for `moment.frozen.duration.fn`: it will not have + `moment.duration.fn` as its prototype for the same reason.) [↩](#a4) + + +[#1754]: https://github.com/moment/moment/issues/1754 +[Frozen Moment]: https://github.com/WhoopInc/frozen-moment/ +[maggiepint post]: https://maggiepint.com/2016/06/24/why-moment-js-isnt-immutable-yet/ +[ValueObject]: http://martinfowler.com/bliki/ValueObject.html From 0bd6b34e341acb017a7a1720c7a1c5b1ad51c47d Mon Sep 17 00:00:00 2001 From: Lucas Sanders Date: Sat, 16 Jul 2016 10:02:04 -0400 Subject: [PATCH 2/5] Immutability RFC edit: new users should generally use Frozen API --- 0000-immutability.md | 61 ++++++++++++++++++++++++-------------------- 1 file changed, 34 insertions(+), 27 deletions(-) diff --git a/0000-immutability.md b/0000-immutability.md index d7b97d9..3c2f378 100644 --- a/0000-immutability.md +++ b/0000-immutability.md @@ -1,5 +1,5 @@ - Start Date: 2016-07-07 -- RFC PR: (leave this empty) +- RFC PR: [#2][pull request] - Moment Issue: [#1754][] # Summary @@ -208,9 +208,10 @@ communicate the distinctions between different builds now that we'd (presumably) have twice as many different pre-built packages available. I'm tempted to say we should sidestep all this build + communication complexity -by building everything from this RFC into the core library, but I'm not 100% -confident that's the correct tradeoff. We probably need some community feedback -(particularly from plugin authors) before locking down the plugin vs core decision. +by building everything from this RFC into the core library, especially since we +probably want to steer new users toward the new APIs. That said, we probably +need some community feedback (particularly from plugin authors) before locking +down the plugin vs core decision. ## Frozen API @@ -232,9 +233,9 @@ namespace without consequence. Users would also be able to mix and match APIs within the same application, allowing existing applications to incrementally migrate from one API to the -other by using both APIs as appropriate, and then parsing Mutables from -Frozens (or vice-versa) at the boundaries to the modified code, as -appropriate. +other by converting some of their code to use Frozens rather than Mutables, +and then parsing Mutables from Frozens (or vice-versa) at the boundaries to +the modified code as necessary. So, from a user's perspective: @@ -338,20 +339,24 @@ determine which path is best (after writing a rough initial implementation in co # How We Teach This -We probably need a dedicated page at momentjs.com introducing users to the -semantic distinctions between the Mutable and Frozen APIs. +Because our own experience and a preponderance of Moment-related support +requests suggest that new users would have fewer problems working with the +Frozen API, we would generally want to push new users to use `moment.frozen` +exclusively and ignore the traditional Mutable API. + +We probably need some dedicated documentation at momentjs.com introducing +existing users to the semantic distinctions between the Mutable and Frozen +APIs, reassuring them that the Mutable APIs aren't disappearing but also +encouraging them to consider adoption of the Frozen APIs if it makes sense +in their situation. We also need to describe the `addFactory` and `addMethod` functions for the API reference documentation at momentjs.org/docs. It would feel pretty heavy to just append (largely duplicative) documentation for all the `moment.frozen.*` APIs to the existing documentation page. -If these APIs are part of a plugin then maybe we need a dedicated subsite -for the plugin (similar to Moment Timezone) with its own copy of the docs -(although hopefully the docs could be automatically generated for both sites -from the same source material, since most of the info would be the same). -If it's all part of core, and/or if we're distributing bundled builds with -core + plugin in a single file, then maybe there could be a toggle switch +Since we generally want to steer new users toward the Frozen APIs, then maybe +we could show just the Frozen APIs by default, with a toggle switch to convert between Mutable and Frozen API docs (similar to how some Coffeescript libraries let you toggle between .js and .coffee syntax in their documentation's code examples). @@ -388,7 +393,7 @@ their implementations. minifiers won't be able to rename those methods, possibly leading to an additional slight increase in file sizes for folks who minify their dependencies together with their custom code. -6. The new plugin registation system will slightly increase the time +6. The new plugin registation system may slightly increase the time required for Moment and its plugins to initialize themselves for use. I don't expect this to be a noticeable issue, though. @@ -428,20 +433,21 @@ not itself "just" another third-party plugin. # Unresolved questions 1. How much of this functionality should be implemented in the core library - vs a first-party plugin? Will we want to offer additional pre-compiled - builds for users to download? If so, which is the default "recommended" - build publicized most heavily at momentjs.com, and how do we describe the - differences for our alternative builds? -2. Will we ever want to proactively encourage new users to use the frozen - APIs instead of the traditional APIs? If so, when and why? If not, how - do we make sure that new users approaching our documentation will - understand their options without first writing a bunch of code against the - wrong API and getting stuck and coming to us for help? + vs a first-party plugin? If it's a plugin, then we should probably still + consider distributing builds that have both Frozen Moment and Moment.js + in a single file to make it easy for new users to adopt the Frozen APIs. + That way the big download button at momentjs.com can get new users set up + with our recommended environment. But then, how would we label the link + to download a build of just the core library, without this RFC's plugin? +2. How quickly and strongly do we want to proactively encourage new users to + use the frozen APIs instead of the traditional APIs? Ditto for existing + users? 3. Do we want to avoid publishing our prototypes in 3.0 and force plugin authors to adopt the new registration API, or should we just strongly encourage plugins to update without breaking current plugins? -4. The documentation strategy needs to be fleshed out further, as we get a - better sense of our answers to the other unresolved questions. +4. The documentation and publicity strategy for this release needs to be + fleshed out further, but that's partly dependent on our answers to the + other unresolved questions. # Footnotes @@ -477,4 +483,5 @@ not itself "just" another third-party plugin. [#1754]: https://github.com/moment/moment/issues/1754 [Frozen Moment]: https://github.com/WhoopInc/frozen-moment/ [maggiepint post]: https://maggiepint.com/2016/06/24/why-moment-js-isnt-immutable-yet/ +[pull request]: https://github.com/moment/moment-rfcs/pull/2 [ValueObject]: http://martinfowler.com/bliki/ValueObject.html From 92732da097049b339cf5fbaf09eab69bc628539e Mon Sep 17 00:00:00 2001 From: Lucas Sanders Date: Sat, 16 Jul 2016 10:21:47 -0400 Subject: [PATCH 3/5] Immutability RFC: Conversions between Mutable and Frozen --- 0000-immutability.md | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/0000-immutability.md b/0000-immutability.md index 3c2f378..39c2233 100644 --- a/0000-immutability.md +++ b/0000-immutability.md @@ -264,6 +264,14 @@ So, from a user's perspective: instead of the `moment.fn` prototype (or `moment.frozen.duration.fn` instead of `moment.duration.fn`, for Durations). +- The `moment()` factory method (and likely all other `moment.*` factories) + will be able to accept a Frozen as input to construct a Mutable with the + same data as the input Frozen. Likewise, the `moment.frozen()` factory + (and likely all other `moment.frozen.*` factories) will be able to accept + a Mutable in order to construct a Frozen with the same data. The `freeze()` + and `thaw()` instance methods from the existing Frozen Moment plugin will + **not** be implemented, because they are not needed. + - Instance methods in the Frozen API (from `moment.frozen.fn` and `moment.frozen.duration.fn`) are wrapped versions of the corresponding methods from the Mutable API. The wrappers simply create a copy of the From c92b2624dc83c6b1698afad2891b21592952e711 Mon Sep 17 00:00:00 2001 From: Lucas Sanders Date: Sun, 17 Jul 2016 23:46:29 -0400 Subject: [PATCH 4/5] Rework plugin registration and API namespaces for immutability RFC Also focus on putting all the new functionality in core, instead of confusing the issue with talk of first-party plugins. That seems to make a simpler, more compelling story all the way around. --- 0000-immutability.md | 654 +++++++++++++++++++------------------------ 1 file changed, 292 insertions(+), 362 deletions(-) diff --git a/0000-immutability.md b/0000-immutability.md index 39c2233..d61ac8a 100644 --- a/0000-immutability.md +++ b/0000-immutability.md @@ -2,11 +2,13 @@ - RFC PR: [#2][pull request] - Moment Issue: [#1754][] + # Summary Add a pseudo-immutable object API to the core Moment library, replicating the full functionality of our current API. + # Motivation [Many][#1754] new-to-Moment users expect that Moment will return a new @@ -31,461 +33,389 @@ less certainty about long-term support, so it's not a satisfactory solution for everyone wanting an "immutable" API. More significantly, there isn't a great way for a plugin to find out about the other Moment plugins that might be in use in any particular environment, which makes it difficult to use -plugins while also using the frozen APIs. +plugins while also using the immutable APIs. To address these concerns, we could add a plugin registration system to the -core Moment library, coupled with a `moment.frozen` namespace that would +core Moment library, coupled with a `moment.immutable` namespace that would serve as a fully equivalent immutable API alternative to the primary `moment` namespace and its various properties and constructors. + # Detailed design -There are two pieces to this proposal: plugin registration and a "Frozen" -pseudo-immutable API that wraps the core library and its registered plugins. +There are two pieces to this proposal: + +1. a user-facing "Immutable" API (actually pseudo-immutable) that wraps the + core library and its registered plugins; +2. mechanisms for plugin authors to easily extend Moment, with feature parity + for both the "Mutable" and "Immutable" APIs. For brevity, I'll refer to moment objects that use the current API as **Mutables** throughout this section, and objects using the proposed -pseudo-immutable API will be called **Frozens**. I'll also refer to the -prototype for Frozens as `moment.frozen.fn`, even though we probably don't -want to publish that prototype for third-party use. +pseudo-immutable API will be called **Immutables**. `moment.immutable.fn` +is the prototype for Immutables. + + +## New APIs for Moment Users + +This RFC adds two new API namespaces: `moment.mutable` and `moment.immutable`. + +The top-level `moment` namespace might be a reference to `moment.mutable` or +`moment.immutable` for convenience, and this might differ between build +configurations. Thus, we might offer both a Compatibility build where `moment` +APIs are equivalent to `moment.mutable`, and a Recommended build where `moment` +APIs are equivalent to `moment.immutable`. + +Users who want to work exclusively with an immutable instance API (the +"Immutable API") would be able to work solely with the `moment.immutable` +namespace, and those who don't want to change their code can simply use the +`moment.mutable` namespace. Third-party code (e.g. UI components) will always +be able to rely on having their desired API available. + +If they're not using third-party code, users might want to simply overwrite the +`moment` global in a traditional web environment (`moment = moment.immutable`) +to reflect the API that they are using. Similarly, I expect it'd be common to +import just one API while discarding the other API when working in a CommonJS +environment (`var moment = require('moment').immutable`), etc. + +That said, users would also be able to mix and match APIs within the same +application, allowing existing applications to incrementally transition between +APIs. We will strongly discourage users from mixing and matching from both +APIs within a single codebase as a long-term design choice. + + +So, from a user's perspective: + +- All `moment.*` static configuration methods will be shamelessly copied + as-is into the `moment.mutable` and `moment.immutable.*` namespaces, using + the same method names in all three namespaces. Note that this includes the + new plugin registration methods described in the previous section. -## Plugin (Method and Constructor) Registration + (Or maybe our static configuration methods should only live in the top-level + `moment` namespace? But then I'd kind of want to force everyone to always + explicitly construct instances from the `mutable` or `immutable` namespaces, + with no top-level convenience wrappers there...) -A Frozen API can be implemented much more robustly if we ask plugins to -explicitly opt in to compatibility with Frozens. Without this, Frozens can -only support the core library's API methods, or we need to somehow guess at -the behavior of other plugins so we can provide their methods on Frozens. +- All factory methods in the `moment` namespace will be wrapped for use + (under the same method names) in the `moment.immutable` namespace. These + wrapped factories will return instances that use the Immutable API. For + example, the wrapped equivalent of `moment.mutable.parseZone()` would be + `moment.immutable.parseZone()`. -### New APIs for Plugin Authors + When we say "all factory methods", this includes factory methods from + the core library as well as factory methods from plugins that have been + updated to support the new API. Note that `moment.immutable()` must itself + be a wrapped version of the core library's `moment.mutable()` factory method, + in additon to wrapping factories that are static methods of `moment.mutable` + (e.g. `moment.mutable.utc`, `moment.mutable.tz`, etc). -There are four types of functions that the Frozen API might need to know about: +- Immutables created from factory methods in the `moment.immutable` namespace + will be exactly the same as Mutables created from the existing factories, + except that they will be backed by the `moment.immutable.fn` prototype + instead of the `moment.mutable.fn` prototype (or + `moment.immutable.duration.fn` instead of `moment.mutable.duration.fn` + for Durations). + +- The `moment.mutable()` factory method (and likely all other `moment.mutable.*` + factories) will be able to accept a Immutable as input to construct a Mutable + with the same data as the input Immutable. Likewise, the `moment.immutable()` + factory (and likely all other `moment.immutable.*` factories) will be able to + accept a Mutable in order to construct a Immutable with the same data. + + The `freeze()` and `thaw()` instance methods from the existing Frozen Moment + plugin will **not** be implemented, because they are not needed. + +- Instance methods in the Immutable API (from `moment.immutable.fn` and + `moment.immutable.duration.fn`) are wrapped versions of the corresponding + methods from the Mutable API. The wrappers simply create a copy of the + object (when appropriate) before delegating to the Mutable API + implementation of that function name. -1. factory functions (colloquially, "constructor" — e.g. `moment.tz()`) + All core methods will be available on Immutables, as well as all + plugin-created methods that were registered using the `addMethod` APIs + described above. + +- The return value of `moment.isMoment()` could be changed to a tri-state enum: + `"mutable"` if the input's prototype is `moment.mutable.fn`; `"immutable"` if + the input's prototype is `moment.immutable.fn`; otherwise `false`. + + +## API Changes for Plugin Authors + +A Immutable API can be implemented much more robustly if we ask plugins to +explicitly opt in to compatibility with Immutables. Without this, Immutables +can only support the core library's API methods, or we need to somehow guess at +the behavior of other plugins so we can provide their methods on Immutables. + +There are six types of functions that plugins could provide, where the plugin +implementations could be expected to differ between the Mutable and Immutable +APIs: + +1. moment factory functions (colloquially, "constructor" — e.g. `moment.tz()`) 2. mutation methods on the `moment.fn` prototype -3. mutation methods on the `moment.duration.fn` prototype -4. methods on the `moment.fn` prototype that return new non-Moment objects, +3. duration factory functions (e.g. `moment.duration()`) +4. mutation methods on the `moment.duration.fn` prototype +5. methods on the `moment.fn` prototype that return new non-Moment objects, and those new objects use Moment's APIs (e.g. moment-range, Twix.js) +6. wrapped copies of the `moment()` factory function (e.g. moment-jalaali, + moment-hijri) + +For these use cases #1 through #4, we'll provide utility methods on `moment` +that generate an Immutable method implementation from a Mutable plugin function. +These same wrapper-generation functions might also be of some limited help for +certain plugins in category #5 (e.g. Twix). At the end of the day, plugins will +be required to explicitly assign their methods into the `moment.mutable` and +`moment.immutable` namespaces as appropriate; the wrapper functions below are +simply provided as a convenience to make it easier for plugin authors to support +both APIs simultaneously. + +Otherwise, we can't do too much to help plugins that do #5 and #6 (aside from +maybe creating new extensibility APIs so that the calendar plugins can stop +wrapping the magic `moment()` factory, but that's way out of scope for this +RFC). For those use cases, we should probably just aim to get out of the way +and let the plugins sort themselves out. + +### `moment.wrapMomentFactory(func, destinationAPI)` + +Example usage: -In order to achieve this, we might ask (or require) plugin authors to use -Moment-provided features to add new prototype methods and factory functions -(rather than simply assigning functions to the `moment`, `moment.fn`, and -`moment.duration.fn` objects). This could be achieved with three new static -methods: +```js +moment.immutable = moment.wrapMomentFactory(moment.mutable, 'immutable'); +moment.mutable.tz = moment.wrapMomentFactory(moment.immutable.tz, 'mutable'); +``` + +Behavior: -#### `moment.addFactory(name, func, options)` +```js +function wrapMomentFactory(func, api) { + return function() { + return moment[api](func.apply(null, arguments)); + }; +} +``` + +### `moment.wrapDurationFactory(func, destinationAPI)` Example usage: ```js -moment.addFactory('tz', () => { return moment() }); -moment.addFactory('twix', (m1, m2) => { return Twix(m1, m2) }, { returnsMoment: false }); +moment.immutable.duration = moment.wrapDurationFactory(moment.mutable.duration, 'immutable'); ``` -This function assigns `func` to `moment` using the key `name`, so that future -calls to `moment.name()` will invoke `func`. +Behavior: -- If `options.returnsMoment === false`, we will also assign `func` to `moment.frozen`. -- Otherwise, we'll wrap `func` to construct and return a Frozen based on the - Mutable created by `func`, and we'll assign that wrapped function to `moment.frozen` - for the key `name`. +```js +function wrapMomentFactory(func, api) { + return function() { + return moment[api].duration(func.apply(null, arguments)); + }; +} +``` -#### `moment.addMethod(name, func, options)` +### `moment.wrapMomentMethod(func, destinationAPI)` Example usage: ```js -moment.addMethod('addSix', function() { return this.add(6) }); -moment.addMethod('getSetMinutes', moment().minutes, { getterSetter: true }); -moment.addMethod('twix', function(end) { return Twix(this, end) }, { returnsMoment: false }); +moment.immutable.add = moment.wrapMomentMethod(moment.mutable.add, 'immutable'); ``` -This function assigns `func` to `moment.fn` using the key `name`, so that future -calls to `moment().name()` will invoke `func`. - -- If `options.returnsMoment === false`, we will also assign `func` to `moment.frozen.fn`. -- If `options.getterSetter === true`, we'll wrap `func` with a function that - clones `this` before invoking `func` if (and only if) `arguments.length > 0`. - That wrapped function will be assigned to `moment.frozen.fn` for the key `name`. -- Otherwise, we'll wrap `func` with a function that always clones `this` before - invoking `func`, and assign that function to `moment.frozen.fn` for the key `name`. +Behavior when `destinationApi` is `"immutable"`: -Thus, we assume that any new prototype method returns a mutated Mutable, -unless the plugin tells us otherwise by setting an appropriate flag in the -optional `options` object. +```js +function wrapMomentMethod(func, 'immutable') { + return function() { + return moment.immutable(func.apply(moment.mutable(this), arguments)); + }; +} +``` -#### `moment.duration.addMethod(name, func, options)` +Behavior when `destinationApi` is `"mutable"`: ```js -moment.duration.addMethod('addSix', function() { return this.add(6) }); -moment.duration.addMethod('getSetMinutes', moment.duration().minutes, { getterSetter: true }); -moment.duration.addMethod('toInteger', function() { return this.asMilliseconds() }, { returnsMoment: false }); +import { copyConfig } from './constructor'; + +function wrapMomentMethod(func, 'mutable') { + return function() { + var newMoment = func.apply(moment.immutable(this), arguments); + copyConfig(this, newMoment); + return this; + }; +} ``` -This is the same as `moment.addMethod()`, except now we're assigning to -`moment.duration.fn` and `moment.frozen.duration.fn`. In loquacious detail: - -This function assigns `func` to `moment.duration.fn` using the key `name`, so that future -calls to `moment.duration().name()` will invoke `func`. - -- If `options.returnsMoment === false`, we will also assign `func` to `moment.frozen.duration.fn`. -- If `options.getterSetter === true`, we'll wrap `func` with a function that - clones `this` before invoking `func` if (and only if) `arguments.length > 0`. - That wrapped function will be assigned to `moment.frozen.duration.fn` for the key `name`. -- Otherwise, we'll wrap `func` with a function that always clones `this` before - invoking `func`, and assign that function to `moment.frozen.duration.fn` for the key `name`. - -Thus, we assume that any new prototype method returns a mutated Mutable, -unless the plugin tells us otherwise by setting an appropriate flag in the -optional `options` object. - -#### Implementation Notes - -Ideally these methods would also be used internally to register Moment's core -factories and methods with the Frozen prototype. - -All plugins will be entirely compatible with the Frozen API proposal if they -correctly register all of their functionality with Moment using these three -methods, provided that all of their new factories and methods return Mutables. -This describes the vast majority of the existing Moment plugin ecosystem. - -Plugins that add factories or methods that return custom objects (*not* Mutables, -e.g. the range plugins) will need to do some additional work to function -correctly when called from Frozens. [1](#fn1) - -We would need to add a new section for "Plugin Authors" to the documentation to -describe these methods and their intended usage. - -If we decide want to actively push new users toward using Frozens instead of -Mutables, then I think we should probably try to simplify the user-facing -plugin compatibility story as much as possible by requiring plugins to use -these APIs if they want to work with Moment 3.0+. This could be easily -achieved for methods if we stop publicly exposing the Moment and Duration -prototypes as `moment.fn` and `moment.duration.fn`, respectively. [2](#fn2) -It's a bit harder to enforce for factories, so it still might not be worth -enforcing that case, but one strategy could be to construct the `moment` -object with a prototype, and then apply `Object.freeze()` to the moment -object itself. Then our implementation of `addFactory()` could assign -functions to the `moment` object's prototype to make them publicly available. - -Depending on how hard we push this API on plugin authors, many plugins might -not proactively adopt this registration API even though it should be very easy -for most plugins to adopt. We might want to send PRs to some high-profile -plugins to make this transition easy for their authors. - -### Plugin vs Core Feature - -I think it might be somewhat confusing for users and plugin authors if we -implement these plugin registration functions as a feature that appears in -some environments but not others — which points toward including this piece -as a feature of the core library rather than building it in a plugin. - -If plugin registration is part of a first-party Frozen Moment plugin and not -part of the core library, then the `addMethod` and `addFactory` APIs would -only be available in the `moment.frozen` namespace (and not the top-level -`moment` namespace). Registered methods would simply be added to the Frozen -API — plugins would continue to extend `moment` and `moment.fn` directly -for the Mutable API. - -This system would certainly ensure backward compatibility with existing plugins -— although we don't **have** to break the way existing plugins currently work, -even if we do build plugin registration APIs into the core library and -encourage plugin authors to use these new APIs. On the other hand, I suspect -that having entirely different systems for plugins to work with Mutable vs -Frozen APIs would ultimately reduce the number of community plugins that choose -to support the Frozen API, assuming that both approaches would be accompanied -by similar levels of support and outreach effort from the Moment maintainers. - -If this registration system is part of an optional plugin, it's essential for -users to load Frozen Moment before any other plugins that they want to use with -the Frozen API, since the plugin registration system won't exist at all until -the Frozen plugin has been loaded. [3](#fn3) If that -optional plugin is a separate file, then some number of users will get the -bundling order wrong and file support requests. On the other hand, if we -distribute a bundled build where the core library and the Frozen plugin appear -in the same file, then we need to decide which is the default build for the big -download button at momentjs.com, and we also need to figure out how to -communicate the distinctions between different builds now that we'd (presumably) -have twice as many different pre-built packages available. - -I'm tempted to say we should sidestep all this build + communication complexity -by building everything from this RFC into the core library, especially since we -probably want to steer new users toward the new APIs. That said, we probably -need some community feedback (particularly from plugin authors) before locking -down the plugin vs core decision. - -## Frozen API - -### New APIs for Moment Users - -This proposal adds `moment.frozen` as a new API namespace. +### `moment.wrapDurationMethod(func, destinationAPI)` -Users who want to work exclusively with an immutable instance API (the -"Frozen API") would be able to ignore all Moment features that are not part -of the `moment.frozen` namespace. If they're not using third-party code, -such users might want to simply overwrite the `moment` global in a traditional -web environment (`moment = moment.frozen`), or import just the frozen APIs -while discarding the traditional mutable APIs when working in a CommonJS -environment (`var moment = require('moment').frozen`), etc. - -Users who want to work exclusively with the current mutable instance API -(the "Mutable API") would be able to ignore the new `moment.frozen` -namespace without consequence. - -Users would also be able to mix and match APIs within the same application, -allowing existing applications to incrementally migrate from one API to the -other by converting some of their code to use Frozens rather than Mutables, -and then parsing Mutables from Frozens (or vice-versa) at the boundaries to -the modified code as necessary. +Example usage: -So, from a user's perspective: +```js +moment.immutable.duration.add = moment.wrapMomentMethod(moment.mutable.duration.add, 'immutable'); +``` -- All `moment.*` static configuration methods will be shamelessly copied - as-is into the `moment.frozen.*` namespace, using the same method names - in both namespaces. Note that this includes the new plugin registration - methods described in the previous section. +Behavior when `destinationApi` is `"immutable"`: -- All factory methods in the `moment` namespace will be wrapped for use - (under the same method names) in the `moment.frozen` namespace. These - wrapped factories will return instances that use the Frozen API. For - example, the wrapped equivalent of `moment.parseZone()` would be - `moment.frozen.parseZone()`. +```js +function wrapMomentMethod(func, 'immutable') { + return function() { + return moment.immutable.duration(func.apply(moment.mutable.duration(this), arguments)); + }; +} +``` - When we say "all factory methods", this includes factory methods from - the core library as well as factory methods from plugins that have been - registered using the `moment.addFactory()` API described earlier. - Note that `moment.frozen()` must itself be a wrapped version of the - core library's `moment()` factory method, in additon to wrapping - factories that are static methods of `moment` (e.g. `moment.utc`, - `moment.tz`, etc). +Behavior when `destinationApi` is `"mutable"`: -- Frozens created from factory methods in the `moment.frozen` namespace - will be exactly the same as Mutables created from the existing factories, - except that they will be backed by the `moment.frozen.fn` prototype - instead of the `moment.fn` prototype (or `moment.frozen.duration.fn` - instead of `moment.duration.fn`, for Durations). - -- The `moment()` factory method (and likely all other `moment.*` factories) - will be able to accept a Frozen as input to construct a Mutable with the - same data as the input Frozen. Likewise, the `moment.frozen()` factory - (and likely all other `moment.frozen.*` factories) will be able to accept - a Mutable in order to construct a Frozen with the same data. The `freeze()` - and `thaw()` instance methods from the existing Frozen Moment plugin will - **not** be implemented, because they are not needed. - -- Instance methods in the Frozen API (from `moment.frozen.fn` and - `moment.frozen.duration.fn`) are wrapped versions of the corresponding - methods from the Mutable API. The wrappers simply create a copy of the - object (when appropriate) before delegating to the Mutable API - implementation of that function name. +```js +import { copyDurationConfig } from '../implement/this/somewhere'; + +function wrapMomentMethod(func, 'mutable') { + return function() { + var newDuration = func.apply(moment.immutable.duration(this), arguments); + copyDurationConfig(this, newDuration); + return this; + }; +} +``` - All core methods will be available on Frozens, as well as all - plugin-created methods that were registered using the `addMethod` APIs - described above. -### Implementation Plan +## Implementation Notes -A proposal for bootstrapping the `moment.frozen` namespace as described above: +A proposal for bootstrapping the new namespaces described above: -1. First, create `moment.frozen` as a wrapped version of the `moment` - factory function. We can't use `moment.addFactory()` for this, so it - will be hardcoded. If everything is in core, we create `moment.frozen` - immediately after creating `moment`, and before registering all the - core methods onto the Mutable prototype. If this RFC is a separate - plugin, then this is the first thing that happens in the plugin's - implementation. +1. First, create `moment.immutable` as a wrapped version of the `moment` + factory function immediately after creating `moment` and `moment.mutable`, + before registering all the core methods onto the Mutable prototype. 2. Ditto for Durations: we need hardcoded logic to create - `moment.frozen.duration` as a wrapped version of the - `moment.duration` factory, and we need that code to run as early - as possible. -3. Create `addFactory` as a method of `moment` (if this is part of core) - and `moment.frozen`. Ditto for `moment.addMethod` and - `moment.duration.addMethod`. -4. Use `moment.frozen.addFactory()` to register all factory functions - and static config methods from the core library. (Static config - methods would be registered with `returnsMoment: false`.) - `moment.frozen.addFactory` is responsible for wrapping and copying - those methods into the `moment.frozen` namespace. -5. Use `moment.frozen.addMethod()` and `moment.frozen.duration.addMethod()` - to register all Mutable instance methods from the core library. - The `addMethod` methods are responsible for wrapping and copying - those methods into the appropriate Frozen namespace, same as described - below for plugin methods. - -The Frozen API would be implemented with a separate prototype -(`moment.frozen.fn` vs `moment.fn`) for Frozens than for Mutables. -Similarly, there would be a separate prototype for Frozen Durations -(`moment.frozen.duration.fn`) than for Mutable Durations -(`moment.duration.fn`). - -Each time a plugin calls `moment.addMethod`, the provided function -will be wrapped (if appropriate), and then the wrapped copy will be -assigned for the specified function name on `moment.frozen.fn`. -[4](#fn4) - -Ditto for `moment.duration.addMethod`: -Each time a plugin calls `moment.duration.addMethod`, the provided function -will be wrapped (if appropriate), and then the wrapped copy will be -assigned for the specified function name on `moment.frozen.duration.fn`. - -### Plugin vs Core Feature - -This could be implemented directly in core, or with a first-party plugin -(which could be bundled into certain distribution builds for convenience). - -The Frozen API implementation will need to hook into the behavior of the -`addFactory` and `addMethod` APIs. This is trivial if everything is in core -(or if everything is in a plugin). If the plugin registration system is in core -but the Frozen API is not, then we'll want some mechanism for the plugin to -register a callback so it can do the right thing when `addFactory` and `addMethod` -get called. We'd also need to hardcode wrappers for core methods, and/or build a -way for the plugin to get a list of previously-registered methods at the same -time as the plugin sets up these callbacks. - -That said, it might make sense to set a maximum file size increase that we're -willing for these features to impose on the core library, and use that cutoff to -determine which path is best (after writing a rough initial implementation in core). + `moment.immutable.duration` as a wrapped version of the + `moment.mutable.duration` factory, and we need that code to run as early as + possible. +3. Create `wrapMomentFactory`, `wrapDurationFactory`, `wrapMomentMethod`, and + `wrapDurationMethod` functions as static methods of `moment`. (See below.) +4. Create the 4 prototypes: `moment.mutable.fn`, `moment.mutable.duration.fn`, + `moment.immutable.fn`, and `moment.immutable.duration.fn`. +5. Add all core functionality to the 4 prototypes, using the `wrap*` methods as + appropriate. + +All plugins will be entirely compatible with the Immutable API proposal if they +correctly attach all of their functionality to both the `moment.mutable` and +`moment.immutable` namespaces. The `moment.wrap*` static methods will help +most plugin authors do this with relatively low effort. + # How We Teach This Because our own experience and a preponderance of Moment-related support requests suggest that new users would have fewer problems working with the -Frozen API, we would generally want to push new users to use `moment.frozen` -exclusively and ignore the traditional Mutable API. +Immutable API, we would generally want to push new users to use +`moment.immutable` exclusively and ignore the traditional Mutable API. -We probably need some dedicated documentation at momentjs.com introducing -existing users to the semantic distinctions between the Mutable and Frozen -APIs, reassuring them that the Mutable APIs aren't disappearing but also -encouraging them to consider adoption of the Frozen APIs if it makes sense +We probably need some detailed release documentation at momentjs.com introducing +existing users to the semantic distinctions between the Mutable and Immutable +APIs, reassuring them that the Mutable APIs haven't been removed (yet) but also +encouraging them to consider adoption of the Immutable APIs if it makes sense in their situation. -We also need to describe the `addFactory` and `addMethod` functions for the -API reference documentation at momentjs.org/docs. +We also need to update our API reference documentation at momentjs.org/docs to +account for all these changes. It would feel pretty heavy to just append +(largely duplicative) documentation for all the `moment.immutable.*` APIs to +the existing documentation page. Since we generally want to steer new users +toward the Immutable APIs, then maybe we could show just the Immutable APIs by +default, with a toggle switch to convert between Mutable and Immutable API docs +(similar to how some Coffeescript libraries let you toggle between .js and +.coffee syntax in their documentation's code examples). -It would feel pretty heavy to just append (largely duplicative) documentation -for all the `moment.frozen.*` APIs to the existing documentation page. -Since we generally want to steer new users toward the Frozen APIs, then maybe -we could show just the Frozen APIs by default, with a toggle switch -to convert between Mutable and Frozen API docs (similar to how some -Coffeescript libraries let you toggle between .js and .coffee syntax in -their documentation's code examples). +Finally, we'll need some targeted documentation explaining how and (especially) +why plugin authors should update their plugins to work with the new version of +Moment. We might consider submitting PRs to a few popular plugins to help them +adopt the new registration APIs, and then link to those PRs from the +documentation as examples of how other plugin authors can update their +implementations. -Finally, we'll need some targeted documentation explaining how and -(especially) why plugin authors should register their methods for use with -the Frozen API. We might consider submitting PRs to a few popular plugins -to help them adopt the new registration APIs, and then link to those PRs -from the documentation as examples of how other plugin authors can update -their implementations. # Drawbacks -1. Doubling the number of exposed API methods (for "frozen" and traditional +1. Doubling the number of exposed API methods (for "immutable" and traditional versions of everything) will inevitably increase our documentation and support burdens, even if almost all of the code is the same. It's also likely to increase the testing burden for new feature additions from - maintainers and external contributors alike, although hopefully the - impact on *implementing* new features will be negligible. -2. By giving developers a choice of frozen/mutating APIs, new users of + maintainers and external contributors alike, although hopefully the impact + on *implementing* new features will be negligible. +2. By giving developers a choice of immutable/mutating APIs, new users of Moment will have more friction getting started with the library because they'll need to choose an API before they can do anything useful. -3. This feature has often been described as an "immutable" API, but we're - not actually making the objects immutable — developers can assign and - modify custom properties on these objects as much as they want. Indeed, - "frozen" instances would still be mutating under the hood in certain - situations (because some internal properties are lazily computed). This - may be confusing for some developers and we will likely get questions - about it. -4. We expect these changes will increase the library's file size somewhat, - even for users who don't use any of the new APIs, simply because we're - adding code to the library. +3. This feature has often been described as an "immutable" API, but we're not + actually making the objects immutable — developers can assign and modify + custom properties on these objects as much as they want. Indeed, + "immutable" instances would still be mutating under the hood in certain + situations (because some internal properties are lazily computed). This may + be confusing for some developers and we will likely get questions about it. +4. We expect these changes will increase the library's file size somewhat, even + for users who don't use any of the new APIs, simply because we're adding + code to the library. 5. Because plugin methods and factories are created with string names, minifiers won't be able to rename those methods, possibly leading to an additional slight increase in file sizes for folks who minify their dependencies together with their custom code. -6. The new plugin registation system may slightly increase the time - required for Moment and its plugins to initialize themselves for use. - I don't expect this to be a noticeable issue, though. +6. The new plugin registation system may slightly increase the time required + for Moment and its plugins to initialize themselves for use. I don't expect + this to be a noticeable issue, though. + # Alternatives As hinted in the Motivation section, if we don't do this (or something similar), then we're effectively asking the folks who keep +1'ing [#1754][] -to build their own community around the existing third-party plugin, or to -find or build another library instead of using Moment. This would not be the -end of the world, but it would be a little frustrating since we agree that -date-time objects should ideally be an immutable type. +to build their own community around the existing third-party plugin, or to find +or build another library instead of using Moment. This would not be the end of +the world, but it would be a little frustrating since we agree that date-time +objects should ideally be an immutable type. To date, the current third-party Frozen Moment plugin has had much less reach than we might expect for a first-party solution. We might be able to close that gap somewhat by promoting the third-party plugin more heavily on momentjs.com, since it doesn't appear to be mentioned anywhere in the official documentation right now. This sort of cross-promotion scheme could achieve -some of the same usage benefits while delegating much of the communication -and maintenance burden for this functionality outside the core Moment team — -at the cost of continuing with some additional perceived and actual risk -(relative to a first-party solution) as to whether that plugin would continue -to be maintained appropriately over the long term. - -There is no technical reason why the current third-party plugin could not grow -to do everything that a first-party plugin would do under this RFC, including -plugin registration and distributing bundled builds. In fact, I will likely -work to make that happen if this RFC is not adopted. But any third-party solution -will be handicapped by the fact that users don't expect plugins to be able to -override core API decisions like this, so they're less likely to find and adopt -a third-party plugin for this purpose (even if that plugin was well-publicized, -etc). +some of the same usage benefits while delegating much of the communication and +maintenance burden for this functionality outside the core Moment team — at the +cost of continuing with some additional perceived and actual risk (relative to +a first-party solution) as to whether that plugin would continue to be +maintained appropriately over the long term. + +There is no technical reason why the current third-party plugin, or a new +first-party plugin, could not grow to do everything that this RFC proposes. +In fact, I will likely work to make that happen within the Frozen Moment plugin +if this RFC is not adopted. But any plugin-based solution will be handicapped +by the fact that users don't expect plugins to be able to override core API +decisions like this, so they're less likely to find and adopt a plugin for this +purpose (even if that plugin was well-publicized, etc). I also hope that it will become easier to get other plugin authors to write -compatible plugins (by using the registration system, etc) if Frozen Moment is -not itself "just" another third-party plugin. +compatible plugins (by using the registration system, etc) if these capabilities +are in core, rather than "just" another third-party plugin. + # Unresolved questions -1. How much of this functionality should be implemented in the core library - vs a first-party plugin? If it's a plugin, then we should probably still - consider distributing builds that have both Frozen Moment and Moment.js - in a single file to make it easy for new users to adopt the Frozen APIs. - That way the big download button at momentjs.com can get new users set up - with our recommended environment. But then, how would we label the link - to download a build of just the core library, without this RFC's plugin? -2. How quickly and strongly do we want to proactively encourage new users to - use the frozen APIs instead of the traditional APIs? Ditto for existing - users? -3. Do we want to avoid publishing our prototypes in 3.0 and force plugin - authors to adopt the new registration API, or should we just strongly - encourage plugins to update without breaking current plugins? -4. The documentation and publicity strategy for this release needs to be - fleshed out further, but that's partly dependent on our answers to the - other unresolved questions. +1. 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? +2. The documentation and publicity strategy for this release needs to be + fleshed out further as we continue planning. +3. Do static "configuration" methods get copied into `moment.mutable` and + `moment.immutable`, or do they only exist on the top-level `moment`? + # Footnotes -1. Thankfully I don't think moment-range uses mutator methods - much, if at all, so it should be easy to update. It looks like most of - Twix's functionality should also be trivial to update, although its range - iterators would need to be reimplemented. [↩](#a1) - -2. Technically, third-party code could still sidestep our - registration API by messing with `Object.getPrototypeOf(moment())`. - At that point, though, it's easier to just use our APIs instead, so I don't - think any plugin authors would actually do that. It's not like we could - possibly stop someone who is **that** motivated to muck with our internals, - anyway. [↩](#a2) - -3. If the registration system is in core but the - APIs are not, then the registration system could keep track of metadata about - all the plugin methods that have already been registered. Then any plugins - that want to hook into that system could also retrieve the metadata for all - the methods that were registered before their plugin was loaded. [↩](#a3) - -4. Thus, unlike the existing third-party Frozen Moment - plugin, `moment.frozen.fn` will **not** have `moment.fn` as its prototype. - This means that all Frozen API methods must explicitly appear on the Frozen +1. Thus, unlike the existing third-party Frozen Moment plugin, + `moment.immutable.fn` will **not** have `moment.fn` as its prototype. This + means that all Immutable API methods must explicitly appear on the Immutable prototype, instead of simply falling through to the Mutable prototype for methods that are not expected to mutate the object. This change is made to - ensure correctness; this way, unregistered plugin methods will not appear on - Frozens so that they cannot accidentally mutate the value of a Frozen. - (The same is true for `moment.frozen.duration.fn`: it will not have - `moment.duration.fn` as its prototype for the same reason.) [↩](#a4) + ensure correctness; this way, Mutable plugin methods that don't support the + Immutable API will not appear on Immutables so that they cannot accidentally + mutate the value of a Immutable. (The same is true for + `moment.immutable.duration.fn`: it will not have `moment.duration.fn` as its + prototype for the same reason.) [↩](#a1) [#1754]: https://github.com/moment/moment/issues/1754 From 280af4f1e452ca8f84430b4ad93254680895023e Mon Sep 17 00:00:00 2001 From: Lucas Sanders Date: Sun, 31 Jul 2016 22:04:52 -0400 Subject: [PATCH 5/5] Revise immutability RFC to completely remove mutable APIs in 3.0 --- 0000-immutability.md | 404 ++++++++++--------------------------------- 1 file changed, 87 insertions(+), 317 deletions(-) diff --git a/0000-immutability.md b/0000-immutability.md index d61ac8a..f67ca98 100644 --- a/0000-immutability.md +++ b/0000-immutability.md @@ -5,8 +5,7 @@ # Summary -Add a pseudo-immutable object API to the core Moment library, replicating -the full functionality of our current API. +Change all APIs so that all moments will be psuedo-immutable in Moment 3. # Motivation @@ -23,9 +22,7 @@ At the same time, Moment has been used to build millions of existing applications, all of which rely on the library's current API design. Many groups with existing codebases, especially those who already use Moment pervasively, strongly value the API stability that Moment has maintained over -the years to date. In addition, many developers coming to Moment from the -native Javascript Date API tend to expect an API where a single object's -value can change over time. +the years to date. There is a plugin, [Frozen Moment][], that has attempted to address these issues. Unfortunately, as a third-party plugin it has limited visibility and @@ -35,329 +32,112 @@ great way for a plugin to find out about the other Moment plugins that might be in use in any particular environment, which makes it difficult to use plugins while also using the immutable APIs. -To address these concerns, we could add a plugin registration system to the -core Moment library, coupled with a `moment.immutable` namespace that would -serve as a fully equivalent immutable API alternative to the primary `moment` -namespace and its various properties and constructors. +We've talked with a few authors of widely-used plugins about these challenges, +and heard that some plugin maintainers would prefer to support exactly one API +(mutable or immutable) per version of their plugin -- in other words, if we're +going to add an immutable API, they would prefer a hard cutover where the +immutable API replaces the old mutable API rather than a solution where both +APIs might co-exist simultaneously. +**To address these concerns,** we'd like to change Moment's public APIs to +provide an immutable API (similar to the `moment.frozen` functionality from +Frozen Moment). We'd also like to work with plugin authors to help them +migrate to the new API. -# Detailed design +We might also extend our build system to generate a 2.x-compatible version of +Moment from the same codebase (by changing a few files from one build to the +other) and use that functionality to support both APIs during a transitional +period. This might most productively be expressed as a 3-6 month period of 2.x +releases and simultaneous 3.0 "beta"/"preview" releases, leading up to a final +3.0 release and/or discontinuation of the existing 2.x release stream. During +this period, and possibly for some time thereafter, we'd want new releases of +the Moment Timezone plugin to maintain compatibility with both Moment 2.x and 3.x. -There are two pieces to this proposal: +Users who prefer the current mutable API can stick with the very stable 2.x +codebase, or they might build a plugin to implement a mutable API on top of +our own immutable API (like Frozen Moment, but moving the opposite direction). -1. a user-facing "Immutable" API (actually pseudo-immutable) that wraps the - core library and its registered plugins; -2. mechanisms for plugin authors to easily extend Moment, with feature parity - for both the "Mutable" and "Immutable" APIs. - -For brevity, I'll refer to moment objects that use the current API as -**Mutables** throughout this section, and objects using the proposed -pseudo-immutable API will be called **Immutables**. `moment.immutable.fn` -is the prototype for Immutables. - - -## New APIs for Moment Users - -This RFC adds two new API namespaces: `moment.mutable` and `moment.immutable`. - -The top-level `moment` namespace might be a reference to `moment.mutable` or -`moment.immutable` for convenience, and this might differ between build -configurations. Thus, we might offer both a Compatibility build where `moment` -APIs are equivalent to `moment.mutable`, and a Recommended build where `moment` -APIs are equivalent to `moment.immutable`. - -Users who want to work exclusively with an immutable instance API (the -"Immutable API") would be able to work solely with the `moment.immutable` -namespace, and those who don't want to change their code can simply use the -`moment.mutable` namespace. Third-party code (e.g. UI components) will always -be able to rely on having their desired API available. - -If they're not using third-party code, users might want to simply overwrite the -`moment` global in a traditional web environment (`moment = moment.immutable`) -to reflect the API that they are using. Similarly, I expect it'd be common to -import just one API while discarding the other API when working in a CommonJS -environment (`var moment = require('moment').immutable`), etc. - -That said, users would also be able to mix and match APIs within the same -application, allowing existing applications to incrementally transition between -APIs. We will strongly discourage users from mixing and matching from both -APIs within a single codebase as a long-term design choice. - - -So, from a user's perspective: - -- All `moment.*` static configuration methods will be shamelessly copied - as-is into the `moment.mutable` and `moment.immutable.*` namespaces, using - the same method names in all three namespaces. Note that this includes the - new plugin registration methods described in the previous section. - - (Or maybe our static configuration methods should only live in the top-level - `moment` namespace? But then I'd kind of want to force everyone to always - explicitly construct instances from the `mutable` or `immutable` namespaces, - with no top-level convenience wrappers there...) - -- All factory methods in the `moment` namespace will be wrapped for use - (under the same method names) in the `moment.immutable` namespace. These - wrapped factories will return instances that use the Immutable API. For - example, the wrapped equivalent of `moment.mutable.parseZone()` would be - `moment.immutable.parseZone()`. - - When we say "all factory methods", this includes factory methods from - the core library as well as factory methods from plugins that have been - updated to support the new API. Note that `moment.immutable()` must itself - be a wrapped version of the core library's `moment.mutable()` factory method, - in additon to wrapping factories that are static methods of `moment.mutable` - (e.g. `moment.mutable.utc`, `moment.mutable.tz`, etc). - -- Immutables created from factory methods in the `moment.immutable` namespace - will be exactly the same as Mutables created from the existing factories, - except that they will be backed by the `moment.immutable.fn` prototype - instead of the `moment.mutable.fn` prototype (or - `moment.immutable.duration.fn` instead of `moment.mutable.duration.fn` - for Durations). - -- The `moment.mutable()` factory method (and likely all other `moment.mutable.*` - factories) will be able to accept a Immutable as input to construct a Mutable - with the same data as the input Immutable. Likewise, the `moment.immutable()` - factory (and likely all other `moment.immutable.*` factories) will be able to - accept a Mutable in order to construct a Immutable with the same data. - - The `freeze()` and `thaw()` instance methods from the existing Frozen Moment - plugin will **not** be implemented, because they are not needed. - -- Instance methods in the Immutable API (from `moment.immutable.fn` and - `moment.immutable.duration.fn`) are wrapped versions of the corresponding - methods from the Mutable API. The wrappers simply create a copy of the - object (when appropriate) before delegating to the Mutable API - implementation of that function name. - - All core methods will be available on Immutables, as well as all - plugin-created methods that were registered using the `addMethod` APIs - described above. - -- The return value of `moment.isMoment()` could be changed to a tri-state enum: - `"mutable"` if the input's prototype is `moment.mutable.fn`; `"immutable"` if - the input's prototype is `moment.immutable.fn`; otherwise `false`. - - -## API Changes for Plugin Authors - -A Immutable API can be implemented much more robustly if we ask plugins to -explicitly opt in to compatibility with Immutables. Without this, Immutables -can only support the core library's API methods, or we need to somehow guess at -the behavior of other plugins so we can provide their methods on Immutables. - -There are six types of functions that plugins could provide, where the plugin -implementations could be expected to differ between the Mutable and Immutable -APIs: - -1. moment factory functions (colloquially, "constructor" — e.g. `moment.tz()`) -2. mutation methods on the `moment.fn` prototype -3. duration factory functions (e.g. `moment.duration()`) -4. mutation methods on the `moment.duration.fn` prototype -5. methods on the `moment.fn` prototype that return new non-Moment objects, - and those new objects use Moment's APIs (e.g. moment-range, Twix.js) -6. wrapped copies of the `moment()` factory function (e.g. moment-jalaali, - moment-hijri) - -For these use cases #1 through #4, we'll provide utility methods on `moment` -that generate an Immutable method implementation from a Mutable plugin function. -These same wrapper-generation functions might also be of some limited help for -certain plugins in category #5 (e.g. Twix). At the end of the day, plugins will -be required to explicitly assign their methods into the `moment.mutable` and -`moment.immutable` namespaces as appropriate; the wrapper functions below are -simply provided as a convenience to make it easier for plugin authors to support -both APIs simultaneously. - -Otherwise, we can't do too much to help plugins that do #5 and #6 (aside from -maybe creating new extensibility APIs so that the calendar plugins can stop -wrapping the magic `moment()` factory, but that's way out of scope for this -RFC). For those use cases, we should probably just aim to get out of the way -and let the plugins sort themselves out. - -### `moment.wrapMomentFactory(func, destinationAPI)` - -Example usage: - -```js -moment.immutable = moment.wrapMomentFactory(moment.mutable, 'immutable'); -moment.mutable.tz = moment.wrapMomentFactory(moment.immutable.tz, 'mutable'); -``` - -Behavior: - -```js -function wrapMomentFactory(func, api) { - return function() { - return moment[api](func.apply(null, arguments)); - }; -} -``` - -### `moment.wrapDurationFactory(func, destinationAPI)` - -Example usage: - -```js -moment.immutable.duration = moment.wrapDurationFactory(moment.mutable.duration, 'immutable'); -``` - -Behavior: - -```js -function wrapMomentFactory(func, api) { - return function() { - return moment[api].duration(func.apply(null, arguments)); - }; -} -``` - -### `moment.wrapMomentMethod(func, destinationAPI)` - -Example usage: - -```js -moment.immutable.add = moment.wrapMomentMethod(moment.mutable.add, 'immutable'); -``` - -Behavior when `destinationApi` is `"immutable"`: - -```js -function wrapMomentMethod(func, 'immutable') { - return function() { - return moment.immutable(func.apply(moment.mutable(this), arguments)); - }; -} -``` - -Behavior when `destinationApi` is `"mutable"`: - -```js -import { copyConfig } from './constructor'; -function wrapMomentMethod(func, 'mutable') { - return function() { - var newMoment = func.apply(moment.immutable(this), arguments); - copyConfig(this, newMoment); - return this; - }; -} -``` - -### `moment.wrapDurationMethod(func, destinationAPI)` - -Example usage: - -```js -moment.immutable.duration.add = moment.wrapMomentMethod(moment.mutable.duration.add, 'immutable'); -``` - -Behavior when `destinationApi` is `"immutable"`: - -```js -function wrapMomentMethod(func, 'immutable') { - return function() { - return moment.immutable.duration(func.apply(moment.mutable.duration(this), arguments)); - }; -} -``` - -Behavior when `destinationApi` is `"mutable"`: +# Detailed design -```js -import { copyDurationConfig } from '../implement/this/somewhere'; +## For Moment Users -function wrapMomentMethod(func, 'mutable') { - return function() { - var newDuration = func.apply(moment.immutable.duration(this), arguments); - copyDurationConfig(this, newDuration); - return this; - }; -} -``` +All externally-facing APIs that currently modify the value of a Moment instance +will instead return a new Moment. This includes many of the instance methods +listed in the "Get + Set", "Manipulate", and "i18n" categories of our [docs][]. +All externally-facing APIs that currently modify the value of a Duration +instance will instead return a new Duration. (I believe this is just +`Duration#add` and `Duration#subtract`.) -## Implementation Notes +Set-selection functions (`moment.max` and `moment.min`) will return one of the +original input instances, **not** a copy -- unless one of the inputs was invalid, +in which case we'll return a canonical "invalid" moment that doesn't include +any interesting information in `_i`. -A proposal for bootstrapping the new namespaces described above: +## For Moment Developers -1. First, create `moment.immutable` as a wrapped version of the `moment` - factory function immediately after creating `moment` and `moment.mutable`, - before registering all the core methods onto the Mutable prototype. -2. Ditto for Durations: we need hardcoded logic to create - `moment.immutable.duration` as a wrapped version of the - `moment.mutable.duration` factory, and we need that code to run as early as - possible. -3. Create `wrapMomentFactory`, `wrapDurationFactory`, `wrapMomentMethod`, and - `wrapDurationMethod` functions as static methods of `moment`. (See below.) -4. Create the 4 prototypes: `moment.mutable.fn`, `moment.mutable.duration.fn`, - `moment.immutable.fn`, and `moment.immutable.duration.fn`. -5. Add all core functionality to the 4 prototypes, using the `wrap*` methods as - appropriate. +I think we'll want to take a Frozen Moment-style approach to the implementation. +If a method is going to mutate the instance then internally clone it before doing +anything else, and then use the old mutable implementations of everything. When +calling other functions internally, we should continue to call the internal +mutable version of the API rather than the externally-facing immutable APIs. +This approach will minimize the effort needed to develop v3.0 and minimize the +performance hit from moving our users onto an immutable API. Because this will +be a relatively small change to our codebase, this approach is also very useful +if/when we want to build parallel v2.x/v3.0 releases while our users transition +to the new APIs. -All plugins will be entirely compatible with the Immutable API proposal if they -correctly attach all of their functionality to both the `moment.mutable` and -`moment.immutable` namespaces. The `moment.wrap*` static methods will help -most plugin authors do this with relatively low effort. +We'll want to update and add unit tests asserting the immutable nature of our +external APIs, etc. I suspect this will require more effort than updating the +core library code itself. I think we want to avoid maintaining two sets of tests, +even temporarily -- avoiding additional pain and effort in testing is probably +the best argument against doing parallel 2.x/3.x builds for a transition period. # How We Teach This Because our own experience and a preponderance of Moment-related support -requests suggest that new users would have fewer problems working with the -Immutable API, we would generally want to push new users to use -`moment.immutable` exclusively and ignore the traditional Mutable API. - -We probably need some detailed release documentation at momentjs.com introducing -existing users to the semantic distinctions between the Mutable and Immutable -APIs, reassuring them that the Mutable APIs haven't been removed (yet) but also -encouraging them to consider adoption of the Immutable APIs if it makes sense -in their situation. +requests suggest that new users would have fewer problems working with an +immutable API, we will generally want to push new users to adopt 3.x ASAP. +To that end, we'll want to carefully craft our 3.x release notes to help +introduce existing users to the changes and explain what they'll need to do +in order to prepare their codebases for the upgrade. We also need to update our API reference documentation at momentjs.org/docs to -account for all these changes. It would feel pretty heavy to just append -(largely duplicative) documentation for all the `moment.immutable.*` APIs to -the existing documentation page. Since we generally want to steer new users -toward the Immutable APIs, then maybe we could show just the Immutable APIs by -default, with a toggle switch to convert between Mutable and Immutable API docs -(similar to how some Coffeescript libraries let you toggle between .js and -.coffee syntax in their documentation's code examples). +account for all these changes. We should maybe consider publishing an archived +copy of the v2.x docs at a new URL when we update the "standard" docs to the 3.x +API for the new release. On the other hand, the doc changes should be fairly +minor, so maybe that's not a big deal (especially if the docs are clearly and +pervasively marked as describing v3.x). I think we mostly just want to avoid +having 2.x users rely on the 3.x docs and therefore create mutability bugs. Finally, we'll need some targeted documentation explaining how and (especially) why plugin authors should update their plugins to work with the new version of Moment. We might consider submitting PRs to a few popular plugins to help them -adopt the new registration APIs, and then link to those PRs from the -documentation as examples of how other plugin authors can update their -implementations. +upgrade, and then link to those PRs from the documentation as examples of how +other plugin authors can update their implementations. # Drawbacks -1. Doubling the number of exposed API methods (for "immutable" and traditional - versions of everything) will inevitably increase our documentation and - support burdens, even if almost all of the code is the same. It's also - likely to increase the testing burden for new feature additions from - maintainers and external contributors alike, although hopefully the impact - on *implementing* new features will be negligible. -2. By giving developers a choice of immutable/mutating APIs, new users of - Moment will have more friction getting started with the library because - they'll need to choose an API before they can do anything useful. -3. This feature has often been described as an "immutable" API, but we're not +1. Some large existing codebases may find it difficult to upgrade from 2.x to 3.x, + especially if they heavily relied on Moment's mutability. +2. This feature has often been described as an "immutable" API, but we're not actually making the objects immutable — developers can assign and modify custom properties on these objects as much as they want. Indeed, "immutable" instances would still be mutating under the hood in certain situations (because some internal properties are lazily computed). This may be confusing for some developers and we will likely get questions about it. -4. We expect these changes will increase the library's file size somewhat, even - for users who don't use any of the new APIs, simply because we're adding - code to the library. -5. Because plugin methods and factories are created with string names, - minifiers won't be able to rename those methods, possibly leading to an - additional slight increase in file sizes for folks who minify their - dependencies together with their custom code. -6. The new plugin registation system may slightly increase the time required - for Moment and its plugins to initialize themselves for use. I don't expect - this to be a noticeable issue, though. +3. This will likely add a few bytes in file size to the core library, because + the implementation approach is purely additive. Similarly, "mutation" methods + will likely take slightly longer to run, due to the need to allocate memory + and copy values into a new object. We may want to do performance testing to + determine the exact level of impact here, but the effects should be very + minimal and we don't expect many users to notice. In fact, users who always + call `#clone` before mutating their moments may even see a slight improvement + in overall code size and performance with the new approach, since they can now + remove a lot of extraneous method calls. # Alternatives @@ -395,27 +175,16 @@ are in core, rather than "just" another third-party plugin. # Unresolved questions -1. 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? -2. The documentation and publicity strategy for this release needs to be - fleshed out further as we continue planning. -3. Do static "configuration" methods get copied into `moment.mutable` and - `moment.immutable`, or do they only exist on the top-level `moment`? - - -# Footnotes - -1. Thus, unlike the existing third-party Frozen Moment plugin, - `moment.immutable.fn` will **not** have `moment.fn` as its prototype. This - means that all Immutable API methods must explicitly appear on the Immutable - prototype, instead of simply falling through to the Mutable prototype for - methods that are not expected to mutate the object. This change is made to - ensure correctness; this way, Mutable plugin methods that don't support the - Immutable API will not appear on Immutables so that they cannot accidentally - mutate the value of a Immutable. (The same is true for - `moment.immutable.duration.fn`: it will not have `moment.duration.fn` as its - prototype for the same reason.) [↩](#a1) +1. Should we remove `moment#clone` in 3.0? If not, I feel it should probably + continue to return a new instance to minimize confusion -- unless we're keeping + it solely to ease the pain of transition for existing codebases, in which case + we should return the same instance and spew deprecation warnings for a planned + removal in v4.0. +2. Do we want to publish parallel 2.x/3.x builds for a while to ease the transition? + If so, how do we label this, and how do we justify the timing for shutting off + the 2.x compatibility builds? +3. The documentation and publicity strategy for this release needs to be fleshed + out further as we continue planning. [#1754]: https://github.com/moment/moment/issues/1754 @@ -423,3 +192,4 @@ are in core, rather than "just" another third-party plugin. [maggiepint post]: https://maggiepint.com/2016/06/24/why-moment-js-isnt-immutable-yet/ [pull request]: https://github.com/moment/moment-rfcs/pull/2 [ValueObject]: http://martinfowler.com/bliki/ValueObject.html +[docs]: http://momentjs.com/docs/