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: use better types for intl-messageformat #9570

Merged
merged 3 commits into from
Aug 20, 2019
Merged

i18n: use better types for intl-messageformat #9570

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
be15fb9
i18n: use better types for intl-messageformat
brendankenny Aug 17, 2019
4d1b106
remove explicit intl-messageformat-parser dependency
brendankenny Aug 19, 2019
facfd35
line breaks
brendankenny Aug 19, 2019
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
1 change: 1 addition & 0 deletions build/build-bundle.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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')
Expand Down
82 changes: 47 additions & 35 deletions lighthouse-core/lib/i18n/i18n.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 */
Copy link
Member Author

Choose a reason for hiding this comment

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

this is still a direct reference to what is now a transitive dependency, but the intl-messageformat d.ts files type these by reference into its dependencies, so I don't know any other nice way to type these but this (unless we want to do some kind of ReturnType<MessageFormat['getAst']>['elements'][0] :)

Copy link
Collaborator

Choose a reason for hiding this comment

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

I definitely prefer the approach you already have here :)

/** @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.
Expand All @@ -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');
}
Expand Down Expand Up @@ -128,14 +135,15 @@ function lookupLocale(locale) {
/**
* @param {string} icuMessage
* @param {Record<string, string | number>} [values]
* @return {Record<string, string | number>}
*/
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()), clonedValues);
return _processParsedElements(icuMessage, Array.from(elements.values()), clonedValues);
}

/**
Expand All @@ -152,23 +160,23 @@ 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<import('intl-messageformat-parser').Element>} icuElements
* @param {Map<string, import('intl-messageformat-parser').Element>} seenElementsById
* @param {Array<MessageElement>} icuElements
* @param {Map<string, ArgumentElement>} seenElementsById
* @return {Map<string, ArgumentElement>}
*/
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
if (!el.format || el.format.type !== 'pluralFormat') continue;
// 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);
}
}

Expand All @@ -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<import('intl-messageformat-parser').Element>} argumentElements
* @param {string} icuMessage
Copy link
Member Author

Choose a reason for hiding this comment

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

included for logging. While playing around with some other error conditions I found it basically impossible to know which message the erroring itemCount or timeInMs was coming from, so this was really needed in some form

* @param {Array<ArgumentElement>} argumentElements
* @param {Record<string, string | number>} [values]
* @return {Record<string, string | number>}
*/
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
Copy link
Member

Choose a reason for hiding this comment

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

Lots of ignoring line-length here 😢

Copy link
Collaborator

Choose a reason for hiding this comment

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

summoning @connorjclark and the word-wrap gang! 😈

image

Copy link
Member Author

Choose a reason for hiding this comment

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

Lots of ignoring line-length here 😢

I think in the tests there's no value in breaking the string literals up, it just makes the expectations harder to read.

For these two, they're error messages that are only for development time (if a user gets them that means LH will be totally broken), so I don't see much benefit in breaking them in two and multiple lines kind of distracts from the non-error flow of the function, but I also don't feel strongly about it.

Copy link
Collaborator

Choose a reason for hiding this comment

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

imo the line length lint rule should ignore long lines that are 80% a single string 😎

Copy link
Member

Choose a reason for hiding this comment

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

imo the line length lint rule should ignore long lines that are 80% a single string

Copy link
Member Author

Choose a reason for hiding this comment

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

broke the strings...for cases of disagreement we should default to our established style guide/lint rules :)

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`);
Copy link
Member Author

Choose a reason for hiding this comment

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

makes tsc happy to prove these are numbers (for the formatting divisions below) and it seems like a reasonable error condition to call out

}

// 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;
}
Expand Down
18 changes: 15 additions & 3 deletions lighthouse-core/test/lib/i18n/i18n-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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', () => {
Expand All @@ -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', () => {
Expand All @@ -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`);
});
});
});
6 changes: 3 additions & 3 deletions package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand Down Expand Up @@ -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",
Copy link
Collaborator

Choose a reason for hiding this comment

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

wow that's a big jump, didn't we just add this a year ago? I thought intl rules would be more stable by now 😆

Copy link
Member Author

Choose a reason for hiding this comment

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

wow that's a big jump, didn't we just add this a year ago? I thought intl rules would be more stable by now 😆

yeah, it was unchanged for two years until three months ago, at which point the versions started exploding :)

https://www.npmjs.com/package/intl-messageformat?activeTab=versions

I mentioned above, but I didn't pick the latest version because of the change in the AST format in 5.0.0 that didn't seem worth updating for given that everything works today.

"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",
Expand Down
10 changes: 0 additions & 10 deletions types/intl-messageformat-parser/index.d.ts
View file
Open in desktop

This file was deleted.

37 changes: 23 additions & 14 deletions yarn.lock
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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"
Expand Down Expand Up @@ -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"

[email protected]:
version "1.0.11"
resolved "https://registry.yarnpkg.com/makeerror/-/makeerror-1.0.11.tgz#e01a5c9109f2af79660e4e8b9587790184f5a96c"
Expand Down