cli: convert to ES modules #13045

connorjclark · 2021-09-10T22:22:39Z

Converts lighthouse-cli/ to esm.
Upgrades to newest jest, to use jest.unstable_mockModule (only cli test that uses mocks is bin-test.js).
Updates build-lr to also bundle up static-server.js, necessary since it now imports non-core modules (And bundling it all up is simplest way to run it in google3).

(split off into: #13046)

connorjclark · 2021-09-10T22:23:10Z

lighthouse-cli/bin.js

@@ -65,9 +68,18 @@ async function begin() {
  if (cliFlags.configPath) {
    // Resolve the config file path relative to where cli was called.
    cliFlags.configPath = path.resolve(process.cwd(), cliFlags.configPath);
-    configJson = require(cliFlags.configPath);


connorjclark · 2021-09-10T22:23:20Z

lighthouse-cli/test/smokehouse/frontends/smokehouse-bin.js

+  if (rawTestDefns) {
+    // Support commonjs.
+    rawTestDefns = rawTestDefns.default || rawTestDefns;
+  }


connorjclark · 2021-09-10T22:27:14Z

Error [ERR_UNSUPPORTED_ESM_URL_SCHEME]: Only file and data URLs are supported by the default ESM loader. On Windows, absolute paths must be valid file:// URLs. Received protocol 'd:'

wut. so... import('d:/some/file.js') should be import('file://d:/some/file.js') on windows????

brendankenny · 2021-09-10T22:45:04Z

There's some pretty major stuff going on in here, can this be split up? Even if a lot of it is pretty straightforward, it would be nice to consider cli-flags changes separate from bundling static-sever in the lr bundle

adamraine · 2021-09-13T19:24:46Z

lighthouse-cli/test/cli/cli-flags-test.js

-const getFlags = require('../../cli-flags.js').getFlags;
+
+import {strict as assert} from 'assert';
+import fs from 'fs';


Do we need to be consistent with import fs from 'fs' or import * as fs from 'fs?

fs is commonjs and so has no default export, so the entire module (aka * as fs) is treated as the default.

I have no idea which to go with, my first thought is leaning towards using the default import style.

import fs from 'fs' is my preferred version by far

lighthouse-cli/test/smokehouse/test-definitions/issues/expectations.js

brendankenny · 2021-09-14T20:56:32Z

lighthouse-cli/test/cli/run-test.js

-// Map plugin name to fixture since not actually installed in node_modules/.
-jest.mock('lighthouse-plugin-simple', () => {


how does this work without this?

somehow require.resolve('lighthouse-plugin-simple') resolves to /Users/cjamcl/src/lighthouse/lighthouse-core/test/fixtures/config-plugins/lighthouse-plugin-simple/plugin-simple.js here. Not sure how the module is registered... the plugin-simple.js bit at least is explained by that folder's package.json main.

so weird, works here too. This completely goes against what I thought I knew about how resolve works :)

just checked from master too, same there. this mock isn't needed for some reason unrelated to this PR. weeeeird

this doesn't help but here:

{ moduleIdentifier: 'lighthouse-plugin-simple', configDir: undefined, category: 'plugin', 'require.resolve(moduleIdentifier)': '/Users/cjamcl/src/lighthouse/lighthouse-core/test/fixtures/config-plugins/lighthouse-plugin-simple/plugin-simple.js', 'require.resolve.paths(moduleIdentifier)': [ '/Users/cjamcl/src/lighthouse/lighthouse-core/config/node_modules', '/Users/cjamcl/src/lighthouse/lighthouse-core/node_modules', '/Users/cjamcl/src/lighthouse/node_modules', '/Users/cjamcl/src/node_modules', '/Users/cjamcl/node_modules', '/Users/node_modules', '/node_modules' ] }

OK so it 100% fails when using the CLI directly (node lighthouse-cli https://www.example.com --plugins=lighthouse-plugin-simple), so this must be a jest side effect. (--experimental-vm-modules made no difference)

didn't find much documentation on it, but looks like yeah, jest walks through your project and automatically finds all the package.json files. jest-resolve has a ModuleMap, which internally had a _raw.map, which looked like:

'gulp-lighthouse-recipe' => 'docs/recipes/gulp/package.json' 'custom-lighthouse-recipe' => 'docs/recipes/custom-audit/package.json' 'custom-lighthouse-pptr-recipe' => 'docs/recipes/custom-gatherer-puppeteer/package.json' 'lighthouse-plugin-example' => 'docs/recipes/lighthouse-plugin-example/package.json' 'lighthouse-logger' => 'lighthouse-logger/package.json' 'lighthouse' => 'package.json' 'lighthouse-plugin-config' => 'lighthouse-core/test/fixtures/config/lighthouse-plugin-config-helper/package.json' 'lighthouse-plugin-no-groups' => 'lighthouse-core/test/fixtures/config-plugins/lighthouse-plugin-no-groups/package.json' 'lighthouse-plugin-no-category' => 'lighthouse-core/test/fixtures/config-plugins/lighthouse-plugin-no-category/package.json' 'lighthouse-plugin-simple' => 'lighthouse-core/test/fixtures/config-plugins/lighthouse-plugin-simple/package.json' 'legacy-javascript' => 'lighthouse-core/scripts/legacy-javascript/package.json'

could be their haste modules, which apparently are "package.json files outside of node_modules folders anywhere in the file system". I didn't verify that's the path triggering this but close enough :)

Maybe worth leaving a comment that jest automatically resolves the '--plugins=lighthouse-plugin-simple' flag to the correct location?

brendankenny · 2021-09-14T21:13:35Z

lighthouse-cli/bin.js

+    if (cliFlags.configPath.endsWith('.json')) {
+      configJson = JSON.parse(fs.readFileSync(cliFlags.configPath, 'utf-8'));
+    } else {
+      const configModuleUrl = url.pathToFileURL(cliFlags.configPath).href;
+      configJson = (await import(configModuleUrl)).default;
+    }
  } else if (cliFlags.preset) {
-    configJson = require(`../lighthouse-core/config/${cliFlags.preset}-config.js`);
+    configJson = (await import(`../lighthouse-core/config/${cliFlags.preset}-config.js`)).default;


this seems like a place where a createRequire would be justified. Pretty awkward without it

wouldn't that fail for es modules?

oh yeah, good point :(

brendankenny · 2021-09-14T21:16:10Z

lighthouse-cli/bin.js

-module.exports = {
-  begin,
+export {
+  begin


not an emergency, but it feels like we probably want

'comma-dangle': [2, { imports: 'always-multiline', exports: 'always-multiline', }]

seems weird not to have one

agreed, will put up a separate PR

lighthouse-cli/cli-flags.js

brendankenny · 2021-09-14T21:17:59Z

lighthouse-cli/cli-flags.js

+  const y = manualArgv ?
+    // @ts-expect-error - undocumented, but yargs() supports parsing a single `string`.
+    yargs(manualArgv) :
+    yargs(yargsHelpers.hideBin(process.argv));


Why the change? Seems way worse than having yargs handle it :)

oh. at one point I may have tried updating yargs and found this to be necessary. I can probably revert.

hmm... not sure what changed, as there is no yargs upgrade, but the old code results in this error:

TypeError: y.help is not a function at getFlags (file:///Users/cjamcl/src/lighthouse/lighthouse-cli/cli-flags.js:28:18) at begin (file:///Users/cjamcl/src/lighthouse/lighthouse-cli/bin.js:53:20) at file:///Users/cjamcl/src/lighthouse/lighthouse-cli/index.js:11:1 at ModuleJob.run (internal/modules/esm/module_job.js:152:23) at async Loader.import (internal/modules/esm/loader.js:177:24) at async Object.loadESM (internal/process/esm_loader.js:68:5)

hmm... not sure what changed, as there is no yargs upgrade, but the old code results in this error:

must be some change to the esmodules export. Here's a similar case: yargs/yargs#1774

shurg. this is exactly what their demo code does https://github.com/yargs/yargs#esm

tweaking the import to be * creates type errors.

brendankenny · 2021-09-14T21:20:12Z

lighthouse-cli/commands/commands.js

-  listAudits,
-  listTraceCategories,
-};
+export {listAudits} from './list-audits.js';


complete nit, but not sure about this style. Maybe re-exporting is awkward no matter what, but I do like the explicitness of the other way

lighthouse-cli/commands/list-trace-categories.js

lighthouse-cli/test/cli/index-test.js

brendankenny · 2021-09-14T21:42:02Z

lighthouse-cli/cli-flags.js

+  const y = manualArgv ?
+    // @ts-expect-error - undocumented, but yargs() supports parsing a single `string`.
+    yargs(manualArgv) :
+    yargs(yargsHelpers.hideBin(process.argv));


hmm... not sure what changed, as there is no yargs upgrade, but the old code results in this error:

must be some change to the esmodules export. Here's a similar case: yargs/yargs#1774

brendankenny · 2021-09-14T22:28:57Z

lighthouse-cli/run.js

@@ -7,22 +7,22 @@

 /* eslint-disable no-console */

-const path = require('path');
-const psList = require('ps-list');
+/** @typedef {Error & {code: string, friendlyMessage?: string}} ExitError */


what did you think about my typedef after imports thing from #13046 (comment)?

I prefer local types at the top of the file, but not enough to discuss it much. I'm reverting the changes that were just moving things around.

ha, damn, I thought I had convinced you, but oh well :)

the short of it is that exactly what is being imported isn't too informing (and sometimes the import list is so long you glaze over all of it like it's a Java file); but the types are quite useful as to the data structures involved in the file. so for the same reason that fileoverview would be at top, I like local types to be too

For all the same reasons I think they should be closer to the code that uses them :) Definitions, just in the type domain instead of the value domain, so should be right next to the value definitions.

brendankenny

Just the last dangling comma ~~and typedef style~~ questions, but looks good! A very smooth transition

connorjclark added 13 commits August 23, 2021 20:21

first pass

0b0fbc4

ugh maybe babel will help

dc10e88

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

f4b6656

mocks in bin-test

2af3d97

smokehoouse

03e996f

linnt

705a9d5

removemock

10558a2

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

22daf0e

fix yargs types

6eadebf

nobabel

453571d

build static server for google3 roll

d97ad94

fix yarn build-smokehouse-bundle

2f11d0d

dont need that

76c906a

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

connorjclark requested review from adamraine and removed request for a team September 10, 2021 22:22

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

connorjclark commented Sep 10, 2021

View reviewed changes

devtools-bot assigned adamraine Sep 10, 2021

devtools-bot added the waiting4reviewer label Sep 10, 2021

fix test

d11f410

vercel bot deployed to Preview September 10, 2021 22:30 View deployment

connorjclark mentioned this pull request Sep 10, 2021

tests(smokehouse): convert to ES modules #13046

Merged

adamraine reviewed Sep 13, 2021

View reviewed changes

connorjclark added 3 commits September 13, 2021 13:55

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

3829720

fix merge

4306743

rm temporary package jsons

3427307

revert file

852ae22

vercel bot deployed to Preview September 13, 2021 23:08 View deployment

fix windows

89726e6

vercel bot deployed to Preview September 13, 2021 23:15 View deployment

adamraine reviewed Sep 14, 2021

View reviewed changes

lighthouse-cli/test/smokehouse/test-definitions/issues/expectations.js Outdated Show resolved Hide resolved

connorjclark added 2 commits September 14, 2021 13:30

oops

593a629

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

98aa222

vercel bot deployed to Preview September 14, 2021 20:38 View deployment

adamraine approved these changes Sep 14, 2021

View reviewed changes

brendankenny reviewed Sep 14, 2021

View reviewed changes

update

2f8b7da

vercel bot deployed to Preview September 14, 2021 21:33 View deployment

brendankenny reviewed Sep 14, 2021

View reviewed changes

fix index-test

dcca468

vercel bot deployed to Preview September 14, 2021 21:48 View deployment

comment

0800684

brendankenny reviewed Sep 14, 2021

View reviewed changes

typedef

f6cdebe

brendankenny reviewed Sep 14, 2021

View reviewed changes

vercel bot deployed to Preview September 14, 2021 22:33 View deployment

connorjclark added 2 commits September 14, 2021 15:55

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

662a727

lint

a9bb355

vercel bot deployed to Preview September 14, 2021 22:57 View deployment

connorjclark merged commit 0e89735 into master Sep 14, 2021

connorjclark deleted the esm-cli branch September 14, 2021 23:15

This was referenced Sep 22, 2021

docs: correct the Node.js version base #13099

Merged

cli: fix cjs import for Sentry #13101

Closed

connorjclark mentioned this pull request Jan 25, 2022

--cli-flags-path no longer works #13589

Closed

connorjclark mentioned this pull request Feb 23, 2022

☂️ Convert to ES modules #12689

Closed

17 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

cli: convert to ES modules #13045

cli: convert to ES modules #13045

connorjclark commented Sep 10, 2021 •

edited

Loading

connorjclark Sep 10, 2021

connorjclark Sep 10, 2021

connorjclark commented Sep 10, 2021

brendankenny commented Sep 10, 2021

adamraine Sep 13, 2021

connorjclark Sep 13, 2021

brendankenny Sep 14, 2021

brendankenny Sep 14, 2021

connorjclark Sep 14, 2021

brendankenny Sep 14, 2021

connorjclark Sep 14, 2021

connorjclark Sep 14, 2021

connorjclark Sep 14, 2021 •

edited

Loading

brendankenny Sep 14, 2021

brendankenny Sep 14, 2021

connorjclark Sep 14, 2021

brendankenny Sep 14, 2021

brendankenny Sep 14, 2021

connorjclark Sep 14, 2021

brendankenny Sep 14, 2021

connorjclark Sep 14, 2021

connorjclark Sep 14, 2021

brendankenny Sep 14, 2021

connorjclark Sep 14, 2021

brendankenny Sep 14, 2021

brendankenny Sep 14, 2021

brendankenny Sep 14, 2021

connorjclark Sep 14, 2021 •

edited

Loading

brendankenny Sep 14, 2021

connorjclark Sep 14, 2021

brendankenny Sep 14, 2021

brendankenny left a comment •

edited

Loading

		// Map plugin name to fixture since not actually installed in node_modules/.
		jest.mock('lighthouse-plugin-simple', () => {

cli: convert to ES modules #13045

cli: convert to ES modules #13045

Conversation

connorjclark commented Sep 10, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

connorjclark commented Sep 10, 2021

brendankenny commented Sep 10, 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

connorjclark Sep 14, 2021 • edited Loading

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

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

connorjclark Sep 14, 2021 • edited Loading

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

brendankenny left a comment • edited Loading

Choose a reason for hiding this comment

connorjclark commented Sep 10, 2021 •

edited

Loading

connorjclark Sep 14, 2021 •

edited

Loading

connorjclark Sep 14, 2021 •

edited

Loading

brendankenny left a comment •

edited

Loading