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

core(i18n): add locale fallbacks when language not supported #5746

Merged
merged 3 commits into from
Jul 30, 2018

Conversation

brendankenny
Copy link
Member

cf. #5719

Sets up locale fallbacks, using the pretty common method of pruning on the last - until some prefix of the original locale is found, falling back to the default locale if no match is found.

Also adds a test to make sure that for any locale we set we also have a fallback for that language. So if e.g. we had pt-BR but not pt-PT for some reason, we'll be guaranteed to have pt to fall back to. Looking at our (eventual) list of supported languages, we shouldn't have to do any artificial mappings to support this like I'm doing in this PR.

Since locale is part of config.settings, it makes sense to initialize and check it in Config, so this moves it there. This allows removing some of the optional uses that had defaults coming from a few different places depending on the execution path taken.

simplify default locale control flow by centralizing in Config
* @param {string=} locale
* @return {LH.Locale}
*/
function lookupLocale(locale = MessageFormat.defaultLocale) {
Copy link
Member Author

Choose a reason for hiding this comment

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

MessageFormat.defaultLocale is actually en when no locale info is found, not en-US. Not sure if we have an issue with that since we assume it's en-US everywhere else. As a result, we'll only get the en-US default set at the end of this function if a specific locale we don't support is requested. Do we want to change the last line to return en instead? Or just live with this

Copy link
Member

Choose a reason for hiding this comment

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

these words wash over me like an ocean wave. 🌊 i don't know what hit me but it was powerful and disorienting.

😕

Copy link
Member Author

Choose a reason for hiding this comment

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

these words wash over me like an ocean wave

haha. Basically everywhere we pretend the default has been en-US, but the current i18n.getDefaultLocale has been trying to return en since that's what the default fallback of MessageFormat.defaultLocale is. It just then finds out there is no en strings file, so the function falls back to the hardcoded en-US. Once we have an en strings file, that will change.

So,

  • if you request whatever the default is for where LH is running, you'll get en
  • if you request a locale that we don't have (and have no fallback for), you'll get en-US

I'm wondering if we should

  • change default fallback everywhere to en to match MessageFormat
  • change MessageFormat.defaultLocale so that if it returns en, to be en-US and not change everywhere else
  • live with the inconsistency

@@ -118,7 +134,7 @@ const _icuMessageInstanceMap = new Map();
* @return {{formattedString: string, icuMessage: string}}
*/
function _formatIcuMessage(locale, icuMessageId, icuMessage, values) {
const localeMessages = LOCALES[locale] || {};
const localeMessages = LOCALES[locale];
Copy link
Member Author

Choose a reason for hiding this comment

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

locale is always a LH.Locale, so standing behind our type checking here by having no fallback :)

const locales = {
'ar': require('./ar-XB.json'), // TODO: fallback not needed when ar translation available
'ar-XB': require('./ar-XB.json'),
'en': require('./en-US.json'), // en-* fallback
Copy link
Member Author

Choose a reason for hiding this comment

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

both ar and en will exist when we get our full languages back. If we want to do other mappings we can do them in here like this or make a simple map in i18n.js internal to lookupLocale()


// Override any applicable settings with CLI flags
const settingsWithFlags = merge(settingWithDefaults || {}, cleanFlagsForSettings(flags), true);

// Use locale specified in flags or settings, allowing i18n system to select fallback if needed.
settingsWithFlags.locale = i18n.lookupLocale((flags && flags.locale) || settingsJson.locale);
Copy link
Member

Choose a reason for hiding this comment

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

can you extract (flags && flags.locale) || settingsJson.locale to a const that is defined above L415. between fallbacks and merges i want to be quite clear what is affecting locale and what isnt. (in this case the merge apparently doesnt)

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmm, I was putting it down here to attempt to make it clear that you can just ignore everything above for locale, this is the place that it's determined and set (and that defaultSettings.locale can't possibly influence the value).

Do you want me to set it on settingWithDefaults or cleanFlagsForSettings(flags) and let the merge merge it in?

@@ -39,7 +39,7 @@ const defaultSettings = {

// the following settings have no defaults but we still want ensure that `key in settings`
// in config will work in a typechecked way
locale: null, // default determined by the intl library
locale: 'en-US', // actual default determined by Config using lib/i18n
Copy link
Member

Choose a reason for hiding this comment

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

since Config.initSettings will never use this, what is it used for

Copy link
Member Author

@brendankenny brendankenny Jul 30, 2018

Choose a reason for hiding this comment

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

since Config.initSettings will never use this, what is it used for

since we declare the constants in this file to be a valid Config.Settings, unfortunately we have to give some valid value here. I can't think of a good way to not do that while still retaining all the benefits we currently have of the type-checked Settings and making sure every settings property that needs to be defined has a valid value.

*/
function lookupLocale(locale = MessageFormat.defaultLocale) {
// TODO: could do more work to sniff out defaultLocale
const canonicalLocale = Intl.getCanonicalLocales(locale)[0];
Copy link
Member

Choose a reason for hiding this comment

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

compat-wise, how do we look here across browsers and node. do we need this one conditionally polyfilled too?

Copy link
Member Author

Choose a reason for hiding this comment

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

compat-wise, how do we look here across browsers and node. do we need this one conditionally polyfilled too?

it comes with Intl, and it seems like we're assuming at least that exists? The function doesn't check valid tags or need a list of real locales or anything, it's just the capitalization and...maybe some other formatting stuff I can't remember.

Though from https://nodejs.org/api/intl.html it seems like maybe there's no Intl object at all if node was compiled with --with-intl=none?

Copy link
Member

Choose a reason for hiding this comment

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

the mdn compat table is out of date, but it at least indicates support was added later after the Intl basics:

image

unfortunately the only polyfill appears to be deep inside of Intl.js so hopefully we don't need it?

* @param {string=} locale
* @return {LH.Locale}
*/
function lookupLocale(locale = MessageFormat.defaultLocale) {
Copy link
Member

Choose a reason for hiding this comment

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

these words wash over me like an ocean wave. 🌊 i don't know what hit me but it was powerful and disorienting.

😕

@@ -218,7 +217,7 @@ class Runner {
*/
static async _runAudit(auditDefn, artifacts, settings, runWarnings) {
const audit = auditDefn.implementation;
const status = `Evaluating: ${i18n.getFormatted(audit.meta.title)}`;
const status = `Evaluating: ${i18n.getFormatted(audit.meta.title, 'en-US')}`;
Copy link
Member

Choose a reason for hiding this comment

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

nice. good call.

@@ -13,6 +13,7 @@
"./typings"
],

"resolveJsonModule": true,
Copy link
Member

Choose a reason for hiding this comment

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

this is for our messages files?

Copy link
Member Author

Choose a reason for hiding this comment

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

this is for our messages files?

yeah, no reason to keep tsc from not seeing them

@paulirish paulirish merged commit e367395 into master Jul 30, 2018
@paulirish paulirish deleted the localefallback branch July 30, 2018 21:47
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.

2 participants