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

Comments

Projects
None yet
5 participants
@joshuapinter

joshuapinter commented Apr 24, 2016

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

This comment has been minimized.

Member

maggiepint commented Apr 24, 2016

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

This comment has been minimized.

joshuapinter commented Apr 24, 2016

@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

This comment has been minimized.

Member

maggiepint commented Apr 24, 2016

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

This comment has been minimized.

joshuapinter commented Apr 24, 2016

@ichernev

This comment has been minimized.

Contributor

ichernev commented Jun 12, 2016

@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

Use proper locale inheritance for the base locale
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

Use proper locale inheritance for the base locale
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

Use proper locale inheritance for the base locale
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

This comment has been minimized.

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

This comment has been minimized.

laurent1 commented Jun 14, 2017

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