-
Notifications
You must be signed in to change notification settings - Fork 9.4k
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
report: swap locale, enable in viewer #10148
Conversation
|
||
window.Lighthouse.i18n.registerLocaleData(locale, lhlMessages); | ||
const newLhr = window.Lighthouse.i18n.swapLocale(this.json, locale).lhr; | ||
this._swapLocaleOptions.render(newLhr); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't love this indirection - any better ideas?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah don't love this either, but a trigger to refresh
seems reasonable for the UI features class to have.
a slightly cleaner way I'm thinking of would be this class emitting a refresh
event with the new LHR that the report viewer can listen to and reinvoke render
+ initFeatures
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
|
||
// TODO: esmodules would be nice here... | ||
// @ts-ignore | ||
if (!window.Lighthouse || !window.Lighthouse.i18n) await import('./i18n-module.js'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the viewer is 216KB while i18n-module.js is 432KB.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wowza without any locales too, is that just the i18n libs themselves?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yea - altho I wasn't minimizing the latter. minimizes it's ~200
// but for locale code to locale module _name_, so that we can fetch w/ that same | ||
// name here. | ||
const localModuleName = locale; | ||
const lhlMessages = await this._swapLocaleOptions.fetchData(localModuleName); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could skip this fetch for subsequent requests for the same locale, but would need a method to query if a property in LOCALES
exists. not an important optimization so I think better to ignore it (and not expand i18n interface).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could also just create a cache in the closure of this fn we invoke 🤷♂
case 'swap-locale': { | ||
if (!this._swapLocaleOptions) throw new Error('must call .initSwapLocale first'); | ||
|
||
// TODO: esmodules would be nice here... |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I figure we can gate this feature to just browsers supporting ESModules. The only complication is actually generating an esmodule bundle from commonjs code ... Should probably use rollup for this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not 100% sold the complexity of introducing rollup is lower than the complexity of
const script = document.createElement('script')
script.src = './i18n-bundle.js'
document.body.appendChild(script)
await new Promise(r => document.addEventListener('i18nloaded', r, {once: true})
// in bundle
document.dispatchEvent(new Event('i18nloaded'))
buuuuut if you want to use this as an opportunity to start transitioning some to rollup, I understand :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
alright let's do it this way. using import
will require, as you said, rollup investment, but also upgrading eslint (import
keyword causes parse error right now) and I don't wanna update our linter right now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Run the viewer locally to try it out.
we should try to get the viewer into now deployments too :D
pretty neat! were there any particular requests for this? :)
case 'swap-locale': { | ||
if (!this._swapLocaleOptions) throw new Error('must call .initSwapLocale first'); | ||
|
||
// TODO: esmodules would be nice here... |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not 100% sold the complexity of introducing rollup is lower than the complexity of
const script = document.createElement('script')
script.src = './i18n-bundle.js'
document.body.appendChild(script)
await new Promise(r => document.addEventListener('i18nloaded', r, {once: true})
// in bundle
document.dispatchEvent(new Event('i18nloaded'))
buuuuut if you want to use this as an opportunity to start transitioning some to rollup, I understand :)
|
||
// TODO: esmodules would be nice here... | ||
// @ts-ignore | ||
if (!window.Lighthouse || !window.Lighthouse.i18n) await import('./i18n-module.js'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wowza without any locales too, is that just the i18n libs themselves?
// but for locale code to locale module _name_, so that we can fetch w/ that same | ||
// name here. | ||
const localModuleName = locale; | ||
const lhlMessages = await this._swapLocaleOptions.fetchData(localModuleName); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could also just create a cache in the closure of this fn we invoke 🤷♂
|
||
window.Lighthouse.i18n.registerLocaleData(locale, lhlMessages); | ||
const newLhr = window.Lighthouse.i18n.swapLocale(this.json, locale).lhr; | ||
this._swapLocaleOptions.render(newLhr); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah don't love this either, but a trigger to refresh
seems reasonable for the UI features class to have.
a slightly cleaner way I'm thinking of would be this class emitting a refresh
event with the new LHR that the report viewer can listen to and reinvoke render
+ initFeatures
?
@@ -190,6 +190,16 @@ class LighthouseReportViewer { | |||
|
|||
const features = new ViewerUIFeatures(dom, saveCallback); | |||
features.initFeatures(json); | |||
features.initSwapLocale({ | |||
async fetchData(localModuleName) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
localModuleName
is roughly a canonicalLocaleName
right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm thinking yes, but I'm finding that part of the locale code hard to understand. if so then yea some better names are in order.
The tsc error is the continued issue with using global types. Report doesn't know about |
shared/localization/format.js
Outdated
@@ -354,6 +359,10 @@ function hasLocale(requestedLocale) { | |||
return hasIntlSupport && hasMessages; | |||
} | |||
|
|||
function getCanonicalLocales() { | |||
return DEFAULT_LOCALES; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cc @brendankenny the swap locale ui shouldn't show the "duplicate" locales
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cc @brendankenny the swap locale ui shouldn't show the "duplicate" locales
hmm, it seems like there's a bug somewhere in #13146 then? getAvailableLocales
should be returning the union of DEFAULT_LOCALES
and Object.keys(LOCALE_MESSAGES)
. If locales.js
is shimmed to export default {}
, shouldn't const LOCALE_MESSAGES = require('./locales.js');
be just {}
, so the union of DEFAULT_LOCALES
and Object.keys(LOCALE_MESSAGES)
would be just DEFAULT_LOCALES
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, so it was fine in the viewer src, but the issue was that the jest test evaluates this in node as-is, and was including tons of extra locales failing the assertion.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if it's in the pptr test it should be ok, then? Otherwise there's not much point to most of getAvailableLocales
and it should just be updated to return Object.keys(LOCALE_MESSAGES)
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It still serves a purpose here, no?
lighthouse/lighthouse-core/lib/i18n/i18n.js
Line 137 in 1fe1ce8
const localesWithMessages = getAvailableLocales(); |
if it's in the pptr test it should be ok, then?
expect(resultBeforeSwap.options).toEqual(getCanonicalLocales());
would fail if it instead called getAvailableLocales
(even if swap-locale-feature used it too), because only the viewer build causes getAvailableLocales
to return just the "canonical" locales
This is just an issue in the test... as long as this module shim for locales.js
exists, could remove the second "get locales" function if the test just asserts length > 10
or something.
But I think the split between "canonical" (is there a better word) and "all" locales makes it more apparent what's going on.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It still serves a purpose here, no?
lighthouse/lighthouse-core/lib/i18n/i18n.js
Line 137 in 1fe1ce8
const localesWithMessages = getAvailableLocales();
that's what I meant. If getAvailableLocales
returns Object.keys(LOCALE_MESSAGES)
that line should be fine.
expect(resultBeforeSwap.options).toEqual(getCanonicalLocales());
would fail if it instead calledgetAvailableLocales
(even if swap-locale-feature used it too), because only the viewer build causesgetAvailableLocales
to return just the "canonical" locales
but viewer-test-pptr.js
is running against the viewer build, isn't it?
But I think the split between "canonical" (is there a better word) and "all" locales makes it more apparent what's going on.
yeah, I'm coming around to this view as well, just need to document the methods to make sure it's clear.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Gave updating these methods a shot, what do you think?
but viewer-test-pptr.js is running against the viewer build, isn't it?
Yup! Sorry, I've lost track of the confusion here :)
@@ -39,6 +45,12 @@ export class ViewerUIFeatures extends ReportUIFeatures { | |||
this._dom.find('.lh-tools__dropdown a[data-action="save-gist"]', this._document); | |||
saveGistItem.setAttribute('disabled', 'true'); | |||
} | |||
|
|||
this._getI18nModule().then(async (i18nModule) => { | |||
const locales = await i18nModule.format.getCanonicalLocales(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
at the moment this isn't actually "lazy". Should rendering the select element be deferred to when the user clicks on the translate button to expand?
when we include the whole i18n shared lib in the bundle always, this will be moot, so perhaps best to not bother.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
seems fine to do it like this for viewer, it's not a large file
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
still LGTM
{path: 'src/main.js', rollup: true}, | ||
{path: 'src/main.js', rollup: true, rollupPlugins: [ | ||
rollupPlugins.replace({ | ||
delimiters: ['', ''], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
after looking at the docs, this definitely deserves a comment :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ya, updated
@@ -39,6 +45,12 @@ export class ViewerUIFeatures extends ReportUIFeatures { | |||
this._dom.find('.lh-tools__dropdown a[data-action="save-gist"]', this._document); | |||
saveGistItem.setAttribute('disabled', 'true'); | |||
} | |||
|
|||
this._getI18nModule().then(async (i18nModule) => { | |||
const locales = await i18nModule.format.getCanonicalLocales(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
seems fine to do it like this for viewer, it's not a large file
Run the viewer locally to try it out.
design doc (must request access)
(selector is hidden behind button toggle)