From be15fb9ff5b95e302c63c477f1a4b7ba6fc0dc6a Mon Sep 17 00:00:00 2001 From: Brendan Kenny Date: Fri, 16 Aug 2019 17:49:53 -0700 Subject: [PATCH 1/3] i18n: use better types for intl-messageformat --- build/build-bundle.js | 1 + lighthouse-core/lib/i18n/i18n.js | 82 +++++++++++++--------- lighthouse-core/test/lib/i18n/i18n-test.js | 18 ++++- package.json | 6 +- types/intl-messageformat-parser/index.d.ts | 10 --- yarn.lock | 37 ++++++---- 6 files changed, 89 insertions(+), 65 deletions(-) delete mode 100644 types/intl-messageformat-parser/index.d.ts diff --git a/build/build-bundle.js b/build/build-bundle.js index 356f4c86c917..0cbc96f348ae 100644 --- a/build/build-bundle.js +++ b/build/build-bundle.js @@ -62,6 +62,7 @@ async function browserifyFile(entryPath, distPath) { bundle.ignore('source-map') .ignore('debug/node') .ignore('intl') + .ignore('intl-pluralrules') .ignore('raven') .ignore('mkdirp') .ignore('rimraf') diff --git a/lighthouse-core/lib/i18n/i18n.js b/lighthouse-core/lib/i18n/i18n.js index 8547f1b07256..7604327fb77a 100644 --- a/lighthouse-core/lib/i18n/i18n.js +++ b/lighthouse-core/lib/i18n/i18n.js @@ -9,10 +9,13 @@ const path = require('path'); const isDeepEqual = require('lodash.isequal'); const log = require('lighthouse-logger'); const MessageFormat = require('intl-messageformat').default; -const MessageParser = require('intl-messageformat-parser'); +const MessageParser = require('intl-messageformat-parser').default; const lookupClosestLocale = require('lookup-closest-locale'); const LOCALES = require('./locales.js'); +/** @typedef {import('intl-messageformat-parser').Element} MessageElement */ +/** @typedef {import('intl-messageformat-parser').ArgumentElement} ArgumentElement */ + const LH_ROOT = path.join(__dirname, '../../../'); const MESSAGE_INSTANCE_ID_REGEX = /(.* \| .*) # (\d+)$/; // Above regex is very slow against large strings. Use QUICK_REGEX as a much quicker discriminator. @@ -29,6 +32,10 @@ const MESSAGE_INSTANCE_ID_QUICK_REGEX = / # \d+$/; Intl.NumberFormat = IntlPolyfill.NumberFormat; Intl.DateTimeFormat = IntlPolyfill.DateTimeFormat; + + // Intl.PluralRules polyfilled on require(). + // @ts-ignore + require('intl-pluralrules'); } catch (_) { log.warn('i18n', 'Failed to install `intl` polyfill'); } @@ -128,6 +135,7 @@ function lookupLocale(locale) { /** * @param {string} icuMessage * @param {Record} [values] + * @return {Record} */ function _preprocessMessageValues(icuMessage, values = {}) { const clonedValues = JSON.parse(JSON.stringify(values)); @@ -135,7 +143,7 @@ function _preprocessMessageValues(icuMessage, values = {}) { const elements = _collectAllCustomElementsFromICU(parsed.elements); - return _processParsedElements(Array.from(elements.values()), clonedValues); + return _processParsedElements(icuMessage, Array.from(elements.values()), clonedValues); } /** @@ -152,14 +160,15 @@ function _preprocessMessageValues(icuMessage, values = {}) { * be stored in a set because they are not equal since their locations are different, * thus they are stored via a Map keyed on the "id" which is the ICU varName. * - * @param {Array} icuElements - * @param {Map} seenElementsById + * @param {Array} icuElements + * @param {Map} seenElementsById + * @return {Map} */ function _collectAllCustomElementsFromICU(icuElements, seenElementsById = new Map()) { for (const el of icuElements) { // We are only interested in elements that need ICU formatting (argumentElements) if (el.type !== 'argumentElement') continue; - // @ts-ignore - el.id is always defined when el.format is defined + seenElementsById.set(el.id, el); // Plurals need to be inspected recursively @@ -167,8 +176,7 @@ function _collectAllCustomElementsFromICU(icuElements, seenElementsById = new Ma // Look at all options of the plural (=1{} =other{}...) for (const option of el.format.options) { // Run collections on each option's elements - _collectAllCustomElementsFromICU(option.value.elements, - seenElementsById); + _collectAllCustomElementsFromICU(option.value.elements, seenElementsById); } } @@ -180,36 +188,40 @@ function _collectAllCustomElementsFromICU(icuElements, seenElementsById = new Ma * will apply Lighthouse custom formatting to the values based on the argumentElement * format style. * - * @param {Array} argumentElements + * @param {string} icuMessage + * @param {Array} argumentElements * @param {Record} [values] + * @return {Record} */ -function _processParsedElements(argumentElements, values = {}) { - // Throw an error if a message's value isn't provided - argumentElements - .filter(el => el.type === 'argumentElement') - .forEach(el => { - if (el.id && (el.id in values) === false) { - throw new Error(`ICU Message contains a value reference ("${el.id}") that wasn't provided`); - } - }); - - // Round all milliseconds to the nearest 10 - argumentElements - .filter(el => el.format && el.format.style === 'milliseconds') - // @ts-ignore - el.id is always defined when el.format is defined - .forEach(el => (values[el.id] = Math.round(values[el.id] / 10) * 10)); - - // Convert all seconds to the correct unit - argumentElements - .filter(el => el.format && el.format.style === 'seconds' && el.id === 'timeInMs') - // @ts-ignore - el.id is always defined when el.format is defined - .forEach(el => (values[el.id] = Math.round(values[el.id] / 100) / 10)); - - // Replace all the bytes with KB - argumentElements - .filter(el => el.format && el.format.style === 'bytes') - // @ts-ignore - el.id is always defined when el.format is defined - .forEach(el => (values[el.id] = values[el.id] / 1024)); +function _processParsedElements(icuMessage, argumentElements, values = {}) { + for (const {id, format} of argumentElements) { + // Throw an error if a message's value isn't provided + if (id && (id in values) === false) { + // eslint-disable-next-line max-len + throw new Error(`ICU Message "${icuMessage}" contains a value reference ("${id}") that wasn't provided`); + } + + // Direct `{id}` replacement and non-numeric values need no formatting. + if (!format || format.type !== 'numberFormat') continue; + + const value = values[id]; + if (typeof value !== 'number') { + // eslint-disable-next-line max-len + throw new Error(`ICU Message "${icuMessage}" contains a numeric reference ("${id}") but provided value was not a number`); + } + + // Format values for known styles. + if (format.style === 'milliseconds') { + // Round all milliseconds to the nearest 10. + values[id] = Math.round(value / 10) * 10; + } else if (format.style === 'seconds' && id === 'timeInMs') { + // Convert all seconds to the correct unit (currently only for `timeInMs`). + values[id] = Math.round(value / 100) / 10; + } else if (format.style === 'bytes') { + // Replace all the bytes with KB. + values[id] = value / 1024; + } + } return values; } diff --git a/lighthouse-core/test/lib/i18n/i18n-test.js b/lighthouse-core/test/lib/i18n/i18n-test.js index 53c5789dbe47..74b078ff1911 100644 --- a/lighthouse-core/test/lib/i18n/i18n-test.js +++ b/lighthouse-core/test/lib/i18n/i18n-test.js @@ -149,13 +149,15 @@ describe('i18n', () => { it('throws an error when values are needed but not provided', () => { expect(_ => i18n.getFormatted(str_(UIStrings.helloBytesWorld), 'en-US')) - .toThrow(`ICU Message contains a value reference ("in") that wasn't provided`); + // eslint-disable-next-line max-len + .toThrow(`ICU Message "Hello {in, number, bytes} World" contains a value reference ("in") that wasn't provided`); }); it('throws an error when a value is missing', () => { expect(_ => i18n.getFormatted(str_(UIStrings.helloWorldMultiReplace, {hello: 'hello'}), 'en-US')) - .toThrow(`ICU Message contains a value reference ("world") that wasn't provided`); + // eslint-disable-next-line max-len + .toThrow(`ICU Message "{hello} {world}" contains a value reference ("world") that wasn't provided`); }); it('formats a message with plurals', () => { @@ -165,7 +167,8 @@ describe('i18n', () => { it('throws an error when a plural control value is missing', () => { expect(_ => i18n.getFormatted(str_(UIStrings.helloPlural), 'en-US')) - .toThrow(`ICU Message contains a value reference ("itemCount") that wasn't provided`); + // eslint-disable-next-line max-len + .toThrow(`ICU Message "{itemCount, plural, =1{1 hello} other{hellos}}" contains a value reference ("itemCount") that wasn't provided`); }); it('formats a message with plurals and nested custom ICU', () => { @@ -179,5 +182,14 @@ describe('i18n', () => { in: 1875}); expect(helloStr).toBeDisplayString('hellos 1 goodbye 2'); }); + + it('throws an error if a string value is used for a numeric placeholder', () => { + const helloStr = str_(UIStrings.helloTimeInMsWorld, { + timeInMs: 'string not a number', + }); + expect(_ => i18n.getFormatted(helloStr, 'en-US')) + // eslint-disable-next-line max-len + .toThrow(`ICU Message "Hello {timeInMs, number, seconds} World" contains a numeric reference ("timeInMs") but provided value was not a number`); + }); }); }); diff --git a/package.json b/package.json index c9381a77dd52..224810a101a0 100644 --- a/package.json +++ b/package.json @@ -79,7 +79,6 @@ "@types/gh-pages": "^2.0.0", "@types/google.analytics": "0.0.39", "@types/inquirer": "^0.0.35", - "@types/intl-messageformat": "^1.3.0", "@types/jest": "^24.0.9", "@types/jpeg-js": "^0.3.0", "@types/lodash.isequal": "^4.5.2", @@ -140,8 +139,9 @@ "http-link-header": "^0.8.0", "inquirer": "^3.3.0", "intl": "^1.2.5", - "intl-messageformat": "^2.2.0", - "intl-messageformat-parser": "^1.4.0", + "intl-messageformat": "^4.4.0", + "intl-messageformat-parser": "^1.8.1", + "intl-pluralrules": "^1.0.3", "jpeg-js": "0.1.2", "js-library-detector": "^5.4.0", "jsonld": "^1.5.0", diff --git a/types/intl-messageformat-parser/index.d.ts b/types/intl-messageformat-parser/index.d.ts deleted file mode 100644 index 02df41d18fd5..000000000000 --- a/types/intl-messageformat-parser/index.d.ts +++ /dev/null @@ -1,10 +0,0 @@ -declare module 'intl-messageformat-parser' { - export interface Element { - type: 'messageTextElement'|'argumentElement'; - id?: string - value?: string - format?: null | {type: string; style?: string; options?: any}; - } - function parse(message: string): {elements: Element[]}; - export {parse}; -} diff --git a/yarn.lock b/yarn.lock index 63d046e4c513..ff14e886e3c2 100644 --- a/yarn.lock +++ b/yarn.lock @@ -493,11 +493,6 @@ dependencies: "@types/node" "*" -"@types/intl-messageformat@^1.3.0": - version "1.3.0" - resolved "https://registry.yarnpkg.com/@types/intl-messageformat/-/intl-messageformat-1.3.0.tgz#6af7144802b13d62ade9ad68f8420cdb33b75916" - integrity sha1-avcUSAKxPWKt6a1o+EIM2zO3WRY= - "@types/istanbul-lib-coverage@^1.1.0": version "1.1.0" resolved "https://registry.yarnpkg.com/@types/istanbul-lib-coverage/-/istanbul-lib-coverage-1.1.0.tgz#2cc2ca41051498382b43157c8227fea60363f94a" @@ -3941,17 +3936,24 @@ insert-module-globals@^7.0.0: undeclared-identifiers "^1.1.2" xtend "^4.0.0" -intl-messageformat-parser@1.4.0, intl-messageformat-parser@^1.4.0: - version "1.4.0" - resolved "https://registry.yarnpkg.com/intl-messageformat-parser/-/intl-messageformat-parser-1.4.0.tgz#b43d45a97468cadbe44331d74bb1e8dea44fc075" - integrity sha1-tD1FqXRoytvkQzHXS7Ho3qRPwHU= +intl-messageformat-parser@^1.8.1: + version "1.8.1" + resolved "https://registry.yarnpkg.com/intl-messageformat-parser/-/intl-messageformat-parser-1.8.1.tgz#0eb14c5618333be4c95c409457b66c8c33ddcc01" + integrity sha512-IMSCKVf0USrM/959vj3xac7s8f87sc+80Y/ipBzdKy4ifBv5Gsj2tZ41EAaURVg01QU71fYr77uA8Meh6kELbg== -intl-messageformat@^2.2.0: - version "2.2.0" - resolved "https://registry.yarnpkg.com/intl-messageformat/-/intl-messageformat-2.2.0.tgz#345bcd46de630b7683330c2e52177ff5eab484fc" - integrity sha1-NFvNRt5jC3aDMwwuUhd/9eq0hPw= +intl-messageformat@^4.4.0: + version "4.4.0" + resolved "https://registry.yarnpkg.com/intl-messageformat/-/intl-messageformat-4.4.0.tgz#aa196a4d04b573f4090bc417f982d81de4f74fad" + integrity sha512-z+Bj2rS3LZSYU4+sNitdHrwnBhr0wO80ZJSW8EzKDBowwUe3Q/UsvgCGjrwa+HPzoGCLEb9HAjfJgo4j2Sac8w== dependencies: - intl-messageformat-parser "1.4.0" + intl-messageformat-parser "^1.8.1" + +intl-pluralrules@^1.0.3: + version "1.0.3" + resolved "https://registry.yarnpkg.com/intl-pluralrules/-/intl-pluralrules-1.0.3.tgz#25438469f76fd13a4ac54a68a0ae4c0d30248a47" + integrity sha512-N+q+NmxJD5Pi+h7DoemDcNZN86e1dPSFUsWUtYtnSc/fKRen4vxCTcsmGveWOUgI9O9uSLTaiwJpC9f5xa2X4w== + dependencies: + make-plural "^4.3.0" intl@^1.2.5: version "1.2.5" @@ -5215,6 +5217,13 @@ make-dir@^2.0.0: pify "^4.0.1" semver "^5.6.0" +make-plural@^4.3.0: + version "4.3.0" + resolved "https://registry.yarnpkg.com/make-plural/-/make-plural-4.3.0.tgz#f23de08efdb0cac2e0c9ba9f315b0dff6b4c2735" + integrity sha512-xTYd4JVHpSCW+aqDof6w/MebaMVNTVYBZhbB/vi513xXdiPT92JMVCo0Jq8W2UZnzYRFeVbQiQ+I25l13JuKvA== + optionalDependencies: + minimist "^1.2.0" + makeerror@1.0.x: version "1.0.11" resolved "https://registry.yarnpkg.com/makeerror/-/makeerror-1.0.11.tgz#e01a5c9109f2af79660e4e8b9587790184f5a96c" From 4d1b1069d1fe1ce78e69c27f6d6d9fe924647a9a Mon Sep 17 00:00:00 2001 From: Brendan Kenny Date: Mon, 19 Aug 2019 13:10:11 -0700 Subject: [PATCH 2/3] remove explicit intl-messageformat-parser dependency --- lighthouse-core/lib/i18n/i18n.js | 15 ++++++++------- package.json | 1 - 2 files changed, 8 insertions(+), 8 deletions(-) diff --git a/lighthouse-core/lib/i18n/i18n.js b/lighthouse-core/lib/i18n/i18n.js index 7604327fb77a..b442e26da697 100644 --- a/lighthouse-core/lib/i18n/i18n.js +++ b/lighthouse-core/lib/i18n/i18n.js @@ -9,7 +9,6 @@ const path = require('path'); const isDeepEqual = require('lodash.isequal'); const log = require('lighthouse-logger'); const MessageFormat = require('intl-messageformat').default; -const MessageParser = require('intl-messageformat-parser').default; const lookupClosestLocale = require('lookup-closest-locale'); const LOCALES = require('./locales.js'); @@ -133,15 +132,16 @@ function lookupLocale(locale) { } /** + * Preformat values for the message based on how they're used, like KB or milliseconds. * @param {string} icuMessage + * @param {MessageFormat} messageFormatter * @param {Record} [values] * @return {Record} */ -function _preprocessMessageValues(icuMessage, values = {}) { +function _preformatValues(icuMessage, messageFormatter, values = {}) { const clonedValues = JSON.parse(JSON.stringify(values)); - const parsed = MessageParser.parse(icuMessage); - const elements = _collectAllCustomElementsFromICU(parsed.elements); + const elements = _collectAllCustomElementsFromICU(messageFormatter.getAst().elements); return _processParsedElements(icuMessage, Array.from(elements.values()), clonedValues); } @@ -269,12 +269,13 @@ function _formatIcuMessage(locale, icuMessageId, uiStringMessage, values) { // when using accented english, force the use of a different locale for number formatting const localeForMessageFormat = (locale === 'en-XA' || locale === 'en-XL') ? 'de-DE' : locale; - // pre-process values for the message format like KB and milliseconds - const valuesForMessageFormat = _preprocessMessageValues(localeMessage, values); const formatter = new MessageFormat(localeMessage, localeForMessageFormat, formats); - const formattedString = formatter.format(valuesForMessageFormat); + // preformat values for the message format like KB and milliseconds + const valuesForMessageFormat = _preformatValues(localeMessage, formatter, values); + + const formattedString = formatter.format(valuesForMessageFormat); return {formattedString, icuMessage: localeMessage}; } diff --git a/package.json b/package.json index 224810a101a0..558e48369bdc 100644 --- a/package.json +++ b/package.json @@ -140,7 +140,6 @@ "inquirer": "^3.3.0", "intl": "^1.2.5", "intl-messageformat": "^4.4.0", - "intl-messageformat-parser": "^1.8.1", "intl-pluralrules": "^1.0.3", "jpeg-js": "0.1.2", "js-library-detector": "^5.4.0", From facfd35f75ab8a6957a49c8144d1fe3acb17cdef Mon Sep 17 00:00:00 2001 From: Brendan Kenny Date: Mon, 19 Aug 2019 16:43:09 -0700 Subject: [PATCH 3/3] line breaks --- lighthouse-core/lib/i18n/i18n.js | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/lighthouse-core/lib/i18n/i18n.js b/lighthouse-core/lib/i18n/i18n.js index b442e26da697..fea132ec615e 100644 --- a/lighthouse-core/lib/i18n/i18n.js +++ b/lighthouse-core/lib/i18n/i18n.js @@ -197,8 +197,8 @@ function _processParsedElements(icuMessage, argumentElements, values = {}) { for (const {id, format} of argumentElements) { // Throw an error if a message's value isn't provided if (id && (id in values) === false) { - // eslint-disable-next-line max-len - throw new Error(`ICU Message "${icuMessage}" contains a value reference ("${id}") that wasn't provided`); + throw new Error(`ICU Message "${icuMessage}" contains a value reference ("${id}") ` + + `that wasn't provided`); } // Direct `{id}` replacement and non-numeric values need no formatting. @@ -206,8 +206,8 @@ function _processParsedElements(icuMessage, argumentElements, values = {}) { const value = values[id]; if (typeof value !== 'number') { - // eslint-disable-next-line max-len - throw new Error(`ICU Message "${icuMessage}" contains a numeric reference ("${id}") but provided value was not a number`); + throw new Error(`ICU Message "${icuMessage}" contains a numeric reference ("${id}") ` + + 'but provided value was not a number'); } // Format values for known styles.