misc(treemap): i18n by connorjclark · Pull Request #12441 · GoogleChrome/lighthouse

connorjclark · 2021-05-04T20:57:18Z

~~Strings added via rendererFormattedStrings.~~ Considered an approach where the app would work out its own strings, but that would require 1) an extra round trip to get strings based off the LHR/useragent before we can render the app or 2) bundle strings for all languages into the bundle. ~~Using rendererFormattedStrings also keeps things simpler.~~

connorjclark · 2021-05-04T20:59:14Z

+  /** Text for an option in a dropdown menu, when selected the current view of the app is set to all scripts. */
+  treemapAllScripts: 'All Scripts',
+  /** Label for a column where the values are URLs, JS module names, or arbitrary identifiers. For simplicity, just 'name' is used. */
+  treemapName: 'Name',


Name or URL / Module ?

which column is this in treemap app exactly? it's been awhile since I've used it should we make it part of the vercel deployments on PRs?

Identifier perhaps?

connorjclark · 2021-05-04T21:12:11Z


      const percent = Math.floor(100 * dataRow.unusedBytes / dataRow.resourceBytes);
-      return `${percent}% bytes unused`;
+      return `${Util.i18n.formatNumber(percent)}% bytes unused`;


I don't think we have a good way to handle this; thoughts? It's just a tooltip...

should we do the "nearly correct" thing and just string concat this formatted number w/ a Util.i18n.bytesUnused ?

oof, um what about filling with a prefilled value and replacing in the report?

e.g. ask translators to do {percentUnused}% bytes unused and for rendererFormattedStrings we prefill percentUnused to be 123456789 or some specific predetermined number?

but how would we do the value replacement at runtime? can we do something like

const data = replaceIcuMessages({string: str_(myStringWithPlaceholders, {value: 123}}) // get string from data ....

Yeah IDK, but either way I think we can punt for now :)

Maybe we start an issue with i18n frontend use cases? This happens in the report to some extent too we just chopped off the explanations to make them less helpful.

connorjclark · 2021-05-04T21:12:47Z


    if (bytes !== undefined && total !== undefined) {
-      let str = `${TreemapUtil.formatBytes(bytes)} (${Math.round(bytes / total * 100)}%)`;
+      const percentStr = `${Util.i18n.formatNumber(Math.round(bytes / total * 100))}%`;


is concating % here OK?

I'm not sure, good question, but there's a lot of concat going on here with the partitionByStr: and then parts.join too so I'm not sure that's even our biggest issue. Since we're best effort on this already, seems fine file an issue to solve this report use case? 🤷

for plain % (not concatenating to another string), there's new Intl.NumberFormat(locale, {style: 'percent'}).format(value). You have to be careful with report formatting stuff because Safari formatting support has really lagged the other browsers (and Node), but 'percent' has been supported for a while now.

but then this value isn't just ##%, it's put into let str = `${TreemapUtil.formatBytes(bytes)} (${percentStr})`;, so agree with the 🤷 :)

brendankenny · 2021-05-04T21:39:53Z

bundle strings for all languages into the bundle

Can you talk a bit about the downsides of this approach? 8 pretty short strings seems not so bad. It seems like the hardest part would be replicating locales.js without actually replicating it, then trimming down the locale files to just strings under lighthouse-treemap/.

rendererFormattedStrings, on the other hand, have to be baked into the proto (forever), are less forward compatible, etc.

connorjclark · 2021-05-04T21:57:29Z

Rough estimate: at 100 bytes per string per locale, and 8 strings, that is 50 * 800 = 40 kB. Currently the bundle is ~200 kB (on disk). Is that acceptable?

As for implementation, I suppose it wouldn't be too bad. Instead of

const i18n = new I18n(report.configSettings.locale, {
    // Set missing renderer strings to default (english) values.
    ...Util.UIStrings,
    ...report.i18n.rendererFormattedStrings,
  });

we just do

const i18n = new I18n(report.configSettings.locale, {
    ...require('./locales.js')['en'],
    ...require('./locales.js')[report.configSettings.locale], // need to use lookupLocale ???
  });

where locales.js is a file just like the main locales. lighthouse-treemap/app/src/locales/ could contain all the locale files, and we'd have to modify the collect-strings script to input strings from lighthouse-treemap/app/src and output them to lighthouse-treemap/app/src/locales/.

patrickhulce

Big picture wise, it feels really weird to have another app's strings just hanging out in the LHR on the chance that someone has source maps and then wants to open them up in the treemap viewer.

an extra round trip to get strings based off the LHR/useragent before we can render the app

This really doesn't sound that bad to me. This would be a super small payload, could be loaded speculatively first based on user's locale (possibly even better than LHR's locale?), and on a warm connection even on moderate 3G get these in 300ms.

I'm not expecting treemap viewer to be the best mobile experience anyway (I mean the LH mobile report experience is already awful, the metrics get cutoff so you can't even see the most important values)

Was there an underlying issue that this approach was more difficult to accomplish or more work? I wouldn't let this extra round trip stop separation of these strings for treemap :)

patrickhulce · 2021-05-04T21:25:49Z

+  /** Text for an option in a dropdown menu, when selected the current view of the app is set to all scripts. */
+  treemapAllScripts: 'All Scripts',
+  /** Label for a column where the values are URLs, JS module names, or arbitrary identifiers. For simplicity, just 'name' is used. */
+  treemapName: 'Name',


which column is this in treemap app exactly? it's been awhile since I've used it should we make it part of the vercel deployments on PRs?

Identifier perhaps?

patrickhulce · 2021-05-04T21:26:45Z

+  treemapName: 'Name',
+  /** Label for a value associated with how many bytes a URL/file is, on-disk. */
+  treemapResourceBytes: 'Resource Bytes',
+  /** Label for a value associated with how many bytes a URL/file is, over-network. */


this one doesn't seem right

Suggested change

/** Label for a value associated with how many bytes a URL/file is, over-network. */

/** Label for a value stating how many bytes of a URL/file were not used. */

maybe?

patrickhulce · 2021-05-04T21:27:31Z

+  treemapAllScripts: 'All Scripts',
+  /** Label for a column where the values are URLs, JS module names, or arbitrary identifiers. For simplicity, just 'name' is used. */
+  treemapName: 'Name',
+  /** Label for a value associated with how many bytes a URL/file is, on-disk. */


I'm not sure I understand the on disk distinction. Just saying it's the uncompressed size?

patrickhulce · 2021-05-04T21:28:06Z

Sign in to view

  if (collisions > 0) {
    console.log(`MEANING COLLISION: ${collisions} string(s) have the same content.`);
-    assert.equal(collisions, 30, `The number of duplicate strings have changed, update this assertion if that is expected, or reword strings. Collisions: ${collisionStrings.join('\n')}`);
+    assert.equal(collisions, 32, `The number of duplicate strings have changed, update this assertion if that is expected, or reword strings. Collisions: ${collisionStrings.join('\n')}`);


name, and....?

patrickhulce · 2021-05-04T21:30:40Z


      const percent = Math.floor(100 * dataRow.unusedBytes / dataRow.resourceBytes);
-      return `${percent}% bytes unused`;
+      return `${Util.i18n.formatNumber(percent)}% bytes unused`;


oof, um what about filling with a prefilled value and replacing in the report?

e.g. ask translators to do {percentUnused}% bytes unused and for rendererFormattedStrings we prefill percentUnused to be 123456789 or some specific predetermined number?

patrickhulce · 2021-05-04T21:33:36Z


    if (bytes !== undefined && total !== undefined) {
-      let str = `${TreemapUtil.formatBytes(bytes)} (${Math.round(bytes / total * 100)}%)`;
+      const percentStr = `${Util.i18n.formatNumber(Math.round(bytes / total * 100))}%`;


I'm not sure, good question, but there's a lot of concat going on here with the partitionByStr: and then parts.join too so I'm not sure that's even our biggest issue. Since we're best effort on this already, seems fine file an issue to solve this report use case? 🤷

patrickhulce · 2021-05-04T21:38:17Z

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


why do we need this? I thought this was just for the performance category renderer

google-cla · 2021-05-04T22:00:47Z

☹️ Sorry, but only Googlers may change the label cla: yes.

Sign in to view

  if (collisions > 0) {
    console.log(`MEANING COLLISION: ${collisions} string(s) have the same content.`);
-    assert.equal(collisions, 30, `The number of duplicate strings have changed, update this assertion if that is expected, or reword strings. Collisions: ${collisionStrings.join('\n')}`);
+    assert.equal(collisions, 32, `The number of duplicate strings have changed, update this assertion if that is expected, or reword strings. Collisions: ${collisionStrings.join('\n')}`);


mspaansen

Ik probeer het. Maar kost me veel tijd

connorjclark · 2021-05-06T23:03:45Z

waiting for feedback on #12441 (comment)

@patrickhulce I believe I commented this at the exact time you submitted your review, so likely you missed it.

patrickhulce

LGTM, just the minor concern about the collision check undefined strangeness

patrickhulce · 2021-05-11T18:27:18Z

+function buildLocales() {
+  const locales = require('../lighthouse-core/lib/i18n/locales.js');
+  const clonedLocales = JSON.parse(JSON.stringify(locales));
+
+  for (const lhlMessages of Object.values(clonedLocales)) {
+    for (const key of Object.keys(lhlMessages)) {
+      if (!key.startsWith('lighthouse-treemap')) {
+        delete lhlMessages[key];
+      }
+    }
+  }
+
+  return 'const locales =' + JSON.stringify(clonedLocales, null, 2) + ';';
+}


just for fun since we can use fromEntries, what are your thoughts on the readability of the below compared to the delete?

Suggested change

function buildLocales() {

const locales = require('../lighthouse-core/lib/i18n/locales.js');

const clonedLocales = JSON.parse(JSON.stringify(locales));

for (const lhlMessages of Object.values(clonedLocales)) {

for (const key of Object.keys(lhlMessages)) {

if (!key.startsWith('lighthouse-treemap')) {

delete lhlMessages[key];

}

}

}

return 'const locales =' + JSON.stringify(clonedLocales, null, 2) + ';';

}

function buildLocales() {

const locales = Object.entries(require('../lighthouse-core/lib/i18n/locales.js'));

const treemapLocales = locales.map(([locale, messageMap]) => {

const lhlMessages = Object.entries(messageMap);

const treemapMessages = lhlMessages.filter(([key]) => key.startsWith('lighthouse-treemap'));

return [locale, Object.fromEntries(treemapMessages)];

});

return 'const locales =' + JSON.stringify(Object.fromEntries(treemapLocales), null, 2) + ';';

}

(not serious review, just curious) :)

because it's nested, i prefer the current approach

haha, agree with @connorjclark on this one :)

patrickhulce · 2021-05-11T18:29:08Z

+    }
+  }
+
+  return 'const locales =' + JSON.stringify(clonedLocales, null, 2) + ';';


with something this specially injected, WDYT about the name being a little more in your face?

BUILD_INJECTED_LOCALES/__buildInjectedLocales__/sOmEtHiNgElSe?

I don't think the cloak and dagger is necessary here, since we control the code that runs here (and it's not a library) :)

I'm mostly interested in improving the experience for someone reading locales referenced without a definition and wondering where in the world it comes from. If you tried to grep locales in our codebase you'd get like 160 results. With localesInjectedAtBuildTime you'd get 1 :)

A comment then?

self-describing variable names >> comments :)

but yes a comment would somewhat address my concern too.

patrickhulce · 2021-05-11T18:37:05Z


    for (const [group, depthOneNodes] of Object.entries(this.depthOneNodesByGroup)) {
-      makeOption({type: 'group', value: group}, `All ${group}`);
+      const allLabel = {


what's the purpose of this object? are there about to be many types here?

maybe, i don't know, I'm being careful here.

is there something dangerous that could happen by using the string directly? ;)

we have just one group now (scripts).

there's also a small part of me thinking someone will send us their own treemap data, and their own groups. unlikely, but....

anyhow, the object will be needed regardless as soon as we have a second group (however that turns out–I think resource summary will be a group? idk yet)

patrickhulce · 2021-05-11T18:41:03Z

+  const i18n = new I18n(options.lhr.configSettings.locale, {
+    // Set missing renderer strings to default (english) values.
+    ...TreemapUtil.UIStrings,
+    ...getStrings(locales[options.lhr.configSettings.locale]),


does this handle all that fallback business? I can't remember off the top of my head if the configSettings locale is normalized to always match our defined locale (e.g. for user-provided de-CH which matches our de key, will this work or miss?)

EDIT: seems like we should be fine

lighthouse/lighthouse-core/config/config-helpers.js

Line 319 in f06807b

settingsWithFlags.locale = locale;

brendankenny · 2021-05-11T19:16:47Z

 const fs = require('fs');
 const GhPagesApp = require('./gh-pages-app.js');

+function buildLocales() {


a jsdoc comment on this function would be appreciated so you don't have to know the structure of locales.js and the files it requires and what the significance of key.startsWith('lighthouse-treemap') is to know what this is doing to the locale files :)

brendankenny · 2021-05-11T19:17:30Z

+function buildLocales() {
+  const locales = require('../lighthouse-core/lib/i18n/locales.js');
+  const clonedLocales = JSON.parse(JSON.stringify(locales));
+
+  for (const lhlMessages of Object.values(clonedLocales)) {
+    for (const key of Object.keys(lhlMessages)) {
+      if (!key.startsWith('lighthouse-treemap')) {
+        delete lhlMessages[key];
+      }
+    }
+  }
+
+  return 'const locales =' + JSON.stringify(clonedLocales, null, 2) + ';';
+}


haha, agree with @connorjclark on this one :)

brendankenny · 2021-05-11T19:33:53Z

+const MiB = KiB * KiB;

+/**
+ * @template T


parameterizing on strings when none of the I18n object uses strings except the getter for strings works for this PR but makes me think it really shouldn't be in this object at all and should just be a global (or on Util directly), but we can save that for a future report refactor PR :)

Moving to Util/TreemapUtil SGTM, and so does worrying about it in a future refactor ;)

brendankenny · 2021-05-11T19:37:57Z

+        assert.ok(val in Util.UIStrings, `Invalid data-i18n value of: "${val}" not found.`);
+      }
+
+      // Do the same for the strings in treemap app.


can this move to a treemap test since report-ui-features isn't involved?

also, FWIW, if it's just the single string, could just set it manually and avoid the loop here and in main.js

there's just so much BLEGH to do re: jsdom setup... altho I guess it could go into the puppeteer page?

if it's just the single string,

I think there will be more later (if we do the settings cog).

there's just so much BLEGH to do re: jsdom setup

normally true but I think it's all self contained in this case. Free test for you:

const assert = require('assert').strict; const fs = require('fs'); const jsdom = require('jsdom'); describe('data-i18n', () => { it('should have only valid data-i18n values in treemap html', () => { const TreemapUtil = require('../../../../../lighthouse-treemap/app/src/util.js'); const TREEMAP_INDEX = fs.readFileSync(__dirname + '/../../../../../lighthouse-treemap/app/index.html', 'utf8'); const dom = new jsdom.JSDOM(TREEMAP_INDEX); for (const node of dom.window.document.querySelectorAll('[data-i18n]')) { const val = node.getAttribute('data-i18n'); assert.ok(val in TreemapUtil.UIStrings, `Invalid data-i18n value of: "${val}" not found.`); } }); });

though the paths will have to be updated :)

brendankenny · 2021-05-11T19:46:01Z

+
+TreemapUtil.UIStrings = {
+  /** Label for a button that alternates between showing or hiding a table. */
+  toggleTable: 'Toggle Table',


in the main report we tend to be more self documenting with these, e.g.toggleTableButtonLabel for this. It seems like a good approach generally?

I guess I'm less concerned about 'toggleTable' and more about things like 'all'.

brendankenny · 2021-05-11T19:54:14Z

+  /** Label for a value associated with how many bytes a URL/file is, over-network. */
+  unusedBytes: 'Unused Bytes',


did this description and string diverge?

wait yeah, I had these exact same comments already, did those get those in the move?

The strings moved from Util to TreemapUtil, so github prob hid ur comments

havent fixed the original comments yet :P

brendankenny · 2021-05-11T20:03:13Z

+    const [filename, varName] = icuMessageId.split(' | ');
+    if (!filename.endsWith('util.js')) throw new Error(`Unexpected message: ${icuMessageId}`);
+
+    const key = /** @type {keyof LH.I18NRendererStrings} */ (varName);


definitely not LH.I18NRendererStrings :)

Can chopping off the path be moved to the build step? If we don't use them anyways, it would certainly cut down that 40KiB to something much smaller :)

Co-authored-by: Brendan Kenny <bckenny@gmail.com>

patrickhulce

still missing the straggler OG comments, but I'll approve :)

patrickhulce · 2021-05-12T03:08:25Z

+  for (const lhlMessages of Object.values(clonedLocales)) {
+    for (const icuMessageId of Object.keys(lhlMessages)) {
+      const lhlMessage = lhlMessages[icuMessageId];
+      delete lhlMessages[icuMessageId];


ok, c'mon y'all now it's really not a clone 😆 still no construction love?

lol ok i surrender

also renamed this to strings since it is different from the real locales

patrickhulce · 2021-05-12T03:09:04Z

+  tableColumnName: 'Name',
+  /** Label for column giving the size of a file in bytes. */
+  resourceBytesLabel: 'Resource Bytes',
+  /** Label for a value associated with how many bytes a URL/file is, over-network. */


patrickhulce

Still lgtm

connorjclark added 2 commits April 30, 2021 17:33

wip

51bae87

update

7ff5861

connorjclark requested a review from a team as a code owner May 4, 2021 20:57

connorjclark requested review from patrickhulce and removed request for a team May 4, 2021 20:57

google-cla Bot added the cla: yes label May 4, 2021

devtools-bot assigned patrickhulce May 4, 2021

devtools-bot added the waiting4reviewer label May 4, 2021

connorjclark commented May 4, 2021

View reviewed changes

Comment thread build/build-treemap.js Outdated

connorjclark commented May 4, 2021

View reviewed changes

Comment thread lighthouse-treemap/app/src/util.js Outdated

patrickhulce reviewed May 4, 2021

View reviewed changes

patrickhulce added waiting4committer and removed cla: yes waiting4reviewer labels May 4, 2021

google-cla Bot added the cla: yes label May 4, 2021

Merge remote-tracking branch 'origin/master' into treemap-i18n

3ca66c4

mspaansen approved these changes May 5, 2021

View reviewed changes

connorjclark added waiting4reviewer and removed waiting4committer labels May 5, 2021

update

c06b918

vercel Bot deployed to Preview May 5, 2021 23:32 View deployment

connorjclark mentioned this pull request May 5, 2021

Lighthouse Treemap ☂️ #11254

Closed

24 tasks

fixtest

12b1ba8

vercel Bot deployed to Preview May 11, 2021 16:40 View deployment

tweak

e150536

vercel Bot deployed to Preview May 11, 2021 16:41 View deployment

Merge remote-tracking branch 'origin/master' into treemap-i18n

b76273f

vercel Bot deployed to Preview May 11, 2021 17:59 View deployment

patrickhulce reviewed May 11, 2021

View reviewed changes

comment

78f168c

vercel Bot deployed to Preview May 11, 2021 19:01 View deployment

brendankenny reviewed May 11, 2021

View reviewed changes

Apply suggestions from code review

a97202d

Co-authored-by: Brendan Kenny <bckenny@gmail.com>

vercel Bot deployed to Preview May 11, 2021 21:29 View deployment

connorjclark added 2 commits May 11, 2021 15:52

pr

44a203f

updatestrings

eddd1af

vercel Bot deployed to Preview May 11, 2021 21:56 View deployment

lol

9a7f0f8

vercel Bot deployed to Preview May 11, 2021 23:25 View deployment

patrickhulce approved these changes May 12, 2021

View reviewed changes

patrickhulce added waiting4committer and removed waiting4reviewer labels May 12, 2021

connorjclark added 2 commits May 14, 2021 11:03

strings

cdc698e

comment

6f9e53c

connorjclark added waiting4reviewer and removed waiting4committer labels May 14, 2021

vercel Bot deployed to Preview May 14, 2021 18:07 View deployment

patrickhulce approved these changes May 14, 2021

View reviewed changes

connorjclark merged commit 3247457 into master May 14, 2021

connorjclark deleted the treemap-i18n branch May 14, 2021 20:19

brendankenny mentioned this pull request May 17, 2021

i18n: import new strings #12492

Merged

brendankenny mentioned this pull request Jun 25, 2021

i18n: fix multi-directory collision checking #12698

Closed

	/** Label for a value associated with how many bytes a URL/file is, over-network. */
	/** Label for a value stating how many bytes of a URL/file were not used. */

		/** Label for a value associated with how many bytes a URL/file is, over-network. */
		unusedBytes: 'Unused Bytes',

Conversation

connorjclark commented May 4, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

brendankenny commented May 4, 2021

Uh oh!

connorjclark commented May 4, 2021

Uh oh!

patrickhulce left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

This comment was marked as spam.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

google-cla Bot commented May 4, 2021

Uh oh!

This comment was marked as spam.

Uh oh!

mspaansen left a comment

Choose a reason for hiding this comment

Uh oh!

connorjclark commented May 6, 2021

Uh oh!

patrickhulce left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

connorjclark commented May 4, 2021 •

edited

Loading