i18n: introduce script to swap in new locale to LHR #8755

paulirish · 2019-04-30T23:06:55Z

icuMessagePaths has been sitting there waiting for us for a while! Finally we can put it to use.

It's written as isomorphic code (lol), assuming we may want to do this clientside or whatever... But the initial motivation was to generate reports in all locales in our Now deployments. That PR will followup this one.

patrickhulce

Huzzah!!!

patrickhulce · 2019-04-30T23:41:55Z

lighthouse-core/test/lib/i18n/swap-locale-test.js

@@ -0,0 +1,28 @@
+/**
+ * @license Copyright 2018 Google Inc. All Rights Reserved.


patrickhulce · 2019-04-30T23:43:03Z

lighthouse-core/lib/i18n/swap-locale.js

+  Object.entries(icuMessagePaths).forEach(([icuMessageId, messageInstancesInLHR]) => {
+    for (const instance of messageInstancesInLHR) {
+      // The path that _formatPathAsString() generated
+      const path = /** @type {LH.I18NMessageValuesEntry} */ (instance).path || /** @type {string} */ (instance);


should we typeof check this instead of the type assertions?

patrickhulce · 2019-04-30T23:44:15Z

lighthouse-core/lib/i18n/swap-locale.js

+    for (const instance of messageInstancesInLHR) {
+      // The path that _formatPathAsString() generated
+      const path = /** @type {LH.I18NMessageValuesEntry} */ (instance).path || /** @type {string} */ (instance);
+      const values = /** @type {LH.I18NMessageValuesEntry} */ (instance).values || undefined;


probably worth commenting that values are the string template values we need to replace

patrickhulce · 2019-04-30T23:46:10Z

lighthouse-core/lib/i18n/i18n.js

+  const icuMessageIdRegex = /(.* \| .*)$/;
+  if (!icuMessageIdRegex.test(icuMessageId)) throw new Error('This is not an ICU message ID');
+
+  const {formattedString} = _formatIcuMessage(locale, icuMessageId, '', values);


empty fallback message concerns me a bit, maybe we should have _formatIcuMessage throw if we can't find the icuMessageId and the fallback message is undefined? Maybe we should use a placeholder string that's non-empty?

not sure what to do there 🤷‍♂

patrickhulce · 2019-04-30T23:46:36Z

lighthouse-core/lib/i18n/swap-locale.js

+const i18n = require('./i18n.js');
+
+/**
+ * Replaces all strings within an LHR with ones from a different locale


I wonder if this might be worth being the place where we explain how the icuMessagePaths property is setup and meant to be used?

patrickhulce · 2019-04-30T23:46:51Z

lighthouse-core/lib/i18n/swap-locale.js

+
+/* eslint-disable no-console, max-len */
+
+const _set = require('lodash.set');


paulirish · 2019-05-02T19:49:16Z

lighthouse-core/lib/i18n/i18n.js

  // when using accented english, force the use of a different locale for number formatting
  const localeForMessageFormat = locale === 'en-XA' ? 'de-DE' : locale;
  // pre-process values for the message format like KB and milliseconds
-  const valuesForMessageFormat = _preprocessMessageValues(icuMessage, values);
+  const valuesForMessageFormat = _preprocessMessageValues(messageForMessageFormat, values);


@patrickhulce note this change.. it was using the fallback message instead of the one pulled from the locales files. which seemed odd. right?

it was using the fallback message instead of the one pulled from the locales files. which seemed odd. right?

It is weird. As long as there isn't an old mismatched translation for the locale it shouldn't matter, but agreed that checking that the values will actually be able to go into the string we want them to (and preparing them to do so) is the right thing to do.

Mismatched translations could become a problem at some point. If we've updated a string in en-US.json and it has different values than the not-yet-updated strings in all the other locales, I'm pretty sure that will either throw in _preprocessMessageValues or below in the formatter.

Maybe we should have a check in string collection that deletes strings in other locales if the expected values don't match anymore.

paulirish · 2019-05-03T21:30:16Z

fwiw if you check out https://lighthouse-git-arabic.googlechrome.now.sh/ you can see where i'm going with this.

connorjclark · 2019-05-03T22:49:21Z

we should add this:

<meta name="google" content="notranslate">

either just for these now deployments, or directly in the report.

brendankenny

it does seem like we have a missing abstraction that could unify both this and the regular translation process nicely. Basically to be able to pass icuMessagePaths to a method that populates _icuMessageInstanceMap from it, then run i18n.replaceIcuMessageInstanceIds() on the LHR exactly as we do in Runner.

But this is self contained enough (and refactorable if we ever feel compelled to do so) it seems fine to move forward with

brendankenny · 2019-05-03T21:22:10Z

lighthouse-core/lib/i18n/swap-locale.js

+      } else {
+        path = /** @type {LH.I18NMessageValuesEntry} */ (instance).path;
+        // `values` are the string template values to be used. eg. `values: {wastedBytes: 9028}`
+        values = /** @type {LH.I18NMessageValuesEntry} */ (instance).values;


don't need these casts anymore

lighthouse-core/test/lib/i18n/swap-locale-test.js

+
+/* eslint-env jest */
+
+describe('swap-locale', () => {


lighthouse-core/lib/i18n/swap-locale.js

brendankenny · 2019-05-03T23:42:50Z

lighthouse-core/lib/i18n/i18n.js

  // when using accented english, force the use of a different locale for number formatting
  const localeForMessageFormat = locale === 'en-XA' ? 'de-DE' : locale;
  // pre-process values for the message format like KB and milliseconds
-  const valuesForMessageFormat = _preprocessMessageValues(icuMessage, values);
+  const valuesForMessageFormat = _preprocessMessageValues(messageForMessageFormat, values);


it was using the fallback message instead of the one pulled from the locales files. which seemed odd. right?

It is weird. As long as there isn't an old mismatched translation for the locale it shouldn't matter, but agreed that checking that the values will actually be able to go into the string we want them to (and preparing them to do so) is the right thing to do.

Mismatched translations could become a problem at some point. If we've updated a string in en-US.json and it has different values than the not-yet-updated strings in all the other locales, I'm pretty sure that will either throw in _preprocessMessageValues or below in the formatter.

Maybe we should have a check in string collection that deletes strings in other locales if the expected values don't match anymore.

brendankenny · 2019-05-04T00:09:05Z

lighthouse-core/lib/i18n/swap-locale.js

+        const formattedStr = i18n.formatMessageFromIdWithValues(locale, icuMessageId, values);
+        // Write string back into the LHR
+        _set(lhr, path, formattedStr);
+      } catch (e) {}


unconditional catch here seems excessive since all problems would be silenced. Some wouldn't be interesting (e.g. string from removed audit isn't available in new locale), but some could be a real problem. Maybe at least log the issue?

Maybe at least log the issue?

or log the ones that aren't 'No ICU message string to format' if that gets excessive

brendankenny · 2019-05-04T00:10:56Z

lighthouse-core/lib/i18n/i18n.js

+ * @param {*} [values]
+ * @return {string}
+ */
+function formatMessageFromIdWithValues(locale, icuMessageId, values) {


what about

Suggested change

function formatMessageFromIdWithValues(locale, icuMessageId, values) {

function getFormattedFromIdAndValues(locale, icuMessageId, values) {

to match getFormatted (param order for getFormatted vs _formatIcuMessage is unfortunate but whatever)

lighthouse-core/test/lib/i18n/swap-locale-test.js

+    // Renderer formatted strings
+    expect(lhrEn.i18n.rendererFormattedStrings.notApplicableAuditsGroupTitle).toEqual('Not applicable');
+    expect(lhrEs.i18n.rendererFormattedStrings.notApplicableAuditsGroupTitle).toEqual('No aplicable');
+  });


patrickhulce

looks great to me!

patrickhulce · 2019-05-04T04:37:31Z

lighthouse-core/lib/i18n/swap-locale.js

+ *    which will be used in the replacement within `i18n._formatIcuMessage()`
+ *
+ * An example:
+ *    "icuMessagePaths": {


nit: unindent this 2 spaces so it's easy to tell its nested? :)

lighthouse-core/lib/i18n/swap-locale.js

+  Object.entries(icuMessagePaths).forEach(([icuMessageId, messageInstancesInLHR]) => {
+    for (const instance of messageInstancesInLHR) {
+      // The path that _formatPathAsString() generated
+      let path;


patrickhulce · 2019-05-04T04:39:42Z

lighthouse-core/test/lib/i18n/swap-locale-test.js

+    /* eslint-disable max-len */
+    // Renderer formatted strings
+    expect(lhrEn.i18n.rendererFormattedStrings.notApplicableAuditsGroupTitle).toEqual('Not applicable');
+    expect(lhrEs.i18n.rendererFormattedStrings.notApplicableAuditsGroupTitle).toEqual('No aplicable');


I didn't even notice this was translated 😆 is there a better example?

patrickhulce · 2019-05-04T04:40:23Z

types/lhr.d.ts

@@ -8,7 +8,8 @@ import LHError = require('../lighthouse-core/lib/lh-error.js');

 declare global {
  module LH {
-    export type I18NMessageEntry = string | {path: string, values: any};
+    export type I18NMessageValuesEntry = {path: string, values: any};


maybe we should type values as Record<string, string | number>?

googlebot · 2019-05-28T21:06:54Z

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and have the pull request author add another comment and the bot will run again. If the bot doesn't comment, it means it doesn't think anything has changed.

ℹ️ Googlers: Go here for more info.

Co-Authored-By: Brendan Kenny <bckenny@gmail.com>

paulirish · 2019-05-28T22:40:31Z

ready for another look

patrickhulce

looks pretty good to me! just a few primetime cleanup nits

patrickhulce · 2019-05-28T23:21:35Z

lighthouse-core/lib/i18n/swap-locale.js

+ */
+'use strict';
+
+/* eslint-disable no-console, max-len */


probably shouldn't leave these on if this is for real now :)

maybe we have a script version that does the console.loging?

patrickhulce · 2019-05-28T23:22:38Z

lighthouse-core/test/lib/i18n/swap-locale-test.js

+
+    // via Spanish
+    const lhrEnEsRT = swapLocale(swapLocale(lhrEn, 'es'), 'en-US');
+    expect(lhrEnEsRT).toMatchObject(lhrEn);


should toEqual, right? or do things get deleted

patrickhulce · 2019-05-28T23:23:31Z

lighthouse-core/lib/i18n/swap-locale.js

+  });
+
+  if (missingIcuMessageIds.length) {
+    console.error(`No message in locale (${locale}) found for:\n`, missingIcuMessageIds);


maybe these get returned as warnings if we split the logging into a script?

patrickhulce · 2019-05-28T23:24:55Z

lighthouse-core/lib/i18n/swap-locale.js

+        // Write string back into the LHR
+        _set(lhr, path, formattedStr);
+      } catch (err) {
+        if (err.message === 'No ICU message string to format') {


this feels like it will get out of date and we won't notice :)

any way you can think of to make this a little more foolproof? export the string in i18n maybe? a flag on the error? 🤷‍♂

patrickhulce · 2019-05-28T23:25:26Z

lighthouse-core/test/lib/i18n/swap-locale-test.js

+beforeEach(() => {
+  // silence console.error spam about messages not found
+  // eslint-disable-next-line no-console
+  console.error = jest.fn();


hopefully won't need this one :)

exterkamp

LGTM, just make the CI happy!

exterkamp · 2019-06-07T21:35:47Z

lighthouse-core/lib/i18n/swap-locale.js

+const _set = require('lodash.set');
+
+const i18n = require('./i18n.js');
+const LHError = require('../lh-error.js');


Appveyor is mad about this unused import.

googlebot · 2019-06-07T21:36:49Z

A Googler has manually verified that the CLAs look good.

(Googler, please make sure the reason for overriding the CLA status is clearly documented in these comments.)

ℹ️ Googlers: Go here for more info.

googlebot · 2019-06-12T01:05:20Z

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and have the pull request author add another comment and the bot will run again. If the bot doesn't comment, it means it doesn't think anything has changed.

ℹ️ Googlers: Go here for more info.

googlebot · 2019-06-12T01:05:54Z

A Googler has manually verified that the CLAs look good.

(Googler, please make sure the reason for overriding the CLA status is clearly documented in these comments.)

ℹ️ Googlers: Go here for more info.

paulirish added 4 commits April 19, 2019 16:29

i18n: introduce script to swap in new locale to LHR

a8ff2a0

rename

81b0dc4

add start of a test.

e12f714

revert now build changes.

30bb475

paulirish requested a review from brendankenny as a code owner April 30, 2019 23:06

paulirish changed the title ~~i18n: introduce script to swap in new locale to LHR~~ i18n: introduce script to swap in new locale to LHR Apr 30, 2019

patrickhulce reviewed Apr 30, 2019

View reviewed changes

feedback

059bc0f

vercel bot deployed to staging May 2, 2019 19:48 View deployment

paulirish commented May 2, 2019

View reviewed changes

rename to fallbackMessage

54c4833

vercel bot deployed to staging May 2, 2019 19:50 View deployment

paulirish added 2 commits May 3, 2019 13:41

fix espanol string

bdd29b4

Merge branch 'master' into swaplocale

fdf96e8

vercel bot deployed to staging May 3, 2019 20:42 View deployment

brendankenny reviewed May 4, 2019

View reviewed changes

patrickhulce reviewed May 4, 2019

View reviewed changes

paulirish added the waiting4committer label May 25, 2019

brendankenny mentioned this pull request May 28, 2019

Custom locales #9043

Closed

vercel bot temporarily deployed to staging May 28, 2019 21:06 Inactive

vercel bot deployed to staging May 28, 2019 21:07 View deployment

paulirish requested a review from exterkamp as a code owner May 28, 2019 22:31

vercel bot deployed to staging May 28, 2019 22:31 View deployment

googlebot added the cla: no label May 28, 2019

vercel bot deployed to staging May 28, 2019 22:33 View deployment

paulirish and others added 2 commits May 28, 2019 15:36

Update lighthouse-core/lib/i18n/swap-locale.js

5255aee

Co-Authored-By: Brendan Kenny <bckenny@gmail.com>

Update lighthouse-core/lib/i18n/swap-locale.js

627f7be

Co-Authored-By: Brendan Kenny <bckenny@gmail.com>

paulirish added 2 commits May 28, 2019 15:37

console time

5654af1

Merge branch 'master' into swaplocale

a225504

paulirish force-pushed the swaplocale branch from f6ae638 to a225504 Compare May 28, 2019 22:37

vercel bot deployed to staging May 28, 2019 22:37 View deployment

drop casts

853bfd2

vercel bot deployed to staging May 28, 2019 22:40 View deployment

paulirish removed the waiting4committer label May 28, 2019

patrickhulce reviewed May 28, 2019

View reviewed changes

paulirish added 2 commits June 5, 2019 14:25

return warnings. feedback

e3f2d3f

Merge branch 'master' into swaplocale

5efe7eb

vercel bot deployed to staging June 5, 2019 12:27 View deployment

exterkamp approved these changes Jun 7, 2019

View reviewed changes

exterkamp added cla: yes and removed cla: no labels Jun 7, 2019

remove lherror dep

cf0d3b7

paulirish added the land-when-ci-is-green label Jun 12, 2019

vercel bot deployed to staging June 12, 2019 01:05 View deployment

googlebot added cla: no and removed cla: yes labels Jun 12, 2019

paulirish added cla: yes and removed land-when-ci-is-green cla: no labels Jun 12, 2019

paulirish merged commit d3d3828 into master Jun 25, 2019

paulirish deleted the swaplocale branch June 25, 2019 01:18

brendankenny mentioned this pull request Jul 25, 2019

i18n: align type names with docs #9461

Merged

snyk-bot mentioned this pull request Mar 21, 2020

[Snyk] Upgrade lighthouse from 5.1.0 to 5.6.0 godaddy/lighthouse4u#13

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

i18n: introduce script to swap in new locale to LHR #8755

i18n: introduce script to swap in new locale to LHR #8755

This comment was marked as resolved.

This comment was marked as outdated.

This comment was marked as resolved.

		@@ -0,0 +1,28 @@
		/**
		* @license Copyright 2018 Google Inc. All Rights Reserved.


		/* eslint-disable no-console, max-len */

		const _set = require('lodash.set');

	function formatMessageFromIdWithValues(locale, icuMessageId, values) {
	function getFormattedFromIdAndValues(locale, icuMessageId, values) {

i18n: introduce script to swap in new locale to LHR #8755

i18n: introduce script to swap in new locale to LHR #8755

Conversation

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment was marked as resolved.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment was marked as outdated.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment was marked as resolved.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment