From a8b4f0f5d2f85a8200d863ada47725e45ace7c71 Mon Sep 17 00:00:00 2001 From: Shane Exterkamp Date: Thu, 25 Jul 2019 14:38:43 -0700 Subject: [PATCH 1/8] plural ICU recursive preprocessing --- lighthouse-core/audits/resource-summary.js | 4 +- lighthouse-core/lib/i18n/i18n.js | 51 +++++++++++++++++---- lighthouse-core/lib/i18n/locales/en-US.json | 2 +- lighthouse-core/lib/i18n/locales/en-XL.json | 2 +- 4 files changed, 46 insertions(+), 13 deletions(-) diff --git a/lighthouse-core/audits/resource-summary.js b/lighthouse-core/audits/resource-summary.js index 1c45de7408ba..9bd1ee03e763 100644 --- a/lighthouse-core/audits/resource-summary.js +++ b/lighthouse-core/audits/resource-summary.js @@ -16,8 +16,8 @@ const UIStrings = { description: 'To set budgets for the quantity and size of page resources,' + ' add a budget.json file. [Learn more](https://developers.google.com/web/tools/lighthouse/audits/budgets).', /** [ICU Syntax] Label for an audit identifying the number of requests and kilobytes used to load the page. */ - displayValue: `{requestCount, plural, =1 {1 request} other {# requests}}` + - ` • {byteCount, number, bytes} KB`, + displayValue: `{requestCount, plural, =1 {1 request • {byteCount, number, bytes} KB} ` + + `other {# requests • {byteCount, number, bytes} KB}}`, }; const str_ = i18n.createMessageInstanceIdFn(__filename, UIStrings); diff --git a/lighthouse-core/lib/i18n/i18n.js b/lighthouse-core/lib/i18n/i18n.js index 4ebd6c4fa885..a8bbc50be3e8 100644 --- a/lighthouse-core/lib/i18n/i18n.js +++ b/lighthouse-core/lib/i18n/i18n.js @@ -130,10 +130,43 @@ function lookupLocale(locale) { * @param {Record} [values] */ function _preprocessMessageValues(icuMessage, values = {}) { - const clonedValues = JSON.parse(JSON.stringify(values)); + let clonedValues = JSON.parse(JSON.stringify(values)); const parsed = MessageParser.parse(icuMessage); + + const elements = _collectAllCustomElementsFromICU(parsed.elements); + + clonedValues = _processParsedElements(Array.from(elements.values()), clonedValues); + + return clonedValues; +} + +function _collectAllCustomElementsFromICU(parsedIcu, elements = new Map()) { + // Rescurse into Plurals + parsedIcu + .filter(el => el.format && el.format.type === 'pluralFormat') + .forEach(el => { + const elemSet = new Map(); + el.format.options.forEach(opt => opt.value.elements.forEach(elem => elemSet.set(elem.id, elem))); + const e = Array.from(elemSet.values()); + + // de dupe into main elements + const rElements = _collectAllCustomElementsFromICU(e, elements); + + rElements.forEach((k, v) => elements[k] = v); + }); + + // add arguementElements + parsedIcu + .filter(el => el.type === 'argumentElement') + // @ts-ignore - el.id is always defined when el.format is defined + .forEach(el => elements.set(el.id, el)); + + return elements; +} + +function _processParsedElements(icuElementsList, values = {}) { // Throw an error if a message's value isn't provided - parsed.elements + icuElementsList .filter(el => el.type === 'argumentElement') .forEach(el => { if (el.id && (el.id in values) === false) { @@ -142,24 +175,24 @@ function _preprocessMessageValues(icuMessage, values = {}) { }); // Round all milliseconds to the nearest 10 - parsed.elements + icuElementsList .filter(el => el.format && el.format.style === 'milliseconds') // @ts-ignore - el.id is always defined when el.format is defined - .forEach(el => (clonedValues[el.id] = Math.round(clonedValues[el.id] / 10) * 10)); + .forEach(el => (values[el.id] = Math.round(values[el.id] / 10) * 10)); // Convert all seconds to the correct unit - parsed.elements + icuElementsList .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 => (clonedValues[el.id] = Math.round(clonedValues[el.id] / 100) / 10)); + .forEach(el => (values[el.id] = Math.round(values[el.id] / 100) / 10)); // Replace all the bytes with KB - parsed.elements + icuElementsList .filter(el => el.format && el.format.style === 'bytes') // @ts-ignore - el.id is always defined when el.format is defined - .forEach(el => (clonedValues[el.id] = clonedValues[el.id] / 1024)); + .forEach(el => (values[el.id] = values[el.id] / 1024)); - return clonedValues; + return values; } /** diff --git a/lighthouse-core/lib/i18n/locales/en-US.json b/lighthouse-core/lib/i18n/locales/en-US.json index cd8d17f1a0fc..acf844d254b2 100644 --- a/lighthouse-core/lib/i18n/locales/en-US.json +++ b/lighthouse-core/lib/i18n/locales/en-US.json @@ -816,7 +816,7 @@ "message": "To set budgets for the quantity and size of page resources, add a budget.json file. [Learn more](https://developers.google.com/web/tools/lighthouse/audits/budgets)." }, "lighthouse-core/audits/resource-summary.js | displayValue": { - "message": "{requestCount, plural, =1 {1 request} other {# requests}} • {byteCount, number, bytes} KB" + "message": "{requestCount, plural, =1 {1 request • {byteCount, number, bytes} KB} other {# requests • {byteCount, number, bytes} KB}}" }, "lighthouse-core/audits/resource-summary.js | title": { "message": "Keep request counts low and transfer sizes small" diff --git a/lighthouse-core/lib/i18n/locales/en-XL.json b/lighthouse-core/lib/i18n/locales/en-XL.json index 626650ce15f6..43ffea7f806b 100644 --- a/lighthouse-core/lib/i18n/locales/en-XL.json +++ b/lighthouse-core/lib/i18n/locales/en-XL.json @@ -816,7 +816,7 @@ "message": "T̂ó ŝét̂ b́ûd́ĝét̂ś f̂ór̂ t́ĥé q̂úâńt̂ít̂ý âńd̂ śîźê óf̂ ṕâǵê ŕêśôúr̂ćêś, âd́d̂ á b̂úd̂ǵêt́.ĵśôń f̂íl̂é. [L̂éâŕn̂ ḿôŕê](https://developers.google.com/web/tools/lighthouse/audits/budgets)." }, "lighthouse-core/audits/resource-summary.js | displayValue": { - "message": "{requestCount, plural, =1 {1 r̂éq̂úêśt̂} other {# ŕêq́ûéŝt́ŝ}} • {byteCount, number, bytes} ḰB̂" + "message": "{requestCount, plural, =1 {1 r̂éq̂úêśt̂ • {byteCount, number, bytes} ḰB̂} other {# ŕêq́ûéŝt́ŝ • {byteCount, number, bytes} ḰB̂}}" }, "lighthouse-core/audits/resource-summary.js | title": { "message": "K̂éêṕ r̂éq̂úêśt̂ ćôún̂t́ŝ ĺôẃ âńd̂ t́r̂án̂śf̂ér̂ śîźêś ŝḿâĺl̂" From c1956bba2186a2aea9012e0b69effeff99cbfb84 Mon Sep 17 00:00:00 2001 From: Shane Exterkamp Date: Wed, 14 Aug 2019 17:56:10 -0700 Subject: [PATCH 2/8] fix ts, add comments, make loops more clear, add tests. --- lighthouse-core/audits/resource-summary.js | 3 +- lighthouse-core/lib/i18n/i18n.js | 53 +++++++++++++++++----- lighthouse-core/test/lib/i18n/i18n-test.js | 28 ++++++++++++ types/intl-messageformat-parser/index.d.ts | 4 +- 4 files changed, 73 insertions(+), 15 deletions(-) diff --git a/lighthouse-core/audits/resource-summary.js b/lighthouse-core/audits/resource-summary.js index 9bd1ee03e763..7b19f3c96002 100644 --- a/lighthouse-core/audits/resource-summary.js +++ b/lighthouse-core/audits/resource-summary.js @@ -16,7 +16,8 @@ const UIStrings = { description: 'To set budgets for the quantity and size of page resources,' + ' add a budget.json file. [Learn more](https://developers.google.com/web/tools/lighthouse/audits/budgets).', /** [ICU Syntax] Label for an audit identifying the number of requests and kilobytes used to load the page. */ - displayValue: `{requestCount, plural, =1 {1 request • {byteCount, number, bytes} KB} ` + + displayValue: `{requestCount, plural, ` + + `=1 {1 request • {byteCount, number, bytes} KB} ` + `other {# requests • {byteCount, number, bytes} KB}}`, }; diff --git a/lighthouse-core/lib/i18n/i18n.js b/lighthouse-core/lib/i18n/i18n.js index bfb08ace359d..482c4317a8f1 100644 --- a/lighthouse-core/lib/i18n/i18n.js +++ b/lighthouse-core/lib/i18n/i18n.js @@ -140,22 +140,43 @@ function _preprocessMessageValues(icuMessage, values = {}) { return clonedValues; } +/** + * Function to retrieve all 'arguementElement's from an ICU message. An arguementElement + * is an ICU element that has an arguement attached like {varName, number, bytes}. This + * differs from 'messageElement's which are just arbitrary text in a message. + * + * Note: This function will recursively inspect plural elements for nested arguementElements. + * + * @param {Array} parsedIcu + * @param {Map} elements + */ function _collectAllCustomElementsFromICU(parsedIcu, elements = new Map()) { // Rescurse into Plurals - parsedIcu - .filter(el => el.format && el.format.type === 'pluralFormat') - .forEach(el => { - const elemSet = new Map(); - el.format.options.forEach(opt => opt.value.elements.forEach(elem => elemSet.set(elem.id, elem))); - const e = Array.from(elemSet.values()); - - // de dupe into main elements - const rElements = _collectAllCustomElementsFromICU(e, elements); + for (const el of parsedIcu) { + if (!el.format || el.format.type !== 'pluralFormat') continue; + // We need to find all the elements from the plural format sections, but + // they need to be deduplicated. I.e. "=1{hello {icu}} =other{hello {icu}}" + // the variable "icu" would appear twice if it wasn't de duplicated. + const elemSet = new Map(); + // Look at all options of the plural (=1{} =other{}...) + for (const option of el.format.options) { + // Look at each element of each plural option + for (const element of option.value.elements) { + if (el.type !== 'argumentElement') continue; + // If the element is an arguement, then add it to the de-dupe map + elemSet.set(element.id, element); + } + } + const e = Array.from(elemSet.values()); + // Add the nested plural elements to the main elements set + const rElements = _collectAllCustomElementsFromICU(e, elements); - rElements.forEach((k, v) => elements[k] = v); - }); + for (const [key, value] of rElements) { + elements.set(key, value); + } + } - // add arguementElements + // add other arguementElements parsedIcu .filter(el => el.type === 'argumentElement') // @ts-ignore - el.id is always defined when el.format is defined @@ -164,6 +185,14 @@ function _collectAllCustomElementsFromICU(parsedIcu, elements = new Map()) { return elements; } +/** + * This function takes a list of ICU arguementElements and a map of values and + * will apply Lighthouse custom formatting to the values based on the arguementElement + * format style. + * + * @param {Array} icuElementsList + * @param {Record} [values] + */ function _processParsedElements(icuElementsList, values = {}) { // Throw an error if a message's value isn't provided icuElementsList diff --git a/lighthouse-core/test/lib/i18n/i18n-test.js b/lighthouse-core/test/lib/i18n/i18n-test.js index 2782a2bb1d44..eeb48c37a7b0 100644 --- a/lighthouse-core/test/lib/i18n/i18n-test.js +++ b/lighthouse-core/test/lib/i18n/i18n-test.js @@ -103,6 +103,17 @@ describe('i18n', () => { helloTimeInMsWorld: 'Hello {timeInMs, number, seconds} World', helloPercentWorld: 'Hello {in, number, extendedPercent} World', helloWorldMultiReplace: '{hello} {world}', + helloPlural: '{itemCount, plural, =1{1 hello} other{hellos}}', + helloPluralNestedICU: '{itemCount, plural, ' + + '=1{1 hello {in, number, bytes}} ' + + 'other{hellos {in, number, bytes}}}', + helloPluralNestedPluralAndICU: '{itemCount, plural, ' + + '=1{{innerItemCount, plural, ' + + '=1{1 hello 1 goodbye {in, number, bytes}} ' + + 'other{1 hello, goodbyes {in, number, bytes}}}} ' + + 'other{{innerItemCount, plural, ' + + '=1{hellos 1 goodbye {in, number, bytes}} ' + + 'other{hellos, goodbyes {in, number, bytes}}}}}', }; const str_ = i18n.createMessageInstanceIdFn(__filename, UIStrings); @@ -146,5 +157,22 @@ describe('i18n', () => { {hello: 'hello'}), 'en-US')) .toThrow(`ICU Message contains a value reference ("world") that wasn't provided`); }); + + it('formats a message with plurals', () => { + const helloStr = str_(UIStrings.helloPlural, {itemCount: 3}); + expect(helloStr).toBeDisplayString('hellos'); + }); + + it('formats a message with plurals and nested custom ICU', () => { + const helloStr = str_(UIStrings.helloPluralNestedICU, {itemCount: 3, in: 1875}); + expect(helloStr).toBeDisplayString('hellos 2'); + }); + + it('formats a message with plurals and nested custom ICU and nested plural', () => { + const helloStr = str_(UIStrings.helloPluralNestedPluralAndICU, {itemCount: 3, + innerItemCount: 1, + in: 1875}); + expect(helloStr).toBeDisplayString('hellos 1 goodbye 2'); + }); }); }); diff --git a/types/intl-messageformat-parser/index.d.ts b/types/intl-messageformat-parser/index.d.ts index 8f0f2b380ad8..02df41d18fd5 100644 --- a/types/intl-messageformat-parser/index.d.ts +++ b/types/intl-messageformat-parser/index.d.ts @@ -1,9 +1,9 @@ declare module 'intl-messageformat-parser' { - interface Element { + export interface Element { type: 'messageTextElement'|'argumentElement'; id?: string value?: string - format?: null | {type: string; style?: string}; + format?: null | {type: string; style?: string; options?: any}; } function parse(message: string): {elements: Element[]}; export {parse}; From 9c1f7636c49552b372eb8356e4aa1dae60d6de5a Mon Sep 17 00:00:00 2001 From: Shane Exterkamp Date: Wed, 14 Aug 2019 18:03:51 -0700 Subject: [PATCH 3/8] fixed spacing --- lighthouse-core/audits/resource-summary.js | 4 ++-- lighthouse-core/lib/i18n/locales/en-US.json | 2 +- lighthouse-core/lib/i18n/locales/en-XL.json | 2 +- 3 files changed, 4 insertions(+), 4 deletions(-) diff --git a/lighthouse-core/audits/resource-summary.js b/lighthouse-core/audits/resource-summary.js index 7b19f3c96002..2413e18d365c 100644 --- a/lighthouse-core/audits/resource-summary.js +++ b/lighthouse-core/audits/resource-summary.js @@ -17,8 +17,8 @@ const UIStrings = { ' add a budget.json file. [Learn more](https://developers.google.com/web/tools/lighthouse/audits/budgets).', /** [ICU Syntax] Label for an audit identifying the number of requests and kilobytes used to load the page. */ displayValue: `{requestCount, plural, ` + - `=1 {1 request • {byteCount, number, bytes} KB} ` + - `other {# requests • {byteCount, number, bytes} KB}}`, + `=1 {1 request • {byteCount, number, bytes} KB} ` + + `other {# requests • {byteCount, number, bytes} KB}}`, }; const str_ = i18n.createMessageInstanceIdFn(__filename, UIStrings); diff --git a/lighthouse-core/lib/i18n/locales/en-US.json b/lighthouse-core/lib/i18n/locales/en-US.json index bab247565922..f86c5606b480 100644 --- a/lighthouse-core/lib/i18n/locales/en-US.json +++ b/lighthouse-core/lib/i18n/locales/en-US.json @@ -879,7 +879,7 @@ "message": "To set budgets for the quantity and size of page resources, add a budget.json file. [Learn more](https://developers.google.com/web/tools/lighthouse/audits/budgets)." }, "lighthouse-core/audits/resource-summary.js | displayValue": { - "message": "{requestCount, plural, =1 {1 request • {byteCount, number, bytes} KB} other {# requests • {byteCount, number, bytes} KB}}" + "message": "{requestCount, plural, =1 {1 request • {byteCount, number, bytes} KB} other {# requests • {byteCount, number, bytes} KB}}" }, "lighthouse-core/audits/resource-summary.js | title": { "message": "Keep request counts low and transfer sizes small" diff --git a/lighthouse-core/lib/i18n/locales/en-XL.json b/lighthouse-core/lib/i18n/locales/en-XL.json index 1a5aa7151214..dceafdf2b656 100644 --- a/lighthouse-core/lib/i18n/locales/en-XL.json +++ b/lighthouse-core/lib/i18n/locales/en-XL.json @@ -879,7 +879,7 @@ "message": "T̂ó ŝét̂ b́ûd́ĝét̂ś f̂ór̂ t́ĥé q̂úâńt̂ít̂ý âńd̂ śîźê óf̂ ṕâǵê ŕêśôúr̂ćêś, âd́d̂ á b̂úd̂ǵêt́.ĵśôń f̂íl̂é. [L̂éâŕn̂ ḿôŕê](https://developers.google.com/web/tools/lighthouse/audits/budgets)." }, "lighthouse-core/audits/resource-summary.js | displayValue": { - "message": "{requestCount, plural, =1 {1 r̂éq̂úêśt̂ • {byteCount, number, bytes} ḰB̂} other {# ŕêq́ûéŝt́ŝ • {byteCount, number, bytes} ḰB̂}}" + "message": "{requestCount, plural, =1 {1 r̂éq̂úêśt̂ • {byteCount, number, bytes} ḰB̂} other {# ŕêq́ûéŝt́ŝ • {byteCount, number, bytes} ḰB̂}}" }, "lighthouse-core/audits/resource-summary.js | title": { "message": "K̂éêṕ r̂éq̂úêśt̂ ćôún̂t́ŝ ĺôẃ âńd̂ t́r̂án̂śf̂ér̂ śîźêś ŝḿâĺl̂" From 2b5d09f1cfa42c02a071d0e977acf2649e1b2a7c Mon Sep 17 00:00:00 2001 From: Shane Exterkamp Date: Thu, 15 Aug 2019 15:02:55 -0700 Subject: [PATCH 4/8] Fix spelling Co-Authored-By: Patrick Hulce --- lighthouse-core/lib/i18n/i18n.js | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/lighthouse-core/lib/i18n/i18n.js b/lighthouse-core/lib/i18n/i18n.js index 482c4317a8f1..8e751c478eec 100644 --- a/lighthouse-core/lib/i18n/i18n.js +++ b/lighthouse-core/lib/i18n/i18n.js @@ -141,8 +141,8 @@ function _preprocessMessageValues(icuMessage, values = {}) { } /** - * Function to retrieve all 'arguementElement's from an ICU message. An arguementElement - * is an ICU element that has an arguement attached like {varName, number, bytes}. This + * Function to retrieve all 'argumentElement's from an ICU message. An argumentElement + * is an ICU element that has an argument attached like {varName, number, bytes}. This * differs from 'messageElement's which are just arbitrary text in a message. * * Note: This function will recursively inspect plural elements for nested arguementElements. @@ -163,7 +163,7 @@ function _collectAllCustomElementsFromICU(parsedIcu, elements = new Map()) { // Look at each element of each plural option for (const element of option.value.elements) { if (el.type !== 'argumentElement') continue; - // If the element is an arguement, then add it to the de-dupe map + // If the element is an argument, then add it to the de-dupe map elemSet.set(element.id, element); } } @@ -186,8 +186,8 @@ function _collectAllCustomElementsFromICU(parsedIcu, elements = new Map()) { } /** - * This function takes a list of ICU arguementElements and a map of values and - * will apply Lighthouse custom formatting to the values based on the arguementElement + * This function takes a list of ICU argumentElements and a map of values and + * will apply Lighthouse custom formatting to the values based on the argumentElement * format style. * * @param {Array} icuElementsList From 068d4d88061d364839fb33408f1cee4fbe2062e3 Mon Sep 17 00:00:00 2001 From: Shane Exterkamp Date: Thu, 15 Aug 2019 15:48:47 -0700 Subject: [PATCH 5/8] patrick feedback --- lighthouse-core/lib/i18n/i18n.js | 40 ++++++++++++++------------------ 1 file changed, 17 insertions(+), 23 deletions(-) diff --git a/lighthouse-core/lib/i18n/i18n.js b/lighthouse-core/lib/i18n/i18n.js index 8e751c478eec..963d4096f851 100644 --- a/lighthouse-core/lib/i18n/i18n.js +++ b/lighthouse-core/lib/i18n/i18n.js @@ -147,42 +147,36 @@ function _preprocessMessageValues(icuMessage, values = {}) { * * Note: This function will recursively inspect plural elements for nested arguementElements. * - * @param {Array} parsedIcu - * @param {Map} elements + * @param {Array} elementsList + * @param {Map} seenElelementsById */ -function _collectAllCustomElementsFromICU(parsedIcu, elements = new Map()) { +function _collectAllCustomElementsFromICU(elementsList, seenElelementsById = new Map()) { + // add argumentElements + elementsList + .filter(el => el.type === 'argumentElement') + // @ts-ignore - el.id is always defined when el.format is defined + .forEach(el => seenElelementsById.set(el.id, el)); + // Rescurse into Plurals - for (const el of parsedIcu) { + for (const el of elementsList) { if (!el.format || el.format.type !== 'pluralFormat') continue; // We need to find all the elements from the plural format sections, but // they need to be deduplicated. I.e. "=1{hello {icu}} =other{hello {icu}}" // the variable "icu" would appear twice if it wasn't de duplicated. - const elemSet = new Map(); + let childElementsById = new Map(); // Look at all options of the plural (=1{} =other{}...) for (const option of el.format.options) { - // Look at each element of each plural option - for (const element of option.value.elements) { - if (el.type !== 'argumentElement') continue; - // If the element is an argument, then add it to the de-dupe map - elemSet.set(element.id, element); - } + // Run collections on each option's elements + childElementsById = _collectAllCustomElementsFromICU(option.value.elements, + seenElelementsById); } - const e = Array.from(elemSet.values()); // Add the nested plural elements to the main elements set - const rElements = _collectAllCustomElementsFromICU(e, elements); - - for (const [key, value] of rElements) { - elements.set(key, value); + for (const [key, value] of childElementsById) { + seenElelementsById.set(key, value); } } - // add other arguementElements - parsedIcu - .filter(el => el.type === 'argumentElement') - // @ts-ignore - el.id is always defined when el.format is defined - .forEach(el => elements.set(el.id, el)); - - return elements; + return seenElelementsById; } /** From 28a5f245bcce4b1e4f3ed01ee699c4f98472c1f8 Mon Sep 17 00:00:00 2001 From: Shane Exterkamp Date: Fri, 16 Aug 2019 10:13:14 -0700 Subject: [PATCH 6/8] feedback +1, massively simplify recursion, add new test. --- lighthouse-core/lib/i18n/i18n.js | 39 ++++++++++------------ lighthouse-core/test/lib/i18n/i18n-test.js | 5 +++ 2 files changed, 22 insertions(+), 22 deletions(-) diff --git a/lighthouse-core/lib/i18n/i18n.js b/lighthouse-core/lib/i18n/i18n.js index 963d4096f851..6345aa560750 100644 --- a/lighthouse-core/lib/i18n/i18n.js +++ b/lighthouse-core/lib/i18n/i18n.js @@ -130,50 +130,45 @@ function lookupLocale(locale) { * @param {Record} [values] */ function _preprocessMessageValues(icuMessage, values = {}) { - let clonedValues = JSON.parse(JSON.stringify(values)); const parsed = MessageParser.parse(icuMessage); const elements = _collectAllCustomElementsFromICU(parsed.elements); - clonedValues = _processParsedElements(Array.from(elements.values()), clonedValues); - - return clonedValues; + return _processParsedElements(Array.from(elements.values()), JSON.parse(JSON.stringify(values))); } /** * Function to retrieve all 'argumentElement's from an ICU message. An argumentElement - * is an ICU element that has an argument attached like {varName, number, bytes}. This + * is an ICU element with an argument in it, like '{varName}' or '{varName, number, bytes}'. This * differs from 'messageElement's which are just arbitrary text in a message. * - * Note: This function will recursively inspect plural elements for nested arguementElements. + * Notes: + * This function will recursively inspect plural elements for nested argumentElements. + * + * We need to find all the elements from the plural format sections, but + * they need to be deduplicated. I.e. "=1{hello {icu}} =other{hello {icu}}" + * the variable "icu" would appear twice if it wasn't de duplicated. And they cannot + * 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} elementsList * @param {Map} seenElelementsById */ function _collectAllCustomElementsFromICU(elementsList, seenElelementsById = new Map()) { - // add argumentElements - elementsList - .filter(el => el.type === 'argumentElement') - // @ts-ignore - el.id is always defined when el.format is defined - .forEach(el => seenElelementsById.set(el.id, el)); - - // Rescurse into Plurals for (const el of elementsList) { + // 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 + seenElelementsById.set(el.id, el); + + // Plurals need to be inspected recursively if (!el.format || el.format.type !== 'pluralFormat') continue; - // We need to find all the elements from the plural format sections, but - // they need to be deduplicated. I.e. "=1{hello {icu}} =other{hello {icu}}" - // the variable "icu" would appear twice if it wasn't de duplicated. - let childElementsById = new Map(); // Look at all options of the plural (=1{} =other{}...) for (const option of el.format.options) { // Run collections on each option's elements - childElementsById = _collectAllCustomElementsFromICU(option.value.elements, + _collectAllCustomElementsFromICU(option.value.elements, seenElelementsById); } - // Add the nested plural elements to the main elements set - for (const [key, value] of childElementsById) { - seenElelementsById.set(key, value); - } } return seenElelementsById; diff --git a/lighthouse-core/test/lib/i18n/i18n-test.js b/lighthouse-core/test/lib/i18n/i18n-test.js index eeb48c37a7b0..53c5789dbe47 100644 --- a/lighthouse-core/test/lib/i18n/i18n-test.js +++ b/lighthouse-core/test/lib/i18n/i18n-test.js @@ -163,6 +163,11 @@ describe('i18n', () => { expect(helloStr).toBeDisplayString('hellos'); }); + 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`); + }); + it('formats a message with plurals and nested custom ICU', () => { const helloStr = str_(UIStrings.helloPluralNestedICU, {itemCount: 3, in: 1875}); expect(helloStr).toBeDisplayString('hellos 2'); From 9f6f5f37e974125734351712f7772f47a32e584a Mon Sep 17 00:00:00 2001 From: Shane Exterkamp Date: Fri, 16 Aug 2019 11:55:50 -0700 Subject: [PATCH 7/8] rename vars --- lighthouse-core/lib/i18n/i18n.js | 26 +++++++++++++------------- 1 file changed, 13 insertions(+), 13 deletions(-) diff --git a/lighthouse-core/lib/i18n/i18n.js b/lighthouse-core/lib/i18n/i18n.js index 6345aa560750..56c8f8702ab9 100644 --- a/lighthouse-core/lib/i18n/i18n.js +++ b/lighthouse-core/lib/i18n/i18n.js @@ -151,15 +151,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} elementsList - * @param {Map} seenElelementsById + * @param {Array} icuElements + * @param {Map} seenElementsById */ -function _collectAllCustomElementsFromICU(elementsList, seenElelementsById = new Map()) { - for (const el of elementsList) { +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 - seenElelementsById.set(el.id, el); + seenElementsById.set(el.id, el); // Plurals need to be inspected recursively if (!el.format || el.format.type !== 'pluralFormat') continue; @@ -167,11 +167,11 @@ function _collectAllCustomElementsFromICU(elementsList, seenElelementsById = new for (const option of el.format.options) { // Run collections on each option's elements _collectAllCustomElementsFromICU(option.value.elements, - seenElelementsById); + seenElementsById); } } - return seenElelementsById; + return seenElementsById; } /** @@ -179,12 +179,12 @@ function _collectAllCustomElementsFromICU(elementsList, seenElelementsById = new * will apply Lighthouse custom formatting to the values based on the argumentElement * format style. * - * @param {Array} icuElementsList + * @param {Array} argumentElements * @param {Record} [values] */ -function _processParsedElements(icuElementsList, values = {}) { +function _processParsedElements(argumentElements, values = {}) { // Throw an error if a message's value isn't provided - icuElementsList + argumentElements .filter(el => el.type === 'argumentElement') .forEach(el => { if (el.id && (el.id in values) === false) { @@ -193,19 +193,19 @@ function _processParsedElements(icuElementsList, values = {}) { }); // Round all milliseconds to the nearest 10 - icuElementsList + 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 - icuElementsList + 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 - icuElementsList + 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)); From 1bcbaa39b72f625ebb6647fd28f00c39b5f37569 Mon Sep 17 00:00:00 2001 From: Shane Exterkamp Date: Fri, 16 Aug 2019 11:58:09 -0700 Subject: [PATCH 8/8] clonedValues --- lighthouse-core/lib/i18n/i18n.js | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/lighthouse-core/lib/i18n/i18n.js b/lighthouse-core/lib/i18n/i18n.js index 56c8f8702ab9..8547f1b07256 100644 --- a/lighthouse-core/lib/i18n/i18n.js +++ b/lighthouse-core/lib/i18n/i18n.js @@ -130,11 +130,12 @@ function lookupLocale(locale) { * @param {Record} [values] */ function _preprocessMessageValues(icuMessage, values = {}) { + const clonedValues = JSON.parse(JSON.stringify(values)); const parsed = MessageParser.parse(icuMessage); const elements = _collectAllCustomElementsFromICU(parsed.elements); - return _processParsedElements(Array.from(elements.values()), JSON.parse(JSON.stringify(values))); + return _processParsedElements(Array.from(elements.values()), clonedValues); } /**