core(i18n): localize strings at end of run #5655

patrickhulce · 2018-07-11T18:32:19Z

An example of how we might be able to "fix" the strings before shipping the LHR, basically the i18n formatting function just returns a reference to the string to be used which is stored in a map, then at the end before returning, runner asks i18n to replace all the references in the LHR with the string of the appropriate locale. BOOM done! a log is kept of what replacements were made and added to the LHR as localeLog so that later the report could be translated again without re-running.

patrickhulce · 2018-07-11T18:32:56Z

lighthouse-core/lib/i18n.js

+      keyUsages.push({key, template, values});
+      formattedStringUsages.set(key, keyUsages);
+
+      return `${key}#${keyUsages.length - 1}`;


just a reference is returned here instead of the real string

patrickhulce · 2018-07-11T18:33:16Z

lighthouse-core/lib/i18n.js

+          const occurrences = templateLogRecord.occurrences || []
+          occurrences.push({values: usage.values, path: currentPath})
+
+          const {message, template} = formatTemplate(locale, templateKey, usage.template, usage.values);


we do the real formatting here in the specified locale

patrickhulce · 2018-07-11T18:33:36Z

lighthouse-core/test/results/sample_v2.json

+        }
+      ]
+    },
+    "lighthouse-core/lib/i18n.js!#ms": {


a log keeps track of all the strings we used

patrickhulce · 2018-07-11T18:33:47Z

lighthouse-core/test/results/sample_v2.json

+          "values": {
+            "timeInMs": 4927.278
+          },
+          "path": [


as well as the path in the LHR of where you can find them to replace them

patrickhulce · 2018-07-11T18:34:15Z

lighthouse-core/test/results/sample_v2.json

@@ -3416,5 +3416,144 @@
      "title": "Crawling and Indexing",
      "description": "To appear in search results, crawlers need access to your app."
    }
+  },
+  "localeLog": {


this will get big, but maybe it can be an optional thing?

Yah +1 to this being an option that's off by default.

paulirish

I like where things are going here. :D

paulirish · 2018-07-13T05:05:16Z

lighthouse-core/config/default-config.js

@@ -406,3 +414,7 @@ module.exports = {
    },
  },
 };
+
+Object.defineProperty(module.exports, 'UIStrings', {


can you add a comment on why we export it all fancy like this?
( i dont totally grok why.. :)

sure :)

it's because config data gets cloned and validated and whatnot so we want a getter and not put the real strings on the actual object

paulirish · 2018-07-17T21:21:44Z

lighthouse-core/test/results/sample_v2.json

@@ -3416,5 +3416,144 @@
      "title": "Crawling and Indexing",
      "description": "To appear in search results, crawlers need access to your app."
    }
+  },
+  "localeLog": {


Yah +1 to this being an option that's off by default.

paulirish · 2018-07-17T21:30:48Z

lighthouse-core/test/results/sample_v2.json

+        }
+      ]
+    },
+    "lighthouse-core/lib/i18n.js!#columnURL": {


how about JSONPath?

"lighthouse-core/lib/i18n.js!#columnURL": { "occurrences": [ { "path": [ "audits", "render-blocking-resources", "details", "headings", "0", "label" ] } ] },

could become:

"lighthouse-core/lib/i18n.js!#columnURL": { "occurrences": ["$.audits['render-blocking-resources'].details.headings[0].label"], },

obviously it's more dense, but I think its a lot more readable.

(actually looks like lodash's get also supports this style, anyhow.)
We don't really need ALL of jsonpath, just the basics.

do you want JSONPath or what lodash get supports? ;)

I'm on board for this 👍

I should've said, "I'm on board for this, so long as we use someone else's getter code :)"

sg! based on their docs, the string form with lodash get lgtm

patrickhulce · 2018-07-18T21:45:45Z

lighthouse-core/config/default-config.js


-/* eslint-disable max-len */
+const UIStrings = {


FWIW @brendankenny I think these are the only strings we really need for performance section, so it's relatively small

if you want to be noodling on a much better config solution before we start on the other categories, nows a good time :)

patrickhulce · 2018-07-18T21:49:26Z

lighthouse-core/lib/i18n.js

+ */
+
+/** @type {Map<string, StringUsage[]>} */
+const formattedStringUsages = new Map();


this is the new singleton, we might want to put some disclaimers into README somewhere about YMMV using LH as a long-lived module not in a worker. CLI/DevTools/Extension all don't really care about these types of concerns so inevitably we'll make some trade-offs not great for node module users (example: #5683)

patrickhulce · 2018-07-18T21:57:31Z

I think this is ready for ready for re-review.

I'm going to throw one more thought out there as I was thinking about report strings. instead of lhr.localeLog...

lhr.localization = {
  log: {...},
  uiMessages: {},
}

thoughts?

paulirish · 2018-07-19T21:52:52Z

typings/lhr.d.ts

@@ -6,6 +6,12 @@

 declare global {
  module LH {
+    export type LocaleLogEntry = string | {path: string, values: any};
+
+    export interface LocaleLog {


Naming bikeshed plz? 😁

Doesn't feel like a log. More of a map? or just locations? how about.......

lhr.i18n.messageLocations lhr.i18n.uiMessages

how about

lhr.i18n.messages

I has saving uiMessages for the strings that the renderer will use but are in the LHR anywhere otherwise

:)

paulirish · 2018-07-19T22:24:41Z

lighthouse-core/lib/i18n.js

+    }
+  }
+
+  return pathAsString;
 }

 module.exports = {


the export style is a little funky here. starts halfway and nests these other fns, but not formatPathAsString

it was whatever is used inside the module gets defined outside everything else is inline, I'll just move it all out though

paulirish · 2018-07-19T22:28:02Z

lighthouse-core/lib/i18n.js

+ * @param {string} template
+ * @param {*} [values]
+ */
+function formatTemplate(locale, templateKey, template, values) {


function _formatTemplate

paulirish · 2018-07-19T22:40:30Z

lighthouse-core/lib/i18n.js

+}
+
+/** @param {string[]} path */
+function formatPathAsString(path) {


also with a _ prefix? (even though we export for testing)?

paulirish · 2018-07-19T22:51:55Z

lighthouse-core/lib/i18n.js

-      const formatter = new MessageFormat(messageForMessageFormat, localeForMessageFormat, formats);
-      return formatter.format(values);
+      const unixStyleFilename = path.relative(LH_ROOT, filenameToLookup).replace(/\\/g, '/');
+      const key = unixStyleFilename + '!#' + keyname;


thinking we should use stronger delimiters. How about |.. e.g.

"lighthouse-core/audits/metrics/interactive.js | description"

paulirish · 2018-07-19T23:50:40Z

lighthouse-core/lib/i18n.js

+      const unixStyleFilename = path.relative(LH_ROOT, filenameToLookup).replace(/\\/g, '/');
+      const key = unixStyleFilename + '!#' + keyname;
+      const keyUsages = formattedStringUsages.get(key) || [];
+      keyUsages.push({key, template, values});


There's something funky going on here:

how did you get that? are you looking at the test output? the tests invoke FCP .meta lots of times so that's probably what you're seeing

yup it's the getter of meta, as you suspected. we'll have to dedupe.

paulirish · 2018-07-19T23:54:27Z

lighthouse-core/lib/i18n.js

-      return formatter.format(values);
+      const unixStyleFilename = path.relative(LH_ROOT, filenameToLookup).replace(/\\/g, '/');
+      const key = unixStyleFilename + '!#' + keyname;
+      const keyUsages = formattedStringUsages.get(key) || [];


(related to next comment, but..)

why are there multiple usages of a given string ID? Well I guess i'd expect it of the i18n ones, but not typically the strings held in the audits, right?

yeah it'd be unexpected for more than 1 audit title to be used, but for strings that will be used for table cells it will be expected

paulirish · 2018-07-19T23:56:17Z

lighthouse-core/lib/i18n.js

+      keyUsages.push({key, template, values});
+      formattedStringUsages.set(key, keyUsages);
+
+      return `${key}#${keyUsages.length - 1}`;


similar to previous one. giving this some space.. ${key} # {length - 1}

paulirish · 2018-07-20T00:11:43Z

lighthouse-core/lib/i18n.js

+    /**
+     * @param {*} objectInLHR
+     * @param {LH.LocaleLog} log
+     * @param {string[]} path


only for testing?

are you referring to why path is an array of strings in this function signature? it's so that we can easily push onto it and convert the name at each step rather than have two functions that create the full string from an array and one that builds incrementally

paulirish · 2018-07-20T00:13:30Z

lighthouse-core/lib/i18n.js

-  setLocale(newLocale) {
-    if (!newLocale) return;
-    locale = newLocale;
+  replaceLocaleStringReferences(lhr, locale) {


naming

okay we have:
string IDs?
string references?
object paths?

i'm also seeing: template key, key name, locale string reference, keyusages / usages.

can we make these names a bit more concrete and consistent?

yeah this is a bit confusing, I'll do a pass to try to make them all consistent

patrickhulce · 2018-07-20T17:52:20Z

alright @paulirish I think the names have been sorted out except this area https://github.com/GoogleChrome/lighthouse/pull/5655/files#diff-7e3be24bffdebd4c067d12d0596858f9R171

now that it's not log and we also have usages saved in our formattedStringUsages not sure what to call these two different things

paulirish

New namessss

formatter 👍
uiStrings 👍
rename 'template'.
kill 'localeString'
rename message in _formatTemplate => formattedString
createStringFormatter -> createMessageInstanceIdFn
maybe 'usages' -> 'instances', but nobigdeal

icuMessage
icuMessageId
icuMessageInstanceId

formattedString

icuMessageInstanceIdMap. Map(<icuMessageID, [*]>)

lhr.i18n.icuMessagePaths
lhr.i18n.rendererFormattedStrings

paulirish · 2018-07-20T22:34:03Z

typings/lhr.d.ts

@@ -6,6 +6,12 @@

 declare global {
  module LH {
+    export type LocaleLogEntry = string | {path: string, values: any};
+
+    export interface LocaleLog {


paulirish · 2018-07-20T22:44:31Z

lighthouse-core/lib/i18n.js

+      const unixStyleFilename = path.relative(LH_ROOT, filenameToLookup).replace(/\\/g, '/');
+      const key = unixStyleFilename + '!#' + keyname;
+      const keyUsages = formattedStringUsages.get(key) || [];
+      keyUsages.push({key, template, values});


yup it's the getter of meta, as you suspected. we'll have to dedupe.

paulirish · 2018-07-20T22:51:38Z

lighthouse-core/lib/i18n.js

+ * @param {string} templateID
+ * @param {string} template
+ * @param {*} [values]
+ */


can you add a @return

paulirish · 2018-07-20T22:58:14Z

lighthouse-core/lib/i18n.js

@@ -43,57 +41,150 @@ const formats = {

 /**
 * @param {string} msg
- * @param {Record<string, *>} values
+ * @param {Record<string, *>} [values]
 */
 function preprocessMessageValues(msg, values) {


_preprocessMessageValues

paulirish · 2018-07-20T23:08:57Z

lighthouse-core/lib/i18n.js

+      const currentPathInLHR = pathInLHR.concat([property]);
+
+      // Check to see if the value in the LHR looks like a string reference. If it is, replace it.
+      if (typeof value === 'string' && /.* \| .* # \d+$/.test(value)) {


wanna throw in the param groups that you have on 177 so these look more similar?

paulirish · 2018-07-21T01:57:34Z

@brendankenny u want to take a pass?

patrickhulce · 2018-07-21T05:14:35Z

lighthouse-core/lib/i18n.js

@@ -20,6 +19,8 @@ try {

  // @ts-ignore
  const IntlPolyfill = require('intl');
+  if (!IntlPolyfill.NumberFormat) throw new Error('Invalid polyfill');


@paulirish FYI this was my fix to what you discovered in #5702

ah cool. nice.

The msg is a little weird though. Seems like this case is described as: "Polyfill is a no-op due to existing implementation". The fact that we exit via throwing is a little random. can we do a IIFE and return instead?

yeah IFFE makes a lot more sense, done 👍

paulirish · 2018-07-23T16:55:30Z

👍

patrickhulce requested review from brendankenny and paulirish as code owners July 11, 2018 18:32

patrickhulce commented Jul 11, 2018

View reviewed changes

paulirish mentioned this pull request Jul 17, 2018

[WIP] report: i18n spike #5614

Closed

paulirish reviewed Jul 17, 2018

View reviewed changes

brendankenny mentioned this pull request Jul 17, 2018

core(i18n): initial utility library #5644

Merged

core(i18n): localize strings at end of run

f555090

patrickhulce force-pushed the i18n_core_but_replaceable_spike branch from 3df449a to f555090 Compare July 18, 2018 00:34

patrickhulce changed the base branch from i18n_spike to master July 18, 2018 00:34

patrickhulce changed the title ~~WIP spike into config i18n and post-run translation~~ core(i18n): localize strings at end of run Jul 18, 2018

GoogleChrome deleted a comment from googlebot Jul 18, 2018

patrickhulce commented Jul 18, 2018

View reviewed changes

collect more strings

2c51922

patrickhulce added 4 commits July 18, 2018 14:59

move to runner

06350fa

windows compat

4fbe54a

Merge branch 'master' into i18n_core_but_replaceable_spike

a2da6aa

lint fixes

0e6de1a

patrickhulce mentioned this pull request Jul 19, 2018

core(i18n): extract performance strings to UIStrings #5692

Closed

paulirish requested changes Jul 20, 2018

View reviewed changes

patrickhulce added 3 commits July 20, 2018 10:17

feedback

c42ba60

add comment

5b219f9

fix extension

118ea60

fix the render blocking template

24b4e24

patrickhulce force-pushed the i18n_core_but_replaceable_spike branch from 16b49ce to 24b4e24 Compare July 20, 2018 18:29

patrickhulce added 2 commits July 20, 2018 15:47

de-dupe usages

f7cc09d

real string usage

dd98104

paulirish reviewed Jul 20, 2018

View reviewed changes

patrickhulce added 3 commits July 20, 2018 17:07

more consistency

f46a104

tests

4758c6d

regex

23e8efe

paulirish approved these changes Jul 21, 2018

View reviewed changes

patrickhulce commented Jul 21, 2018

View reviewed changes

iffeeeeee

87ac59e

patrickhulce merged commit 4e11f25 into master Jul 23, 2018

This was referenced Jul 24, 2018

core(i18n): export rendererFormattedStrings #5713

Merged

core(i18n): extract performance strings to UIStrings #5716

Merged

paulirish mentioned this pull request Jul 24, 2018

Internationalization of Lighthouse (Phase 1) #5719

Closed

24 tasks

paulirish deleted the i18n_core_but_replaceable_spike branch January 11, 2019 02:43

core(i18n): localize strings at end of run #5655

core(i18n): localize strings at end of run #5655

Conversation

patrickhulce commented Jul 11, 2018

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

paulirish left a 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

patrickhulce commented Jul 18, 2018

Choose a reason for hiding this comment

patrickhulce Jul 20, 2018 • edited Loading

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

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

patrickhulce Jul 20, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

patrickhulce commented Jul 20, 2018

paulirish left a comment • edited Loading

Choose a reason for hiding this comment

New namessss

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

paulirish commented Jul 21, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paulirish commented Jul 23, 2018

patrickhulce Jul 20, 2018 •

edited

Loading

patrickhulce Jul 20, 2018 •

edited

Loading

paulirish left a comment •

edited

Loading

paulirish commented Jul 21, 2018 •

edited

Loading