report: remove Util.UIStrings mutation, add I18n renderer class #10153

connorjclark · 2019-12-27T22:39:40Z

Cleanup for https://github.com/GoogleChrome/lighthouse/pull/9166/files#r361547882

connorjclark · 2019-12-27T22:40:21Z

lighthouse-core/report/html/renderer/report-renderer.js

@@ -249,9 +243,6 @@ class ReportRenderer {
  }
 }

-/** @type {LH.I18NRendererStrings} */
-ReportRenderer._UIStringsStash = {};


connorjclark · 2019-12-27T22:40:52Z

lighthouse-core/report/html/renderer/details-renderer.js

   */
-  constructor(dom) {
-    /** @type {DOM} */
+  constructor(dom, report) {


easiest way to get the report strings everywhere

patrickhulce

the only thing I don't like about this is that different parts of the code access the renderer formatted strings differently now with some direct JSON, some indirectly through the detailsRenderer (which feels a little muddying the purpose of detailsRenderer just for the strings :/)

WDYT about a separate object passed around called i18n or something that they can all reference consistently and perhaps use the formatting from? a pain yes, but the current tradeoff of mutability for inconsistency doesn't really seem like a clear victory

connorjclark · 2019-12-30T20:13:53Z

perhaps use the formatting from

would that include getEmulationDescriptions?

connorjclark · 2019-12-30T21:22:23Z

alright, with this second pass I moved all the stuff that is mutable (strings, locale, formatters) to an I18n class, and made it globally accessible via Util.i18n. This gets rid of the nasty mutations of Util.UIStrings without passing an extra object everywhere. Also, the audits that used some formatting now actually use the correct locale (before, it was just doing en).

patrickhulce

a far more incremental improvement, but I love that everything i18n is collected in a disposable class and split out from util now 👍

lighthouse-core/lib/i18n/i18n.js

patrickhulce · 2019-12-30T21:40:39Z

lighthouse-core/report/html/renderer/i18n.js

+
+/* globals self, URL */
+
+// Not named `NBSP` because that creates a duplicate identifier (util.js). 


😬, on of these days the grimaces will add up to a build step for renderer :)

lighthouse-core/report/html/renderer/i18n.js

lighthouse-core/report/html/renderer/report-renderer.js

lighthouse-core/report/html/renderer/report-ui-features.js

lighthouse-core/report/html/renderer/i18n.js

patrickhulce · 2019-12-30T21:50:50Z

lighthouse-core/audits/dobetterweb/dom-size.js

@@ -97,28 +96,30 @@ class DOMSize extends Audit {
      {key: 'value', itemType: 'numeric', text: str_(UIStrings.columnValue)},
    ];

+    const i18n = new I18n(context.settings.locale);


it's a bummer these won't get into the localeLog, maybe we file a separate issue to track that?

no idea what this means :)

Oh sorry I meant icuMessagePaths

when we create i18n'd strings using str we get to track it in the lhr so we can use swap-locale later, it'd be great if these usages could take advantage of that

ah right. an additional challenge - besides figuring out how to sink these paths to a central place - the swap locale script only works for UIStrings, not for value formatting.

Instead, I think we shouldn't format in the audit at all. These values should be raw numbers - and all the numeric detail types should be formatted by the report renderer (odd that this isn't the case now ...). That way there's nothing to swap.

does that make sense?

oh sorry! yes!

I think we shouldn't format in the audit at all. These values should be raw numbers - and all the numeric detail types should be formatted by the report renderer (odd that this isn't the case now ...). That way there's nothing to swap.

Agreed I think there was even a todo I put in one of my PRs during review on this topic that I've totally forgotten to merge...

Cool.. so I'll leave the changes in the audits as they are now. It's still an improvement. We can follow up on formatting in just the renderer later.

I'll fix those tests now..

…rings

connorjclark · 2020-01-04T01:34:54Z

lighthouse-core/report/html/renderer/i18n.js

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


you're avoiding the condensed version though?? 😢 😢

lol didnt notice, i just here to copy and paste stuff man

exterkamp

Love the removal of that mutation, it was always confusing. I don't have any huge feedback 👍

This is a good incremental improvement, but is it part of a larger change? I feel like it is and I'm missing the context from the PR desc.

exterkamp · 2020-01-06T23:39:28Z

lighthouse-core/report/html/renderer/i18n.js

+  /**
+   * Format number.
+   * @param {number} number
+   * @param {number=} granularity Number of decimal places to include. Defaults to 0.1.


nit: remove granularity in most of these functions.

We never send a granularity, and sending it as a decimal, instead of the number of digits you want to be significant seems like a bad API.

don't want to change functionality in this pr. this change would at least require changing many tests.

None of the unit-tests use granularity except 1. I'm fine if it's a follow up/not done, just seems like old YAGNI.

ah, i assumed.

detailsRenderer uses this a bit.

lighthouse-core/report/html/renderer/i18n.js

exterkamp · 2020-01-06T23:58:14Z

lighthouse-core/report/html/renderer/report-renderer.js

@@ -44,19 +44,13 @@ class ReportRenderer {
   * @return {Element}
   */
  renderReport(result, container) {
-    // Mutate the UIStrings if necessary (while saving originals)
-    const originalUIStrings = JSON.parse(JSON.stringify(Util.UIStrings));


connorjclark · 2020-01-07T00:17:37Z

This is a good incremental improvement, but is it part of a larger change? I feel like it is and I'm missing the context from the PR desc.

yea https://github.com/GoogleChrome/lighthouse/pull/9166/files#r361547882

Co-Authored-By: Shane Exterkamp <[email protected]>

…rings

…ghthouse into refactor-util-ui-strings

patrickhulce

LGTM! (with license and minor future concern) :)

patrickhulce · 2020-01-07T02:27:33Z

lighthouse-core/report/html/renderer/category-renderer.js

      textEl.classList.add('tooltip-boundary');
      const tooltip = this.dom.createChildOf(textEl, 'div', 'tooltip tooltip--error');
-      tooltip.textContent = audit.result.errorMessage || Util.UIStrings.errorMissingAuditInfo;
+      tooltip.textContent =
+        audit.result.errorMessage || strings.errorMissingAuditInfo;


didn't this line get shorter? must be a leftover newline from earlier version :)

patrickhulce · 2020-01-07T02:28:54Z

lighthouse-core/report/html/renderer/i18n.js

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


you're avoiding the condensed version though?? 😢 😢

patrickhulce · 2020-01-07T02:34:05Z

lighthouse-core/report/html/renderer/report-renderer.js

+      ...Util.UIStrings,
+      ...report.i18n.rendererFormattedStrings,
+    });
+    Util.i18n = i18n;


I'm a little concerned that somewhere, someday we're going to do the const strings = Util.i18n.strings shortcut in a place that doesn't happen on every render and we're going to have a subtle bug we'll probably never catch, but I don't have great suggestions around this, .strings returns an object that proxies properties to the latest Util.i18n._strings prop?

any thoughts on this?

that would require stashing that value in what is effectively a global (if it isn't being reset every render i assume it must be a global). I think good PRs / healthy dosages of "global is evil" will prevent that.

if it isn't being reset every render i assume it must be a global

This assumption during reviewing is another reason I worry about this :)

wip

b97ea5e

connorjclark requested a review from paulirish as a code owner December 27, 2019 22:39

googlebot added the cla: yes label Dec 27, 2019

connorjclark mentioned this pull request Dec 27, 2019

i18n(report): runtime settings and tools #9166

Merged

connorjclark commented Dec 27, 2019

View reviewed changes

patrickhulce reviewed Dec 29, 2019

View reviewed changes

i18n class

c9af0c5

connorjclark requested a review from brendankenny as a code owner December 30, 2019 21:19

vercel bot deployed to Preview December 30, 2019 21:19 View deployment

misc

4ede333

vercel bot deployed to Preview December 30, 2019 21:23 View deployment

patrickhulce reviewed Dec 30, 2019

View reviewed changes

pr

cdf85ac

vercel bot deployed to Preview December 31, 2019 00:43 View deployment

rm setNumberDateLocale

e4f2476

vercel bot deployed to Preview December 31, 2019 00:45 View deployment

connorjclark changed the title ~~report: remove mutable Util.UIStrings~~ report: remove Util.UIStrings mutation, add I18n class Dec 31, 2019

connorjclark changed the title ~~report: remove Util.UIStrings mutation, add I18n class~~ report: remove Util.UIStrings mutation, add I18n renderer class Dec 31, 2019

connorjclark added 2 commits January 3, 2020 15:35

Merge remote-tracking branch 'origin/master' into refactor-util-ui-st…

ff99eae

…rings

fix tests

7f5ad9e

vercel bot deployed to Preview January 4, 2020 00:09 View deployment

fix more tests

2a3b7a7

vercel bot deployed to Preview January 4, 2020 00:18 View deployment

2020

e545bf1

vercel bot deployed to Preview January 4, 2020 01:34 View deployment

connorjclark commented Jan 4, 2020

View reviewed changes

exterkamp reviewed Jan 7, 2020

View reviewed changes

Apply suggestions from code review

b4b8eed

Co-Authored-By: Shane Exterkamp <[email protected]>

vercel bot deployed to Preview January 7, 2020 00:19 View deployment

connorjclark added 2 commits January 6, 2020 16:35

Merge remote-tracking branch 'origin/master' into refactor-util-ui-st…

73e92b6

…rings

Merge branch 'refactor-util-ui-strings' of github.com:GoogleChrome/li…

bc081dd

…ghthouse into refactor-util-ui-strings

vercel bot deployed to Preview January 7, 2020 00:35 View deployment

patrickhulce approved these changes Jan 7, 2020

View reviewed changes

pr

93e6794

vercel bot deployed to Preview January 7, 2020 19:07 View deployment

connorjclark merged commit 81c6e92 into master Jan 7, 2020

connorjclark deleted the refactor-util-ui-strings branch January 7, 2020 20:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

report: remove Util.UIStrings mutation, add I18n renderer class #10153

report: remove Util.UIStrings mutation, add I18n renderer class #10153

connorjclark commented Dec 27, 2019 •

edited

Loading

connorjclark Dec 27, 2019

connorjclark Dec 27, 2019

patrickhulce left a comment

connorjclark commented Dec 30, 2019

connorjclark commented Dec 30, 2019 •

edited

Loading

patrickhulce left a comment

patrickhulce Dec 30, 2019

patrickhulce Dec 30, 2019

connorjclark Dec 31, 2019

patrickhulce Dec 31, 2019

connorjclark Dec 31, 2019 •

edited

Loading

connorjclark Jan 3, 2020

patrickhulce Jan 3, 2020

connorjclark Jan 3, 2020

connorjclark Jan 4, 2020

patrickhulce Jan 7, 2020

connorjclark Jan 7, 2020

exterkamp left a comment

exterkamp Jan 6, 2020

connorjclark Jan 7, 2020 •

edited

Loading

exterkamp Jan 7, 2020

connorjclark Jan 7, 2020

exterkamp Jan 6, 2020

connorjclark commented Jan 7, 2020

patrickhulce left a comment

patrickhulce Jan 7, 2020

patrickhulce Jan 7, 2020

patrickhulce Jan 7, 2020

connorjclark Jan 7, 2020 •

edited

Loading

patrickhulce Jan 7, 2020


		/* globals self, URL */

		// Not named `NBSP` because that creates a duplicate identifier (util.js).

report: remove Util.UIStrings mutation, add I18n renderer class #10153

report: remove Util.UIStrings mutation, add I18n renderer class #10153

Conversation

connorjclark commented Dec 27, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

patrickhulce left a comment

Choose a reason for hiding this comment

connorjclark commented Dec 30, 2019

connorjclark commented Dec 30, 2019 • edited Loading

patrickhulce 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

connorjclark Dec 31, 2019 • 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

exterkamp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

connorjclark Jan 7, 2020 • 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

connorjclark commented Jan 7, 2020

patrickhulce 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

connorjclark Jan 7, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

connorjclark commented Dec 27, 2019 •

edited

Loading

connorjclark commented Dec 30, 2019 •

edited

Loading

connorjclark Dec 31, 2019 •

edited

Loading

connorjclark Jan 7, 2020 •

edited

Loading

connorjclark Jan 7, 2020 •

edited

Loading