clients(devtools): only use locales that have locale files to download #13214

brendankenny · 2021-10-14T20:59:51Z

follow up to the removed DevTools part of #13211

Before Lighthouse is run in DevTools, localization is figured out:

run lookupLocale() with the requested locale(s), get a supported Lighthouse locale back
if returned locale isn't default en/en-US, fetch `third_party/lighthouse/locales/${locale}.json`
add fetched locale json to Lighthouse with registerLocaleData()

In the subsequent Lighthouse run, messages are available for locale

Step 1 relies on the list provided by getAvailableLocales (which is basically Object.keys(require('./locales.js'))) to know what locales are available, so the DevTools bundle is reliant on our current bundling strategy of replacing each locale with a {} (resulting in {'en-US': {}, 'en': {}, 'en-AU': {}, ...}) instead of all of locales.js with a single {}.

This is what broke in (and was removed from) #13211: Object.keys({}) is [], so there are no available locales, lookupLocale always returns the default en/en-US, no other locale is ever fetched, and the report is always in english.

But there's also a bug in the current implementation: with locales.js replaced with an object like {'en-US': {}, 'en': {}, 'en-AU': {}, ...}, Object.keys(locales) also picks up all the locale aliases, but then Step 2 will fail to fetch the alias because those don't have files to fetch (e.g. in is a valid locale to return from lookupLocale, but there is no third_party/lighthouse/locales/in.json file to download, so it will fall back to en-US instead of the much better id (which does have an id.json to fetch)).

In practice the bug may not be apparent because DevTools has its own dropdown list of locales, so there may only be a handful of possible selections that would result in a Lighthouse alias instead of a locale with a messages file.

This PR

adds an additional option to lookupLocale to provide a set of locales to (optionally) replace the default getAvailableLocales that the locale will be chosen from. devtools-entry.js then uses this option to limit the choices to just getCanonicalLocales(), which is the set of locales that have actual locale files to download
- It's no longer possible to pick a locale that can't be loaded into Lighthouse
- we're no longer dependent on the particular locales.js bundling pattern
- this feels a little fiddly, so I'm happy to hear other API ideas
restores the commit removed from i18n: add better support for the default locale in bundles #13211, replacing locales.js with a {} in the devtools bundle, as per our new bundling style

the lighthouse-i18n-run chromium webtest is happy again, and can confirm with yarn open-devtools that localization is working.

brendankenny · 2021-10-14T21:01:07Z

clients/devtools-entry.js

@@ -85,6 +96,7 @@ if (typeof self !== 'undefined') {
  self.listenForStatus = listenForStatus;
  // @ts-expect-error
  self.registerLocaleData = registerLocaleData;
+  // TODO: expose as lookupCanonicalLocale in LighthouseService.ts?


not sure how much we care, though long term it's nice when the external thing is named the same as the internal thing for cross referencing

brendankenny · 2021-10-14T21:04:41Z

lighthouse-core/lib/i18n/i18n.js

+  // TODO: lookupLocale may need to be split into two functions, one that canonicalizes
+  // locales and one that looks up the best locale filename for a given locale.
+  // e.g. `en-IE` is canonical, but uses `en-GB.json`. See TODO in locales.js


@paulirish depending on locale.js keys for aliasing continues to feel a little gross, so it would feel good to take a swing at the aliasing thing soon. This is one possible split that might make sense. Also happy to remove this TODO, now that I'm looking at it it kind of reads like a PR comment :)

brendankenny · 2021-10-14T21:06:14Z

also we probably need a better name for getCanonicalLocales which is Lighthouse locales that have a locale json file, but is distinct from canonicalized locales. I don't have a better name but this is kind of confusing when they come together in the same function :)

paulirish

could you write a test that would break prior to this PR? something around just exposing the {} locale obj to devtools?

aside from that, this sg

paulirish · 2021-10-15T17:10:34Z

clients/devtools-entry.js

@@ -60,6 +60,17 @@ function listenForStatus(listenCallback) {
  log.events.addListener('status', listenCallback);
 }

+/**
+ * Does a locale lookup but limits the result to the *canonical* Lighthouse


The names feel a bit messed ATM. shared/localization/format.js has a "canonical locales" that is our "available" locales.
And then i18n.js has "canonical locales" as well, but that's the "correct" definition matching the result of Intl.getCanonicalLocales

the i18n object uses "supportedLocales" as well. i think this naming fits in a few places, since we basically have created a concept that's congruent with (a hypothetical) intl.messageformat.supportedLocalesOf()

i'm fine addressing this outside of this PR, but just flagging it now while its fresh.

yeah, agree 100%

paulirish · 2021-10-15T17:14:54Z

build/build-bundle.js

-
-        return minifyFileTransform(file);
-      },
+      readFileTransform: minifyFileTransform,


i didnt spot this change in your description. this does mean all brfs'd items get minified yeah?

i didnt spot this change in your description. this does mean all brfs'd items get minified yeah?

yeah, it's still running the minify transform over all the inlined files. The current readFileTransform replaces the inlined file with {} if it's one of the locale files, otherwise it runs minifyFileTransform. Now we don't need to handle the individual locale files, so we can just always run minifyFileTransform.

More or less reverting that part of #12921 since we can go back to just ignoring all of locales.js

brendankenny · 2021-10-15T19:47:21Z

could you write a test that would break prior to this PR? something around just exposing the {} locale obj to devtools?

The lighthouse-i18n-run webtest actually did catch this case:

--- /Users/runner/work/lighthouse/lighthouse/lighthouse/.tmp/chromium-web-tests/content-shells/931120/out/Release/layout-test-results/retry_3/http/tests/devtools/lighthouse/lighthouse-i18n-run-expected.txt
+++ /Users/runner/work/lighthouse/lighthouse/lighthouse/.tmp/chromium-web-tests/content-shells/931120/out/Release/layout-test-results/retry_3/http/tests/devtools/lighthouse/lighthouse-i18n-run-actual.txt
@@ -11,9 +11,9 @@
 [x] Clear storage
 [x] Simulated throttling
 Generate report: enabled visible
-resolved to locale es
+resolved to locale en-US
 
-i18n footerIssue: "Notificar un problema"
+i18n footerIssue: "undefined"
 
-Footer Issue Link Text: "Notificar un problema"
+Footer Issue Link Text: "File an issue"

that only fails if locales.js is set to {}, though. I could have the test try to run es-AR. That will fail before this PR because DevTools will try to download es-AR.json, fail, and fall back to en-US, but after this PR it should pick up es (which isn't quite right but is closer). Not sure if we want to test that, though

paulirish · 2021-10-15T19:56:19Z

could you write a test that would break prior to this PR? something around just exposing the {} locale obj to devtools?

The lighthouse-i18n-run webtest actually did catch this case:

ah right. yes okay then we good. :)

brendankenny added 2 commits October 14, 2021 15:13

clients(devtools): only use locales that have locale files to download

70552a2

remove locales at locales.js level in DevTools bundle

d265c47

brendankenny requested a review from a team as a code owner October 14, 2021 20:59

brendankenny requested review from connorjclark and removed request for a team October 14, 2021 20:59

google-cla bot added the cla: yes label Oct 14, 2021

devtools-bot assigned connorjclark Oct 14, 2021

devtools-bot added the waiting4reviewer label Oct 14, 2021

brendankenny commented Oct 14, 2021

View reviewed changes

test fix for small-icu in node 12

967a114

vercel bot deployed to Preview October 14, 2021 21:20 View deployment

brendankenny assigned paulirish Oct 14, 2021

paulirish approved these changes Oct 15, 2021

View reviewed changes

brendankenny added the land-when-ci-is-green label Oct 15, 2021

devtools-bot merged commit af0bb33 into master Oct 15, 2021

devtools-bot deleted the devtools-locales branch October 15, 2021 21:09

devtools-bot removed the land-when-ci-is-green label Oct 15, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

clients(devtools): only use locales that have locale files to download #13214

clients(devtools): only use locales that have locale files to download #13214

brendankenny commented Oct 14, 2021

brendankenny Oct 14, 2021

brendankenny Oct 14, 2021

brendankenny commented Oct 14, 2021

paulirish left a comment

paulirish Oct 15, 2021

brendankenny Oct 15, 2021

paulirish Oct 15, 2021

brendankenny Oct 15, 2021

brendankenny commented Oct 15, 2021

paulirish commented Oct 15, 2021

clients(devtools): only use locales that have locale files to download #13214

clients(devtools): only use locales that have locale files to download #13214

Conversation

brendankenny commented Oct 14, 2021

brendankenny Oct 14, 2021

Choose a reason for hiding this comment

brendankenny Oct 14, 2021

Choose a reason for hiding this comment

brendankenny commented Oct 14, 2021

paulirish left a comment

Choose a reason for hiding this comment

paulirish Oct 15, 2021

Choose a reason for hiding this comment

brendankenny Oct 15, 2021

Choose a reason for hiding this comment

paulirish Oct 15, 2021

Choose a reason for hiding this comment

brendankenny Oct 15, 2021

Choose a reason for hiding this comment

brendankenny commented Oct 15, 2021

paulirish commented Oct 15, 2021