core(i18n): support descriptions #5718

patrickhulce · 2018-07-24T19:57:52Z

Adds support for string descriptions and defaulting all failureTitles to one for example

Related Issues/PRs
~~blocked on #5716~~

paulirish

This is totally rad. I like it.

paulirish · 2018-07-24T21:54:01Z

lighthouse-core/scripts/i18n/collect-strings.js

+        const content = fs.readFileSync(fullPath, 'utf8');
+        if (!UISTRINGS_REGEX.test(content)) continue;
+        const exportVars = require(fullPath);
+        if (!exportVars.UIStrings) throw new Error('UIStrings not exported');


UIStrings defined but not exported

paulirish · 2018-07-24T21:54:29Z

lighthouse-core/scripts/i18n/collect-strings.js

+
+        // @ts-ignore regex just matched
+        const justUIStrings = content.match(UISTRINGS_REGEX)[0];
+        // just parse the UIStrings substring to avoid ES version issues, save time, etc


👍 i like this approach.

brendankenny · 2018-07-25T00:13:17Z

lighthouse-core/scripts/i18n/collect-strings.js

@@ -10,8 +10,17 @@

 const fs = require('fs');
 const path = require('path');
+// @ts-ignore - TODO: https://github.com/DefinitelyTyped/DefinitelyTyped/pull/25410


looks like that landed?
DefinitelyTyped/DefinitelyTyped#25410

patrickhulce · 2018-07-26T00:35:11Z

@brendankenny you gellin'?

brendankenny

general approach looks good!

brendankenny · 2018-07-26T17:51:55Z

lighthouse-core/scripts/i18n/collect-strings.js

+  failureTitle: 'Shown to users as the title of the audit when it is in a failing state.',
+};
+
+// @ts-ignore - waiting for esprima types, see above TODO


no more ts-ignore? or annoying to type even with types

updated the comment, @types/esprima doesn't actually come with typedefs for any of this

brendankenny · 2018-07-26T17:55:51Z

package.json

@@ -23,16 +23,13 @@
    "build-viewer": "cd ./lighthouse-viewer && yarn build",
    "clean": "rimraf *.report.html *.report.dom.html *.report.json *.screenshots.html *.screenshots.json *.devtoolslog.json *.trace.json || true",
    "lint": "[ \"$CI\" = true ] && eslint --quiet -f codeframe . || eslint .",
-


revert these or do we just want to give up on them?

I'm personally for giving up, but I can revert if we're still trying to keep the dream alive

brendankenny · 2018-07-26T17:56:34Z

package.json

@@ -119,7 +115,7 @@
    "chrome-launcher": "^0.10.2",
    "configstore": "^3.1.1",
    "devtools-timeline-model": "1.1.6",
-    "esprima": "^4.0.0",
+    "esprima": "^4.0.1",


do we want to fix on a specific version since it's a (possibly) finicky extraction task?

eh 🤷‍♀️ we've got yarn.lock for the script and the other usage isn't that sensitive

we've got yarn.lock for the script

ha, well I feel like that's a reason to just drop the ^ since we don't honor it anyways, but

I disagree :)

This approach makes sure our usage is locked in for our script but when others install lighthouse we accept any version of esprima 4 when it doesn't matter as much in the audit

brendankenny · 2018-07-26T17:57:44Z

lighthouse-core/scripts/i18n/collect-strings.js

+        // @ts-ignore regex just matched
+        const justUIStrings = 'const ' + content.match(UISTRINGS_REGEX)[0];
+        // just parse the UIStrings substring to avoid ES version issues, save time, etc
+        // @ts-ignore - esprima's type definition is supremely lacking


yeah, it's pretty terrible as I recall

brendankenny · 2018-07-26T17:59:48Z

lighthouse-core/scripts/i18n/collect-strings.js

+        const content = fs.readFileSync(fullPath, 'utf8');
+        if (!UISTRINGS_REGEX.test(content)) continue;
+        const exportVars = require(fullPath);
+        if (!exportVars.UIStrings) throw new Error('UIStrings defined but not exported');


we should probably also do the opposite test too, UIStrings exported but failing the regex just in case one of them ends up weird in one file or something

brendankenny · 2018-07-26T18:04:03Z

lighthouse-core/scripts/i18n/collect-strings.js

 */
 function writeStringsToLocaleFormat(locale, strings) {
  const fullPath = path.join(LH_ROOT, `lighthouse-core/lib/locales/${locale}.json`);
  const output = {};
-  for (const [key, message] of Object.entries(strings)) {
-    output[key] = {message};
+  for (const [key, object] of Object.entries(strings)) {


nit: above defn is used...might be better than object here

brendankenny · 2018-07-26T18:05:31Z

lighthouse-core/scripts/i18n/collect-strings.js

+
+// @ts-ignore - waiting for esprima types, see above TODO
+function computeDescription(ast, property, startRange) {
+  const endRange = property.range[0];


do we want to prepend [ICU Syntax] here if it is?

@paulirish was against that since it only applies to ones that have the template and there are only ~5, I was persuaded as well :)

brendankenny · 2018-07-26T18:10:29Z

lighthouse-core/scripts/i18n/collect-strings.js

+const UISTRINGS_REGEX = /UIStrings = (.|\s)*?\};\n/gim;
+
+/**
+ * @typedef ICUMessage


ICUMessage.message is a little unfortunate (e.g. destructuring like const [key, msg] of Object.entries(strings) below would be weird because then you'd need to say msg.message) , but I don't have anything better. ICUMessageDefn :) Not sure if there are any other terms of art

*Defn it is!

brendankenny

LGTM!

brendankenny · 2018-07-26T22:27:24Z

package.json

@@ -119,7 +115,7 @@
    "chrome-launcher": "^0.10.2",
    "configstore": "^3.1.1",
    "devtools-timeline-model": "1.1.6",
-    "esprima": "^4.0.0",
+    "esprima": "^4.0.1",


we've got yarn.lock for the script

ha, well I feel like that's a reason to just drop the ^ since we don't honor it anyways, but

brendankenny · 2018-07-26T22:27:42Z

package.json

@@ -23,16 +23,13 @@
    "build-viewer": "cd ./lighthouse-viewer && yarn build",
    "clean": "rimraf *.report.html *.report.dom.html *.report.json *.screenshots.html *.screenshots.json *.devtoolslog.json *.trace.json || true",
    "lint": "[ \"$CI\" = true ] && eslint --quiet -f codeframe . || eslint .",
-


brendankenny · 2018-07-26T22:29:00Z

lighthouse-core/lib/locales/en-US.json

@@ -3,7 +3,8 @@
    "message": "JavaScript boot-up time"
  },
  "lighthouse-core/audits/bootup-time.js | failureTitle": {
-    "message": "JavaScript boot-up time is too high"
+    "message": "JavaScript boot-up time is too high",


this file might need to be regenerated now

patrickhulce requested review from brendankenny and paulirish as code owners July 24, 2018 19:57

paulirish approved these changes Jul 24, 2018

View reviewed changes

paulirish mentioned this pull request Jul 24, 2018

Internationalization of Lighthouse (Phase 1) #5719

Closed

24 tasks

brendankenny reviewed Jul 25, 2018

View reviewed changes

patrickhulce changed the base branch from i18n_all_perf_strings to master July 25, 2018 22:11

patrickhulce added 5 commits July 25, 2018 15:12

core(i18n): support descriptions

f55a0b1

better errors

5f9fdc8

shown to users

2f5e112

esprima typedefs

4c48ee5

merge fixes

9931884

patrickhulce force-pushed the i18n_descriptions branch from 32ca3e5 to 9931884 Compare July 25, 2018 22:16

brendankenny requested changes Jul 26, 2018

View reviewed changes

patrickhulce added 2 commits July 26, 2018 11:32

more feedback

1590621

lint

b1474c6

brendankenny approved these changes Jul 26, 2018

View reviewed changes

Merge branch 'master' into i18n_descriptions

e7fa0d4

paulirish merged commit 5633819 into master Jul 26, 2018

brendankenny deleted the i18n_descriptions branch July 26, 2018 23:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

core(i18n): support descriptions #5718

core(i18n): support descriptions #5718

patrickhulce commented Jul 24, 2018 •

edited

Loading

paulirish left a comment

paulirish Jul 24, 2018

patrickhulce Jul 24, 2018

paulirish Jul 24, 2018

brendankenny Jul 25, 2018

patrickhulce commented Jul 26, 2018

brendankenny left a comment

brendankenny Jul 26, 2018

patrickhulce Jul 26, 2018

brendankenny Jul 26, 2018

patrickhulce Jul 26, 2018

brendankenny Jul 26, 2018

brendankenny Jul 26, 2018

patrickhulce Jul 26, 2018

brendankenny Jul 26, 2018

patrickhulce Jul 26, 2018

brendankenny Jul 26, 2018

brendankenny Jul 26, 2018

patrickhulce Jul 26, 2018

brendankenny Jul 26, 2018

patrickhulce Jul 26, 2018

brendankenny Jul 26, 2018

patrickhulce Jul 26, 2018 •

edited

Loading

brendankenny Jul 26, 2018

patrickhulce Jul 26, 2018

brendankenny left a comment

brendankenny Jul 26, 2018

brendankenny Jul 26, 2018

brendankenny Jul 26, 2018

core(i18n): support descriptions #5718

core(i18n): support descriptions #5718

Conversation

patrickhulce commented Jul 24, 2018 • edited Loading

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

patrickhulce commented Jul 26, 2018

brendankenny 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

patrickhulce Jul 26, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brendankenny 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

patrickhulce commented Jul 24, 2018 •

edited

Loading

patrickhulce Jul 26, 2018 •

edited

Loading