-
Notifications
You must be signed in to change notification settings - Fork 9.3k
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
core: remove util.cjs #14703
core: remove util.cjs #14703
Conversation
shared/util.js
Outdated
@@ -832,7 +449,7 @@ const UIStrings = { | |||
/** Label indicating that Lighthouse throttled the page using custom throttling settings. */ | |||
runtimeCustom: 'Custom throttling', | |||
}; | |||
Util.UIStrings = UIStrings; |
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.
These strings look specific to the report renderer. Would it make more sense for them to live in ReportUtils
?
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.
Made this change, but it required not initializing strings
as a copy of UIStrings since that made a cyclic dependency between report-utils.js and report-globals.js. Only matter for test code, which had to be updated.
I then saw a nice cleanup to the globals to provide a apply
function to set them all up.
|
||
const {document} = new jsdom.JSDOM().window; | ||
dom = new DOM(document); | ||
detailsRenderer = new DetailsRenderer(dom); | ||
}); | ||
|
||
after(() => { | ||
Util.i18n = undefined; | ||
Globals.i18n = undefined; |
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.
WDYT about a Globals.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.
since it'd be just for tests - nah
(first attempt: #14378)