Immutable API #3548

butterflyhug · 2016-10-30T02:22:06Z

This is an implementation of moment/moment-rfcs#2, which was written to address #1754. This is obviously a breaking API change that would require a new major version number (e.g. "3.0").

The only major change from the RFC draft's discussion is that moment.updateOffset() and moment.duration()._bubble() will now return moment and duration objects, respectively. Otherwise it's pretty much impossible for external code to customize the behavior of these hooks.

Feedback welcome! 😃

ichernev · 2016-10-30T08:48:16Z

That is amazing. I'll review it very thoroughly on Tuesday utc. Also we need to make sure to backport whatever is released before 3.0 (if we decide to release). Or we can ship this as 3.0-rc1 and keep the two branches for a few months.

butterflyhug · 2016-10-30T18:47:20Z

Thanks! I'm adding a few comments now about things that I think are worth extra scrutiny, but I'd love to get a comprehensive review.

butterflyhug · 2016-10-30T18:52:42Z

src/lib/duration/prototype.js

 proto.localeData     = localeData;

 // Deprecations
 import { deprecate } from '../utils/deprecate';

 proto.toIsoString = deprecate('toIsoString() is deprecated. Please use toISOString() instead (notice the capitals)', toISOString);
-proto.lang = lang;
+proto.lang = deprecate('duration.lang() is deprecated. Use locale() or localeData() instead.', wrap(Duration, lang));


The lang method has been deprecated in the docs for several releases now, so I went ahead and added the warning here. But technically we said we'd get rid of all the deprecated stuff in our next major release, so maybe we should just delete old compatibility shims like this...

kill with fire 🔥 🔥 🔥

I'd remove everything that has been deprecated besides good old 'moment construction falls back to JS date'. Unfortunately I think we'll break the whole internet if we remove that functionality.

butterflyhug · 2016-10-30T18:56:32Z

src/lib/locale/constructor.js

 export function Locale(config) {
    if (config != null) {
-        this.set(config);
+        set.call(this, config);


This got reconfigured because the prototype's copy of set is wrapped for immutability, which is decidedly unhelpful inside a constructor.

Note that we hadn't explicitly discussed making Locale objects immutable, but it seemed like the obvious thing to do.

butterflyhug · 2016-10-30T18:58:02Z

src/lib/moment/locale.js

@@ -18,16 +18,13 @@ export function locale (key) {
    }
 }

-export var lang = deprecate(
-    'moment().lang() is deprecated. Instead, use moment().localeData() to get the language configuration. Use moment().locale() to change languages.',


This deprecation wrapper got moved to the prototype definition, the same way we handle all the other deprecated APIs.

butterflyhug · 2016-10-30T19:01:16Z

src/lib/utils/wrap.js

@@ -0,0 +1,11 @@
+export default function wrap(Type, fn, dontCloneWithNoArgs) {


I'm curious to know if other folks think this function is sensible. It provides a conveniently concise and reusable API, but I fear the implementation might be a little hard for us humans to parse.

It's clear enough to me. I'm not exactly sure how one would go about doing what you are doing besides this, while still mostly preserving existing code.

I would put exactly how this works in the contributing file. Also needs to be a standard part of code review to make sure this gets used if it needs to be.

With regard to moment-timezone - it calls update on the moment, and there's no need to be cloning the moment to do that - that's just a perf killer. Can we track updateInProgress in this function and just mutate if that's the case? I think that's fine to do because JavaScript is single threaded (thank god!).

butterflyhug · 2016-10-30T19:19:00Z

src/lib/moment/get-set.js

@@ -43,7 +43,7 @@ export function stringSet (units, value) {
        units = normalizeObjectUnits(units);
        var prioritized = getPrioritizedUnits(units);
        for (var i = 0; i < prioritized.length; i++) {
-            this[prioritized[i].unit](units[prioritized[i].unit]);
+            prioritized[i].getSet.call(this, prioritized[i].value);


This change prompted all of the other getter/setter upheaval in this commit. The old line of code didn't work as-is, because the prototype's getter/setter functions will now return new objects instead of mutating the old ones. A simpler change would've been:

var ret = this; for (var i = 0; i < prioritized.length; i++) { ret = ret[prioritized[i].unit](units[prioritized[i].unit]); }

But that would involve generating a bunch of unnecessary moments. At the time I wrote this commit, I was worried about the resulting performance impact. I became somewhat less concerned about keeping copies to a minimum in later commits, after I discovered that the updateOffset API requires some "extra" copying no matter what.

Let me know if you'd like to see any changes in the way I've balanced performance vs readability throughout this PR.

butterflyhug · 2016-10-30T19:20:13Z

src/lib/units/priorities.js

+            value: unitsObj[u],
+            priority: priorities[u].priority,
+            getSet: priorities[u].getSet
+        });


All the getter/setter functions get registered here, in addition to registering them directly on the Moment prototype as before, so that we can access their unwrapped implementations in the generic .set() API (aka "stringSet", where I left the other comment above).

Yes, this code was a bit too "advanced" :)

butterflyhug · 2016-10-30T19:22:58Z

src/lib/units/offset.js


        if (keepLocalTime) {
-            this.subtract(getDateOffset(this), 'm');
+            ret = ret.subtract(getDateOffset(ret), 'm');


On the other hand, I obviously wasn't quite as worried about creating intermediate copies when I updated this function...

I don't see an issue about intermediate copies. In general, with immutable API, how do you avoid them?

If we'd done our original plan of wrapping mutable internals, then we wouldn't have intermediate copies within the library implementation. But this comment is outdated -- we decided to work toward immutable internals instead, to make future development more understandable.

butterflyhug · 2016-10-30T19:39:37Z

src/lib/moment/constructor.js

-        updateInProgress = true;
-        hooks.updateOffset(this);
-        updateInProgress = false;
-    }


I moved this code to createFromConfig, which seems to cover all cases of creating Moments from non-Moment input. In doing so, I'm assuming that it's safe to clone any Moment without running updateOffset. That seems like a reasonably safe assumption, but I'd like to highlight it in case there's anything I'm not aware of.

When I kept the updateOffset call here, I had a subtle bug with the way moments got cloned when the parseZone algorithm calls the wrapped copy of utcOffset from our prototype. It occurs to me now that I probably could've fixed that by (1) re-ordering operations within parseZone, and/or (2) using the unwrapped implementation behind utcOffset instead of simply calling the prototype's copy. I'd be happy to try replacing this with one or both of those solutions if you're concerned about this particular edit -- but if this edit seems safe, I think it's probably faster and easier to reason about.

(Note that this edit avoids the messiness of trying to make an immutable updateOffset work inside the constructor:

updateInProgress = true; var updated = hooks.updateOffset(this); copyConfig(this, updated); this._d = updated._d; updateInProgress = false;

Which is ugly and annoying.)

I'm not sure I follow you here (maybe this comment is outdated). I'm left with the impression that there is no "wrap" any more.

Yeah, most of this comment is outdated. The first paragraph is still true -- I've still got this call to updateOffset in create/from-anything.js -- but not for any particularly great reason anymore, so I should probably just move it back to the constructor.

butterflyhug · 2016-10-30T19:41:48Z

src/lib/duration/constructor.js

+    // _bubble returns a new Duration because immutability,
+    // but here in the constructor we need to update `this` with the new values
+    var bubbled = this._bubble();
+    copyDuration(this, bubbled);


Yuck. I don't see a way around this, though, unless we un-publish the _bubble method so that it can remain an unwrapped mutator.

Aaand I got my commits out of order while rebasing things. The implementation of copyDuration appears in the next commit. (In retrospect, I should've just squashed these two commits into one.)

maggiepint · 2016-10-30T21:41:45Z

Not sure how everyone else feels, but my inclination with this PR is to divide up the tests between me, @icambron , @mj1856, and @ichernev. Each of us can take a set to review. You're then responsible for reviewing the entire code path down from the test. This will cause the most commonly used code paths to be reviewed several times, and the less commonly used to be reviewed once.
I think we also need to perf benchmark against the existing build to see if we have introduced any problem areas.
Finally, we should cut a build of this for a few interested individuals to use, before we do any kind of release. Better to catch with some dedicated beta testers.
Impressive as heck @butterflyhug!

maggiepint · 2016-10-30T21:43:33Z

Also, we're going to probably have a few merge conflicts with existing PRs. Not sure on the strategy for resolving that, but to me it makes sense to start merging them to develop, even if we aren't planning on a release. I think poor Lucas is going to be stuck with resolving for a large part, though we all can.

butterflyhug · 2016-10-31T22:42:01Z

So long as we don't do another ES6 rewrite in the meantime, I think merging changes from develop between now and 3.0 should be a doable (albeit large) task. After all, I've done it before.

ichernev · 2016-11-03T06:03:21Z

OK, here are my notes:

should we (do we) freeze moments/durations/locales to make sure they're not changed by something we've missed?
can we add _clone method that clones instead of passing the type in wrap. It will be for internal use only
in src/lib/create/from-anything.js why do we updateOffset (we weren't before)
in src/lib/duration/constructor.js bubble -- if its internal and used only in constructor why can't it just modify stuff
in src/lib/duration/prototype.js - why is abs not wrapped?
about updateOffset madness -- we can just implement the new API and remove the old one Implement timezone interface #3134
in src/lib/moment/start-end-of.js - why is startOf wrapped, when its implementation is immutable, same goes for endOf
in src/lib/utils/day-of-week.js - getSetDayOfWeek is immutable, do we need wrap (others in file too)
in src/lib/units/day-of-year.js - getSetDayOfYear -- no need to wrap
we have a generic get/set for all builtin units (ms,s,h,d,M,y), so I think we can make it immutable, then remove the wrap on all methods. This half-way immutability might give us an immutable API but it is crazy to develop, never knowing which functions can mutate and which can't
in src/lib/units/quarter.js -- the getSetQuarter can be made immutable (by relying on .month pseudo-immutability)
how do we port the locales and the locales tests? How did you fix all tests now?

I'm a bit lost on something -- how come the plugin did work while not changing any code inside moment. Because it had both frozen and mutating API, and the inners where always using the mutating api, the frozen stuff was only for the user, using the mutating api + clone. Couldn't we just hide the mutating API from the user and only expose the frozen, which will be autogen with the plugin code? Just overall -- I was expecting a change that was basically the plugin code shipped with the core code. If we spend time to make it properly immutable we might as well do that and remove wrap from the remaining methods. The core mutating methods are not that much, most rely on other mutating methods.

butterflyhug · 2016-11-06T13:47:11Z

Re @ichernev:

I'm not freezing moments here, and I think we probably shouldn't add that. Object.freeze causes silent failures which I expect would be hugely problematic to debug here -- unless you're in strict mode, in which case it raises exceptions, but Moment APIs aren't supposed to raise exceptions. Also, Object.freeze is generally quite slow. If we care about cloning performance, then freezing seems like a non-starter.

I suppose we could only do the freezing in (some or all) tests, but I think maybe we just write a new testcase to prove that everything still works if the user freezes the moment, and call it a day.
Sure, will do.
Because I removed updateOffset from the constructor (so that it doesn't run during internal cloning), and it still needs to happen somewhere when users call moment(). details
You might disagree, but I feel like any prototype methods (even if private) should be immutable. Even if we slap everyone's wrist every time they do it, people will call prototype methods and then come looking to us for support, and I feel like our job is easier if we know that they can't run any of our mutating code.

So, I'd be happy to keep this mutating if we pull it off the prototype; otherwise my instinct is to make it immutable. I suppose that means I should import the raw mutable implementation into the constructor and use that instead of the current property-copying hack, regardless of whether we keep it on the prototype or not.
Oops, will fix.

6-11. Ooh, good point, I should use #3134 as a starting point. Without updateOffset in the mix, I should be able to keep everything internally mutable, more like the plugin. (During this work, I realized that my plugin presumably never worked properly with updateOffset / Moment Timezone.)

-12. I did all of these tests by hand, and I don't have a better system in place for porting locales and their tests. I'm tempted to try writing a script to do simple code rewriting (assigning the result of statements that have mutation methods back to the original variable name) and see how far that gets us, but I haven't done that sort of coding in JS before. Suggestions welcome here.

Re: "I'm a bit lost" -- because updateOffset, which shouldn't be an issue if I move this on top of #3134. I assume that consistent internal mutability is preferable to consistent internal immutability for performance reasons, even if it forces contributors to worry about more things (e.g. they can't directly call prototype APIs internally), but let me know if you disagree. I'm willing to push this in whichever direction we feel is best for the library long-term.

maggiepint · 2016-11-06T14:38:38Z

One concern about moving this change on top of 3134 - code churn. It feels like that introduces a large number of changes at once. 3134 is a big deal.

If we go down that road, I would get 3134 shipped in moment 3.0, and follow about two or three months later with moment 4.0 being immutable. I'm okay with that if it's the right decision for the code, but I would not send those two things together.

ichernev · 2016-11-07T11:23:15Z

@butterflyhug just verifying -- the whole reason for the internal mutability is updateOffset? Now that I think about it -- its hard now, because update offset is called on every mutator and might mutate again. But I don't buy the performance reason. Right now we clone the whole moment object on every mutation just because we might mutate it with updateOffset. This should be much more inefficient. If updateOffset was made to return a new moment it will only happen very rarely, and all mutators can just create one copy, correct from the start.

I'd ship the two changes together. I disagree with @maggiepint because, the updateOffset needs to match the immutability in 3.0. So we can't just ship one without the other. We could ship only new TZ interface, but I do not consider it a big change. Because it is kind of internal API to moment-timezone. If somebody was using it -- he can't in an immutable version, so porting your code to immutability should include that small nuance (updateOffset).

butterflyhug · 2016-11-11T17:37:09Z

@ichernev Yes, I think we're basically saying the same thing in slightly different words.

With updateOffset, there are two possible end states:

Convert all of our code (including internal APIs) to immutable APIs. This gets us a consistently immutable codebase at the cost of more cloning internally (potentially ~10 clones when calling certain public methods, like startOf).
Keep the existing mutable APIs internally and wrap functions on the prototype to expose an immutable API to users. Whenever we need to call updateOffset, we copy the resulting moment's properties back into the original moment, thus preserving mutability internally while giving users an immutable updateOffset. This minimizes the algorithmic changes (although there'd still be a bit of code churn to use unwrapped APIs internally instead of calling mehods from the ptototype), at the cost of forcing contributors to think about and manage the mutable->immutable boundary.

Either way, updateOffset implementations won't be able to rely on adding new properties to the moment being updated anymore (e.g. these tests) -- but that's true for any of the immutable APIs so it's not really a surprise.

The current code in this PR is a middle ground between these two end states, because I was indecisive. You're right to flag the inconsistency as a major issue, so I should ideally bring the code into alignment with whichever path we'd prefer to maintain over the long term.

butterflyhug · 2016-11-11T17:43:11Z

And if we're willing to remove updateOffset and just ship the TZ interface from #3134, then that makes it a bit cleaner to keep a mutable API internally while wrapping the prototype for immutability.

ichernev · 2016-11-11T18:54:30Z

@butterflyhug why is the new api better for mut internals and immut api?

I think I like the first option more. The problem is some methods are slower but that's what people want.

The problem is, having the mutable methods around is very flexible, allowing us to do a lot of questionable magic, like auto-batch mutations (record mutations and batch exec them on read) or provide a mutable interface directly to speed things up.

Ideally we'd do one or both (your suggestions) and benchmark with existing code.

butterflyhug · 2016-11-12T04:49:35Z

@ichernev I guess maybe it isn't any better. I thought #3134 had deprecated updateOffset with the new Timezone stuff, but upon further review I guess I remembered that wrong.

I'll aim to take another pass at this within the next week or two.

ichernev · 2016-11-12T06:25:05Z

@butterflyhug I'm pretty sure if we introduce the new TZ api we need to get rid of updateOffset. Basically on every mutation we'll ask the library, and it will return the offset, which we need to store (but will probably be the same as the old one). It just has a cleaner API.

If we're to have half our methods mutable internally, half immutable then we need veery good guidelines on what and where needs to be mutable and try to decrease that to a minimum. And possibly a package (directory) inside lib for all mutable stuff. But again, I think this will cause huge butthurt in the future.

butterflyhug · 2016-11-12T14:32:31Z

@ichernev Right, okay. That's what I initially thought. The Timezone interface would be better for mutable internals and immutable API. Moment-internal code would ask the Timezone for the correct offset and update the Moment instance when appropriate.

The problem with the updateOffset API is that we have external code (from Moment Timezone) that needs to edit a moment in the middle of the internal mutable code path. That's fundamentally at odds with building a clean internal=mutable / external=immutable boundary. Of course we can hack around the issue, but it's cleaner if we don't need to.

maggiepint · 2016-11-12T15:01:30Z

Friendly reminder: the moment-timezone code to make the TZ interface work has not yet been written :-)

ichernev · 2016-11-12T16:12:46Z

@maggiepint it hasn't. But I fail to find a better interface for updating the offset. So more or less it will be what the rfc says. If you have another idea that will magically solve the issue at hand I'm all ears.

moment#clone() is now a deprecated no-op, because immutability.

The bubble function now returns a new data object instead of mutating the duration's existing data (which had always been empty when we called _bubble anyway).

Also make the Duration#add and Duration#subtract implementations internally immutable (so we don't need to wrap them for the prototype).

Also ensure that we always test both the 11:00 hour and the non-11:00 formatting variations, instead of simply testing whichever hour it happens to be when we run the test.

butterflyhug · 2017-03-22T11:24:49Z

Okay, I've updated this code and it should read a bit better now. For internal consistency, all prototype methods now have immutable implementations instead of using a Frozen Moment-style wrap function in the prototype definition. All existing tests are passing, and I've added a few additional tests for good measure.

I did not remove old deprecated functions here; that will be a quick and easy change for 3.0, but I think it belongs in a separate PR. This is also still using updateOffset instead of the new Timezone API. Otherwise I think I've addressed the bulk of the feedback on this PR -- let me know what you think!

I haven't written any new benchmarks to test things thoroughly, but based on existing resources, the performance impact seems to be relatively small:

Our existing benchmarks seem to be roughly the same speed as before. Obviously this is far from all-inclusive, but it's a promising result. Amusingly, the big outlier is that this PR's no-op clone is dramatically slower than before -- apparently our deprecation warnings are relatively slow.
Anecdotally, running the entire test suite is between 0.5 and 1 seconds slower (~4-8%) on my machine with this code than v2.18.1, although there's a lot of variance from one run to the next so this is a very rough estimate. I suspect this is a rough upper bound on the actual performance changes in our test suite (when running in Node), given that I've added a few more tests and deprecation warnings.

butterflyhug · 2017-03-22T11:25:42Z

Oh, and this is rebased on top of last night's v2.18.1 release.

ichernev

When I was reviewing two things popped to my mind:

we need to provide an interface for users to check if the moment is mutable or not (because with 3.x and 2.x out in the wild it will be hard to tell)
we need to do somehow test that moment is actually immutable (that is -- all functions don't change anything). Even if it is now, we'll have trouble keeping it this way just by "looking at the code".

The one thing that still bothers me with the code itself is the way we make changes to internals, just after clone in a bunch of methods (places where we call new Moment(...)). In the places where we modify _d, couldn't we just take _d, clone it, modify it and then create a moment with it -- so modify before clone, not after. Now this is dangerous because could mess something up, when its spread to many places (12 right now). On the other hand we can ship this way and work our way toward all-mutability-in-constructor in future (not necessarily major) versions.

ichernev · 2017-04-30T20:22:18Z

src/lib/units/offset.js

+            if (!keepLocalTime || mom._changeInProgress) {
+                mom = addSubtract(mom, createDuration(input - offset, 'm'), 1, false);
+            } else if (!mom._changeInProgress) {
+                mom._changeInProgress = true;


This _changeInProgress was there because of mutability. Now I don't think its necessary. Or maybe it is necessary, but we should put it on the new moment, so it want fall into a cycle. Maybe that is related to the other piece of cycle-break-check in the constructor.

Oh, as written this is a bug! I agree that we probably don't need it anymore; I'll try removing it. Good catch!

We actually need to implement the the new moment-timezone that handles both 2.x an 3.x moments.

ichernev · 2017-04-30T20:24:03Z

src/lib/units/offset.js


        if (keepLocalTime) {
-            this.subtract(getDateOffset(this), 'm');
+            ret = ret.subtract(getDateOffset(ret), 'm');


I don't see an issue about intermediate copies. In general, with immutable API, how do you avoid them?

ichernev · 2017-04-30T20:26:38Z

src/lib/moment/constructor.js

-        updateInProgress = true;
-        hooks.updateOffset(this);
-        updateInProgress = false;
-    }


I'm not sure I follow you here (maybe this comment is outdated). I'm left with the impression that there is no "wrap" any more.

ichernev · 2017-04-30T20:31:13Z

src/lib/moment/locale.js

        newLocaleData = getLocale(key);
        if (newLocaleData != null) {
-            this._locale = newLocaleData;
+            clone._locale = newLocaleData;


I get a bit uneasy when reading this. Sure, its a new clone, but I can see a bug slipping through if one is not very careful. Would it be possible to cram all the "mutability" inside the constructor, so it gets all arguments and produces a frozen (maybe not literally) moment.

Sure, I'll take a stab at that.

ichernev · 2017-04-30T20:35:42Z

src/lib/moment/start-end-of.js

    // the following switch intentionally omits break keywords
    // to utilize falling through the cases.
    switch (units) {
        case 'year':
-            this.month(0);
+            m = m.month(0);


I'd say this particular function could use a lower level API to avoid unnecessary cloning. Because resetting all units lower than a given unit is the most expected operation dealing with multiple units and it would be nice if it was fast. Of course we can ship this way, and fix (improve) later.

ichernev · 2017-04-30T20:44:00Z

src/lib/units/priorities.js

+            value: unitsObj[u],
+            priority: priorities[u].priority,
+            getSet: priorities[u].getSet
+        });


Yes, this code was a bit too "advanced" :)

ichernev · 2017-04-30T20:49:54Z

src/lib/units/offset.js

@@ -196,8 +203,8 @@ export function hasAlignedHourOffset (input) {

 export function isDaylightSavingTime () {
    return (
-        this.utcOffset() > this.clone().month(0).utcOffset() ||
-        this.utcOffset() > this.clone().month(5).utcOffset()
+        this.utcOffset() > new Moment(this).month(0).utcOffset() ||


why is this needed? Can't we just .month(X).utfOffset()?

Hmm, yes, I think this is a stale change. I'll simplify this as you suggest.

ichernev · 2017-04-30T21:17:38Z

One more thing before I forget -- we might need allow some APIs for plugins to hook into the constructor, because if they need to keep a piece of data with moment, now that its immutable it needs to be in the constructor, but we don't have any hooks for that. Or maybe plugins don't need to keep data in moment, or maybe the data should be only lazily computed (and keep moment at least observably immutable).

butterflyhug · 2017-05-02T12:16:04Z

I think it's probably best for plugins to lazily compute values. It's possible for someone to create a moment instance before loading all their plugins, so ideally plugins will need to handle the case where they didn't exist when the constructor ran anyway. I suspect they're more likely to do it correctly if we make that the normal case. 😉

Also, I'm sure we'll still be lazily computing a few things in the core library unless we do a rigorous code audit specifically aimed at moving all lazy computations into the constructor. As long as nobody ever updates existing values (and all new values are derived solely from the moment's existing data), I think it'll be fine.

That said: we/I should probably write a guide to help plugin authors to update their code, and this would be a good issue to highlight in that doc.

butterflyhug · 2017-05-02T12:20:13Z

Also: the proposal for distinguishing immutable from mutable moments was to add a version property to the instance prototypes, to supplement the existing moment.version global. I'll try to take a pass at adding that and addressing your other comments later today.

Thanks for the thorough review, @ichernev!

ichernev · 2017-05-02T13:03:14Z

I'd rather add moment.isImmutable property. With version you have to split and compare and so on.

I didn't think too much but we should be able to hack some code that verifies all moment properties did not change after each function (for tests only). And we need to consider lazily computed props and not fail if those change.

I added moment.isImmutable and fixed unnecessary cloning.

muescha · 2019-08-27T21:19:52Z

just note for someone who come over this PR:

the referenced #1754 is closed in favour of Luxon with is immutable

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

icambron · 2019-08-28T04:09:21Z

Yeah, I think we can safely close this.

muescha · 2019-08-29T21:40:29Z

Maybe we add a note to the moment readme to point to LUXON?

The not-immutable was the first pain point with the momentjs for me so it would be nice to know that exist a new improved tool for it

butterflyhug force-pushed the immutability branch from ec9b92b to c872241 Compare October 30, 2016 18:28

butterflyhug commented Oct 30, 2016

View reviewed changes

butterflyhug force-pushed the immutability branch from c872241 to a1dea22 Compare March 10, 2017 04:09

butterflyhug force-pushed the immutability branch 2 times, most recently from f6104a5 to e65ca1d Compare March 19, 2017 04:32

butterflyhug added 3 commits March 22, 2017 06:12

Fix tests to avoid moment#clone() unless explicitly testing #clone()

111a2fd

moment#clone() is now a deprecated no-op, because immutability.

Fix hooks.updateOffset to return an immutable Moment

aacc6bf

Remove _bubble() from Duration prototype and make its API immutable

65352e0

The bubble function now returns a new data object instead of mutating the duration's existing data (which had always been empty when we called _bubble anyway).

butterflyhug force-pushed the immutability branch from e8fedc0 to ab03657 Compare March 22, 2017 10:14

butterflyhug added 10 commits March 22, 2017 07:00

Construct immutable Durations

ac0caac

Test fixes for immutable API

e7eb1f3

Fix code style

eaad776

Fix Duration#isValid after #add or #subtract

fdde971

Also make the Duration#add and Duration#subtract implementations internally immutable (so we don't need to wrap them for the prototype).

Immutable Locale#set

9834aaa

Immutable internal implementations of Moment#locale and Duration#locale

13e159c

Finish removing wrap() in favor of immutable internals

cb7dcf0

Add a test for Duration.abs() and make it immutable

a488f4c

Update Ukrainian locale tests for immutability

4b98a63

Also ensure that we always test both the 11:00 hour and the non-11:00 formatting variations, instead of simply testing whichever hour it happens to be when we run the test.

Add more mutability tests

7c0d219

butterflyhug force-pushed the immutability branch from ab03657 to 7c0d219 Compare March 22, 2017 11:00

benesch mentioned this pull request Mar 24, 2017

ui: Liveness info on Nodes List page cockroachdb/cockroach#14258

Merged

ichernev reviewed Apr 30, 2017

View reviewed changes

Address some review comments

9d535ff

I added moment.isImmutable and fixed unnecessary cloning.

marwahaha added Breaking change Discussion labels Dec 22, 2017

icambron closed this Aug 28, 2019

		@@ -0,0 +1,11 @@
		export default function wrap(Type, fn, dontCloneWithNoArgs) {

Immutable API #3548

Immutable API #3548

Conversation

butterflyhug commented Oct 30, 2016 • edited

ichernev commented Oct 30, 2016

butterflyhug commented Oct 30, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

butterflyhug Oct 30, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maggiepint commented Oct 30, 2016

maggiepint commented Oct 30, 2016

butterflyhug commented Oct 31, 2016

ichernev commented Nov 3, 2016

butterflyhug commented Nov 6, 2016

maggiepint commented Nov 6, 2016

ichernev commented Nov 7, 2016

butterflyhug commented Nov 11, 2016

butterflyhug commented Nov 11, 2016

ichernev commented Nov 11, 2016

butterflyhug commented Nov 12, 2016

ichernev commented Nov 12, 2016

butterflyhug commented Nov 12, 2016

maggiepint commented Nov 12, 2016

ichernev commented Nov 12, 2016

butterflyhug commented Mar 22, 2017

butterflyhug commented Mar 22, 2017

ichernev left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

butterflyhug May 2, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ichernev commented Apr 30, 2017

butterflyhug commented May 2, 2017

butterflyhug commented May 2, 2017

ichernev commented May 2, 2017

muescha commented Aug 27, 2019

icambron commented Aug 28, 2019

muescha commented Aug 29, 2019

butterflyhug commented Oct 30, 2016 •

edited

butterflyhug Oct 30, 2016 •

edited

butterflyhug May 2, 2017 •

edited