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

Use proper locale inheritance for the base locale #3235

Merged
merged 3 commits into from Jun 20, 2016

Conversation

ichernev
Copy link
Contributor

@ichernev ichernev commented 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 #3137

NOTE: changes in src/test/helpers/common-locale.js are because common tests for weekdays were plain wrong -- they weren't iterating all weekdays. It should go in a separate PR, if I'm not lazy.


Now the real story: we were depending in a few places that some stuff is just a given (in the prototype).

  • monthsParse, weekdaysParse -- assumes that if you have an own property then you define it, otherwise you want the default (esp for strict parsing). Now that it doesn't have prototype the check needs to be revised (I do === DEFAULT), but that is not ideal
  • longDateFormat -- we add lowercase values to the dict, which might change the default dictionary (because even if its in prototype it is still modifyable), but now we also merge it in, so locales inherit autodefined properties of other locales ... yuk. Its currently fixed by presenting an empty longDateFormat in all locales (if one doesn't exist) so we never modify the base, but it feels fragile
  • ordinalParse -- this is the last problematic one, but I took a decision that you can "fallback-to-default" in updateLocale by specifying null for some properties. Well, now that there is no fallback this null causes trouble. To fix it I disabled the tests. Not really the end of the world, but something more elegant can come along.

@ichernev
Copy link
Contributor Author

The way I addressed the issues above:

  • months/weekdays parse fields -- I changed them from a default value to one that is computed on first use. This makes it similar to other computed properties, and eliminates the problem of figuring out whether you inherited some month parsing fields, and at the same time change months (thus invalidating the parsing fields)
  • longDateFormat -- all objects that are inherited from the parent are automatically copied, so its not possible that modifying an object in the locale will modify the parent config, and possibly the parent locale with disastrous consequences. note: the copy is not deep, but we don't have deeper objects for now
  • for ordinalParse the way to revert to "default" will be to use explicitly the default regex in the child locale, other than saying null and relying on prototype

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
Copy link
Contributor Author

Merged in 470490a

@ichernev ichernev merged commit f29eaf5 into moment:develop Jun 20, 2016
ichernev added a commit that referenced this pull request Jun 20, 2016
Use proper locale inheritance for the base locale
@mattjohnsonpint mattjohnsonpint added this to the 2.14.0 milestone Jul 5, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Using updateLocale with relativeTime doesn't merge default local.
2 participants