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

[bugfix] Fix #3883 lazy load parentLocale in defineLocale, fallback to global if missing #4310

Merged
merged 3 commits into from Mar 2, 2018

Conversation

cmyers
Copy link
Contributor

@cmyers cmyers commented Nov 18, 2017

Fixes #3883. It's currently possible to define a locale with a parent that is not yet defined. However, in the case of this bug when attempting to create a moment using a child locale with a parent that hasn't been loaded yet or hasn't been defined causes an exception to be thrown during prepareConfig.

This fix first attempts to load the parent locale during defineLocale if it can't be found initially. If in the case that a moment is created or a locale is set with a child who's parent can't be loaded or hasn't been defined, chooseLocale returns the global locale instead of null. This mitigates the error described in this bug report in the instance the parentLocale can't be located, and follows the same logic when defining a child can't load a parent. I've added a test for this case.

As this is an edge case it may not be necessary, but makes for a nicer experience rather than having an unhandled exception thrown. I considered adding a warning in chooseLocale but this affected a number of tests.

Also, a warning was previously shown to indicate the parentLocale is missing but was removed (assuming due to the ability to define a parent after a child). However, the documentation specifies that it should still show the warning? I'm assuming in this instance the documentation needs updating.

Apologies if these scenarios have already been discussed and addressed.

@marwahaha marwahaha changed the title Fix #3883 lazy load parentLocale in defineLocale, fallback to global if missing [bug] Fix #3883 lazy load parentLocale in defineLocale, fallback to global if missing Nov 20, 2017
@cmyers cmyers changed the title [bug] Fix #3883 lazy load parentLocale in defineLocale, fallback to global if missing [bugfix] Fix #3883 lazy load parentLocale in defineLocale, fallback to global if missing Nov 30, 2017
@marwahaha
Copy link
Member

@ichernev - you should look at this one

Copy link
Contributor

@ichernev ichernev left a comment

Choose a reason for hiding this comment

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

LGTM

@marwahaha
Copy link
Member

@cmyers - could you write some quick documentation for this over here? Or, if you think this doesn't deserve docs, just let me know.

@cmyers
Copy link
Contributor Author

cmyers commented Jan 2, 2018

@marwahaha No problem I'll take a look at that section and update as necessary. I planned on updating the warnings section for parent-locale also being as that warning was removed some time ago.

cmyers added a commit to cmyers/momentjs.com that referenced this pull request Jan 6, 2018
This adds the description of defining a locale with a parent that hasn't itself been defined or loaded yet which has been available since 2.16.0. This adds the additional behavior when creating a moment of lazy loading the parent locale if it isn't already loaded, or defaulting to the global locale if the parent doesn't exist. See PR (moment/moment#4310) As the next version number isn't determined yet, this should be merged only with the next release that includes the above PR.
@cmyers
Copy link
Contributor Author

cmyers commented Jan 6, 2018

Submitted momentjs.com PRs for documentation:

moment/momentjs.com#484
moment/momentjs.com#485

marwahaha pushed a commit to moment/momentjs.com that referenced this pull request Mar 2, 2018
* Update Customise parent locale

This adds the description of defining a locale with a parent that hasn't itself been defined or loaded yet which has been available since 2.16.0. This adds the additional behavior when creating a moment of lazy loading the parent locale if it isn't already loaded, or defaulting to the global locale if the parent doesn't exist. See PR (moment/moment#4310) As the next version number isn't determined yet, this should be merged only with the next release that includes the above PR.

* Update 00-intro.md
@marwahaha marwahaha merged commit f6c7069 into moment:develop Mar 2, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Cannot use a locale defined with parent
3 participants