misc: convert lighthouse-core/scripts to ES modules #13121

connorjclark · 2021-09-24T22:33:11Z

notes from manual testing:

-----
top-level scripts:

node lighthouse-core/scripts/benchmark-plus-extras.js
node lighthouse-core/scripts/benchmark.js
node lighthouse-core/scripts/build-test-flow-report.js
yarn diff:sample-json
node lighthouse-core/scripts/cleanup-vuln-snapshot.js third-party/snyk/snapshot.json
node lighthouse-core/scripts/compare-runs.js --name my-collection --collect -n 1 --lh-flags='--only-audits=unminified-javascript' --urls https://www.example.com
node lighthouse-core/scripts/compare-runs.js --name my-collection --summarize --filter 'loadPage|connect'
node lighthouse-core/scripts/compare-runs.js --name my-collection --name my-collection --compare
node lighthouse-core/scripts/generate-timing-trace.js latest-run/lhr.report.json
cat latest-run/lhr.report.json | node lighthouse-core/scripts/json-size.js
node lighthouse-core/scripts/manual-chrome-launcher.js
yarn run-devtools https://www.example.com
node lighthouse-core/scripts/print-a11y-scoring.js
node lighthouse-core/scripts/print-contributors.js v8.4.0 HEAD
node lighthouse-core/scripts/update-flow-fixtures.js
yarn update:sample-artifacts
------
in a folder:

yarn i18n:collect-strings
yarn test-lantern
yarn test-legacy-javascript
node lighthouse-core/scripts/release/bump-versions.js 123.1.2
------
skipped testing: `gcp-collection`, `internal-analysis`, `lantern/collect` (somewhat high setup cost, any issues should be easily fixable for next person needing to run these scripts)

connorjclark · 2021-09-24T22:34:19Z

lighthouse-core/lib/page-functions.js

-  getNodeSelector: getNodeSelector,
-  getNodeLabel: getNodeLabel,
+  getNodeSelector,
+  getNodeLabel,


drive by changes... I was hoping these changes would be enough to pass Node's commonjs named exports heuristics, but it was not. Anyhow, these properties don't need to use renaming syntax.

connorjclark · 2021-09-24T22:35:52Z

lighthouse-core/scripts/esm-utils.js

@@ -0,0 +1,18 @@
+/**


This file may later find its way into lighthouse-core, depending on if it also needs require.resolve (I think it will?)

Definitely will though I imagine might need more context/bundle awareness

connorjclark · 2021-09-24T22:36:22Z

root.js

+  readJson(path) {
+    // TODO: could be nice to accept paths relative to LH_ROOT ? this would still
+    // allow absolute paths.
+    // return JSON.parse(fs.readFileSync(path.resolve(__dirname, path), 'utf-8'));


thoughts? this would simplify a lot of call sites.

wfm 👍

the bigger issue that's become clear to me is the pain introduced by LH_ROOT. makes me think a dirname method that accepts import.meta.url might have preserved the original intention better in most cases, but I'll leave that up to you.

FWIW, I've found that when I have the choice of a dirname replacement and an equivalent to LH_ROOT, I end up usually reaching for the LH_ROOT, especially when the subdirectories are well separated and I have to go way up and back down again anyways :)

OTOH, I've also found as I've gotten used to them that dealing with import.meta.url and fileURLToPath to be increasingly not an impediment to writing or reading code. I wonder how much this appeals in a mass refactor as a way to centralize an annoying step, while it's no big deal authoring individual files. It sucks to lose json require (until import assertions finally land), but in the past when I couldn't require a json file (needing to mutate it or whatever), it really has never been a big deal to throw in a JSON.parse(fs.readFileSync) line.

Not objecting to adding any helpful utilities, just saying we should maintain an open mind as the codebase continues to adapt to full ESM :)

especially when the subdirectories are well separated and I have to go way up and back down again anyways

Yes, definitely the situation where LH_ROOT is preferred. Many of the usages here though are for a local file in the same directory or an immediate neighbor, in which case the respecifying of the entire path to the subdir is suboptimal IMO.

how about readJson(path, baseDirOrImportMeta=LH_ROOT) ?

:D

connorjclark · 2021-09-24T22:38:31Z

lighthouse-core/scripts/generate-timing-trace.js

@@ -45,7 +48,8 @@ function saveTraceFromCLI() {
  const lhrObject = JSON.parse(fs.readFileSync(filename, 'utf8'));
  const jsonStr = createTraceString(lhrObject);

-  const traceFilePath = `${filename}.timing.trace.json`;
+  const pathObj = path.parse(filename);
+  const traceFilePath = path.join(path.dirname(filename), `${pathObj.name}.timing.trace.json`);


drive by changes (after noticing it didn't handle lhr.report.json as expected)

patrickhulce

LGTM

any issues should be easily fixable for next person needing to run these scripts

Depending on who the poor soul that runs these next is, I'm not 100% sure this is true 😉 but OK with that gamble given the usage frequency

patrickhulce · 2021-09-27T14:31:39Z

lighthouse-core/scripts/esm-utils.js

@@ -0,0 +1,18 @@
+/**


Definitely will though I imagine might need more context/bundle awareness

patrickhulce · 2021-09-27T14:47:09Z

root.js

+  readJson(path) {
+    // TODO: could be nice to accept paths relative to LH_ROOT ? this would still
+    // allow absolute paths.
+    // return JSON.parse(fs.readFileSync(path.resolve(__dirname, path), 'utf-8'));


wfm 👍

the bigger issue that's become clear to me is the pain introduced by LH_ROOT. makes me think a dirname method that accepts import.meta.url might have preserved the original intention better in most cases, but I'll leave that up to you.

brendankenny · 2021-09-27T15:16:30Z

lighthouse-core/scripts/i18n/collect-strings.js

+import expect from 'expect';
+import tsc from 'typescript';
+import MessageParser from 'intl-messageformat-parser';
+import esMain from 'es-main';


need to add this to our dev deps

es-main is already there tho

es-main is already there tho

haha oh right

brendankenny · 2021-09-27T16:03:26Z

root.js

+  readJson(path) {
+    // TODO: could be nice to accept paths relative to LH_ROOT ? this would still
+    // allow absolute paths.
+    // return JSON.parse(fs.readFileSync(path.resolve(__dirname, path), 'utf-8'));


FWIW, I've found that when I have the choice of a dirname replacement and an equivalent to LH_ROOT, I end up usually reaching for the LH_ROOT, especially when the subdirectories are well separated and I have to go way up and back down again anyways :)

OTOH, I've also found as I've gotten used to them that dealing with import.meta.url and fileURLToPath to be increasingly not an impediment to writing or reading code. I wonder how much this appeals in a mass refactor as a way to centralize an annoying step, while it's no big deal authoring individual files. It sucks to lose json require (until import assertions finally land), but in the past when I couldn't require a json file (needing to mutate it or whatever), it really has never been a big deal to throw in a JSON.parse(fs.readFileSync) line.

Not objecting to adding any helpful utilities, just saying we should maintain an open mind as the codebase continues to adapt to full ESM :)

connorjclark added 5 commits September 24, 2021 13:49

wip

ba34039

test/scripts

e722927

types

8bde7be

more readJson

e2719ff

manual testing

d18d7b6

connorjclark requested a review from a team as a code owner September 24, 2021 22:33

connorjclark requested review from patrickhulce and removed request for a team September 24, 2021 22:33

google-cla bot added the cla: yes label Sep 24, 2021

devtools-bot assigned patrickhulce Sep 24, 2021

devtools-bot added the waiting4reviewer label Sep 24, 2021

connorjclark commented Sep 24, 2021

View reviewed changes

oops

c9327b8

vercel bot deployed to Preview September 24, 2021 22:37 View deployment

connorjclark changed the title ~~misc: convert lighthouse-core/script to ES modules~~ misc: convert lighthouse-core/scripts to ES modules Sep 24, 2021

connorjclark commented Sep 24, 2021

View reviewed changes

lint

aea3fb3

vercel bot deployed to Preview September 24, 2021 23:18 View deployment

.dir

a04f2c9

vercel bot deployed to Preview September 24, 2021 23:35 View deployment

patrickhulce reviewed Sep 27, 2021

View reviewed changes

patrickhulce approved these changes Sep 27, 2021

View reviewed changes

patrickhulce added waiting4committer and removed waiting4reviewer labels Sep 27, 2021

brendankenny reviewed Sep 27, 2021

View reviewed changes

connorjclark added 3 commits September 27, 2021 13:37

Merge remote-tracking branch 'origin/master' into esm-scripts

3bb245e

Merge remote-tracking branch 'origin/master' into esm-scripts

3bb604b

readJson second param

361c16c

vercel bot deployed to Preview September 28, 2021 00:29 View deployment

remove unused imports

80ee57e

vercel bot deployed to Preview September 28, 2021 00:31 View deployment

connorjclark merged commit 968dd44 into master Sep 28, 2021

connorjclark deleted the esm-scripts branch September 28, 2021 19:22

adamraine mentioned this pull request Sep 28, 2021

report(flow): report result cache #13132

Closed

connorjclark mentioned this pull request Feb 23, 2022

☂️ Convert to ES modules #12689

Closed

17 tasks

connorjclark mentioned this pull request Aug 28, 2023

misc: move esm-utils.js to shared/ #15418

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

misc: convert lighthouse-core/scripts to ES modules #13121

misc: convert lighthouse-core/scripts to ES modules #13121

connorjclark commented Sep 24, 2021

connorjclark Sep 24, 2021

connorjclark Sep 24, 2021

patrickhulce Sep 27, 2021

connorjclark Sep 24, 2021

patrickhulce Sep 27, 2021

brendankenny Sep 27, 2021

patrickhulce Sep 27, 2021

connorjclark Sep 27, 2021

connorjclark Sep 24, 2021

patrickhulce left a comment

patrickhulce Sep 27, 2021

patrickhulce Sep 27, 2021

brendankenny Sep 27, 2021

connorjclark Sep 27, 2021 •

edited

brendankenny Sep 27, 2021

brendankenny Sep 27, 2021

misc: convert lighthouse-core/scripts to ES modules #13121

misc: convert lighthouse-core/scripts to ES modules #13121

Conversation

connorjclark commented Sep 24, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

patrickhulce left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

connorjclark Sep 27, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

connorjclark Sep 27, 2021 •

edited