Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 3 commits into from
Oct 15, 2021
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 6 additions & 22 deletions build/build-bundle.js
Original file line number Diff line number Diff line change
Expand Up @@ -13,14 +13,12 @@
const fs = require('fs');
const path = require('path');
const assert = require('assert').strict;
const stream = require('stream');
const mkdir = fs.promises.mkdir;
const LighthouseRunner = require('../lighthouse-core/runner.js');
const exorcist = require('exorcist');
const browserify = require('browserify');
const terser = require('terser');
const {minifyFileTransform} = require('./build-utils.js');
const {LH_ROOT} = require('../root.js');

const COMMIT_HASH = require('child_process')
.execSync('git rev-parse HEAD')
Expand All @@ -32,9 +30,6 @@ const audits = LighthouseRunner.getAuditList()
const gatherers = LighthouseRunner.getGathererList()
.map(f => './lighthouse-core/gather/gatherers/' + f.replace(/\.js$/, ''));

const locales = fs.readdirSync(LH_ROOT + '/shared/localization/locales/')
.map(f => require.resolve(`../shared/localization/locales/${f}`));

// HACK: manually include the lighthouse-plugin-publisher-ads audits.
/** @type {Array<string>} */
// @ts-expect-error
Expand Down Expand Up @@ -67,23 +62,7 @@ async function browserifyFile(entryPath, distPath) {
})
// Transform the fs.readFile etc into inline strings.
.transform('@wardpeet/brfs', {
/** @param {string} file */
readFileTransform: (file) => {
// Don't include locales in DevTools.
if (isDevtools(entryPath) && locales.includes(file)) {
return new stream.Transform({
transform(chunk, enc, next) {
next();
},
final(next) {
this.push('{}');
next();
},
});
}

return minifyFileTransform(file);
},
readFileTransform: minifyFileTransform,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

global: true,
parserOpts: {ecmaVersion: 12},
})
Expand All @@ -107,6 +86,11 @@ async function browserifyFile(entryPath, distPath) {
bundle.ignore(require.resolve('../report/generator/report-assets.js'));
}

// Don't include locales in DevTools.
if (isDevtools(entryPath)) {
bundle.ignore(require.resolve('../shared/localization/locales.js'));
}

// Expose the audits, gatherers, and computed artifacts so they can be dynamically loaded.
// Exposed path must be a relative path from lighthouse-core/config/config-helpers.js (where loading occurs).
const corePath = './lighthouse-core/';
Expand Down
16 changes: 14 additions & 2 deletions clients/devtools-entry.js
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ const lighthouse = require('../lighthouse-core/index.js');
const RawProtocol = require('../lighthouse-core/gather/connections/raw.js');
const log = require('lighthouse-logger');
const {lookupLocale} = require('../lighthouse-core/lib/i18n/i18n.js');
const {registerLocaleData} = require('../shared/localization/format.js');
const {registerLocaleData, getCanonicalLocales} = require('../shared/localization/format.js');
const constants = require('../lighthouse-core/config/constants.js');

/** @typedef {import('../lighthouse-core/gather/connections/connection.js')} Connection */
Expand Down Expand Up @@ -60,6 +60,17 @@ function listenForStatus(listenCallback) {
log.events.addListener('status', listenCallback);
}

/**
* Does a locale lookup but limits the result to the *canonical* Lighthouse
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, agree 100%

* locales, which are only the locales with a messages locale file that can
* be downloaded and then used via `registerLocaleData`.
* @param {string|string[]=} locales
* @return {LH.Locale}
*/
function lookupCanonicalLocale(locales) {
return lookupLocale(locales, getCanonicalLocales());
}

// For the bundle smoke test.
if (typeof module !== 'undefined' && module.exports) {
// Ideally this could be exposed via browserify's `standalone`, but it doesn't
Expand All @@ -85,6 +96,7 @@ if (typeof self !== 'undefined') {
self.listenForStatus = listenForStatus;
// @ts-expect-error
self.registerLocaleData = registerLocaleData;
// TODO: expose as lookupCanonicalLocale in LighthouseService.ts?
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

// @ts-expect-error
self.lookupLocale = lookupLocale;
self.lookupLocale = lookupCanonicalLocale;
}
16 changes: 13 additions & 3 deletions lighthouse-core/lib/i18n/i18n.js
Original file line number Diff line number Diff line change
Expand Up @@ -120,12 +120,22 @@ const UIStrings = {
* - supported locales in Intl formatters
*
* If `locale` isn't provided or one could not be found, DEFAULT_LOCALE is returned.
*
* By default any of the locales Lighthouse has strings for can be returned, but this
* can be overriden with `possibleLocales`, useful e.g. when Lighthouse is bundled and
* only DEFAULT_LOCALE is available, but `possibleLocales` can be used to select a
* locale available to be downloaded on demand.
* @param {string|string[]=} locales
* @param {Array<string>=} possibleLocales
* @return {LH.Locale}
*/
function lookupLocale(locales) {
// If Node was built with `--with-intl=none`, `Intl` won't exist.
function lookupLocale(locales, possibleLocales) {
// 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
Comment on lines +133 to +135
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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 :)


if (typeof Intl !== 'object') {
// If Node was built with `--with-intl=none`, `Intl` won't exist.
throw new Error('Lighthouse must be run in Node with `Intl` support. See https://nodejs.org/api/intl.html for help');
}

Expand All @@ -135,7 +145,7 @@ function lookupLocale(locales) {
const availableLocales = Intl.NumberFormat.supportedLocalesOf(canonicalLocales);

// Get available locales and transform into object to match `lookupClosestLocale`'s API.
const localesWithMessages = getAvailableLocales();
const localesWithMessages = possibleLocales || getAvailableLocales();
const localesWithmessagesObj = /** @type {Record<LH.Locale, LhlMessages>} */ (
Object.fromEntries(localesWithMessages.map(l => [l, {}])));

Expand Down
19 changes: 19 additions & 0 deletions lighthouse-core/test/lib/i18n/i18n-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -80,5 +80,24 @@ describe('i18n', () => {
it('falls back to en-US if no match is available', () => {
expect(i18n.lookupLocale(invalidLocale)).toEqual('en-US');
});

describe('possibleLocales option', () => {
it('canonicalizes from the possible locales', () => {
expect(i18n.lookupLocale('en-xa', ['ar', 'en-XA'])).toEqual('en-XA');
});

it('takes multiple locale strings and returns a possible, canonicalized one', () => {
expect(i18n.lookupLocale([invalidLocale, 'eS', 'en-xa'], ['ar', 'es']))
.toEqual('es');
});

it('falls back to en-US if no possible match is available', () => {
expect(i18n.lookupLocale('es', ['en-US', 'ru', 'zh'])).toEqual('en-US');
});

it('falls back to en-US if no possible matchs are available at all', () => {
expect(i18n.lookupLocale('ru', [])).toEqual('en-US');
});
});
});
});