s/lang/locale #1761

icambron · 2014-07-05T08:55:33Z

Deprecated moment.lang(), moment.langData(), moment#lang, and
duration#lang
Added moment.locale(), moment.localeData(), moment#localeData, and
druation#localeData
Added moment.defineLocale() that defines a locale but doesn't set it
as the global locale (Instance-level locale leaks to other instances #1750)
Removed the hack in the language concatenator that set the language
to English.
Refactored internal code to use moment#localeData instead of local
functions, and set the language directly on the moment instance at
creation.
Moved all the files and changed the build scripts so that everything
lives is named "locale" instead of "lang", e.g. the locale files under
the "locale" directory.
I did not include build-generated changes like component.json and
the concated locale files, but I did inspect them to see that they look
right. But I did remove the old ones wherever they won't be named correctly in the future.

icambron · 2014-07-05T09:04:45Z

moment.js

+        lang : deprecate(
+            "moment().lang is deprecated. Use moment().locale instead.",
+            function (key) {
+                return this.locale(key);


Looks like this should actually return localeData() to be keep the old API, so I need to fix that. The new one, where moment().locale() just returns the name, but moment().localeData() returns the Locale instances, is more consistent with the behavior of the library-level functions, so this is just a matter of changing what the deprecation does.

That makes sense.

icambron · 2014-07-06T18:43:08Z

Looks like I'll need to rebase. I'll do that after I get some feedback.

ichernev · 2014-07-08T07:43:58Z

test/moment/npm-debug.log

@@ -0,0 +1,18 @@
+0 info it worked if it ends with ok


I don't think we need this file :)

ichernev · 2014-07-08T08:01:25Z

Looks great. You can merge after the rebase.

* Deprecated moment.lang(), moment.langData(), moment#lang, and duration#lang * Added moment.locale(), moment.localeData(), moment#localeData, and druation#localeData * Added moment.defineLocale() that defines a locale but doesn't set it as the global locale (moment#1750) * Removed the hack in the language concatenator that set the language to English. * Refactored internal code to use moment#localeData instead of local functions, and set the language directly on the moment instance at creation. * Moved all the files and changed the build scripts so that everything lives is named "locale" instead of "lang", e.g. the locale files under the "locale" directory. * I did *not* include build-generated changes like component.json and the concated locale files, but I did inspect them to see that they look right.

icambron · 2014-07-14T05:17:41Z

@ichernev Fixed except for the one additional comment I made.

ichernev · 2014-07-16T06:56:15Z

OK, so lets figure out the case with the default language:

If you don't include any language files

Use English. Not that you can use anything else

If you have included only one language file

Option 1: Use that language
Option 2: Use English and set language manually (breaks existing behavior and is kind of unfriendly)

If you have included more than one file

Option 1: Use last included language (consistent with the case with one language)
Option 2: Use English (existing UX, if you're using the bundle or manually bundle a few files). I also think it makes more sense.

To make everybody happy we can have a flag just_load: true, that you pass to the module, so it won't be set as default. It can be used by the bundled script, and by hard core users. For everybody else it will set as default. To make it backwards compatible, it will always bundle (but remember old setting), and return a function for unbundling (if you pass just_load: true).

What do you think?

icambron · 2014-07-21T04:28:04Z

I definitely think that if we pick option 2, we have to pick it in both cases. Otherwise it's too complicated to explain.

I'm not sure I followed that last part:

To make it backwards compatible, it will always bundle (but remember old setting), and return a function for unbundling (if you pass just_load: true).

I guess that means this?

var backToOldLocale = require('locale/fr', {just_load: true}); //or however you pass options to require
moment.locale(); //=> 'fr'
backToOldLocale();
moment.locale(); //=> 'en'

If so, I'm not a huge fan:

How does it work in AMD?
The function only gets used in really specific places, but I get a bit queezy when you keep around closures with side effects like that.
The instance-level locale() setter uses just_load in its require() and then calls the unbundler right away? That's a bit odd, but I guess it's OK, so nevermind this point.
Does this require code in every locale file? I think it would.

I don't think there's anything really bad about making the user call moment.locale() in all cases. I think it's a lot better than random files trouncing the settings. We just need to be good about communicating the breaking change.

…eturn statement in the locale definitions.

icambron · 2014-07-23T03:55:05Z

@ichernev Removed those tests.

icambron · 2014-07-23T04:37:45Z

@ichernev another issue. From your comment in #1750:

moment.lang('fr');  // sets the default for all newly created moments
                    // unless a language has been explicitly specified
                    // in the constructor

That is also intuitive to me and how I made this PR behave without even thinking about it. But I just realized from the discussion and tests in #1785, that that's actually not how moment is designed to behave; moment.lang('fr') sets all existing moments to French if they haven't been explicitly set.

> var moment = require('./moment');
> var inEnglish = moment()
> moment.lang('fr')
'fr'
> inEnglish.lang()._abbr
'fr'

We don't have great tests for that, but it now occurs to me that the code (before this PR), goes out of its way to ensure that, and our cloning logic even takes it into account.

It sort of makes sense; we're setting the language on the prototype, and it's thus being inherited everywhere. And if you're programmatically switching languages in an entire UI, you could conceivably want to set all of the locales on all of the moment objects you have sitting around as UI state. But in another sense it's totally wack, because all this prototype stuff is hidden away as implementation details, and they don't have a way to tell between moment() and moment().locale('en'). It just seems like we're mucking with all their objects. And it's unexpected enough that two of the maintainers didn't really think about it.

In this PR:

> var moment = require('./moment');
> var inEnglish = moment()
> inEnglish.locale()
'en'
> moment.locale('fr')
'fr'
> inEnglish.locale()
'en'

To me, that's on balance an improvement. But it is a big breaking change. What do you think?

/cc @timrwood, who probably has a better perspective on this.

timrwood · 2014-07-23T05:20:59Z

/cc @timrwood, who probably has a better perspective on this.

Originally, the locale was just a global option you could set. It wasn't until 1.7.0 that you were able to set a locale on a instance rather than as a shared global.

I don't know if there is much of a use case for creating a moment without a locale and then expecting it to stay up to date with the global locale when the global locale changes. Looking back on it, it does feel a bit unexpected.

I think an additional deprecation note in the 'upgrade to 3.0' changelog would suffice.

icambron · 2014-07-23T06:06:50Z

I agree.

As as an aside, if we make that change, we should move the global setting from moment.fn to moment, since the prototype won't be of use anymore.

ichernev · 2014-07-23T06:45:45Z

just_load

OK, first about the just_load flag. The problem is people are requiring language files and expecting this to change the default language, so having them explicitly call locale sounds like a breaking change to me (not that bad after all).

I've seen other libraries do it more explicitly -- you take the main library, you take the language module, and then you tell the library to use the module. In this case you have full control over how exactly to load it (should it also switch or not).

So now the options are:

if you load a language, also set that language (existing behavior). I'd still split the code into two functions, now at least it would be clear from reading the lang file that it also sets the default.
make all language files not set the default (breaking change) -- but this should wait for 3.0. It will be a change in defineLocale. I guess we can add a warning if you create a moment without setting a locale manually first

lang existing behavior

To be honest, I never really understood how that worked until now. I've tried to follow the code a few times and every single time I gave up as it didn't make much sense (and I didn't have enough time). So I think changing it is something we can do, and it shouldn't break a lot of workflows. Also about the change being implemented with prototype -- it should be easy to enable passing global as the language which would mean pick the global, whatever it is. So if you really want you can create transparent-language-moments but the default would not be that.

The problem is this PR is already pretty big, and if we have to change it back to prototype-hack-global-lang and then again after 3.0 -- makes no sense. Now we use .localeData() everywhere, so it should be easy to add the global option. For those using moment in super-reactive environments like angular, making the public facing moments use global lang, so you can change the locale dynamically sounds like a sane thing. For everybody else it should be a no-op.

ichernev · 2014-07-23T06:48:31Z

Yeah, definitely use moment instead of moment.fn to store the global. Also duration objects should behave the same way as moment objects, not sure what is the story there.

icambron · 2014-07-23T07:37:01Z

I'd prefer that the user sets the language explicitly; I don't like that the behavior of my code depends on things like what order I put the JS in, and the whole thing feels flimsy and non-obvious. But I do see that it's a real breaking change, so I can put in some hacks for now.

So my proposal on all things:

Move the global setting from moment.fn to moment
Just break compatibility for people who used the locale switcher to update existing moments on the fly and add tests accordingly (for durations too)
Make defineLocale additionally set the locale
Make sure the lazy loader unsets it so that 3 doesn't break our fix for Instance-level locale leaks to other instances #1750.
Revert the removal of the en hack in the moment-with-locales file.
Write a ticket to remove 3-5 for 3.0
Add global meta-locale as a separate PR

Sound good?

ichernev · 2014-07-23T07:47:05Z

That makes sense. I hope its not too different than the current diff :)

icambron · 2014-07-24T22:27:02Z

@ichernev Done!

ichernev · 2014-07-25T06:04:30Z

Looks great. Merging!

s/lang/locale

icambron mentioned this pull request Jul 5, 2014

Instance-level locale leaks to other instances #1750

Closed

icambron reviewed Jul 5, 2014
View reviewed changes

ichernev reviewed Jul 8, 2014
View reviewed changes

test/moment/npm-debug.log

@@ -0,0 +1,18 @@

0 info it worked if it ends with ok

Copy link

Contributor

ichernev Jul 8, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we need this file :)

icambron added 2 commits July 14, 2014 00:46

incorporating ichernev comments

35dbc13

butterflyhug mentioned this pull request Jul 21, 2014

Do something about locale translation modules WhoopInc/frozen-moment-OLD#2

Open

defineLocale() now returns the actual locale. Removed tests for the r…

e1394ac

…eturn statement in the locale definitions.

Fix deprecation warning on moment().locale()

677db20

icambron mentioned this pull request Jul 23, 2014

Add the ability to set the formatting language as part of the format method. #1621

Closed

icambron mentioned this pull request Jul 23, 2014

Optimize clone, 49x improvement #1785

Closed

some reverse compatibility

b09888a

This was referenced Jul 24, 2014

Fix up defineLocale for 3.0 #1796

Closed

Add global meta locale #1797

Closed

icambron added 2 commits July 24, 2014 18:31

fixing tests

1c7022f

fix jsc error

42af025

ichernev added a commit that referenced this pull request Jul 25, 2014

Merge pull request #1761 from icambron/language_rework

ec5c9d2

s/lang/locale

ichernev merged commit ec5c9d2 into moment:develop Jul 25, 2014

butterflyhug mentioned this pull request Jul 27, 2014

s/lang/locale WhoopInc/frozen-moment-OLD#9

Merged

ichernev mentioned this pull request Jul 29, 2014

Convert 4 remaining languages to locale #1807

Merged

kaelig mentioned this pull request Aug 6, 2014

Update npm and ruby dependencies guardian/frontend#5393

Merged

kkirsche mentioned this pull request Feb 10, 2015

Update Moment.js to from 2.0 -> 2.9 rubyloco/rubyloco.com#14

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

s/lang/locale #1761

s/lang/locale #1761

icambron commented Jul 5, 2014

icambron Jul 5, 2014

ichernev Jul 8, 2014

icambron commented Jul 6, 2014

ichernev Jul 8, 2014

ichernev commented Jul 8, 2014

icambron commented Jul 14, 2014

ichernev commented Jul 16, 2014

icambron commented Jul 21, 2014

icambron commented Jul 23, 2014

icambron commented Jul 23, 2014

timrwood commented Jul 23, 2014

icambron commented Jul 23, 2014

ichernev commented Jul 23, 2014

ichernev commented Jul 23, 2014

icambron commented Jul 23, 2014

ichernev commented Jul 23, 2014

icambron commented Jul 24, 2014

ichernev commented Jul 25, 2014

s/lang/locale #1761

s/lang/locale #1761

Conversation

icambron commented Jul 5, 2014

icambron Jul 5, 2014

Choose a reason for hiding this comment

ichernev Jul 8, 2014

Choose a reason for hiding this comment

icambron commented Jul 6, 2014

ichernev Jul 8, 2014

Choose a reason for hiding this comment

ichernev commented Jul 8, 2014

icambron commented Jul 14, 2014

ichernev commented Jul 16, 2014

If you don't include any language files

If you have included only one language file

If you have included more than one file

icambron commented Jul 21, 2014

icambron commented Jul 23, 2014

icambron commented Jul 23, 2014

timrwood commented Jul 23, 2014

icambron commented Jul 23, 2014

ichernev commented Jul 23, 2014

just_load

lang existing behavior

ichernev commented Jul 23, 2014

icambron commented Jul 23, 2014

ichernev commented Jul 23, 2014

icambron commented Jul 24, 2014

ichernev commented Jul 25, 2014