report: swap locale, enable in viewer #10148

connorjclark · 2019-12-27T01:56:11Z

Run the viewer locally to try it out.

design doc (must request access)

(selector is hidden behind button toggle)

connorjclark · 2019-12-27T01:58:07Z

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

+
+        window.Lighthouse.i18n.registerLocaleData(locale, lhlMessages);
+        const newLhr = window.Lighthouse.i18n.swapLocale(this.json, locale).lhr;
+        this._swapLocaleOptions.render(newLhr);


I don't love this indirection - any better ideas?

yeah don't love this either, but a trigger to refresh seems reasonable for the UI features class to have.

a slightly cleaner way I'm thinking of would be this class emitting a refresh event with the new LHR that the report viewer can listen to and reinvoke render + initFeatures?

lighthouse-core/report/html/report-styles.css

connorjclark · 2019-12-27T01:59:29Z

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

+
+        // TODO: esmodules would be nice here...
+        // @ts-ignore
+        if (!window.Lighthouse || !window.Lighthouse.i18n) await import('./i18n-module.js');


the viewer is 216KB while i18n-module.js is 432KB.

wowza without any locales too, is that just the i18n libs themselves?

yea - altho I wasn't minimizing the latter. minimizes it's ~200

connorjclark · 2019-12-27T02:01:35Z

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

+        // but for locale code to locale module _name_, so that we can fetch w/ that same
+        // name here.
+        const localModuleName = locale;
+        const lhlMessages = await this._swapLocaleOptions.fetchData(localModuleName);


could skip this fetch for subsequent requests for the same locale, but would need a method to query if a property in LOCALES exists. not an important optimization so I think better to ignore it (and not expand i18n interface).

could also just create a cache in the closure of this fn we invoke 🤷‍♂

connorjclark · 2019-12-27T02:04:09Z

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

+      case 'swap-locale': {
+        if (!this._swapLocaleOptions) throw new Error('must call .initSwapLocale first');
+
+        // TODO: esmodules would be nice here...


I figure we can gate this feature to just browsers supporting ESModules. The only complication is actually generating an esmodule bundle from commonjs code ... Should probably use rollup for this.

not 100% sold the complexity of introducing rollup is lower than the complexity of

const script = document.createElement('script') script.src = './i18n-bundle.js' document.body.appendChild(script) await new Promise(r => document.addEventListener('i18nloaded', r, {once: true}) // in bundle document.dispatchEvent(new Event('i18nloaded'))

buuuuut if you want to use this as an opportunity to start transitioning some to rollup, I understand :)

alright let's do it this way. using import will require, as you said, rollup investment, but also upgrading eslint (import keyword causes parse error right now) and I don't wanna update our linter right now.

patrickhulce

Run the viewer locally to try it out.

we should try to get the viewer into now deployments too :D

pretty neat! were there any particular requests for this? :)

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

patrickhulce · 2019-12-27T04:04:33Z

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

+      case 'swap-locale': {
+        if (!this._swapLocaleOptions) throw new Error('must call .initSwapLocale first');
+
+        // TODO: esmodules would be nice here...


not 100% sold the complexity of introducing rollup is lower than the complexity of

const script = document.createElement('script') script.src = './i18n-bundle.js' document.body.appendChild(script) await new Promise(r => document.addEventListener('i18nloaded', r, {once: true}) // in bundle document.dispatchEvent(new Event('i18nloaded'))

buuuuut if you want to use this as an opportunity to start transitioning some to rollup, I understand :)

patrickhulce · 2019-12-27T04:05:18Z

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

+
+        // TODO: esmodules would be nice here...
+        // @ts-ignore
+        if (!window.Lighthouse || !window.Lighthouse.i18n) await import('./i18n-module.js');


wowza without any locales too, is that just the i18n libs themselves?

patrickhulce · 2019-12-27T04:05:55Z

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

+        // but for locale code to locale module _name_, so that we can fetch w/ that same
+        // name here.
+        const localModuleName = locale;
+        const lhlMessages = await this._swapLocaleOptions.fetchData(localModuleName);


could also just create a cache in the closure of this fn we invoke 🤷‍♂

patrickhulce · 2019-12-27T04:08:41Z

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

+
+        window.Lighthouse.i18n.registerLocaleData(locale, lhlMessages);
+        const newLhr = window.Lighthouse.i18n.swapLocale(this.json, locale).lhr;
+        this._swapLocaleOptions.render(newLhr);


yeah don't love this either, but a trigger to refresh seems reasonable for the UI features class to have.

a slightly cleaner way I'm thinking of would be this class emitting a refresh event with the new LHR that the report viewer can listen to and reinvoke render + initFeatures?

patrickhulce · 2019-12-27T04:10:22Z

lighthouse-viewer/app/src/lighthouse-report-viewer.js

@@ -190,6 +190,16 @@ class LighthouseReportViewer {

      const features = new ViewerUIFeatures(dom, saveCallback);
      features.initFeatures(json);
+      features.initSwapLocale({
+        async fetchData(localModuleName) {


localModuleName is roughly a canonicalLocaleName right?

I'm thinking yes, but I'm finding that part of the locale code hard to understand. if so then yea some better names are in order.

brendankenny · 2021-10-04T21:39:36Z

The tsc error is the continued issue with using global types. Report doesn't know about LH.IcuMessage but format.js uses it (it wasn't an error until now because report code wasn't importing before). Need to add a export import IcuMessage = IcuMessage_ to html-renderer.d.ts

connorjclark · 2021-10-04T22:53:51Z

shared/localization/format.js

@@ -354,6 +359,10 @@ function hasLocale(requestedLocale) {
  return hasIntlSupport && hasMessages;
 }

+function getCanonicalLocales() {
+  return DEFAULT_LOCALES;


cc @brendankenny the swap locale ui shouldn't show the "duplicate" locales

cc @brendankenny the swap locale ui shouldn't show the "duplicate" locales

hmm, it seems like there's a bug somewhere in #13146 then? getAvailableLocales should be returning the union of DEFAULT_LOCALES and Object.keys(LOCALE_MESSAGES). If locales.js is shimmed to export default {}, shouldn't const LOCALE_MESSAGES = require('./locales.js'); be just {}, so the union of DEFAULT_LOCALES and Object.keys(LOCALE_MESSAGES) would be just DEFAULT_LOCALES?

Ah, so it was fine in the viewer src, but the issue was that the jest test evaluates this in node as-is, and was including tons of extra locales failing the assertion.

if it's in the pptr test it should be ok, then? Otherwise there's not much point to most of getAvailableLocales and it should just be updated to return Object.keys(LOCALE_MESSAGES).

It still serves a purpose here, no?

lighthouse/lighthouse-core/lib/i18n/i18n.js

Line 137 in 1fe1ce8

const localesWithMessages = getAvailableLocales();

if it's in the pptr test it should be ok, then?

expect(resultBeforeSwap.options).toEqual(getCanonicalLocales()); would fail if it instead called getAvailableLocales (even if swap-locale-feature used it too), because only the viewer build causes getAvailableLocales to return just the "canonical" locales

This is just an issue in the test... as long as this module shim for locales.js exists, could remove the second "get locales" function if the test just asserts length > 10 or something.

But I think the split between "canonical" (is there a better word) and "all" locales makes it more apparent what's going on.

It still serves a purpose here, no?

lighthouse/lighthouse-core/lib/i18n/i18n.js

Line 137 in 1fe1ce8

const localesWithMessages = getAvailableLocales();

that's what I meant. If getAvailableLocales returns Object.keys(LOCALE_MESSAGES) that line should be fine.

expect(resultBeforeSwap.options).toEqual(getCanonicalLocales()); would fail if it instead called getAvailableLocales (even if swap-locale-feature used it too), because only the viewer build causes getAvailableLocales to return just the "canonical" locales

but viewer-test-pptr.js is running against the viewer build, isn't it?

But I think the split between "canonical" (is there a better word) and "all" locales makes it more apparent what's going on.

yeah, I'm coming around to this view as well, just need to document the methods to make sure it's clear.

Gave updating these methods a shot, what do you think?

but viewer-test-pptr.js is running against the viewer build, isn't it?

Yup! Sorry, I've lost track of the confusion here :)

connorjclark · 2021-10-06T20:07:54Z

lighthouse-viewer/app/src/viewer-ui-features.js

@@ -39,6 +45,12 @@ export class ViewerUIFeatures extends ReportUIFeatures {
        this._dom.find('.lh-tools__dropdown a[data-action="save-gist"]', this._document);
      saveGistItem.setAttribute('disabled', 'true');
    }
+
+    this._getI18nModule().then(async (i18nModule) => {
+      const locales = await i18nModule.format.getCanonicalLocales();


at the moment this isn't actually "lazy". Should rendering the select element be deferred to when the user clicks on the translate button to expand?

when we include the whole i18n shared lib in the bundle always, this will be moot, so perhaps best to not bother.

seems fine to do it like this for viewer, it's not a large file

patrickhulce

still LGTM

patrickhulce · 2021-10-06T20:36:20Z

build/build-viewer.js

-      {path: 'src/main.js', rollup: true},
+      {path: 'src/main.js', rollup: true, rollupPlugins: [
+        rollupPlugins.replace({
+          delimiters: ['', ''],


after looking at the docs, this definitely deserves a comment :)

ya, updated

patrickhulce · 2021-10-06T20:37:42Z

lighthouse-viewer/app/src/viewer-ui-features.js

@@ -39,6 +45,12 @@ export class ViewerUIFeatures extends ReportUIFeatures {
        this._dom.find('.lh-tools__dropdown a[data-action="save-gist"]', this._document);
      saveGistItem.setAttribute('disabled', 'true');
    }
+
+    this._getI18nModule().then(async (i18nModule) => {
+      const locales = await i18nModule.format.getCanonicalLocales();


seems fine to do it like this for viewer, it's not a large file

lighthouse-viewer/app/src/viewer-ui-features.js

wip

b141527

googlebot added the cla: yes label Dec 27, 2019

connorjclark commented Dec 27, 2019

View reviewed changes

lighthouse-core/report/html/report-styles.css Outdated Show resolved Hide resolved

connorjclark commented Dec 27, 2019

View reviewed changes

patrickhulce reviewed Dec 27, 2019

View reviewed changes

refresh event

43521a7

vercel bot deployed to Preview December 27, 2019 20:08 View deployment

script element load

c777b40

vercel bot deployed to Preview December 27, 2019 20:25 View deployment

connorjclark mentioned this pull request Dec 27, 2019

build: stage viewer with now-build #10151

Merged

Merge remote-tracking branch 'origin/master' into report-swap-locale

f4c54c5

vercel bot had a problem deploying to Preview December 30, 2019 21:27 Failure

Merge remote-tracking branch 'origin/master' into report-swap-locale

e10970e

vercel bot had a problem deploying to Preview January 8, 2020 23:47 Failure

paulirish mentioned this pull request Mar 3, 2020

misc(build): publish dedicated lh-i18n module #10415

Closed

connorjclark added the waiting4committer label Sep 2, 2020

connorjclark assigned patrickhulce Sep 2, 2020

connorjclark added 2 commits September 28, 2020 17:31

Merge remote-tracking branch 'origin/master' into report-swap-locale

011ee3d

first pass

8347129

vercel bot deployed to Preview September 28, 2020 23:11 View deployment

revert old stuff

6673d83

vercel bot deployed to Preview September 28, 2020 23:14 View deployment

connorjclark changed the title ~~report: swap locale~~ report: swap locale in viewer Sep 28, 2020

available locales

b790527

vercel bot deployed to Preview September 28, 2020 23:38 View deployment

tweak

54289bd

vercel bot deployed to Preview September 28, 2020 23:40 View deployment

connorjclark added waiting4reviewer waiting4committer and removed waiting4committer waiting4reviewer labels Oct 4, 2021

connorjclark added 2 commits October 4, 2021 15:28

pr

642037d

test

1fe1ce8

vercel bot had a problem deploying to Preview October 4, 2021 22:53 Failure

connorjclark commented Oct 4, 2021

View reviewed changes

rename locale stuffs

c17398b

vercel bot had a problem deploying to Preview October 4, 2021 23:39 Failure

fix

5904450

vercel bot had a problem deploying to Preview October 5, 2021 00:05 Failure

connorjclark added 2 commits October 6, 2021 13:04

tweak swap-ui-feature

af52ba7

remove _enable

3ec0948

vercel bot had a problem deploying to Preview October 6, 2021 20:06 Failure

removecomment

ca7c925

vercel bot had a problem deploying to Preview October 6, 2021 20:06 Failure

connorjclark commented Oct 6, 2021

View reviewed changes

connorjclark added waiting4reviewer and removed waiting4committer labels Oct 6, 2021

patrickhulce approved these changes Oct 6, 2021

View reviewed changes

patrickhulce added waiting4committer and removed waiting4reviewer labels Oct 6, 2021

update

1f6c2af

vercel bot had a problem deploying to Preview October 7, 2021 00:42 Failure

connorjclark merged commit eb4c1ae into master Oct 7, 2021

connorjclark deleted the report-swap-locale branch October 7, 2021 20:09

connorjclark mentioned this pull request Oct 7, 2021

report: hide locale swap button unless feature is enabled #13192

Merged

connorjclark mentioned this pull request Nov 22, 2021

clients(viewer): add support for flow reports #13260

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

report: swap locale, enable in viewer #10148

report: swap locale, enable in viewer #10148

connorjclark commented Dec 27, 2019 •

edited

Loading

connorjclark Dec 27, 2019

patrickhulce Dec 27, 2019

connorjclark Dec 27, 2019

connorjclark Dec 27, 2019

patrickhulce Dec 27, 2019

connorjclark Dec 27, 2019

connorjclark Dec 27, 2019 •

edited

Loading

patrickhulce Dec 27, 2019

connorjclark Dec 27, 2019

patrickhulce Dec 27, 2019

connorjclark Dec 27, 2019

patrickhulce left a comment

patrickhulce Dec 27, 2019

patrickhulce Dec 27, 2019

patrickhulce Dec 27, 2019

patrickhulce Dec 27, 2019

patrickhulce Dec 27, 2019

connorjclark Dec 27, 2019

brendankenny commented Oct 4, 2021

connorjclark Oct 4, 2021

brendankenny Oct 4, 2021

connorjclark Oct 4, 2021

brendankenny Oct 4, 2021

connorjclark Oct 4, 2021

brendankenny Oct 4, 2021

connorjclark Oct 4, 2021 •

edited

Loading

connorjclark Oct 6, 2021 •

edited

Loading

patrickhulce Oct 6, 2021

patrickhulce left a comment

patrickhulce Oct 6, 2021

connorjclark Oct 7, 2021

patrickhulce Oct 6, 2021

report: swap locale, enable in viewer #10148

report: swap locale, enable in viewer #10148

Conversation

connorjclark commented Dec 27, 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

connorjclark Dec 27, 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brendankenny commented Oct 4, 2021

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

connorjclark Oct 4, 2021 • edited Loading

Choose a reason for hiding this comment

connorjclark Oct 6, 2021 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

connorjclark commented Dec 27, 2019 •

edited

Loading

connorjclark Dec 27, 2019 •

edited

Loading

connorjclark Oct 4, 2021 •

edited

Loading

connorjclark Oct 6, 2021 •

edited

Loading