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

patrickhulce · 2018-07-24T15:46:59Z

this should be all of the audit messages necessary for performance

what's left after this

renderer strings (core(i18n): export rendererFormattedStrings #5713)
audit table cell values
something else I'm forgetting?

Related Issues/PRs
~~blocked on #5655~~
carried over from #5692

brendankenny · 2018-07-24T20:05:30Z

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

@@ -1,16 +1,238 @@
 {
+  "lighthouse-core/audits/bootup-time.js | title": {


we'll probably want to stop generating this file when we get our first translation dump (this locale is automatically generated for us)

paulirish · 2018-07-24T22:47:35Z

lighthouse-core/test/results/sample_v2.json

-        "%10d ms",
-        1548.5690000000002
-      ],
+      "displayValue": "NaN ms",


😮

how about

diff --git a/lighthouse-core/lib/i18n.js b/lighthouse-core/lib/i18n.js index 0c4d6d24..2776af87 100644 --- a/lighthouse-core/lib/i18n.js +++ b/lighthouse-core/lib/i18n.js @@ -65,6 +65,14 @@ function _preprocessMessageValues(icuMessage, values) { const clonedValues = JSON.parse(JSON.stringify(values)); const parsed = MessageParser.parse(icuMessage); + + // Throw an error if a message's value isn't provided + parsed.elements.map(el => el.id).filter(Boolean).forEach(valueId => { + if ((valueId in values) === false) { + throw new Error('ICU Message contains a value reference that wasn\'t provided'); + } + }); +

brendankenny · 2018-07-24T23:36:29Z

lighthouse-cli/test/smokehouse/byte-efficiency/expectations.js

@@ -94,10 +94,6 @@ module.exports = [
        },
      },
      'uses-responsive-images': {
-        displayValue: [


can we assert whatever the new value is?

ya, my comment below applies here too though for rationale :)

brendankenny · 2018-07-24T23:42:22Z

lighthouse-core/audits/byte-efficiency/uses-long-cache-ttl.js

+    'A long cache lifetime can speed up repeat visits to your page. ' +
+    '[Learn more](https://developers.google.com/web/tools/lighthouse/audits/cache-policy).',
+  displayValue: `{itemCount, plural,
+    one {1 resource found}


these one entries may have to all be =1 instead, but we can wait on that

might as well do now, fixed 👍

brendankenny · 2018-07-24T23:44:48Z

lighthouse-core/test/audits/byte-efficiency/byte-efficiency-audit-test.js

@@ -176,7 +176,7 @@ describe('Byte efficiency base audit', () => {
      ],
    }, graph, simulator);

-    assert.ok(result.displayValue.includes(2), 'contains correct KB');
+    assert.ok(result.displayValue);


these definitely seem like less good tests than before. Can we just assert against the default (en-US) results?

the display values coming out of audits now are just temporary string IDs that reference something stored in i18n, so they don't have anything to do with the audit itself.

I'm somewhat against having so many layers asserting displayValue. It's been a large source of churn that hasn't really caught much. The end result in the LHR is really the only thing we care about which is OKishly captured by golden LHR.

the display values coming out of audits now are just temporary string IDs that reference something stored in i18n, so they don't have anything to do with the audit itself

good point. We could do i18n.replaceIcuMessageInstanceIds(result, 'en-US'), then test result.displayValue here, I guess.

I'm somewhat against having so many layers asserting displayValue. It's been a large source of churn that hasn't really caught much. The end result in the LHR is really the only thing we care about which is OKishly captured by golden LHR.

I mean, that's pretty much an argument against all unit tests. 90+% of the time none of them will fail, until that one time we drop a closing } to a pluralization argument and the display value just becomes a literal string (or whatever).

Generating a displayValue is pretty much all several of our audits are doing, the rest is in gatherers or computed artifacts. It doesn't seem unreasonable to assert their actual root source is generating them correctly.

(and dbw_tester.html, while a workhorse, can't cover all the cases we'd care about)

brendankenny · 2018-07-25T00:10:49Z

lighthouse-core/lib/i18n.js

@@ -58,6 +65,13 @@ function _preprocessMessageValues(icuMessage, values) {

  const clonedValues = JSON.parse(JSON.stringify(values));
  const parsed = MessageParser.parse(icuMessage);
+  // Throw an error if a message's value isn't provided
+  parsed.elements.map(el => el.id).filter(Boolean).forEach(valueId => {


the type definition seems to imply all the elements have id. Is that not true?

not sure, @paulirish did you find some without ids? :)

@brendankenny correct the types are wrong, all things with a format have id but the strings in between don't, we'll switch to filter on type === 'ArgumentElement'

can you also update the dfn in typings/intl-messageformat-parser/index.d.ts

patrickhulce · 2018-07-25T16:24:05Z

@brendankenny what did you think of #5720, would that work for you?

brendankenny

LGTM!

amazing work getting all of these extracted

brendankenny · 2018-07-25T21:14:12Z

typings/intl-messageformat-parser/index.d.ts

@@ -1,7 +1,7 @@
 declare module 'intl-messageformat-parser' {
  interface Element {
    type: 'messageTextElement'|'argumentElement';
-    id: string
+    id?: string
    value?: string
    format?: null | {type: string; style?: string};
  }


if format and id are always defined if the other is, it may make sense to make a discriminated union out of this, but fine for now (won't help with the .filter()s anyway)

patrickhulce added 2 commits July 24, 2018 08:44

core(i18n): extract performance strings to UIStrings

f84e8e8

rebased

29de8d5

patrickhulce requested review from brendankenny and paulirish as code owners July 24, 2018 15:46

patrickhulce changed the title ~~I18n all perf strings~~ core(i18n): extract performance strings to UIStrings Jul 24, 2018

patrickhulce added 2 commits July 24, 2018 08:50

plurals best practices

a4217ca

dom size comma

c34515f

brendankenny mentioned this pull request Jul 24, 2018

core: adjust audit titles for consistency #5717

Merged

patrickhulce mentioned this pull request Jul 24, 2018

core(i18n): support descriptions #5718

Merged

brendankenny reviewed Jul 24, 2018

View reviewed changes

NaNNNNN

5ef3742

paulirish reviewed Jul 24, 2018

View reviewed changes

paulirish approved these changes Jul 24, 2018

View reviewed changes

throw

112e2f8

paulirish approved these changes Jul 24, 2018

View reviewed changes

smokes

fc3b14a

paulirish mentioned this pull request Jul 24, 2018

Internationalization of Lighthouse (Phase 1) #5719

Closed

24 tasks

golden lhr

4195cc7

brendankenny requested changes Jul 25, 2018

View reviewed changes

patrickhulce added 3 commits July 24, 2018 17:19

more feedback

af8a2ef

Merge branch 'master' into i18n_all_perf_strings

2595930

argumentElement

fd87a0d

patrickhulce mentioned this pull request Jul 25, 2018

tests: better display value tests with i18n #5720

Merged

optional id typing

59da16c

brendankenny approved these changes Jul 25, 2018

View reviewed changes

brendankenny merged commit e99673b into master Jul 25, 2018

paulirish deleted the i18n_all_perf_strings branch July 26, 2018 00:09

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

patrickhulce commented Jul 24, 2018 •

edited

Loading

brendankenny Jul 24, 2018

paulirish Jul 24, 2018

brendankenny Jul 24, 2018

patrickhulce Jul 25, 2018

brendankenny Jul 24, 2018

patrickhulce Jul 25, 2018

brendankenny Jul 24, 2018

patrickhulce Jul 25, 2018

brendankenny Jul 25, 2018

brendankenny Jul 25, 2018

patrickhulce Jul 25, 2018

patrickhulce Jul 25, 2018

paulirish Jul 25, 2018

patrickhulce Jul 25, 2018

patrickhulce commented Jul 25, 2018

brendankenny left a comment

brendankenny Jul 25, 2018

		@@ -1,16 +1,238 @@
		{
		"lighthouse-core/audits/bootup-time.js \| title": {

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

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

Conversation

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

patrickhulce commented Jul 25, 2018

brendankenny left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

patrickhulce commented Jul 24, 2018 •

edited

Loading