core: remove util.cjs, break up util.js to shared and report utils #14378
Conversation
This reverts commit ff54f559013186e001362ccbbc9b44bb0a33bd0a.
| export { | ||
| Util, | ||
| ReportUtils, | ||
| SharedUtils, | ||
| UIStrings, | ||
| }; | ||
|
|
||
| export * as MarkdownUtils from '../../shared/markdown.js'; |
There was a problem hiding this comment.
git mv report/renderer/util.js shared/util.js is a lot less churn/blame breakage and accomplishes essentially the same thing (and with less awkward naming/renaming). util.js is really already set up for this since it has no deps.
The markdown methods seem like the only part maybe crying out to be a separate file, though they've also been fine.
There was a problem hiding this comment.
IMO the churn is worth it because the weird report globals like ReportUtils.reportJson are now only visible to report code.
We can also add new dependencies to the report utils without worrying about the shared utils needing to remain standalone.
There was a problem hiding this comment.
IMO the churn is worth it because the weird report globals like
ReportUtils.reportJsonare now only visible to report code.
that and Util.i18n are only there due to failure of architecture and we're hiding our shame instead of putting them on window. They could go in or on any global variable.
We can also add new dependencies to the report utils without worrying about the shared utils needing to remain standalone.
the shared utils still have to be standalone because they're still imported by the report.
There was a problem hiding this comment.
I'm unclear on how this could be just a simple mv of report/renderer/util.js -> shared/util.js, because of the existence of Util.i18n (soon to be Util.formatter #13933). That results in a module in shared/ being dependent on report/renderer/i18n.js. IIRC that's why I split the file.
I can rebase things and convince git this is a move, that would be good. I'll do a new PR, and defer the markdown changes, to make move simpler.
There was a problem hiding this comment.
annnnnd to reduce naming problems (so many utils), this seems like a good approach. just be honest about what we doing lol
yarn.lock
Outdated
| @@ -1520,7 +1520,12 @@ | |||
| resolved "https://registry.yarnpkg.com/@types/mocha/-/mocha-9.1.1.tgz#e7c4f1001eefa4b8afbd1eee27a237fee3bf29c4" | |||
| integrity sha512-Z61JK7DKDtdKTWwLeElSEBcWGRLY8g95ic5FoQqI9CMx0ns/Ghep3B4DfcEimiKMvtamNVULVNKEsiwV3aQmXw== | |||
|
|
|||
| "@types/node@*", "@types/node@>=12.12.47", "@types/node@>=13.7.0": | |||
| "@types/node@*": | |||
| version "18.7.16" | |||
There was a problem hiding this comment.
it's not a huge deal, but using the 18.x types can sometimes mislead on what APIs are available. Was there a 16.x that got the URL change?
There was a problem hiding this comment.
looks like latest 16.x works, yeah
| export { | ||
| Util, | ||
| ReportUtils, | ||
| SharedUtils, | ||
| UIStrings, | ||
| }; | ||
|
|
||
| export * as MarkdownUtils from '../../shared/markdown.js'; |
There was a problem hiding this comment.
IMO the churn is worth it because the weird report globals like ReportUtils.reportJson are now only visible to report code.
We can also add new dependencies to the report utils without worrying about the shared utils needing to remain standalone.
| @@ -0,0 +1,264 @@ | |||
| /** | |||
| * @license Copyright 2017 The Lighthouse Authors. All Rights Reserved. | |||
There was a problem hiding this comment.
GH doesn't consider this a file move, should we update the date here?
of other util files
I tried moving some of the URL utility functions to
url-utils.js, but the dependency on core'slh-error.jsand
i18n.jsmade it a larger change than I anticipated so I dropped it. Ditto for trying to move that module toshared.