report(psi): stub out locale swapping #13885
Conversation
shared/localization/swap-locale.js
Outdated
| const {set: _set, get: _get} = require('lodash'); | ||
| // Using these rather than the full 'lodash' saves 134kb to the report/bundle.umd.js | ||
| const _get = require('lodash.get'); | ||
| const _set = require('lodash.set'); |
There was a problem hiding this comment.
Did it really?
When I reviewed #13695 I checked for this and confirmed that all the extra lodash stuff was getting removed from the bundle.
There was a problem hiding this comment.
Did it really?
yup.
i noticed this originally after running sourcemapexplorer on the bundle and lodash was so biiiiig.
I also tried moving the two localization/*.js files to esm so the lodash import was esm for treeshaking and i didnt get any savings in that case either.
I did not know about this CVE thing though.
There was a problem hiding this comment.
I did not know about this CVE thing though.
also annoying.. the alternatives to lodash's get/set i'm finding don't use the square bracket notation we've been using... eg 'audits[is-on-https].title'. (They just use 'audits.is-on-https.title'). makes migration kinda a pain
There was a problem hiding this comment.
I just checked again and confirmed that PR did not change the size, so something else is going on here.
There was a problem hiding this comment.
But it did change the viewer i18n module:
in hindsight none of this lodash changes touched a report, except the i18n stuff, but only for the module/optional report code.
There was a problem hiding this comment.
yeah the dt bundle increased in size, which def. uses lodash. oops :(
There was a problem hiding this comment.
looking at the imported module for lodash...it's a big mess of IIFE so no way treeshaking would work
but i think we can use modules directly: lodash/isEqual.js. I'll try that and maybe open a pr.
| * @param {LH.Locale} locale | ||
| */ | ||
| async function swapLhrLocale(locale) { | ||
| const response = await fetch(`https://www.gstatic.com/pagespeed/insights/ui/locales/${locale}.json`); |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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. :)
report/clients/bundle.js
Outdated
| @@ -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'; | |||
There was a problem hiding this comment.
what uses the esm report? I forget, but it uses bundle.js file too so it will have these in there now
There was a problem hiding this comment.
DevTools uses bundle.esm.js.
(old psi used a psi.js build that was also esm, but that's dead now. new psi uses bundle.umd.js)
There was a problem hiding this comment.
so this adds code to the devtool report bundle, when it isn't needed there. should we avoid that?
or should we add swap locale to LH panel :)
Co-authored-by: Connor Clark <cjamcl@google.com>
now addressed. @connorjclark ^ |
| // Exclude this 30kb from the devtools bundle for now. | ||
| rollupPlugins.shim({ | ||
| [`${LH_ROOT}/shared/localization/i18n-module.js`]: | ||
| 'export const swapLocale = _ => {}; export const format = _ => {};', |
There was a problem hiding this comment.
Can you do what build-bundle does instead? https://github.com/GoogleChrome/lighthouse/blob/master/build/build-bundle.js#L112
There was a problem hiding this comment.
Oh, I guess you need some dummy functions exported here.
Mostly meant as a recipe for our PSI implementation.
However, we add in these APIs to the report bundle. (and save >100kb by switching the lodash import)
and needed to adopt some of the rollup/fs magic from our other builds to avoid pulling in all the JSON into the build, etc.