whoopinc / frozen-moment-old Goto Github PK

Timezone offset functionality is broken, probably because the pluggable updateOffset function is basically an expensive noop -- it wants to do mutation on a FrozenMoment. (That function should move to the builder API, but I need to get a better understanding of the relevant code paths to figure out exactly how that should happen.)

Not sure how this should work. Current idea:

• A global setting is used to write a locale selection into every FrozenMoment as it is created. Changing the global locale doesn't change the locale for existing objects.
• Object-specific locales can only be changed by thawing and re-freezing the object -- that is, an object's locale state is immutable just like everything else.
• All FrozenMoment methods that use the locale accept a locale key (e.g. "en-gb") as an optional method parameter that overrides the locale setting for just that call.

I think this proposal feels nicely consistent with the rest of the immutable API, but I'm not sure how it'll feel in use, and we at WhoopInc aren't currently using moment localization so I won't quickly gain much experience to drive my instincts on this issue.

Same idea as the Moment -> FrozenMoment conversion.

Format functions [1] should accept an optional locale key. If provided, the specified locale overrides the one associated with the instance itself, for just one formatting operation.

See also #12, we may want an optional options hash with timezone and locale as possible options.

[1] For this purpose, "format functions" are frozenMoment.format(), frozenMoment.calendar(), and anything else sensible in the "Display" section of the docs.

@icambron's speed improvements look awesome -- we should merge with our code when his PR lands upstream.

Format functions [1] should accept an optional locale key. If provided, the specified locale overrides the one associated with the instance itself, for just one formatting operation.

Related to #2. Could be a port of moment#1621. See also #13, we may want an optional options hash with timezone and locale as possible options.

[1] For this purpose, "format functions" are frozenMoment.format(), frozenMoment.calendar(), and anything else sensible in the "Display" section of the docs)

Up to now, we've been copying Moment's forward-looking locale API.

That Moment API has a setter and getter for a global locale setting. These can be used to change the behavior of the parsing functions at any time, and Moment instances can be bound to this global setting so that their locale changes whenever the global setting changes.

Thing is, the very idea of having global state that's changeable at runtime is a very poor fit for a library where immutability is the primary selling point. As a result, we will be sharply curtailing the scope of FrozenMoment's global locale setting.

Our revised API will focus on enabling developers to configure a non-English default locale for all new FrozenMoments created in a particular runtime environment. (A runtime environment is a web page in the browser, or a Node application.) Applications that need to change locales at runtime should always set the locale on their FrozenMoment instances, and/or override each instance's locale setting for each formatting operation.

Here's a minimal set of required API changes:

• Refuse to set the global locale with frozenMoment.locale() after instantiating an object using that setting (whether a FrozenMoment, a Duration, or a builder for either type). Complain loudly in the console logs when the setter fails for this reason.
• Return a truthy value from the global locale setter when it succeeds and a falsy value when it fails. (That success value should probably the key of the newly selected global locale, because we can set with an array of locales and we may also coerce the provided value to a more general locale key.)
• Since we're breaking API compatibility with Moment, rename the global locale setter to frozenMoment.setDefaultLocale() (or something similarly appropriate).
• Remove the getter for the global locale -- but let's keep the global localeData getter, at least for now.
• Update the documentation to reflect these API changes.

(This follows up on observations from PR #9, as part of my continued work on issue #2.)

instance.thaw().mutate().freeze() is kind of wordy if you just want to generate a new FrozenMoment while applying a single mutation method on the builder.

If we offer a convenience syntax for this, I'm currently biased toward something like instance.copy.mutate() instead of replicating moment's current API, because I think it'll be confusing if we implement a moment API method name with different semantics. (Especially for setters.) Note that multiple mutations with this syntax would be instance.copy.mutate().copy.mutate() -- I feel like that would start to push people back toward the more efficient builder syntax for those use cases, which also seems like a feature of this approach.