-
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
clients(devtools): expose registerLocaleData to worker #9645
Conversation
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.
What do you think about removing registerLocaleData
from the lighthouse module and calling it on i18n
in here?
If the intention is for registerLocaleData
to only ever be used by devtools and it has to be exposed on devtools-entry
regardless of whether it's on lighthouse
(and my bundler solution doesn't work for you), then that would at least keep the singleton-stomping power limited to a well-behaved client.
great suggestion. i'll do that. |
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.
lgtm with the restore code fix
afterEach(() => { | ||
// eslint-disable-next-line no-unused-vars | ||
let replacedLocales = require('../../../lib/i18n/locales.js'); | ||
replacedLocales = clonedLocales; |
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 like this wouldn't restore it (just overwrite the local var)? It might have to restore all the individual props (or just the single test could do it for just the one locale)
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.
quite true. addressed in cd9f8fd
clients/devtools-entry.js
Outdated
@@ -8,6 +8,7 @@ | |||
const lighthouse = require('../lighthouse-core/index.js'); | |||
const RawProtocol = require('../lighthouse-core/gather/connections/raw.js'); | |||
const log = require('lighthouse-logger'); | |||
const {registerLocaleData} = require('../lighthouse-core/lib/i18n/i18n.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.
Would you like to export the lookupLocale with this function too?
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.
good call. done.
// @ts-ignore | ||
self.registerLocaleData = registerLocaleData; | ||
// @ts-ignore | ||
self.lookupLocale = lookupLocale; |
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.
@exterkamp FYI
No description provided.