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.

Sign up for GitHub

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

report(psi): stub out locale swapping #13885

Merged
merged 10 commits into from
May 3, 2022
Merged

report(psi): stub out locale swapping #13885

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
24450e5
report(psi): stub out locale swap functionality
paulirish Apr 20, 2022
d36998d
lil cleanup
paulirish Apr 21, 2022
dfb8fef
use specific lodash packages to save 130kb
paulirish Apr 21, 2022
819c687
use hosted locales that 404. and cleanup
paulirish Apr 21, 2022
b81975f
Merge remote-tracking branch 'origin/master' into psiswaplocale
paulirish Apr 21, 2022
6dade11
Merge remote-tracking branch 'origin/master' into psiswaplocale
paulirish Apr 26, 2022
b3e7f9f
drop lodash changes in favor of #13914
paulirish Apr 26, 2022
3e398ee
Update report/clients/bundle.js
paulirish Apr 26, 2022
5ed960b
Merge remote-tracking branch 'origin/master' into psiswaplocale
paulirish May 2, 2022
68935ef
exclude swaplocale from devtools' report bundle
paulirish May 2, 2022
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
13 changes: 13 additions & 0 deletions build/build-report.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -96,6 +96,11 @@ async function buildEsModulesBundle() {
input: 'report/clients/bundle.js',
plugins: [
rollupPlugins.commonjs(),
// Exclude this 30kb from the devtools bundle for now.
rollupPlugins.shim({
[`${LH_ROOT}/shared/localization/i18n-module.js`]:
'export const swapLocale = _ => {}; export const format = _ => {};',
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you do what build-bundle does instead? https://github.com/GoogleChrome/lighthouse/blob/master/build/build-bundle.js#L112

Copy link
Collaborator

Choose a reason for hiding this comment

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

Oh, I guess you need some dummy functions exported here.

}),
],
});

Expand All @@ -110,19 +115,27 @@ async function buildUmdBundle() {
const bundle = await rollup.rollup({
input: 'report/clients/bundle.js',
plugins: [
rollupPlugins.inlineFs({verbose: true}),
rollupPlugins.commonjs(),
rollupPlugins.terser({
format: {
beautify: true,
},
}),
// Shim this empty to ensure the bundle isn't 10MB
connorjclark marked this conversation as resolved.
Show resolved Hide resolved
rollupPlugins.shim({
[`${LH_ROOT}/shared/localization/locales.js`]: 'export default {}',
'fs': 'export default {}',
}),
rollupPlugins.nodeResolve({preferBuiltins: true}),
],
});

await bundle.write({
file: 'dist/report/bundle.umd.js',
format: 'umd',
name: 'report',
sourcemap: Boolean(process.env.DEBUG),
});
await bundle.close();
}
Expand Down
1 change: 1 addition & 0 deletions report/clients/bundle.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,3 +15,4 @@ export {DOM} from '../renderer/dom.js';
export {ReportRenderer} from '../renderer/report-renderer.js';
export {ReportUIFeatures} from '../renderer/report-ui-features.js';
export {renderReport} from '../renderer/api.js';
export {swapLocale, format} from '../../shared/localization/i18n-module.js';
5 changes: 3 additions & 2 deletions report/test-assets/faux-psi-template.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -119,7 +119,7 @@
border-width: 1px 0;
padding: 4px;
}
button {
#reanalyze {
background-color: hsl(216deg 100% 50%);
border-radius: 4px;
color: white;
Expand All @@ -133,8 +133,9 @@
</head>
<body >
<noscript>PSI requires JavaScript. Please enable.</noscript>
<button type=button id="translate">→ Español</button>
<div class="page">
<button type=button>Reanalyze</button>
<button type=button id="reanalyze">Reanalyze</button>
<div class="tabset">
<input type="radio" name="tabset" id="tab1" aria-controls="mobile" checked />
<label for="tab1">Mobile</label>
Expand Down
23 changes: 22 additions & 1 deletion report/test-assets/faux-psi.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,12 @@ const wait = (ms = 100) => new Promise(resolve => setTimeout(resolve, ms));
(async function __initPsiReports__() {
renderLHReport();

document.querySelector('button')?.addEventListener('click', () => {
document.querySelector('button#reanalyze')?.addEventListener('click', () => {
renderLHReport();
});

document.querySelector('button#translate')?.addEventListener('click', async () => {
await swapLhrLocale('es');
renderLHReport();
});
})();
Expand Down Expand Up @@ -82,6 +87,22 @@ async function renderLHReport() {
}
}

/**
* @param {LH.Locale} locale
*/
async function swapLhrLocale(locale) {
const response = await fetch(`https://www.gstatic.com/pagespeed/insights/ui/locales/${locale}.json`);
Copy link
Member

Choose a reason for hiding this comment

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

In the real thing, is the list of locales suggested in the UI generated from the actual locales available so it's not going to 404?

Copy link
Member Author

Choose a reason for hiding this comment

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

ideally, but there's no certainty the locales obj we have matches what's published here. but.. it should.

I think what you may be pointing out is that we should do a lookupLocale if the locale code coming from PSI hasnt already underwent that. At this point, I'm not sure, but i'll find out. Easier to find out once we can try out this PR. :)

/** @type {import('../../shared/localization/locales').LhlMessages} */
const lhlMessages = await response.json();
console.log(lhlMessages);
if (!lhlMessages) throw new Error(`could not fetch data for locale: ${locale}`);

lighthouseRenderer.format.registerLocaleData(locale, lhlMessages);
// @ts-expect-error LHR global
window.__LIGHTHOUSE_JSON__ = lighthouseRenderer.swapLocale(window.__LIGHTHOUSE_JSON__, locale)
.lhr;
}


/**
* Tweak the LHR to make the desktop and mobile reports easier to identify.
Expand Down