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

misc(treemap): i18n #12441

Merged
merged 20 commits into from
May 14, 2021
Merged
Show file tree
Hide file tree
Changes from 14 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
19 changes: 18 additions & 1 deletion build/build-treemap.js
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,21 @@
const fs = require('fs');
const GhPagesApp = require('./gh-pages-app.js');

function buildLocales() {
Copy link
Member

Choose a reason for hiding this comment

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

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

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) + ';';
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

BUILD_INJECTED_LOCALES/__buildInjectedLocales__/sOmEtHiNgElSe?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

A comment then?

Copy link
Collaborator

Choose a reason for hiding this comment

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

self-describing variable names >> comments :)

but yes a comment would somewhat address my concern too.

}
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

because it's nested, i prefer the current approach

Copy link
Member

Choose a reason for hiding this comment

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

haha, agree with @connorjclark on this one :)


/**
* Build treemap app, optionally deploying to gh-pages if `--deploy` flag was set.
*/
Expand All @@ -28,7 +43,9 @@ async function run() {
fs.readFileSync(require.resolve('tabulator-tables/dist/js/modules/format.js'), 'utf8'),
fs.readFileSync(require.resolve('tabulator-tables/dist/js/modules/resize_columns.js'), 'utf8'),
/* eslint-enable max-len */
{path: 'src/*'},
buildLocales(),
{path: '../../lighthouse-core/report/html/renderer/i18n.js'},
{path: 'src/**/*'},
],
assets: [
{path: 'debug.json'},
Expand Down
2 changes: 1 addition & 1 deletion build/gh-pages-app.js
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,7 @@ const license = `/*
* @return {string[]}
*/
function loadFiles(pattern) {
const filePaths = glob.sync(pattern);
const filePaths = glob.sync(pattern, {nodir: true});
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

no longer necessary, but is more correct.

return filePaths.map(path => fs.readFileSync(path, {encoding: 'utf8'}));
}

Expand Down
3 changes: 2 additions & 1 deletion lighthouse-core/lib/i18n/locales.js
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,8 @@
*/
'use strict';

/** @fileoverview
/**
* @fileoverview
* Define message file to be used for a given locale. A few aliases are defined below.
*
* Google locale inheritance rules: https://goto.google.com/ccssm
Expand Down
24 changes: 24 additions & 0 deletions lighthouse-core/lib/i18n/locales/en-US.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

24 changes: 24 additions & 0 deletions lighthouse-core/lib/i18n/locales/en-XL.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

41 changes: 39 additions & 2 deletions lighthouse-core/report/html/renderer/i18n.js
Original file line number Diff line number Diff line change
Expand Up @@ -9,19 +9,25 @@

// Not named `NBSP` because that creates a duplicate identifier (util.js).
const NBSP2 = '\xa0';
const KiB = 1024;
const MiB = KiB * KiB;

/**
* @template T
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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

*/
class I18n {
/**
* @param {LH.Locale} locale
* @param {LH.I18NRendererStrings=} strings
* @param {T=} strings
*/
constructor(locale, strings) {
// When testing, use a locale with more exciting numeric formatting.
if (locale === 'en-XA') locale = 'de';

this._numberDateLocale = locale;
this._numberFormatter = new Intl.NumberFormat(locale);
this._strings = /** @type {LH.I18NRendererStrings} */ (strings || {});
this._percentFormatter = new Intl.NumberFormat(locale, {style: 'percent'});
this._strings = /** @type {T} */ (strings || {});
}

get strings() {
Expand All @@ -39,6 +45,15 @@ class I18n {
return this._numberFormatter.format(coarseValue);
}

/**
* Format percent.
* @param {number} number 0–1
* @return {string}
*/
formatPercent(number) {
return this._percentFormatter.format(number);
}

/**
* @param {number} size
* @param {number=} granularity Controls how coarse the displayed value is, defaults to 0.1
Expand All @@ -50,6 +65,17 @@ class I18n {
return `${kbs}${NBSP2}KiB`;
}

/**
* @param {number} size
* @param {number=} granularity Controls how coarse the displayed value is, defaults to 0.1
* @return {string}
*/
formatBytesToMiB(size, granularity = 0.1) {
const formatter = this._byteFormatterForGranularity(granularity);
const kbs = formatter.format(Math.round(size / 1024 ** 2 / granularity) * granularity);
return `${kbs}${NBSP2}MiB`;
}

/**
* @param {number} size
* @param {number=} granularity Controls how coarse the displayed value is, defaults to 1
Expand All @@ -61,6 +87,17 @@ class I18n {
return `${kbs}${NBSP2}bytes`;
}

/**
* @param {number} size
* @param {number=} granularity Controls how coarse the displayed value is, defaults to 1
* @return {string}
*/
formatBytesWithBestUnit(size, granularity = 1) {
if (size >= MiB) return this.formatBytesToMiB(size, granularity);
if (size >= KiB) return this.formatBytesToKiB(size, granularity);
return this.formatNumber(size, granularity) + '\xa0B';
}

/**
* Format bytes with a constant number of fractional digits, i.e for a granularity of 0.1, 10 becomes '10.0'
* @param {number} granularity Controls how coarse the displayed value is
Expand Down
4 changes: 2 additions & 2 deletions lighthouse-core/report/html/renderer/util.js
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@

/* globals self */

/** @typedef {import('./i18n')} I18n */
/** @template T @typedef {import('./i18n')<T>} I18n */

const ELLIPSIS = '\u2026';
const NBSP = '\xa0';
Expand Down Expand Up @@ -508,7 +508,7 @@ Util.getUniqueSuffix = (() => {
};
})();

/** @type {I18n} */
/** @type {I18n<typeof Util['UIStrings']>} */
// @ts-expect-error: Is set in report renderer.
Util.i18n = null;

Expand Down
5 changes: 2 additions & 3 deletions lighthouse-core/scripts/i18n/bake-ctc-to-lhl.js
Original file line number Diff line number Diff line change
Expand Up @@ -118,10 +118,9 @@ function saveLhlStrings(path, localeStrings) {

/**
* @param {string} dir
* @param {string} outputDir
* @return {Array<string>}
*/
function collectAndBakeCtcStrings(dir, outputDir) {
function collectAndBakeCtcStrings(dir) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

minor refactor, didn't need two parameters.

const lhlFilenames = [];
for (const filename of fs.readdirSync(dir)) {
const fullPath = path.join(dir, filename);
Expand All @@ -131,7 +130,7 @@ function collectAndBakeCtcStrings(dir, outputDir) {
if (!process.env.CI) console.log('Baking', relativePath);
const ctcStrings = loadCtcStrings(relativePath);
const strings = bakePlaceholders(ctcStrings);
const outputFile = outputDir + path.basename(filename).replace('.ctc', '');
const outputFile = path.join(dir, path.basename(filename).replace('.ctc', ''));
saveLhlStrings(outputFile, strings);
lhlFilenames.push(path.basename(filename));
}
Expand Down
7 changes: 4 additions & 3 deletions lighthouse-core/scripts/i18n/collect-strings.js
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ const UISTRINGS_REGEX = /UIStrings = .*?\};\n/s;

const foldersWithStrings = [
`${LH_ROOT}/lighthouse-core`,
`${LH_ROOT}/lighthouse-treemap`,
path.dirname(require.resolve('lighthouse-stack-packs')) + '/packs',
];

Expand All @@ -39,6 +40,7 @@ const ignoredPathComponents = [
'**/test/**',
'**/*-test.js',
'**/*-renderer.js',
'lighthouse-treemap/app/src/main.js',
];

/**
Expand Down Expand Up @@ -586,7 +588,7 @@ function collectAllStringsInDir(dir) {
if (seenStrings.has(ctc.message)) {
ctc.meaning = ctc.description;
const seenId = seenStrings.get(ctc.message);
if (seenId) {
if (seenId && strings[seenId]) {
if (!strings[seenId].meaning) {
patrickhulce marked this conversation as resolved.
Show resolved Hide resolved
strings[seenId].meaning = strings[seenId].description;
collisions++;
Expand Down Expand Up @@ -646,8 +648,7 @@ if (require.main === module) {
console.log('Written to disk!', 'en-XL.ctc.json');

// Bake the ctc en-US and en-XL files into en-US and en-XL LHL format
const lhl = collectAndBakeCtcStrings(path.join(LH_ROOT, 'lighthouse-core/lib/i18n/locales/'),
path.join(LH_ROOT, 'lighthouse-core/lib/i18n/locales/'));
const lhl = collectAndBakeCtcStrings(path.join(LH_ROOT, 'lighthouse-core/lib/i18n/locales/'));
lhl.forEach(function(locale) {
console.log(`Baked ${locale} into LHL format.`);
});
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -569,7 +569,18 @@ describe('ReportUIFeatures', () => {
const container = render(sampleResults);
for (const node of dom.findAll('[data-i18n]', container)) {
const val = node.getAttribute('data-i18n');
assert.ok(val in Util.UIStrings, `Invalid data-i18n value of: "${val}" found.`);
assert.ok(val in Util.UIStrings, `Invalid data-i18n value of: "${val}" not found.`);
}

// Do the same for the strings in treemap app.
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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).

Copy link
Member

@brendankenny brendankenny May 11, 2021

Choose a reason for hiding this comment

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

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

const TreemapUtil = require('../../../../../lighthouse-treemap/app/src/util.js');
const TREEMAP_INDEX = fs.readFileSync(__dirname +
'/../../../../../lighthouse-treemap/app/index.html', 'utf8');
const document = new jsdom.JSDOM(TREEMAP_INDEX);
dom = new DOM(document.window.document);
for (const node of dom.findAll('[data-i18n]', dom._document)) {
const val = node.getAttribute('data-i18n');
assert.ok(val in TreemapUtil.UIStrings, `Invalid data-i18n value of: "${val}" not found.`);
}
});
});
Expand Down
2 changes: 1 addition & 1 deletion lighthouse-treemap/app/index.html
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,7 @@

<span class="lh-header__inputs">
<select class="bundle-selector"></select>
<button class="lh-button lh-button--active lh-button--toggle-table">Toggle Table</button>
<button data-i18n="toggleTable" class="lh-button lh-button--active lh-button--toggle-table"></button>
</span>
</div>

Expand Down
Loading