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
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 13 additions & 4 deletions lighthouse-core/config/config.js
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,8 @@
const defaultConfigPath = './default-config.js';
const defaultConfig = require('./default-config.js');
const fullConfig = require('./full-config.js');
const constants = require('./constants');
const constants = require('./constants.js');
const i18n = require('./../lib/i18n.js');

const isDeepEqual = require('lodash.isequal');
const log = require('lighthouse-logger');
Expand Down Expand Up @@ -401,18 +402,26 @@ class Config {
}

/**
* @param {LH.Config.SettingsJson=} settings
* @param {LH.Config.SettingsJson=} settingsJson
* @param {LH.Flags=} flags
* @return {LH.Config.Settings}
*/
static initSettings(settings = {}, flags) {
static initSettings(settingsJson = {}, flags) {
// If a locale is requested in flags or settings, use it. A typical CLI run will not have one,
// however `lookupLocale` will always determine which of our supported locales to use (falling
// back if necessary).
const locale = i18n.lookupLocale((flags && flags.locale) || settingsJson.locale);

// Fill in missing settings with defaults
const {defaultSettings} = constants;
const settingWithDefaults = merge(deepClone(defaultSettings), settings, true);
const settingWithDefaults = merge(deepClone(defaultSettings), settingsJson, true);

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

// Locale is special and comes only from flags/settings/lookupLocale.
settingsWithFlags.locale = locale;

return settingsWithFlags;
}

Expand Down
2 changes: 1 addition & 1 deletion lighthouse-core/config/constants.js
Original file line number Diff line number Diff line change
Expand Up @@ -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.

blockedUrlPatterns: null,
additionalTraceCategories: null,
extraHeaders: null,
Expand Down
38 changes: 24 additions & 14 deletions lighthouse-core/lib/i18n.js
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ const isDeepEqual = require('lodash.isequal');
const log = require('lighthouse-logger');
const MessageFormat = require('intl-messageformat').default;
const MessageParser = require('intl-messageformat-parser');
const lookupClosestLocale = require('lookup-closest-locale');
const LOCALES = require('./locales');

const LH_ROOT = path.join(__dirname, '../../');
Expand Down Expand Up @@ -66,6 +67,24 @@ const formats = {
},
};

/**
* Look up the best available locale for the requested language through these fall backs:
* - exact match
* - progressively shorter prefixes (`de-CH-1996` -> `de-CH` -> `de`)
* - the default locale ('en-US') if no match is found
*
* If `locale` isn't provided, the default is used.
* @param {string=} locale
* @return {LH.Locale}
*/
function lookupLocale(locale) {
// TODO: could do more work to sniff out default locale
const canonicalLocale = Intl.getCanonicalLocales(locale)[0];

const closestLocale = lookupClosestLocale(canonicalLocale, LOCALES);
return closestLocale || 'en-US';
}

/**
* @param {string} icuMessage
* @param {Record<string, *>} [values]
Expand Down Expand Up @@ -118,7 +137,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 localeMessage = localeMessages[icuMessageId] && localeMessages[icuMessageId].message;
// fallback to the original english message if we couldn't find a message in the specified locale
// better to have an english message than no message at all, in some number cases it won't even matter
Expand Down Expand Up @@ -150,15 +169,6 @@ function _formatPathAsString(pathInLHR) {
return pathAsString;
}

/**
* @return {LH.Locale}
*/
function getDefaultLocale() {
const defaultLocale = MessageFormat.defaultLocale;
if (defaultLocale in LOCALES) return /** @type {LH.Locale} */ (defaultLocale);
return 'en-US';
}

/**
* @param {LH.Locale} locale
* @return {LH.I18NRendererStrings}
Expand Down Expand Up @@ -208,7 +218,7 @@ function createMessageInstanceIdFn(filename, fileStrings) {

/**
* @param {string} icuMessageIdOrRawString
* @param {LH.Locale} [locale]
* @param {LH.Locale} locale
* @return {string}
*/
function getFormatted(icuMessageIdOrRawString, locale) {
Expand All @@ -221,10 +231,10 @@ function getFormatted(icuMessageIdOrRawString, locale) {

/**
* @param {string} icuMessageInstanceId
* @param {LH.Locale} [locale]
* @param {LH.Locale} locale
* @return {{icuMessageInstance: IcuMessageInstance, formattedString: string}}
*/
function _resolveIcuMessageInstanceId(icuMessageInstanceId, locale = 'en-US') {
function _resolveIcuMessageInstanceId(icuMessageInstanceId, locale) {
const matches = icuMessageInstanceId.match(MESSAGE_INSTANCE_ID_REGEX);
if (!matches) throw new Error(`${icuMessageInstanceId} is not a valid message instance ID`);

Expand Down Expand Up @@ -282,7 +292,7 @@ function replaceIcuMessageInstanceIds(lhr, locale) {
module.exports = {
_formatPathAsString,
UIStrings,
getDefaultLocale,
lookupLocale,
getRendererFormattedStrings,
createMessageInstanceIdFn,
getFormatted,
Expand Down
12 changes: 9 additions & 3 deletions lighthouse-core/lib/locales/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -3,11 +3,17 @@
* Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except in compliance with the License. You may obtain a copy of the License at http://www.apache.org/licenses/LICENSE-2.0
* Unless required by applicable law or agreed to in writing, software distributed under the License is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the License for the specific language governing permissions and limitations under the License.
*/
// @ts-nocheck
'use strict';

module.exports = {
/** @typedef {Record<string, {message: string}>} LocaleMessages */

/** @type {Record<LH.Locale, LocaleMessages>} */
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()

'en-US': require('./en-US.json'),
'en-XA': require('./en-XA.json'),
'ar-XB': require('./ar-XB.json'),
};

module.exports = locales;
3 changes: 1 addition & 2 deletions lighthouse-core/runner.js
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,6 @@ class Runner {
try {
const startTime = Date.now();
const settings = opts.config.settings;
settings.locale = settings.locale || i18n.getDefaultLocale();

/**
* List of top-level warnings for this Lighthouse run.
Expand Down Expand Up @@ -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.


log.log('status', status);
let auditResult;
Expand Down
24 changes: 24 additions & 0 deletions lighthouse-core/test/config/config-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ const defaultConfig = require('../../config/default-config.js');
const log = require('lighthouse-logger');
const Gatherer = require('../../gather/gatherers/gatherer');
const Audit = require('../../audits/audit');
const i18n = require('../../lib/i18n');

/* eslint-env jest */

Expand Down Expand Up @@ -597,6 +598,29 @@ describe('Config', () => {
assert.ok(typeof config.settings.maxWaitForLoad === 'number', 'missing setting from default');
});

describe('locale', () => {
it('falls back to default locale if none specified', () => {
const config = new Config({settings: undefined});
// Don't assert specific locale so it isn't tied to where tests are run, but
// check that it's valid and available.
assert.ok(config.settings.locale);
assert.strictEqual(config.settings.locale, i18n.lookupLocale(config.settings.locale));
});

it('uses config setting for locale if set', () => {
const locale = 'ar-XB';
const config = new Config({settings: {locale}});
assert.strictEqual(config.settings.locale, locale);
});

it('uses flag setting for locale if set', () => {
const settingsLocale = 'en-XA';
const flagsLocale = 'ar-XB';
const config = new Config({settings: {locale: settingsLocale}}, {locale: flagsLocale});
assert.strictEqual(config.settings.locale, flagsLocale);
});
});

it('is idempotent when accepting a canonicalized Config as valid ConfigJson input', () => {
const config = new Config(defaultConfig);
const configAgain = new Config(config);
Expand Down
14 changes: 14 additions & 0 deletions lighthouse-core/test/lib/i18n-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -62,4 +62,18 @@ describe('i18n', () => {
expect(strings.scorescaleLabel).toEqual('[Šçöŕé šçåļé: one two]');
});
});

describe('#lookupLocale', () => {
it('canonicalizes the locale', () => {
expect(i18n.lookupLocale('en-xa')).toEqual('en-XA');
});

it('falls back to root tag prefix if specific locale not available', () => {
expect(i18n.lookupLocale('en-JKJK')).toEqual('en');
});

it('falls back to en-US if no match is available', () => {
expect(i18n.lookupLocale('jk-Latn-DE-1996-a-ext-x-phonebk-i-klingon')).toEqual('en-US');
});
});
});
27 changes: 27 additions & 0 deletions lighthouse-core/test/lib/locales/index-test.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
/**
* @license Copyright 2018 Google Inc. All Rights Reserved.
* Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except in compliance with the License. You may obtain a copy of the License at http://www.apache.org/licenses/LICENSE-2.0
* Unless required by applicable law or agreed to in writing, software distributed under the License is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the License for the specific language governing permissions and limitations under the License.
*/
'use strict';

const locales = require('../../../lib/locales/index.js');
const assert = require('assert');

/* eslint-env jest */

describe('locales', () => {
it('has only canonical language tags', () => {
for (const locale of Object.keys(locales)) {
const canonicalLocale = Intl.getCanonicalLocales(locale)[0];
assert.strictEqual(locale, canonicalLocale);
}
});

it('has a base language prefix fallback for all supported languages', () => {
for (const locale of Object.keys(locales)) {
const basePrefix = locale.split('-')[0];
assert.ok(locales[basePrefix]);
}
});
});
2 changes: 0 additions & 2 deletions lighthouse-extension/app/src/lighthouse-background.js
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,6 @@
const RawProtocol = require('../../../lighthouse-core/gather/connections/raw');
const Runner = require('../../../lighthouse-core/runner');
const Config = require('../../../lighthouse-core/config/config');
const i18n = require('../../../lighthouse-core/lib/i18n');
const defaultConfig = require('../../../lighthouse-core/config/default-config.js');
const log = require('lighthouse-logger');

Expand All @@ -28,7 +27,6 @@ function runLighthouseForConnection(
const config = new Config({
extends: 'lighthouse:default',
settings: {
locale: i18n.getDefaultLocale(),
onlyCategories: categoryIDs,
},
}, options.flags);
Expand Down
1 change: 1 addition & 0 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -124,6 +124,7 @@
"js-library-detector": "^4.3.1",
"lighthouse-logger": "^1.0.0",
"lodash.isequal": "^4.5.0",
"lookup-closest-locale": "6.0.4",
"metaviewport-parser": "0.2.0",
"mkdirp": "0.5.1",
"opn": "4.0.2",
Expand Down
1 change: 1 addition & 0 deletions tsconfig.json
Original file line number Diff line number Diff line change
Expand Up @@ -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

"diagnostics": true
},
"include": [
Expand Down
10 changes: 8 additions & 2 deletions typings/externs.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,12 @@ declare global {
code?: string;
}

// Augment Intl to include
// https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Intl/getCanonicalLocales
namespace Intl {
var getCanonicalLocales: (locales?: string | Array<string>) => Array<string>;
}

/** Make properties K in T optional. */
type MakeOptional<T, K extends keyof T> = {
[P in Exclude<keyof T, K>]: T[P]
Expand Down Expand Up @@ -52,13 +58,13 @@ declare global {
cpuSlowdownMultiplier?: number
}

export type Locale = 'en-US'|'en-XA'|'ar-XB';
export type Locale = 'ar'|'ar-XB'|'en'|'en-US'|'en-XA';

export type OutputMode = 'json' | 'html' | 'csv';

interface SharedFlagsSettings {
output?: OutputMode|OutputMode[];
locale?: Locale | null;
locale?: Locale;
maxWaitForLoad?: number;
blockedUrlPatterns?: string[] | null;
additionalTraceCategories?: string | null;
Expand Down
11 changes: 11 additions & 0 deletions typings/lookup-closest-locale/index.d.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
/**
* @license Copyright 2018 Google Inc. All Rights Reserved.
* Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except in compliance with the License. You may obtain a copy of the License at http://www.apache.org/licenses/LICENSE-2.0
* Unless required by applicable law or agreed to in writing, software distributed under the License is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the License for the specific language governing permissions and limitations under the License.
*/

declare module 'lookup-closest-locale' {
function lookupClosestLocale(locale: string|undefined, available: Record<LH.Locale, any>): LH.Locale|undefined;

export = lookupClosestLocale;
}
4 changes: 4 additions & 0 deletions yarn.lock
Original file line number Diff line number Diff line change
Expand Up @@ -4168,6 +4168,10 @@ longest@^1.0.1:
version "1.0.1"
resolved "https://registry.yarnpkg.com/longest/-/longest-1.0.1.tgz#30a0b2da38f73770e8294a0d22e6625ed77d0097"

[email protected]:
version "6.0.4"
resolved "https://registry.yarnpkg.com/lookup-closest-locale/-/lookup-closest-locale-6.0.4.tgz#1279fed7546a601647bbc980f64423ee990a8590"

loose-envify@^1.0.0:
version "1.2.0"
resolved "https://registry.yarnpkg.com/loose-envify/-/loose-envify-1.2.0.tgz#69a65aad3de542cf4ee0f4fe74e8e33c709ccb0f"
Expand Down