From 793bdaf67885859f6363b865b2c2d131e3cf3c37 Mon Sep 17 00:00:00 2001 From: Brendan Kenny Date: Wed, 13 Oct 2021 14:20:37 -0500 Subject: [PATCH 1/3] i18n: add better support for the default locale in bundles --- lighthouse-core/config/config-helpers.js | 1 + lighthouse-core/lib/i18n/i18n.js | 9 ++-- lighthouse-core/test/lib/i18n/i18n-test.js | 16 +++--- shared/localization/format.js | 61 +++++++++++++++++----- shared/localization/locales.js | 6 +++ shared/test/localization/format-test.js | 7 +++ 6 files changed, 74 insertions(+), 26 deletions(-) diff --git a/lighthouse-core/config/config-helpers.js b/lighthouse-core/config/config-helpers.js index b8b6b2e1f0a2..1dbb0d0c6a64 100644 --- a/lighthouse-core/config/config-helpers.js +++ b/lighthouse-core/config/config-helpers.js @@ -283,6 +283,7 @@ function resolveSettings(settingsJson = {}, overrides = undefined) { // 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). + // TODO: could do more work to sniff out the user's locale const locale = i18n.lookupLocale((overrides && overrides.locale) || settingsJson.locale); // Fill in missing settings with defaults diff --git a/lighthouse-core/lib/i18n/i18n.js b/lighthouse-core/lib/i18n/i18n.js index 574feed36091..8cda23e0eec0 100644 --- a/lighthouse-core/lib/i18n/i18n.js +++ b/lighthouse-core/lib/i18n/i18n.js @@ -12,9 +12,11 @@ const lookupClosestLocale = require('lookup-closest-locale'); const {getAvailableLocales} = require('../../../shared/localization/format.js'); const log = require('lighthouse-logger'); const {LH_ROOT} = require('../../../root.js'); -const {isIcuMessage, _formatMessage} = require('../../../shared/localization/format.js'); - -const DEFAULT_LOCALE = 'en'; +const { + isIcuMessage, + _formatMessage, + DEFAULT_LOCALE, +} = require('../../../shared/localization/format.js'); const UIStrings = { /** Used to show the duration in milliseconds that something lasted. The `{timeInMs}` placeholder will be replaced with the time duration, shown in milliseconds (e.g. 63 ms) */ @@ -127,7 +129,6 @@ function lookupLocale(locales) { throw new Error('Lighthouse must be run in Node with `Intl` support. See https://nodejs.org/api/intl.html for help'); } - // TODO: could do more work to sniff out the user's locale const canonicalLocales = Intl.getCanonicalLocales(locales); // Filter by what's available in this runtime. diff --git a/lighthouse-core/test/lib/i18n/i18n-test.js b/lighthouse-core/test/lib/i18n/i18n-test.js index d3129972021d..a00b96184aef 100644 --- a/lighthouse-core/test/lib/i18n/i18n-test.js +++ b/lighthouse-core/test/lib/i18n/i18n-test.js @@ -39,16 +39,16 @@ describe('i18n', () => { }); it('falls back to default if locale not provided or cant be found', () => { - expect(i18n.lookupLocale(undefined)).toEqual('en'); - expect(i18n.lookupLocale(invalidLocale)).toEqual('en'); - expect(i18n.lookupLocale([invalidLocale, invalidLocale])).toEqual('en'); + expect(i18n.lookupLocale(undefined)).toEqual('en-US'); + expect(i18n.lookupLocale(invalidLocale)).toEqual('en-US'); + expect(i18n.lookupLocale([invalidLocale, invalidLocale])).toEqual('en-US'); }); it('logs a warning if locale is not available and the default is used', () => { const logListener = jest.fn(); log.events.on('warning', logListener); - expect(i18n.lookupLocale(invalidLocale)).toEqual('en'); + expect(i18n.lookupLocale(invalidLocale)).toEqual('en-US'); // COMPAT: Node 12 logs an extra warning that full-icu is not available. if (isNode12SmallIcu()) { @@ -56,13 +56,13 @@ describe('i18n', () => { expect(logListener).toHaveBeenNthCalledWith(1, ['i18n', expect.stringMatching(/Requested locale not available in this version of node/)]); expect(logListener).toHaveBeenNthCalledWith(2, ['i18n', - `locale(s) '${invalidLocale}' not available. Falling back to default 'en'`]); + `locale(s) '${invalidLocale}' not available. Falling back to default 'en-US'`]); return; } expect(logListener).toBeCalledTimes(1); expect(logListener).toBeCalledWith(['i18n', - `locale(s) '${invalidLocale}' not available. Falling back to default 'en'`]); + `locale(s) '${invalidLocale}' not available. Falling back to default 'en-US'`]); log.events.off('warning', logListener); }); @@ -77,8 +77,8 @@ describe('i18n', () => { expect(i18n.lookupLocale('es-JKJK')).toEqual('es'); }); - it('falls back to en if no match is available', () => { - expect(i18n.lookupLocale(invalidLocale)).toEqual('en'); + it('falls back to en-US if no match is available', () => { + expect(i18n.lookupLocale(invalidLocale)).toEqual('en-US'); }); }); }); diff --git a/shared/localization/format.js b/shared/localization/format.js index be9c8c8069db..1a9eb985e3de 100644 --- a/shared/localization/format.js +++ b/shared/localization/format.js @@ -13,6 +13,8 @@ const {isObjectOfUnknownValues, isObjectOrArrayOfUnknownValues} = require('../ty /** Contains available locales with messages. May be an empty object if bundled. */ const LOCALE_MESSAGES = require('./locales.js'); +const DEFAULT_LOCALE = 'en-US'; + /** * The locale tags for the localized messages available to Lighthouse on disk. * When bundled, these will be inlined by brfs. @@ -189,14 +191,16 @@ function _formatMessage(message, values = {}, locale) { * @return {string} */ function _localizeIcuMessage(icuMessage, locale) { - const localeMessages = LOCALE_MESSAGES[locale]; - if (!localeMessages) throw new Error(`Unsupported locale '${locale}'`); + const localeMessages = _getLocaleMessages(locale); const localeMessage = localeMessages[icuMessage.i18nId]; - // Fall back to the default (usually the original english message) if we couldn't find a - // message in the specified locale. This could be because of string drift between - // Lighthouse versions or because new strings haven't been updated yet. Better to have - // an english message than no message at all; in some cases it won't even matter. + // Use the DEFAULT_LOCALE fallback (usually the original english message) if we couldn't + // find a message in the specified locale. Possible reasons: + // - string drift between Lighthouse versions + // - in a bundle stripped of locale files but running in the DEFAULT_LOCALE + // - new strings haven't been updated yet in a local dev run + // Better to have an english message than no message at all; in some cases it + // won't even matter. if (!localeMessage) { return icuMessage.formattedDefault; } @@ -209,9 +213,10 @@ function _localizeIcuMessage(icuMessage, locale) { * @return {Record} */ function getRendererFormattedStrings(locale) { - const localeMessages = LOCALE_MESSAGES[locale]; - if (!localeMessages) throw new Error(`Unsupported locale '${locale}'`); + const localeMessages = _getLocaleMessages(locale); + // If `localeMessages` is empty in the bundled and DEFAULT_LOCALE case, this + // will be empty and the report will fall back to the util UIStrings for these. const icuMessageIds = Object.keys(localeMessages).filter(f => f.startsWith('report/')); /** @type {Record} */ const strings = {}; @@ -349,12 +354,38 @@ function replaceIcuMessages(inputObject, locale) { return icuMessagePaths; } +/** + * Returns the locale messages for the given `locale`, if they exist. + * Throws if an unsupported locale. + * + * NOTE: If DEFAULT_LOCALE is requested and this is inside a bundle with locale + * messages stripped, an empty object will be returned. Default fallbacks will need to handle that case. + * @param {LH.Locale} locale + * @return {import('./locales').LhlMessages} + */ +function _getLocaleMessages(locale) { + const localeMessages = LOCALE_MESSAGES[locale]; + if (!localeMessages) { + if (locale === DEFAULT_LOCALE) { + // If the default locale isn't in LOCALE_MESSAGES, this is likely executing + // in a bundle. Let the caller use the fallbacks available. + return {}; + } + throw new Error(`Unsupported locale '${locale}'`); + } + + return localeMessages; +} + /** * Returns whether the `requestedLocale` can be used. * @param {LH.Locale} requestedLocale * @return {boolean} */ function hasLocale(requestedLocale) { + // The default locale is always supported through `IcuMessage.formattedDefault`. + if (requestedLocale === DEFAULT_LOCALE) return true; + const hasIntlSupport = Intl.NumberFormat.supportedLocalesOf([requestedLocale]).length > 0; const hasMessages = Boolean(LOCALE_MESSAGES[requestedLocale]); @@ -362,8 +393,9 @@ function hasLocale(requestedLocale) { } /** - * Returns a list of canonical locales (each of which may have aliases, but those would - * only show in getAvailableLocales) + * Returns a list of canonical locales, as defined by the existent message files. + * In practice, each of these may have aliases in the full list returned by + * `getAvailableLocales()`. * TODO: create a CanonicalLocale type * @return {Array} */ @@ -375,13 +407,13 @@ function getCanonicalLocales() { * Returns a list of available locales. * - if full build, this includes all canonical locales, aliases, and any locale added * via `registerLocaleData`. - * - if bundled and locale messages have been stripped (locales.js shimmed), this includes no - * locales (perhaps available in a separate bundle), and perhaps any locales - * from `registerLocaleData`. + * - if bundled and locale messages have been stripped (locales.js shimmed), this includes + * only DEFAULT_LOCALE and any locales from `registerLocaleData`. * @return {Array} */ function getAvailableLocales() { - return /** @type {Array} */ (Object.keys(LOCALE_MESSAGES).sort()); + const localesWithMessages = new Set([...Object.keys(LOCALE_MESSAGES), DEFAULT_LOCALE]); + return /** @type {Array} */ ([...localesWithMessages].sort()); } /** @@ -407,6 +439,7 @@ function getIcuMessageIdParts(i18nMessageId) { } module.exports = { + DEFAULT_LOCALE, _formatPathAsString, collectAllCustomElementsFromICU, isIcuMessage, diff --git a/shared/localization/locales.js b/shared/localization/locales.js index 75604c3fe270..2a9bcf74bfd6 100644 --- a/shared/localization/locales.js +++ b/shared/localization/locales.js @@ -12,6 +12,12 @@ * Google locale inheritance rules: https://goto.google.com/ccssm * CLDR language aliases: https://www.unicode.org/cldr/charts/latest/supplemental/aliases.html * CLDR locale inheritance: https://github.com/unicode-cldr/cldr-core/blob/master/supplemental/parentLocales.json + * + * For Lighthouse bundles that shouldn't include locale data, the recommended pattern + * is to replace the default export of this file with `{}` so that no locale messages + * are included. Strings will work normally through the IcuMessage.formattedDefault + * fallback, and locale messages can be added on demand (e.g. dynamically fetched) + * through `format.registerLocaleData()`. */ // TODO(paulirish): Centralize locale inheritance (combining this & i18n.lookupLocale()), adopt cldr parentLocale rules. diff --git a/shared/test/localization/format-test.js b/shared/test/localization/format-test.js index 4dbde4db21b8..c0182dd25ebc 100644 --- a/shared/test/localization/format-test.js +++ b/shared/test/localization/format-test.js @@ -9,10 +9,17 @@ const path = require('path'); const format = require('../../localization/format.js'); const i18n = require('../../../lighthouse-core/lib/i18n/i18n.js'); +const constants = require('../../../lighthouse-core/config/constants.js'); /* eslint-env jest */ describe('format', () => { + describe('DEFAULT_LOCALE', () => { + it('is the same as the default config locale', () => { + expect(format.DEFAULT_LOCALE).toBe(constants.defaultSettings.locale); + }); + }); + describe('#_formatPathAsString', () => { it('handles simple paths', () => { expect(format._formatPathAsString(['foo'])).toBe('foo'); From 60c6828eaff34c24fe85542755923f8cd7dcf4cc Mon Sep 17 00:00:00 2001 From: Brendan Kenny Date: Wed, 13 Oct 2021 15:57:43 -0500 Subject: [PATCH 2/3] update node 12/small-icu test expectations --- lighthouse-core/test/config/config-helpers-test.js | 4 ++-- lighthouse-core/test/lib/i18n/i18n-test.js | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/lighthouse-core/test/config/config-helpers-test.js b/lighthouse-core/test/config/config-helpers-test.js index 9267499df2b8..7011023412a6 100644 --- a/lighthouse-core/test/config/config-helpers-test.js +++ b/lighthouse-core/test/config/config-helpers-test.js @@ -249,9 +249,9 @@ describe('.mergePlugins', () => { describe('.resolveSettings', () => { it('resolves the locale', () => { const settings = resolveSettings({locale: 'zh-CN'}); - // COMPAT: Node 12 only has 'en' by default. + // COMPAT: Node 12 only has 'en-US' by default. if (isNode12SmallIcu()) { - expect(settings.locale).toEqual('en'); + expect(settings.locale).toEqual('en-US'); return; } expect(settings.locale).toEqual('zh'); diff --git a/lighthouse-core/test/lib/i18n/i18n-test.js b/lighthouse-core/test/lib/i18n/i18n-test.js index a00b96184aef..50b96a5113e5 100644 --- a/lighthouse-core/test/lib/i18n/i18n-test.js +++ b/lighthouse-core/test/lib/i18n/i18n-test.js @@ -68,9 +68,9 @@ describe('i18n', () => { }); it('falls back to root tag prefix if specific locale not available', () => { - // COMPAT: Node 12 only has 'en' by default. + // COMPAT: Node 12 only has 'en-US' by default. if (isNode12SmallIcu()) { - expect(i18n.lookupLocale('es-JKJK')).toEqual('en'); + expect(i18n.lookupLocale('es-JKJK')).toEqual('en-US'); return; } From aa6361246f1538f9c1ea4a434815c0f2c61e519d Mon Sep 17 00:00:00 2001 From: Brendan Kenny Date: Wed, 13 Oct 2021 18:35:30 -0500 Subject: [PATCH 3/3] add tests for getAvailableLocales and getCanonicalLocales --- shared/test/localization/format-test.js | 37 +++++++++++++++++++++++++ 1 file changed, 37 insertions(+) diff --git a/shared/test/localization/format-test.js b/shared/test/localization/format-test.js index c0182dd25ebc..5755f53e26eb 100644 --- a/shared/test/localization/format-test.js +++ b/shared/test/localization/format-test.js @@ -10,6 +10,7 @@ const path = require('path'); const format = require('../../localization/format.js'); const i18n = require('../../../lighthouse-core/lib/i18n/i18n.js'); const constants = require('../../../lighthouse-core/config/constants.js'); +const locales = require('../../localization/locales.js'); /* eslint-env jest */ @@ -20,6 +21,42 @@ describe('format', () => { }); }); + describe('#getAvailableLocales', () => { + it('has all the available locales', () => { + const availableLocales = format.getAvailableLocales(); + for (const locale of ['en', 'es', 'ru', 'zh']) { + expect(availableLocales).toContain(locale); + } + + const rawLocales = Object.keys(locales).sort(); + expect(availableLocales.sort()).toEqual(rawLocales); + }); + + it('contains the default locale', () => { + expect(format.getAvailableLocales()).toContain(format.DEFAULT_LOCALE); + }); + }); + + describe('#getCanonicalLocales', () => { + it('contains some canonical locales', () => { + const canonicalLocales = format.getCanonicalLocales(); + for (const locale of ['en-US', 'es', 'ru', 'zh']) { + expect(canonicalLocales).toContain(locale); + } + }); + + it('is a subset of the available locales', () => { + const canonicalLocales = format.getCanonicalLocales(); + const availableLocales = format.getAvailableLocales(); + + for (const canonicalLocale of canonicalLocales) { + expect(availableLocales).toContain(canonicalLocale); + } + + expect(canonicalLocales.length).toBeLessThan(availableLocales.length); + }); + }); + describe('#_formatPathAsString', () => { it('handles simple paths', () => { expect(format._formatPathAsString(['foo'])).toBe('foo');