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

i18n: fix custom formatted ICU within plurals #9460

Merged
merged 9 commits into from
Aug 16, 2019
Merged
Show file tree
Hide file tree
Changes from 4 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
5 changes: 3 additions & 2 deletions lighthouse-core/audits/resource-summary.js
Original file line number Diff line number Diff line change
Expand Up @@ -16,8 +16,9 @@ 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);
Expand Down
80 changes: 71 additions & 9 deletions lighthouse-core/lib/i18n/i18n.js
Original file line number Diff line number Diff line change
Expand Up @@ -130,10 +130,72 @@ function lookupLocale(locale) {
* @param {Record<string, string | number>} [values]
*/
function _preprocessMessageValues(icuMessage, values = {}) {
const clonedValues = JSON.parse(JSON.stringify(values));
let clonedValues = JSON.parse(JSON.stringify(values));
exterkamp marked this conversation as resolved.
Show resolved Hide resolved
const parsed = MessageParser.parse(icuMessage);

const elements = _collectAllCustomElementsFromICU(parsed.elements);

clonedValues = _processParsedElements(Array.from(elements.values()), clonedValues);

return clonedValues;
}

/**
* Function to retrieve all 'arguementElement's from an ICU message. An arguementElement
exterkamp marked this conversation as resolved.
Show resolved Hide resolved
* is an ICU element that has an arguement attached like {varName, number, bytes}. This
exterkamp marked this conversation as resolved.
Show resolved Hide resolved
* 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<import('intl-messageformat-parser').Element>} parsedIcu
* @param {Map<string, import('intl-messageformat-parser').Element>} elements
Copy link
Collaborator

Choose a reason for hiding this comment

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

unexpected that this returned a map, can we add the return type explicitly?

*/
function _collectAllCustomElementsFromICU(parsedIcu, elements = new Map()) {
exterkamp marked this conversation as resolved.
Show resolved Hide resolved
// Rescurse into Plurals
exterkamp marked this conversation as resolved.
Show resolved Hide resolved
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();
exterkamp marked this conversation as resolved.
Show resolved Hide resolved
// 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) {
exterkamp marked this conversation as resolved.
Show resolved Hide resolved
if (el.type !== 'argumentElement') continue;
// If the element is an arguement, then add it to the de-dupe map
exterkamp marked this conversation as resolved.
Show resolved Hide resolved
elemSet.set(element.id, element);
}
}
const e = Array.from(elemSet.values());
exterkamp marked this conversation as resolved.
Show resolved Hide resolved
// 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);
}
}

// 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;
}

/**
* This function takes a list of ICU arguementElements and a map of values and
exterkamp marked this conversation as resolved.
Show resolved Hide resolved
* will apply Lighthouse custom formatting to the values based on the arguementElement
exterkamp marked this conversation as resolved.
Show resolved Hide resolved
* format style.
*
* @param {Array<import('intl-messageformat-parser').Element>} icuElementsList
exterkamp marked this conversation as resolved.
Show resolved Hide resolved
* @param {Record<string, string | number>} [values]
*/
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) {
Expand All @@ -142,24 +204,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;
}

/**
Expand Down
2 changes: 1 addition & 1 deletion lighthouse-core/lib/i18n/locales/en-US.json
Original file line number Diff line number Diff line change
Expand Up @@ -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} 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"
Expand Down
2 changes: 1 addition & 1 deletion lighthouse-core/lib/i18n/locales/en-XL.json
Original file line number Diff line number Diff line change
Expand Up @@ -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̂} 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̂"
Expand Down
28 changes: 28 additions & 0 deletions lighthouse-core/test/lib/i18n/i18n-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -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);

Expand Down Expand Up @@ -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');
});
});
});
4 changes: 2 additions & 2 deletions types/intl-messageformat-parser/index.d.ts
Original file line number Diff line number Diff line change
@@ -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};
Expand Down