core(i18n): initial utility library #5644

patrickhulce · 2018-07-09T22:50:31Z

a slightly different take on the dynamic approach we ditched earlier.

I wanted to pitch this one more time because I ran into 2 issues with the static approach that felt 😝
also @paulirish likes this one more :)

patrickhulce · 2018-07-09T22:51:09Z

lighthouse-core/audits/byte-efficiency/render-blocking-resources.js

+    } delayed first paint by {timeInMs, number, milliseconds} ms`
+};
+
+const i18n = i18nUtils.createStringFormatter(__filename, UI_STRINGS);


this is a formatting function that has context bundled in explicitly via __filename and UI_STRINGS

patrickhulce · 2018-07-09T22:51:25Z

lighthouse-core/audits/byte-efficiency/render-blocking-resources.js

-      {key: 'url', valueType: 'url', label: 'URL'},
-      {key: 'totalBytes', valueType: 'bytes', label: 'Size (KB)'},
-      {key: 'wastedMs', valueType: 'timespanMs', label: 'Download Time (ms)'},
+      {key: 'url', valueType: 'url', label: i18n(i18nUtils.UI_STRINGS.columnURL)},


global shared UI_STRINGS can be used too

patrickhulce · 2018-07-09T22:51:44Z

lighthouse-core/index.js

@@ -34,6 +35,7 @@ const Config = require('./config/config');
 async function lighthouse(url, flags, configJSON) {
  // TODO(bckenny): figure out Flags types.
  flags = flags || /** @type {LH.Flags} */ ({});
+  i18n.setLocale(flags.locale || 'en-US');


all it takes to swap locales at runtime is a single call here

we can use something like this https://github.com/brandonhorst/javascript-environment-lang/blob/master/index.js before we fall back to english. (well i suppose it wouldn't ever fall back)

oh yeah this was me being lazy for WIP, I'd have setLocale just use the default locale determined by MessageFormat.defaultLocale when this is undefined

patrickhulce · 2018-07-09T22:52:25Z

lighthouse-core/lib/i18n.js

+      if (!keyname) throw new Error(`Could not locate: ${msg}`);
+
+      const parsed = MessageParser.parse(msg);
+      parsed.elements


by forcing the use of our own format function first, we can do things like preserve our fancy rounding logic for milliseconds, automatically convert bytes to KB, etc.

sg. i like it. let's split into its own fn

patrickhulce · 2018-07-09T22:53:05Z

lighthouse-core/scripts/collect-strings.js

+  'lighthouse-extension',
+]
+
+function collectAllStringsInDir(dir) {


simple script recurses the directories looking for strings

patrickhulce · 2018-07-09T22:53:16Z

lighthouse-core/scripts/collect-strings.js

+fs.writeFileSync(path.join(LH_ROOT, 'lighthouse-core/lib/locales/en-US.js'), 'module.exports = ' + JSON.stringify(strings, null, 2) + ';')
+
+const gibberish = {}
+for (const [key, value] of Object.entries(strings)) {


it's easy to generate other locales

paulirish

this feels great so far

paulirish · 2018-07-09T22:55:46Z

lighthouse-core/scripts/collect-strings.js

+const fs = require('fs')
+const path = require('path')
+
+const LH_ROOT = path.join(__dirname, '../../')


lets put this file in scripts/i18n/

paulirish · 2018-07-09T23:04:58Z

lighthouse-core/index.js

@@ -34,6 +35,7 @@ const Config = require('./config/config');
 async function lighthouse(url, flags, configJSON) {
  // TODO(bckenny): figure out Flags types.
  flags = flags || /** @type {LH.Flags} */ ({});
+  i18n.setLocale(flags.locale || 'en-US');


we can use something like this https://github.com/brandonhorst/javascript-environment-lang/blob/master/index.js before we fall back to english. (well i suppose it wouldn't ever fall back)

paulirish · 2018-07-09T23:08:36Z

lighthouse-core/scripts/collect-strings.js

+
+fs.writeFileSync(path.join(LH_ROOT, 'lighthouse-core/lib/locales/en-US.js'), 'module.exports = ' + JSON.stringify(strings, null, 2) + ';')
+
+const gibberish = {}


wrap into a createPsuedoLocaleStrings fn (or 'gibberish')

paulirish · 2018-07-09T23:09:07Z

lighthouse-core/scripts/collect-strings.js

+collectAllStringsInDir(path.join(LH_ROOT, 'lighthouse-core'))
+console.log('Done!')
+
+fs.writeFileSync(path.join(LH_ROOT, 'lighthouse-core/lib/locales/en-US.js'), 'module.exports = ' + JSON.stringify(strings, null, 2) + ';')


template literal for this multiline thang

paulirish · 2018-07-09T23:12:14Z

lighthouse-core/scripts/collect-strings.js

+        const mod = require(fullPath)
+        if (!mod.UI_STRINGS) continue
+        for (const [key, value] of Object.entries(mod.UI_STRINGS)) {
+          strings[`${relativePath}!#${key}`] = value


wdyt about just adopting the basic chrome i18n format here:

"relativePathAndKey": { "message": "stringValue", // "description": "..." },

Like.. we could translate again into that format, but perhaps its nicer to just start with it here.
also it looks like git diff doesnt handle small changes in these strings files well. considers the WHOLE block to be a change.

yeah sg, done

paulirish · 2018-07-09T23:16:50Z

lighthouse-core/lib/i18n.js

+   * @param {string} filename
+   * @param {Record<string, string>} localStrings
+   */
+  createStringFormatter(filename, localStrings) {


localStrings => fileStrings ? or moduleStrings

local had me thinking local to the i18n file for a sec..

totally 👍 done

paulirish · 2018-07-09T23:18:41Z

lighthouse-core/lib/i18n.js

+  createStringFormatter(filename, localStrings) {
+    const mergedStrings = {...UI_STRINGS, ...localStrings};
+    /** @param {string} msg @param {*} [values] */
+    const format = (msg, values) => {


formatFn (or w/e) to differentiate from all the other 'format's in this file

paulirish · 2018-07-09T23:24:09Z

lighthouse-core/lib/i18n.js

+      const lookupKey = path.relative(LH_ROOT, filenameToLookup) + '!#' + keyname;
+      const localeStrings = LOCALES[locale] || {};
+      const localeForMessageFormat = locale === 'gibberish' ? 'de-DE' : locale;
+      const formatter = new MessageFormat(localeStrings[lookupKey] || msg, localeForMessageFormat, formats);


so we prefer to pull it from the locale file, but if its not present we fall back to the msg passed in? can you comment why we want that fallback behavior?

paulirish · 2018-07-09T23:35:34Z

lighthouse-core/lib/i18n.js

+      const filenameToLookup = keyname in UI_STRINGS ? __filename : filename;
+      const lookupKey = path.relative(LH_ROOT, filenameToLookup) + '!#' + keyname;
+      const localeStrings = LOCALES[locale] || {};
+      const localeForMessageFormat = locale === 'gibberish' ? 'de-DE' : locale;


g3 uses "en-XA" for psuedo locales. (and also en-XB and en-XC)

"en-XA", # Accented English

i'm okay doing that replacement here, though we could just switch to 'en-XA' everywhere now.

perfect, sg 👍

paulirish · 2018-07-09T23:36:49Z

lighthouse-core/lib/i18n.js

+      if (!keyname) throw new Error(`Could not locate: ${msg}`);
+
+      const parsed = MessageParser.parse(msg);
+      parsed.elements


sg. i like it. let's split into its own fn

paulirish · 2018-07-09T23:40:01Z

lighthouse-core/lib/i18n.js

+        .forEach(el => (values[el.id] = values[el.id] / 1024));
+
+      const filenameToLookup = keyname in UI_STRINGS ? __filename : filename;
+      const lookupKey = path.relative(LH_ROOT, filenameToLookup) + '!#' + keyname;


btw is !# a convention?

not that I know of, just seemed like some good characters that won't appear in a valid path and look good for separating things :)

patrickhulce · 2018-07-10T21:24:41Z

lighthouse-core/test/results/sample_v2.json

@@ -1940,7 +1937,7 @@
          {
            "key": "wastedMs",
            "valueType": "timespanMs",
-            "label": "Download Time (ms)"


And so the string consolidation begins :)

patrickhulce · 2018-07-10T21:26:51Z

lighthouse-core/config/constants.js

@@ -39,6 +39,7 @@ const defaultSettings = {

  // the following settings have no defaults but we still want ensure that `key in settings`
  // in config will work in a typechecked way
+  locale: null, // default determined by the intl library


Hm, I'm not 100% sure about this anymore. It might make more sense to default to en-US and then explicitly warn when folks try to change to a locale we don't have

@paulirish WDYT?

we don't need to block this PR on this decision IMO.

but we should be sniffing the user's locale somewhere, right? in both the CLI and browser case.

yeah sg we can punt

paulirish · 2018-07-17T21:10:20Z

Shall we land this so we can start expanding our coverage?

paulirish · 2018-07-17T21:13:47Z

lighthouse-core/audits/byte-efficiency/render-blocking-resources.js

-      {key: 'url', valueType: 'url', label: 'URL'},
-      {key: 'totalBytes', valueType: 'bytes', label: 'Size (KB)'},
-      {key: 'wastedMs', valueType: 'timespanMs', label: 'Download Time (ms)'},
+      {key: 'url', valueType: 'url', label: i18n(i18nUtils.UIStrings.columnURL)},


naming-wise, how about: uiS(i18n.UIStrings.columnURL)
bikeshedding wise, "i18n" feels most like a namespace so i hesitate to use it for a method name.

chatted outside and went with str_ for simplicity and easy future refactoring :)

patrickhulce · 2018-07-17T22:01:43Z

Shall we land this so we can start expanding our coverage?

let's!

patrickhulce · 2018-07-17T22:02:10Z

just need an approval ;)

brendankenny · 2018-07-17T23:40:41Z

lighthouse-core/lib/i18n.js

+
+/**
+ * @param {string} msg
+ * @param {Record<string, *>} values


* could be more specific? (just numbers)

I guess only numbers touched inside this function, but could have other types passed in (just not for ms or bytes), so that wouldn't work. We could probably get more clever here, though, since only a subset of them are going to support being preprocessed. Otherwise we lose all checking on these

brendankenny · 2018-07-17T23:40:47Z

lighthouse-core/lib/i18n.js

+  createStringFormatter(filename, fileStrings) {
+    const mergedStrings = {...UIStrings, ...fileStrings};
+
+    /** @param {string} msg @param {*} [values] */


brendankenny · 2018-07-17T23:44:53Z

lighthouse-core/lib/i18n.js

+      const localeString = localeStrings[lookupKey] && localeStrings[lookupKey].message;
+      // fallback to the original english message if we couldn't find a message in the specified locale
+      // better to have an english message than no message at all, in some number cases it won't even matter
+      const messageForMessageFormat = localeString || msg;


Should we differentiate between cases where there is no particular string for that locale vs there are no strings at all for that locale? If a particular string is missing in a locale should that be an error?

brendankenny · 2018-07-17T23:53:02Z

lighthouse-core/lib/i18n.js

+  /**
+   * @param {LH.Locale|null} [newLocale]
+   */
+  setLocale(newLocale) {


singletons :(

(I guess the only way to avoid is all translations could be done in the style of #5655 and just use the default locale until report generation time)

brendankenny · 2018-07-17T23:53:11Z

lighthouse-core/lib/i18n.js

+    if (!newLocale) return;
+    locale = newLocale;
+  },
+};


need tests for this :)

brendankenny · 2018-07-17T23:56:47Z

lighthouse-core/lib/locales/index.js

+ * Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except in compliance with the License. You may obtain a copy of the License at http://www.apache.org/licenses/LICENSE-2.0
+ * Unless required by applicable law or agreed to in writing, software distributed under the License is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the License for the specific language governing permissions and limitations under the License.
+ */
+// @ts-nocheck


problem here?

patrickhulce requested review from brendankenny and paulirish as code owners July 9, 2018 22:50

patrickhulce commented Jul 9, 2018

View reviewed changes

paulirish reviewed Jul 9, 2018

View reviewed changes

patrickhulce force-pushed the i18n_spike branch from 60f3da8 to 1b22262 Compare July 10, 2018 20:32

core(i18n): initial utility library

0e6bf8a

patrickhulce force-pushed the i18n_spike branch from 1b22262 to 0e6bf8a Compare July 10, 2018 20:33

patrickhulce changed the title ~~[WIP] core: i18n spike~~ core(i18n): initial utility library Jul 10, 2018

locale type checking

95666e7

patrickhulce commented Jul 10, 2018

View reviewed changes

patrickhulce added 2 commits July 10, 2018 14:25

revert package.json changes

017a007

golden lhr

14f594d

patrickhulce commented Jul 10, 2018

View reviewed changes

json locale files

f58423f

paulirish reviewed Jul 17, 2018

View reviewed changes

str_

522a7ed

paulirish approved these changes Jul 17, 2018

View reviewed changes

paulirish merged commit 589162b into master Jul 17, 2018

brendankenny reviewed Jul 17, 2018

View reviewed changes

paulirish mentioned this pull request Jul 24, 2018

Internationalization of Lighthouse (Phase 1) #5719

Closed

24 tasks

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

This was referenced May 26, 2021

[Snyk] Security upgrade puppeteer-core from 2.1.1 to 3.0.0 HijadelUFO/lighthouse#21

Open

[Snyk] Security upgrade puppeteer-core from 2.1.1 to 3.0.0 theblue888/lighthouse#10

Open

MaxMood96 mentioned this pull request Jan 9, 2023

[Snyk] Security upgrade puppeteer-core from 2.1.1 to 3.0.0 MaxMood96/lighthouse#89

Open

Smcwhir532 mentioned this pull request Jan 9, 2023

[Snyk] Security upgrade puppeteer-core from 2.1.1 to 3.0.0 Smcwhir532/lighthouse#9

Open

Benjamin-KY mentioned this pull request Jan 9, 2023

[Snyk] Security upgrade puppeteer-core from 2.1.1 to 3.0.0 Benjamin-KY/lighthouse#11

Merged

HijadelUFO mentioned this pull request Jan 9, 2023

[Snyk] Security upgrade puppeteer-core from 2.1.1 to 3.0.0 HijadelUFO/lighthouse#54

Open

kalpatarurudra mentioned this pull request Jan 10, 2023

[Snyk] Security upgrade puppeteer-core from 2.1.1 to 3.0.0 kalpatarurudra/lighthouse#15

Open

zavras mentioned this pull request Jan 10, 2023

[Snyk] Security upgrade puppeteer-core from 2.1.1 to 3.0.0 zavras/lighthouse#12

Open

ryan-ally mentioned this pull request Jan 10, 2023

[Snyk] Security upgrade puppeteer-core from 2.1.1 to 3.0.0 ryan-ally/lighthouse#24

Open

thexdesk mentioned this pull request Jan 10, 2023

[Snyk] Security upgrade puppeteer-core from 2.1.1 to 3.0.0 thexdesk/lighthouse#14

Open


		fs.writeFileSync(path.join(LH_ROOT, 'lighthouse-core/lib/locales/en-US.js'), 'module.exports = ' + JSON.stringify(strings, null, 2) + ';')

		const gibberish = {}

core(i18n): initial utility library #5644

core(i18n): initial utility library #5644

Conversation

patrickhulce commented Jul 9, 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

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

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

paulirish commented Jul 17, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

patrickhulce commented Jul 17, 2018

patrickhulce commented Jul 17, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

patrickhulce commented Jul 9, 2018 •

edited

Loading