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

clients(devtools): expose registerLocaleData to worker #9645

Merged
merged 7 commits into from Sep 19, 2019

Conversation

paulirish
Copy link
Member

No description provided.

Copy link
Member

@brendankenny brendankenny left a 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.

@paulirish
Copy link
Member Author

What do you think about removing registerLocaleData from the lighthouse module and calling it on i18n in here?

great suggestion. i'll do that.

Copy link
Member

@brendankenny brendankenny left a 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;
Copy link
Member

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)

Copy link
Member Author

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

@@ -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');

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?

Copy link
Member Author

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;
Copy link
Member

Choose a reason for hiding this comment

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants