Skip to content
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

Using updateLocale with relativeTime doesn't merge default local. #3137

Closed
joshuapinter opened this issue Apr 24, 2016 · 7 comments · Fixed by #3235
Closed

Using updateLocale with relativeTime doesn't merge default local. #3137

joshuapinter opened this issue Apr 24, 2016 · 7 comments · Fixed by #3235
Labels

Comments

@joshuapinter
Copy link

For example, I just want to adjust a single relativeTime handler, like so:

moment.updateLocale('en', {
  relativeTime : {
    dd: 'only %d days'
  }
});

But this gives a Cannot read property 'replace' of undefined exception.

My guess is that this isn't overriding just the dd object but instead it overrides the entire relativeTime object.

You can get around this by providing all of the default locales, like so:

moment.updateLocale('en', {
  relativeTime : {
    future: 'in %s',
    past:   '%s ago',
    s:      'a few seconds',
    m:      'a minute',
    mm:     '%d minutes',
    h:      'an hour',
    hh:     '%d hours',
    d:      'a day',
    dd:     'only %d days',
    M:      'a month',
    MM:     '%d months',
    y:      'a year',
    yy:     '%d years'
  }
});

I can see from #2774 that there was an intention to use the default locale values and just update what you provide but I'm not sure if it made it as deep as the individual objects of relativeTime.

Any feedback on this would be great. I'd be happy to write up a PR if this is of interest to anybody as well.

Thanks!

Josh

@maggiepint
Copy link
Member

The actual issue here is that the code doesn't work on the en default locale. I think it actually works fine with any other locale, which is why this was missed in the unit tests.
This is because the en locale isn't actually defined in the locale files - instead it is comprised of a bunch of defaults.
You would notice that @ichernev actually has a todo on that pull request that you referenced to make a proper English base locale. If anything, I think that is what needs to happen to solve this issue. I think we would accept a pull request that does that, but I'd appreciate it if @ichernev chipped in with how he intended that to work.

This bug is actually a duplicate of #3121 that is manifesting in a different way.

@maggiepint maggiepint added the Bug label Apr 24, 2016
@joshuapinter
Copy link
Author

@maggiepint Thanks for the reply Maggie (and on a Saturday evening, no less)! :)

I'll hang tight to see what the verdict is but based on some Stack Overflow Q&As, I think it would be a welcomed mod.

@maggiepint
Copy link
Member

Out of plain curiosity, where is that coming up on Stack Overflow? Matt and I watch the Moment tag pretty closely and we would have raised this issue if we saw it.

@joshuapinter
Copy link
Author

@ichernev
Copy link
Contributor

@maggiepint you are right. If the whole system was based on the inheritance I added in the recent release it would work. But now its a bit of a mess still.

ichernev added a commit to ichernev/moment that referenced this issue Jun 12, 2016
Previously english locale was defined mostly in Locale.prototype. Now it's
moved in locale config (similar to all other locales) that is merged the same
way locales are patched (updated).

Fixes moment#3137
ichernev added a commit to ichernev/moment that referenced this issue Jun 16, 2016
Previously english locale was defined mostly in Locale.prototype. Now it's
moved in locale config (similar to all other locales) that is merged the same
way locales are patched (updated).

Fixes moment#3137
ichernev added a commit to ichernev/moment that referenced this issue Jun 20, 2016
Previously english locale was defined mostly in Locale.prototype. Now it's
moved in locale config (similar to all other locales) that is merged the same
way locales are patched (updated).

Fixes moment#3137
@NKjoep
Copy link

NKjoep commented Jun 1, 2017

I am using "moment": "^2.18.1" and updateLocale() doesn't work as expected.

I am still not able to replace only one property of the moment.LocaleSpecification.

Am I doing something wrong or this bug is still a bug?

@laurent1
Copy link

I don't think you do anything wrong. I have a similar experience.

  • This code moment.updateLocale('en', this.customize('en')); was merging the updates and all the dates were in English as expected.

  • This code moment.updateLocale('fr', this.customize('fr')); was merging the updates in French, but the rest of display was in English instead of the expected French.

  • So, I thought I should be specifying the language as followed moment.locale('fr); before customizing with moment.updateLocale('fr', this.customize('fr'));. The result was that everything was in French, but no merging occurred.

So, there is definitely a bug here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants