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

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

Merged
merged 12 commits into from
Sep 28, 2021
Merged

Conversation

connorjclark
Copy link
Collaborator

ref #12689

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 connorjclark requested a review from a team as a code owner September 24, 2021 22:33
@connorjclark connorjclark requested review from patrickhulce and removed request for a team September 24, 2021 22:33
@google-cla google-cla bot added the cla: yes label Sep 24, 2021
getNodeSelector: getNodeSelector,
getNodeLabel: getNodeLabel,
getNodeSelector,
getNodeLabel,
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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.

@@ -0,0 +1,18 @@
/**
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

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

root.js Outdated
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'));
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

thoughts? this would simplify a lot of call sites.

Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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 :)

Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

how about readJson(path, baseDirOrImportMeta=LH_ROOT) ?

:D

@connorjclark connorjclark changed the title misc: convert lighthouse-core/script to ES modules misc: convert lighthouse-core/scripts to ES modules Sep 24, 2021
@@ -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`);
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

@patrickhulce patrickhulce left a comment

Choose a reason for hiding this comment

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

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

@@ -0,0 +1,18 @@
/**
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

root.js Outdated
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'));
Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

import expect from 'expect';
import tsc from 'typescript';
import MessageParser from 'intl-messageformat-parser';
import esMain from 'es-main';
Copy link
Member

Choose a reason for hiding this comment

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

need to add this to our dev deps

Copy link
Collaborator Author

@connorjclark connorjclark Sep 27, 2021

Choose a reason for hiding this comment

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

es-main is already there tho

Copy link
Member

Choose a reason for hiding this comment

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

es-main is already there tho

haha oh right

root.js Outdated
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'));
Copy link
Member

Choose a reason for hiding this comment

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

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 :)

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