Skip to content

Conversation

@connorjclark
Copy link
Collaborator

@connorjclark connorjclark commented Jan 24, 2023

(first attempt: #14378)

  • deleted core/util.cjs and accompanying file creation
  • moved report/renderer/util.js to shared/util.js, but kept some stuff in new report/renderer/report-utils.js
  • added report-globals.js, where we can be honest about our globals

@connorjclark connorjclark requested a review from a team as a code owner January 24, 2023 01:24
@connorjclark connorjclark requested review from brendankenny and removed request for a team January 24, 2023 01:24
/** Label indicating that Lighthouse throttled the page using custom throttling settings. */
runtimeCustom: 'Custom throttling',
};
Util.UIStrings = UIStrings;
Copy link
Contributor

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?

Copy link
Collaborator Author

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.


after(() => {
Util.i18n = undefined;
Globals.i18n = undefined;
Copy link
Contributor

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()?

Copy link
Collaborator Author

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

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.

4 participants