i18n: use better types for intl-messageformat #9570
Conversation
| @@ -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 | |||
There was a problem hiding this comment.
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
lighthouse-core/lib/i18n/i18n.js
Outdated
| 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`); |
There was a problem hiding this comment.
makes tsc happy to prove these are numbers (for the formatting divisions below) and it seems like a reasonable error condition to call out
|
turns out formatters have a |
| const lookupClosestLocale = require('lookup-closest-locale'); | ||
| const LOCALES = require('./locales.js'); | ||
|
|
||
| /** @typedef {import('intl-messageformat-parser').Element} MessageElement */ |
There was a problem hiding this comment.
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] :)
There was a problem hiding this comment.
I definitely prefer the approach you already have here :)
| const lookupClosestLocale = require('lookup-closest-locale'); | ||
| const LOCALES = require('./locales.js'); | ||
|
|
||
| /** @typedef {import('intl-messageformat-parser').Element} MessageElement */ |
There was a problem hiding this comment.
I definitely prefer the approach you already have here :)
| @@ -140,8 +139,8 @@ | |||
| "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", | |||
There was a problem hiding this comment.
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 😆
There was a problem hiding this comment.
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.
lighthouse-core/lib/i18n/i18n.js
Outdated
| 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 |
There was a problem hiding this comment.
Lots of ignoring line-length here 😢
There was a problem hiding this comment.
summoning @connorjclark and the word-wrap gang! 😈
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
imo the line length lint rule should ignore long lines that are 80% a single string 😎
There was a problem hiding this comment.
imo the line length lint rule should ignore long lines that are 80% a single string
There was a problem hiding this comment.
broke the strings...for cases of disagreement we should default to our established style guide/lint rules :)
brendankenny
force-pushed
the
messagetypes
branch
from
c8f1f40
to
facfd35
Compare
#9460 was somewhat painful to write partially because our custom types for
intl-messageformatandintl-messageformat-parserwere pretty light. A few versions after our deps, though, those two libraries added great d.ts type files, so this PR updates to those and uses them, clearing out some of the old type guards and// @ts-ignorelines (and making any future editing of this code considerably easier).Note that this only updates
intl-messageformat(and the parser) to the version right before a complete rewrite of their AST from this July as adapting to that seemed too disruptive a change.There were few changes in the two libraries: two pretty corner-case bug fixes and a move of
intl-pluralrulesfrom internally bundled to required on the globalIntl.PluralRules. That's now polyfilled likeIntl.NumberFormatandIntl.DateTimeFormatare, and similar to those there will be no bundle impact as the API already exists in the browser so it's ignored in the build step.