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: use IcuMessage objects instead of string IDs #10630

Merged
merged 8 commits into from
Sep 17, 2020
Merged

Conversation

brendankenny
Copy link
Member

As discussed in #10614 and offline, this moves us to using objects for localizable strings. This has several advantages (listed in that issue) and it turns out few disadvantages.

Proposed object:

type IcuMessage = {
  /** The id locating this message in the locale message json files. */
  i18nId: string;
  /** The dynamic values, if any, to insert into the localized string. */
  values?: Record<string, string | number>;
  /** A formatted version of the string, usually as found in a file's `UIStrings` entry, used as backup. */
  formattedDefault: string,
};

Biggest change from the proposal is the formattedDefault which will always be there. This is basically what swap-locale already does (uses the string already in the LHR as backup if lighthouse has changed enough that the message ID no longer refers to a translated string anymore) but uses it for allowing serialization at any stage, as well as for the current UIStrings backup during development (allowing use of a string without having to run yarn update:sample-json after every edit).

The PR changes are a little spread out, but the substantial changes are in

  • i18n.js
  • i18n.d.ts
  • 'swap-locale.js`
  • runner.js
  • details-renderer.js
  • audit.d.ts, audit-details.d.ts, lhr.d.ts
    more or less in that order for significance of the changes.

@brendankenny brendankenny requested a review from a team as a code owner April 24, 2020 15:13
@brendankenny brendankenny requested review from exterkamp and removed request for a team April 24, 2020 15:13
Copy link
Member Author

@brendankenny brendankenny left a comment

Choose a reason for hiding this comment

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

I'm going to update the i18n README so if anyone reviewing would rather wait for that to be committed before looking, feel free to do so :)

@@ -20,7 +20,7 @@ const assetSaver = require('../lighthouse-core/lib/asset-saver.js');

const open = require('open');

/** @typedef {import('../lighthouse-core/lib/lh-error.js')} LighthouseError */
/** @typedef {Error & {code: string, friendlyMessage?: string}} ExitError */
Copy link
Member Author

Choose a reason for hiding this comment

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

wasn't worth pretending this was a LighthouseError since either it's a regular Error or we construct it as just an object down on line 224 :)

* @param {LH.Config.Json} config
* @return {Array<{id: string, title: string}>}
*/
static getCategories(config) {
Copy link
Member Author

@brendankenny brendankenny Apr 24, 2020

Choose a reason for hiding this comment

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

this is only used in two tests now and not any active code (maybe after the extension was removed?), so it seemed easier just to delete than to deal with localization

lighthouse-core/lib/i18n/swap-locale.js Outdated Show resolved Hide resolved
/** @typedef {LH.FormattedIcu<LH.Audit.Details.Opportunity>} OpportunityTable */
/** @typedef {LH.FormattedIcu<LH.Audit.Details.OpportunityColumnHeading>} OpportunityTableHeading */
/** @typedef {LH.FormattedIcu<LH.Audit.Details.Table>} Table */
/** @typedef {LH.FormattedIcu<LH.Audit.Details.TableColumnHeading>} TableHeading */
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 my least favorite part of this change and I haven't come up with a nice solution yet. Unlike with Audit product/result, we don't have a good split on audit details (the same types are used in audits and in the LHR).

The types are all correct on the LHR, but if you want to refer to the type in isolation (and don't want to do LH.Result['audits'][number]['details']['whatever']['etc']['etc']) you need a reference to the type directly like this.

Other options:

  • we have some sort of audit details product/result split?
  • move these under LH.Result namespace?

Copy link
Collaborator

Choose a reason for hiding this comment

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

move these under LH.Result namespace?

That seems reasonable but this works too.

// i18n error strings
err.friendlyMessage = i18n.getFormatted(err.friendlyMessage, settings.locale);
// i18n LighthouseError strings.
if (err.friendlyMessage) {
Copy link
Member Author

Choose a reason for hiding this comment

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

turns out these aren't always LHErrors and we were often formatting undefined :)

*/
function _formatIcuMessage(locale, icuMessageId, uiStringMessage, values = {}) {
function _localizeIcuMessage(icuMessage, locale) {
Copy link
Member

Choose a reason for hiding this comment

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

i like the new split between _localizeIcuMessage and _formatMessage

* generate the string ids.
*
* Returns a function that generates `LH.IcuMessage` objects to localize the
* messages in `fileStrings` and the shared `i18n.UIStrings`.
* @param {string} filename
* @param {Record<string, string>} fileStrings
*/
function createMessageInstanceIdFn(filename, fileStrings) {
Copy link
Member

Choose a reason for hiding this comment

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

can we rename this to createIcuMessageFn internally here.. we can still expose it in module.exports as the long name (for 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.

can we rename this to createIcuMessageFn internally here.. we can still expose it in module.exports as the long name (for now)

should both names be exposed and we convert all Lighthouse audits over to using createIcuMessageFn?

lighthouse-core/lib/i18n/swap-locale.js Outdated Show resolved Hide resolved
Copy link
Collaborator

@patrickhulce patrickhulce left a comment

Choose a reason for hiding this comment

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

so it was mostly s/string/string|IcuMessage/ in audits afterall :)

it does seem pretty smooth and the increased future flexibility seems like a good time to do it, very impressive 👍

@@ -208,7 +208,7 @@ class RenderBlockingResources extends Audit {
static async audit(artifacts, context) {
const {results, wastedMs} = await RenderBlockingResources.computeResults(artifacts, context);

let displayValue = '';
let displayValue;
Copy link
Collaborator

Choose a reason for hiding this comment

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

these are inferred correctly, right? in tsc playground it seems to work out out OK but some of the assertions we're removing later on in jsdoc makes me think some of them were necessary at one point in time for our checkJs setup...

https://www.typescriptlang.org/play/#code/DYUwLgBAZg9jDcAoRBLKEAUBZAhmAFgHQBOOAdgCYwC2GAlBAHwQAMhArAwN6IR-RwIAXghcAHgC4IARgA0EAJ5SATAF9E65AGMYZAM4xQhYDADmGWDEIAvOkA

Copy link
Member Author

Choose a reason for hiding this comment

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

these are inferred correctly, right?

yes, they haven't been necessary for a while. I don't remember when conditional initialization like this was added, but basically the CFG typing keeps getting better.

* @param {unknown} value
* @return {value is string|LH.IcuMessage}
*/
function isStringOrIcuMessage(value) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

should this just live on i18n or you feel like it won't come up again?

Copy link
Member Author

Choose a reason for hiding this comment

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

should this just live on i18n or you feel like it won't come up again?

it seemed like just an internal helper fn for config (since the inline check was too long for the line and repeated 3x), so it's just scoped to the module, but I don't have strong feelings about keeping it locked away.

@@ -213,13 +213,15 @@ CTC is a name that is distinct and identifies this as the Chrome translation for

# Appendix

TODO(bckenny): update
Copy link
Collaborator

Choose a reason for hiding this comment

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

is this TODO as part of this PR or later? :)

Copy link
Member Author

Choose a reason for hiding this comment

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

is this TODO as part of this PR or later? :)

part of this PR :) I'll try to figure out what I thought should be added

lighthouse-core/lib/i18n/i18n.js Show resolved Hide resolved
* @param {MessageFormat} messageFormatter
* @param {Readonly<Record<string, string | number>>} values
* @param {string} icuMessage Used for clear error logging.
Copy link
Collaborator

Choose a reason for hiding this comment

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

should we rename this param now that icuMessage means that object thing returned from a string we want localized

Copy link
Member Author

Choose a reason for hiding this comment

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

should we rename this param now that icuMessage means that object thing returned from a string we want localized

I'll use lhlMessage since it's in that format? Or should it be rawMessage or..something?

lighthouse-core/lib/i18n/i18n.js Show resolved Hide resolved
types/i18n.d.ts Outdated
* Heavy handed and requires more type checks, so prefer explicitly setting
* properties to include `LH.IcuMessage` over this helper if possible.
*/
type Icu<T> = T extends IcuMessage ? T :
Copy link
Collaborator

Choose a reason for hiding this comment

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

how do you feel about calling this UnformattedIcu or RawIcu or something?

it was a bit weird seeing this in usage until I came here to see it sitting in direct contrast to FormattedIcu

Copy link
Member Author

Choose a reason for hiding this comment

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

how do you feel about calling this UnformattedIcu or RawIcu or something?

it was a bit weird seeing this in usage until I came here to see it sitting in direct contrast to FormattedIcu

RawIcu sounds good to me since UnformattedIcu is a little visually confusable with FormattedIcu, but happy to bikeshed more now that it's been 5 months since this comment :)

/** @typedef {LH.FormattedIcu<LH.Audit.Details.Opportunity>} OpportunityTable */
/** @typedef {LH.FormattedIcu<LH.Audit.Details.OpportunityColumnHeading>} OpportunityTableHeading */
/** @typedef {LH.FormattedIcu<LH.Audit.Details.Table>} Table */
/** @typedef {LH.FormattedIcu<LH.Audit.Details.TableColumnHeading>} TableHeading */
Copy link
Collaborator

Choose a reason for hiding this comment

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

move these under LH.Result namespace?

That seems reasonable but this works too.

@@ -32,17 +32,20 @@ describe('i18n', () => {
});

describe('#createMessageInstanceIdFn', () => {
it('returns a string reference', () => {
it('returns a IcuMessage reference', () => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
it('returns a IcuMessage reference', () => {
it('returns an IcuMessage reference', () => {

@@ -52,6 +52,7 @@ describe('swap-locale', () => {
redirects: {
id: 'redirects',
title: 'Avoid multiple page redirects',
doesntExist: 'A string that does not have localized versions',
Copy link
Collaborator

Choose a reason for hiding this comment

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

should we add a different path to something that doesn't exist then? feels like this is a duplicate test of fakeaudit title otherwise

Copy link
Member Author

Choose a reason for hiding this comment

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

should we add a different path to something that doesn't exist then? feels like this is a duplicate test of fakeaudit title otherwise

clarified in a comment that it's slightly different due to the message key, but also added a separate test for paths to things that don't exist.

@paulirish
Copy link
Member

TODO: add test for serializability ;)

@@ -68,7 +68,7 @@ class ImageAspectRatio extends Audit {

if (!Number.isFinite(actualAspectRatio) ||
!Number.isFinite(displayedAspectRatio)) {
return new Error(str_(UIStrings.warningCompute, {url}));
return str_(UIStrings.warningCompute, {url});
Copy link
Member Author

Choose a reason for hiding this comment

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

we never use this as an Error, just a string container (the Error is returned and then if it passes instanceof Error, its message is pushed into a warnings array), so if replacing that string with an IcuMessage, might as well just use that as the whole return value in the error case :)

Copy link
Member Author

@brendankenny brendankenny left a comment

Choose a reason for hiding this comment

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

still TODO:

  • update i18n/readme.md, apparently
  • createIcuMessageFn question
  • serialization check

as mentioned in the meeting today, several more people have spent time with localization and audit-details, etc since this PR was opened, so feel free to weigh in on the design!

@@ -208,7 +208,7 @@ class RenderBlockingResources extends Audit {
static async audit(artifacts, context) {
const {results, wastedMs} = await RenderBlockingResources.computeResults(artifacts, context);

let displayValue = '';
let displayValue;
Copy link
Member Author

Choose a reason for hiding this comment

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

these are inferred correctly, right?

yes, they haven't been necessary for a while. I don't remember when conditional initialization like this was added, but basically the CFG typing keeps getting better.

* @param {unknown} value
* @return {value is string|LH.IcuMessage}
*/
function isStringOrIcuMessage(value) {
Copy link
Member Author

Choose a reason for hiding this comment

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

should this just live on i18n or you feel like it won't come up again?

it seemed like just an internal helper fn for config (since the inline check was too long for the line and repeated 3x), so it's just scoped to the module, but I don't have strong feelings about keeping it locked away.

@@ -213,13 +213,15 @@ CTC is a name that is distinct and identifies this as the Chrome translation for

# Appendix

TODO(bckenny): update
Copy link
Member Author

Choose a reason for hiding this comment

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

is this TODO as part of this PR or later? :)

part of this PR :) I'll try to figure out what I thought should be added

lighthouse-core/lib/i18n/i18n.js Show resolved Hide resolved
* @param {MessageFormat} messageFormatter
* @param {Readonly<Record<string, string | number>>} values
* @param {string} icuMessage Used for clear error logging.
Copy link
Member Author

Choose a reason for hiding this comment

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

should we rename this param now that icuMessage means that object thing returned from a string we want localized

I'll use lhlMessage since it's in that format? Or should it be rawMessage or..something?

* generate the string ids.
*
* Returns a function that generates `LH.IcuMessage` objects to localize the
* messages in `fileStrings` and the shared `i18n.UIStrings`.
* @param {string} filename
* @param {Record<string, string>} fileStrings
*/
function createMessageInstanceIdFn(filename, fileStrings) {
Copy link
Member Author

Choose a reason for hiding this comment

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

can we rename this to createIcuMessageFn internally here.. we can still expose it in module.exports as the long name (for now)

should both names be exposed and we convert all Lighthouse audits over to using createIcuMessageFn?

types/i18n.d.ts Outdated
* Heavy handed and requires more type checks, so prefer explicitly setting
* properties to include `LH.IcuMessage` over this helper if possible.
*/
type Icu<T> = T extends IcuMessage ? T :
Copy link
Member Author

Choose a reason for hiding this comment

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

how do you feel about calling this UnformattedIcu or RawIcu or something?

it was a bit weird seeing this in usage until I came here to see it sitting in direct contrast to FormattedIcu

RawIcu sounds good to me since UnformattedIcu is a little visually confusable with FormattedIcu, but happy to bikeshed more now that it's been 5 months since this comment :)

lighthouse-core/lib/i18n/i18n.js Show resolved Hide resolved
Copy link
Collaborator

@patrickhulce patrickhulce left a comment

Choose a reason for hiding this comment

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

the bones still look great!

@@ -66,23 +66,25 @@ declare global {
wastedPercent?: number;
}

// TODO: consider making some of the `string | IcuMessage` into just `IcuMessage` to require i18n.
Copy link
Collaborator

Choose a reason for hiding this comment

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

is this the main reason we're doing string | IcuMessage manually here instead of using the RawIcu type?

Copy link
Member Author

Choose a reason for hiding this comment

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

is this the main reason we're doing string | IcuMessage manually here instead of using the RawIcu type?

Yeah, and the ergonomics are also just more straightforward to use, since RawIcu<> makes it an indirect step when using in vscode...you can't just see the type on the object, it'll look like LH.RawIcu<{text: string}> or LH.RawIcu<ProductBase> (and if you click through to ProductBase you would only see string), so it would only be when accessing the property directly that you would see the type.

(There's a comment on the RawIcu type that points to prefer making properties | LH.IcuMessage instead of using it for this reason)

I tried making more of these LH.IcuMessage only, and I think for every one of them there was some audit that was currently breaking the rules and had a string instead of a localized message, but that was in April, maybe it's better. There's also the question of if these types are just for us, or also for folks writing plugins/custom audits, and they might not want to bother with localization yet, so not allowing string would break them.

@@ -69,7 +69,7 @@ describe('Third party summary', () => {
{
blockingTime: 0,
transferSize: 8007,
url: expect.any(String),
url: expect.toBeDisplayString('Other resources'),
Copy link
Collaborator

Choose a reason for hiding this comment

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

nice 👍

lighthouse-core/lib/i18n/i18n.js Show resolved Hide resolved
@@ -155,6 +156,63 @@ describe('Runner', () => {
expect(runAuditSpy).toHaveBeenCalled();
});
});

it('serializes IcuMessages in gatherMode and is able to use them in auditMode', async () => {
Copy link
Member Author

Choose a reason for hiding this comment

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

kind of a monster sized test because it does a gatherMode run and an auditMode run, but I also didn't really want to split because it's annoying when tests have interdependencies, so 🤷

Copy link
Collaborator

Choose a reason for hiding this comment

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

It might make sense to split the three -G, -A, and loadArtifacts components separately since we're asserting the exact shape of the artifacts already?

I would prioritize the e2e flow though like you did here anyway 🤷

class WarningAndErrorGatherer extends Gatherer {
afterPass(passContext) {
const warning = str_(i18n.UIStrings.displayValueByteSavings, {wastedBytes: 2222});
passContext.LighthouseRunWarnings.push(warning);
Copy link
Member Author

Choose a reason for hiding this comment

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

lherrors have actually been serializing fine since #9397, it's other strings like runWarnings that have been causing the -G/-A problems (see #9269 (comment)), so this test checks both are working.

expect(lhr.audits['dummy-audit']).toMatchObject({
scoreDisplayMode: 'error',
// eslint-disable-next-line max-len
errorMessage: 'Required WarningAndErrorGatherer gatherer encountered an error: UNSUPPORTED_OLD_CHROME',
Copy link
Member Author

Choose a reason for hiding this comment

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

should probably follow up this PR and make the automatic audit errors that come from artifact errors use friendlyMessage instead of just message (which is set to just the code), because a lot of context is lost here. Downside: error messages get a lot longer.

@brendankenny
Copy link
Member Author

outstanding TODOs are complete, ready for review :)

@brendankenny
Copy link
Member Author

@connorjclark smoke-devtools times out no matter how many times I run this (and it's rebased against latest). Any ideas?

lighthouse-core/lib/i18n/i18n.js Outdated Show resolved Hide resolved
name: 'LHError',
code: 'UNSUPPORTED_OLD_CHROME',
// eslint-disable-next-line max-len
friendlyMessage: expect.toBeDisplayString(`This version of Chrome is too old to support 'VRML'. Use a newer version to see full results.`),
Copy link
Collaborator

Choose a reason for hiding this comment

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

😆

@@ -155,6 +156,63 @@ describe('Runner', () => {
expect(runAuditSpy).toHaveBeenCalled();
});
});

it('serializes IcuMessages in gatherMode and is able to use them in auditMode', async () => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

It might make sense to split the three -G, -A, and loadArtifacts components separately since we're asserting the exact shape of the artifacts already?

I would prioritize the e2e flow though like you did here anyway 🤷

@patrickhulce
Copy link
Collaborator

Are there devtools changes required to make this work with the timeouts? Nothing immediately comes to mind, but definitely suspicious...

I love the direction this takes us! Much more self-contained and explicit about what's i18n'd and what's not 💯 👍

@connorjclark
Copy link
Collaborator

connorjclark commented Sep 17, 2020

The recent changes in devtools broke Lighthouse in release and locally. I think they've reverted or fixed it since.

I don't think there's a way to clear the github action cache... you might need to add a nonce comment in the devtools.yml, to trigger a new cache.

@brendankenny
Copy link
Member Author

hmm, it's the same key and commit that's passed on the latest commit on master, but caches are hard, so I'll try it :)

@connorjclark
Copy link
Collaborator

connorjclark commented Sep 17, 2020

I ran it locally and got this error:

image

oddly, I don't even have pubads checked.

@brendankenny
Copy link
Member Author

brendankenny commented Sep 17, 2020

weird, but that's great, that really is something that needs to be updated. Any ideas on how we could pipe startup errors like that back to github actions instead of timing out?

edit: connor's on it in #11414 :)

@connorjclark connorjclark mentioned this pull request Sep 17, 2020
10 tasks
@brendankenny
Copy link
Member Author

OK:

  • moved isStringOrIcuMessage to i18n.js so config-helpers and config-plugin can use it
  • config-plugin now allows IcuMessage for the relevant config strings
  • added a config-plugin test with a localizable plugin
  • temporarily switched all the string | IcuMessage types to just IcuMessage and verified there aren't any other places that should account for IcuMessage but currently don't

Co-authored-by: Patrick Hulce <[email protected]>
@@ -84,7 +84,7 @@ cp "$DEVTOOLS_PATH/test/webtests/http/tests/devtools/lighthouse/"*-expected.txt
if [ ! $status -eq 0 ]; then
# Print failure diffs to stdout.
find "$LH_ROOT/.tmp/layout-test-results/retry_3" -name '*-diff.txt' -exec cat {} \;
echo "❌❌❌ webtests failed. to rebaseline run: yarn update:webtests ❌❌❌"
echo "❌❌❌ webtests failed. to rebaseline run: yarn update:test-devtools ❌❌❌"
Copy link
Collaborator

Choose a reason for hiding this comment

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

bingo

@@ -25,5 +25,5 @@ ViewportDimensions: {
}


content-width: pass
content-width: pass undefined
Copy link
Collaborator

Choose a reason for hiding this comment

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

hmm?
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.

audit.explanation is optional, so can be undefined :)

It was the empty string before...not sure that's better :)

Copy link
Collaborator

Choose a reason for hiding this comment

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

seems like we should only put audit.explanation into the result when the audit is failing 🤔

Copy link
Collaborator

Choose a reason for hiding this comment

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

can you add to the list? I also want to pull this into a fn in LighthouseTestRunner (also used in other tests)

@patrickhulce
Copy link
Collaborator

new changes LGTM too, so I'll let my review stand. Yay devtools tests for catching things at PR time 😃 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants