New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add global meta locale #1797

Open
icambron opened this Issue Jul 24, 2014 · 15 comments

Comments

Projects
None yet
8 participants
@icambron
Member

icambron commented Jul 24, 2014

Continued from #1761, we should have a way for moments to be "pinned" to the global locale, so that they'll change with global setting. Something like:

moment.locale('global');

@icambron icambron added this to the 3.0 milestone Jul 24, 2014

@butterflyhug

This comment has been minimized.

Member

butterflyhug commented Jul 26, 2014

If you create a moment instance with moment().locale('global') and then call the instance.locale() getter, will you expect that to return 'global'? Or will that be the key for the currently set global locale?

@ichernev

This comment has been minimized.

Contributor

ichernev commented Jul 26, 2014

I guess there should be a isGlobalLocale function, and the rest will behave like a regular locale.

@icambron

This comment has been minimized.

Member

icambron commented Jul 26, 2014

I agree with @ichernev

@cubitworx

This comment has been minimized.

cubitworx commented Jul 19, 2016

@ichernev @icambron The suggestion of isGlobalLocale (per instance) will not work for some. In my case I am using 3rd party libraries over which I have no control how they instantiate moment. They currently call moment the old way which means that each component on my page does not conform to a global locale because of this breaking change.

If you do implement isGlobalLocale I would recommend that it is a global setting so that if it is set to true all instances of moment adhere to the global locale - i.e. you don't have to set isGlobalLocale for each instance you create but only once for all moment instances.

I really cannot believe that this is not the highest priority fix. It is not a new feature considering that it is a breaking change from a previous minor version (at 2.8.1). It is a critical breaking change for some like me because there is currently no work-around. Having it the way it is now makes it totally impractical for me to implement a dynamic globally controlled locale. E.g. in cases where the default locale is superseded when a user has chosen a different locale in an SPA for reasons as stated above.

@icambron

This comment has been minimized.

Member

icambron commented Jul 19, 2016

@cubitworx On having a global flag that automatically pins new Moments, I think that would be a reasonable addition.

Thinking about it a bit more, perhaps the best API is more explicit:

moment().pinLocale();
moment().isLocalePinned(); //=> true
moment.pinLocales = true;

I really cannot believe that this is not the highest priority fix.

Perhaps you'd like to submit a pull request?

@cubitworx

This comment has been minimized.

cubitworx commented Jul 20, 2016

@icambron

Perhaps you'd like to submit a pull request?

I will consider submitting a pull request but I honestly don't know if my JS knowledge is strong enough yet for a collaboration like this. I imagine it will take me hours just to work out where the change should be applied let alone how I would do it and I don't really have time for that at the moment. I am happy just pinning my version to pre-2.8 & hope like hell that no library I use decides to pin itself to a newer version of moment & that I don't need any new features.

I only say 'I cannot believe' because this is such a widely used library and that bringing in a change that breaks backwards compatibility at the minor revision level could potentially affect thousands of people who rely on semver being adhered to ;)

The hiccup for me is probably that most moment users are likely to be US based, developing only for US clients and don't mind the default US formats. What were they thinking putting the month first ;P

@cubitworx

This comment has been minimized.

cubitworx commented Jul 20, 2016

@icambron As for your suggestion on how to do it. Are you implying:

moment().pinLocale(); // pin locale only for this instance
moment().isLocalePinned(); // check if locale is pinned only for this instance

moment.pinLocales; // pin locale globally for all instance

If so then I'm not sure why this complexity is needed?

My thinking would be as follows...

moment.locale('xx'); // sets the global locale

moment().locale('xx'); // set the locale only for that instance
moment(); // locale is pinned to global locale because locale() was not called

moment.locale('xx'); // calling this here will only affect the second moment instance
                        because it is pinned to the global locale

This means that all new moment instance conform to the global setting unless you explicitly set it for that instance.

You could also if you really wanted to have the moment.isGlobalLocale you were talking about to determine if the locale should by default be pinned to global or not.

Am I missing something?

@icambron

This comment has been minimized.

Member

icambron commented Jul 20, 2016

That's almost what I'm implying, except for the last one:

moment.pinLocales = true; //pin locales for all *newly created* instances

i.e. imagine the Moment code:

//creating a new moment
var m = howeverAFreshMomentIsCreatedInternally();
if (moment.pinLocales){
  return m.pinLocale();
}
else {
  return m;
}

(Maybe "pin" is the wrong word -- maybe it's more the opposite, like "float".)

Then you could just set moment.pinLocales = true right after you imported it. The reason for all of this is that we strongly prefer the current behavior by default:

moment.locale('de');
var m = moment();
m.locale(); //=> 'de'
moment.locale('zn');
m.locale() //=> 'de'

I know it used to work the other way, but it was bad for most people most of the time. Moment was effectively reaching in and messing with existing Moment instances, which is weird and counterintuitive. We changed it very consciously; it is much easier to think about the global locale as a flag being set on a factory. It's been that way for almost two years and you are something like the third person to complain (and we have a lot of users who use the locale features; it makes up a significant portion of the support pain for Moment).

I get that there's a use case where you have an SPA in a specific state and you want to simply toggle some global flag and all of the Moments magically update to the new locale. That's why we're willing to consider an optional setting for it. But the current behavior needs to remain as-is.

@butterflyhug

This comment has been minimized.

Member

butterflyhug commented Jul 21, 2016

In other words: most people experienced the 2.8 regression as a bug fix rather than a new bug. It'd be nice to support both behaviors, but it's not worth creating a new minor-rev regression from the perspective of anyone who wrote code post-2.8, so the old behavior needs to come back as an option that's off by default.

That's doubly true because the post-2.8 locale behavior fits even more intuitively with the new or changed APIs that will result from addressing #1754 in 3.0.

@iamrandys

This comment has been minimized.

iamrandys commented Sep 19, 2016

Having an option to set this would be great. Let me know what I can do to get this done. We've been stuck on 2.7 for a long time now.

@remmeier

This comment has been minimized.

remmeier commented Feb 2, 2017

something like

moment.locale('xx'); // sets the global locale

would really be helpful. I'm stuck with 2.8.1 as well as I make use of third-party library and not able to set the locale for each any every instance.

@icambron

This comment has been minimized.

Member

icambron commented Feb 2, 2017

@remmeier to be clear, moment.locale('xx') very much works and sets the locale of all instances created thereafter. What it doesn't do is change the locale of existing instances.

@remmeier

This comment has been minimized.

remmeier commented Feb 3, 2017

well that is currently the reason why I'm stuck with 2.8.1. I make use of Angular 2, Dependency Injection, AoT and moment-based third-party libraries. It is not that easy to make sure my own code is the first one touching moment.js to set the global locale. It likely is possible, but a bit cumbersome, takes time and is a source of confusion. It would be good if the current global locale is used as long as it is not explicitly set of a given instance, regardless of the time of instantiation. Resp. a flag to get back to the 2.8.1 behavior.

@zorgoz

This comment has been minimized.

zorgoz commented Sep 7, 2017

Locale is used to display data in a specific format. But data is usually created way before it is displayed. Switching the view to an other language does not imply that the data is recreated. Why should it? This means that, if there is a way to create moment object with a specific locale, changing the locale globally should affect all already existing objects, that do not have definite locale. At least there should be a way to do this even if not by default for some backward compatibility. At the moment this is not the case.

moment.locale('en');
var m1 = moment();
m1.locale(); // en 
moment.locale('hu');
var m2 = moment();
m2.locale(); // hu
m1.locale(); // en !!!

I see that this is not a new issue, still open. Please keep in mind that SPA-s are emerging, and they explicitly avoid refreshing views in the legacy way. The need to recreate already created moment objects is really not the way...

@marwahaha

This comment has been minimized.

Member

marwahaha commented Apr 13, 2018

I don't think this requires a major version change, but it's a cool feature. Up for grabs :-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment