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] updateLocale now tries to load parent, fixes #3626 #4242

Closed
wants to merge 3 commits into from

Conversation

cmyers
Copy link
Contributor

@cmyers cmyers commented Oct 14, 2017

This addresses #3626.

A fix was suggested in the comments but a PR wasn't submitted. I've refactored the suggested fix which will check if the locale to update exists (if not found in the currently loaded locales) and load it before merging. This mitigates the merge with the current set locale if the submitted locale isn't found in the loaded locales array.

Not sure if it should continue to merge if the locale to merge with can't be loaded, but this could be another issue to discuss.

@@ -136,6 +136,12 @@ export function updateLocale(name, config) {
if (locales[name] != null) {
parentConfig = locales[name]._config;
}
else {
Copy link
Member

Choose a reason for hiding this comment

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

How about:

tmpLocale = locales[name] || loadLocale(name);
if (locale != null) {
  parentConfig = tmpLocale._config;
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looks good, I've committed with the suggested refactor.

@icambron
Copy link
Member

@cmyers Can you add a test?

@cmyers
Copy link
Contributor Author

cmyers commented Oct 19, 2017

@icambron new to qunit but I'll give it a go.

@icambron
Copy link
Member

Actually, another thought: I looked at the loadLocale code and it looks like it checks the cache anyway. So you probably don't even need the locales[locale] || part. Just let loadLocale check for you.

@cmyers
Copy link
Contributor Author

cmyers commented Oct 19, 2017

That's a good catch, I'll revisit then add a test.

@icambron
Copy link
Member

LGTM. Thanks!

@ichernev
Copy link
Contributor

So basically try to load it if its not loaded already? That's mostly for node.js, I guess.

@icambron
Copy link
Member

Yup

@ichernev
Copy link
Contributor

Merged in d26f97e

@ichernev ichernev changed the title Fix #3626: updateLocale merge issue [bugfix] updateLocale now tries to load parent, fixes #3626 Nov 11, 2017
@ichernev ichernev closed this Nov 11, 2017
ichernev added a commit that referenced this pull request Nov 11, 2017
[bugfix] updateLocale now tries to load parent, fixes #3626
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.

None yet

3 participants