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.

Sign up for GitHub

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

Jump to bottom

i18n: reduce unnecessary message formats #14030

Merged
merged 3 commits into from
May 31, 2022
Merged

i18n: reduce unnecessary message formats #14030

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
2ec72f2
i18n: reduce unnecessary message formats
brendankenny May 19, 2022
e6b7090
Merge branch 'master' into cut-the-formats
paulirish May 26, 2022
0f8b5fc
Merge remote-tracking branch 'origin/master' into cut-the-formats
connorjclark May 31, 2022
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
10 changes: 7 additions & 3 deletions shared/localization/format.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -100,7 +100,7 @@ function collectAllCustomElementsFromICU(icuElements, seenElementsById = new Map
* @param {string} lhlMessage Used for clear error logging.
* @return {Record<string, string | number>}
*/
function _preformatValues(messageFormatter, values, lhlMessage) {
function _preformatValues(messageFormatter, values = {}, lhlMessage) {
const elementMap = collectAllCustomElementsFromICU(messageFormatter.getAst().elements);
const argumentElements = [...elementMap.values()];

Expand Down Expand Up @@ -165,11 +165,15 @@ function _preformatValues(messageFormatter, values, lhlMessage) {
* is assumed to already be in the given locale.
* If you need to localize a messagem `getFormatted` is probably what you want.
* @param {string} message
* @param {Record<string, string | number>} values
* @param {Record<string, string | number>|undefined} values
* @param {LH.Locale} locale
* @return {string}
*/
function formatMessage(message, values = {}, locale) {
function formatMessage(message, values, locale) {
// Parsing and formatting can be slow. Don't attempt if the string can't
// contain ICU placeholders, in which case formatting is already complete.
if (!message.includes('{') && values === undefined) return message;
Copy link
Member Author

@brendankenny brendankenny May 19, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

also requires that values is undefined so it preserves the behavior of throwing on invalid values for messages with no placeholders


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

Expand Down
8 changes: 8 additions & 0 deletions shared/test/localization/format-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -400,6 +400,14 @@ describe('format', () => {
.toThrow(`Provided value "sirNotAppearingInThisString" does not match any placeholder in ICU message "Hello {timeInMs, number, seconds} World"`);
});

it('throws an error if a value is provided for a message with no placeholders', () => {
expect(_ => str_(UIStrings.helloWorld, {
extraCreditValue: 100,
}))
// eslint-disable-next-line max-len
.toThrow(`Provided value "extraCreditValue" does not match any placeholder in ICU message "Hello World"`);
});

it('formats correctly with NaN and Infinity numeric values', () => {
const helloInfinityStr = str_(UIStrings.helloBytesWorld, {in: Infinity});
expect(helloInfinityStr).toBeDisplayString('Hello ∞ World');
Expand Down