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

cli: convert to ES modules #13045

Merged
merged 31 commits into from
Sep 14, 2021
Merged
Show file tree
Hide file tree
Changes from 23 commits
Commits
Show all changes
31 commits
Select commit Hold shift + click to select a range
0b0fbc4
first pass
connorjclark Aug 24, 2021
dc10e88
ugh maybe babel will help
connorjclark Aug 25, 2021
f4b6656
Merge remote-tracking branch 'origin/master' into esm-cli
connorjclark Sep 8, 2021
2af3d97
mocks in bin-test
connorjclark Sep 8, 2021
03e996f
smokehoouse
connorjclark Sep 8, 2021
705a9d5
linnt
connorjclark Sep 8, 2021
10558a2
removemock
connorjclark Sep 8, 2021
22daf0e
Merge remote-tracking branch 'origin/master' into esm-cli
connorjclark Sep 10, 2021
6eadebf
fix yargs types
connorjclark Sep 10, 2021
453571d
nobabel
connorjclark Sep 10, 2021
d97ad94
build static server for google3 roll
connorjclark Sep 10, 2021
2f11d0d
fix yarn build-smokehouse-bundle
connorjclark Sep 10, 2021
76c906a
dont need that
connorjclark Sep 10, 2021
d11f410
fix test
connorjclark Sep 10, 2021
3829720
Merge remote-tracking branch 'origin/master' into esm-cli
connorjclark Sep 13, 2021
4306743
fix merge
connorjclark Sep 13, 2021
3427307
rm temporary package jsons
connorjclark Sep 13, 2021
ff317a8
maybe fix
connorjclark Sep 13, 2021
3256ac4
fix
connorjclark Sep 13, 2021
5a9dd20
add test
connorjclark Sep 13, 2021
5e8aa8c
bad merge
connorjclark Sep 13, 2021
852ae22
revert file
connorjclark Sep 13, 2021
89726e6
fix windows
connorjclark Sep 13, 2021
593a629
oops
connorjclark Sep 14, 2021
98aa222
Merge remote-tracking branch 'origin/master' into esm-cli
connorjclark Sep 14, 2021
2f8b7da
update
connorjclark Sep 14, 2021
dcca468
fix index-test
connorjclark Sep 14, 2021
0800684
comment
connorjclark Sep 14, 2021
f6cdebe
typedef
connorjclark Sep 14, 2021
662a727
Merge remote-tracking branch 'origin/master' into esm-cli
connorjclark Sep 14, 2021
a9bb355
lint
connorjclark Sep 14, 2021
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
45 changes: 37 additions & 8 deletions build/build-smokehouse-bundle.js
Original file line number Diff line number Diff line change
Expand Up @@ -5,18 +5,47 @@
*/
'use strict';

const browserify = require('browserify');
const fs = require('fs');
const rollup = require('rollup');

/**
* Rollup plugins don't export types that work with commonjs.
* @template T
* @param {T} module
* @return {T['default']}
*/
function rollupPluginTypeCoerce(module) {
// @ts-expect-error
return module;
}

const nodeResolve = rollupPluginTypeCoerce(require('rollup-plugin-node-resolve'));
const commonjs = rollupPluginTypeCoerce(require('rollup-plugin-commonjs'));
// @ts-expect-error: no types
const shim = require('rollup-plugin-shim');
const {LH_ROOT} = require('../root.js');

const distDir = `${LH_ROOT}/dist`;
const bundleOutFile = `${distDir}/smokehouse-bundle.js`;
const smokehouseLibFilename = './lighthouse-cli/test/smokehouse/frontends/lib.js';
const smokehouseCliFilename =
require.resolve('../lighthouse-cli/test/smokehouse/lighthouse-runners/cli.js');

browserify(smokehouseLibFilename, {standalone: 'Lighthouse.Smokehouse'})
.ignore('./lighthouse-cli/test/smokehouse/lighthouse-runners/cli.js')
.transform('@wardpeet/brfs', {global: true, parserOpts: {ecmaVersion: 12}})
.bundle((err, src) => {
if (err) throw err;
fs.writeFileSync(bundleOutFile, src.toString());
async function build() {
const bundle = await rollup.rollup({
input: smokehouseLibFilename,
context: 'globalThis',
plugins: [
nodeResolve(),
commonjs(),
shim({
[smokehouseCliFilename]: 'export default {}',
}),
],
});

await bundle.write({
file: bundleOutFile,
});
}

build();
27 changes: 27 additions & 0 deletions lighthouse-cli/.eslintrc.cjs
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
/**
* @license Copyright 2021 The Lighthouse Authors. All Rights Reserved.
* Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except in compliance with the License. You may obtain a copy of the License at http://www.apache.org/licenses/LICENSE-2.0
* Unless required by applicable law or agreed to in writing, software distributed under the License is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the License for the specific language governing permissions and limitations under the License.
*/
'use strict';

module.exports = {
env: {
browser: true,
},
rules: {
// TODO(esmodules): move to root eslint when all code is ESM
// or when this is resolved: https://github.com/import-js/eslint-plugin-import/issues/2214
'import/order': [2, {
'groups': [
'builtin',
'external',
['sibling', 'parent'],
'index',
'object',
'type',
],
'newlines-between': 'always',
}],
},
};
54 changes: 32 additions & 22 deletions lighthouse-cli/bin.js
Original file line number Diff line number Diff line change
Expand Up @@ -18,19 +18,23 @@
* cli-flags lh-core/index
*/

const fs = require('fs');
const path = require('path');
const commands = require('./commands/commands.js');
const printer = require('./printer.js');
const {getFlags} = require('./cli-flags.js');
const {runLighthouse} = require('./run.js');
const {generateConfig} = require('../lighthouse-core/index.js');
const log = require('lighthouse-logger');
const pkg = require('../package.json');
const Sentry = require('../lighthouse-core/lib/sentry.js');
const updateNotifier = require('update-notifier');
const {askPermission} = require('./sentry-prompt.js');
const {LH_ROOT} = require('../root.js');
import * as fs from 'fs';
import * as path from 'path';
import * as url from 'url';

import log from 'lighthouse-logger';
import updateNotifier from 'update-notifier';

import * as commands from './commands/commands.js';
import * as Printer from './printer.js';
import {getFlags} from './cli-flags.js';
import {runLighthouse} from './run.js';
import lighthouse from '../lighthouse-core/index.js';
import * as Sentry from '../lighthouse-core/lib/sentry.js';
import {askPermission} from './sentry-prompt.js';
import {LH_ROOT} from '../root.js';

const pkg = JSON.parse(fs.readFileSync(LH_ROOT + '/package.json', 'utf-8'));

/**
* @return {boolean}
Expand Down Expand Up @@ -58,16 +62,22 @@ async function begin() {
commands.listTraceCategories();
}

const url = cliFlags._[0];
const urlUnderTest = cliFlags._[0];

/** @type {LH.Config.Json|undefined} */
let configJson;
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);
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

of note.


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;
Comment on lines +73 to +80
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

wouldn't that fail for es modules?

Copy link
Member

Choose a reason for hiding this comment

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

oh yeah, good point :(

}

if (cliFlags.budgetPath) {
Expand All @@ -88,7 +98,7 @@ async function begin() {

if (
cliFlags.output.length === 1 &&
cliFlags.output[0] === printer.OutputMode.json &&
cliFlags.output[0] === Printer.OutputMode.json &&
!cliFlags.outputPath
) {
cliFlags.outputPath = 'stdout';
Expand All @@ -106,7 +116,7 @@ async function begin() {
}

if (cliFlags.printConfig) {
const config = generateConfig(configJson, cliFlags);
const config = lighthouse.generateConfig(configJson, cliFlags);
process.stdout.write(config.getPrintString());
return;
}
Expand All @@ -119,7 +129,7 @@ async function begin() {
}
if (cliFlags.enableErrorReporting) {
Sentry.init({
url,
url: urlUnderTest,
flags: cliFlags,
environmentData: {
name: 'redacted', // prevent sentry from using hostname
Expand All @@ -132,9 +142,9 @@ async function begin() {
});
}

return runLighthouse(url, cliFlags, configJson);
return runLighthouse(urlUnderTest, cliFlags, configJson);
}

module.exports = {
begin,
export {
begin
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

agreed, will put up a separate PR

};
21 changes: 13 additions & 8 deletions lighthouse-cli/cli-flags.js
Original file line number Diff line number Diff line change
Expand Up @@ -7,18 +7,23 @@

/* eslint-disable max-len */

const yargs = require('yargs');
const fs = require('fs');
const {isObjectOfUnknownValues} = require('../lighthouse-core/lib/type-verifiers.js');
import * as fs from 'fs';
connorjclark marked this conversation as resolved.
Show resolved Hide resolved

import yargs from 'yargs';
import * as yargsHelpers from 'yargs/helpers';

import {isObjectOfUnknownValues} from '../lighthouse-core/lib/type-verifiers.js';

/**
* @param {string=} manualArgv
* @param {{noExitOnFailure?: boolean}=} options
* @return {LH.CliFlags}
*/
function getFlags(manualArgv, options = {}) {
// @ts-expect-error - undocumented, but yargs() supports parsing a single `string`.
const y = manualArgv ? yargs(manualArgv) : yargs;
const y = manualArgv ?
// @ts-expect-error - undocumented, but yargs() supports parsing a single `string`.
yargs(manualArgv) :
yargs(yargsHelpers.hideBin(process.argv));
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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)

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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

tweaking the import to be * creates type errors.


let parser = y.help('help')
.showHelpOnFail(false, 'Specify --help for available options')
Expand Down Expand Up @@ -318,7 +323,7 @@ function getFlags(manualArgv, options = {}) {
throw new Error('Please provide a url');
})
.epilogue('For more information on Lighthouse, see https://developers.google.com/web/tools/lighthouse/.')
.wrap(yargs.terminalWidth());
.wrap(y.terminalWidth());

if (options.noExitOnFailure) {
// Silence console.error() logging and don't process.exit().
Expand Down Expand Up @@ -499,6 +504,6 @@ function coerceScreenEmulation(value) {
return screenEmulationSettings;
}

module.exports = {
getFlags,
export {
getFlags
};
9 changes: 2 additions & 7 deletions lighthouse-cli/commands/commands.js
Original file line number Diff line number Diff line change
Expand Up @@ -5,10 +5,5 @@
*/
'use strict';

const listAudits = require('./list-audits.js');
const listTraceCategories = require('./list-trace-categories.js');

module.exports = {
listAudits,
listTraceCategories,
};
export {listAudits} from './list-audits.js';
Copy link
Member

Choose a reason for hiding this comment

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

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

export {listTraceCategories} from './list-trace-categories.js';
4 changes: 2 additions & 2 deletions lighthouse-cli/commands/list-audits.js
Original file line number Diff line number Diff line change
Expand Up @@ -5,12 +5,12 @@
*/
'use strict';

const lighthouse = require('../../lighthouse-core/index.js');
import lighthouse from '../../lighthouse-core/index.js';

function listAudits() {
const audits = lighthouse.getAuditList().map((i) => i.replace(/\.js$/, ''));
process.stdout.write(JSON.stringify({audits}, null, 2));
process.exit(0);
}

module.exports = listAudits;
export {listAudits};
7 changes: 3 additions & 4 deletions lighthouse-cli/commands/list-trace-categories.js
Original file line number Diff line number Diff line change
Expand Up @@ -5,12 +5,11 @@
*/
'use strict';

const lighthouse = require('../../lighthouse-core/index.js');
import lighthouse from '../../lighthouse-core/index.js';

function listTraceCategories() {
const traceCategories = lighthouse.traceCategories;
process.stdout.write(JSON.stringify({traceCategories}));
process.stdout.write(JSON.stringify({traceCategories: lighthouse.traceCategories}));
connorjclark marked this conversation as resolved.
Show resolved Hide resolved
process.exit(0);
}

module.exports = listTraceCategories;
export {listTraceCategories};
4 changes: 3 additions & 1 deletion lighthouse-cli/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,9 @@
*/
'use strict';

require('./bin.js').begin().catch(err => {
import {begin} from './bin.js';

begin().catch(err => {
process.stderr.write(err.stack);
process.exit(1);
});
File renamed without changes.
9 changes: 5 additions & 4 deletions lighthouse-cli/printer.js
Original file line number Diff line number Diff line change
Expand Up @@ -5,8 +5,9 @@
*/
'use strict';

const fs = require('fs');
const log = require('lighthouse-logger');
import * as fs from 'fs';

import log from 'lighthouse-logger';

/**
* An enumeration of acceptable output modes:
Expand Down Expand Up @@ -91,9 +92,9 @@ function getValidOutputOptions() {
return Object.keys(OutputMode);
}

module.exports = {
export {
checkOutputPath,
write,
OutputMode,
getValidOutputOptions,
getValidOutputOptions
};
32 changes: 16 additions & 16 deletions lighthouse-cli/run.js
Original file line number Diff line number Diff line change
Expand Up @@ -7,22 +7,22 @@

/* eslint-disable no-console */

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

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

@connorjclark connorjclark Sep 14, 2021

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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.


const Printer = require('./printer.js');
const ChromeLauncher = require('chrome-launcher');
import * as path from 'path';

const yargsParser = require('yargs-parser');
const lighthouse = require('../lighthouse-core/index.js');
const log = require('lighthouse-logger');
const getFilenamePrefix = require('../report/generator/file-namer.js').getFilenamePrefix;
const assetSaver = require('../lighthouse-core/lib/asset-saver.js');
const URL = require('../lighthouse-core/lib/url-shim.js');
import psList from 'ps-list';
import * as ChromeLauncher from 'chrome-launcher';
import yargsParser from 'yargs-parser';
import log from 'lighthouse-logger';
import open from 'open';

const open = require('open');
import * as Printer from './printer.js';
import lighthouse from '../lighthouse-core/index.js';
import {getFilenamePrefix} from '../report/generator/file-namer.js';
import * as assetSaver from '../lighthouse-core/lib/asset-saver.js';
import URL from '../lighthouse-core/lib/url-shim.js';

/** @typedef {Error & {code: string, friendlyMessage?: string}} ExitError */

const _RUNTIME_ERROR_CODE = 1;
const _PROTOCOL_TIMEOUT_EXIT_CODE = 67;
Expand Down Expand Up @@ -203,8 +203,8 @@ async function potentiallyKillChrome(launchedChrome) {
* @return {Promise<LH.RunnerResult|undefined>}
*/
async function runLighthouseWithFraggleRock(url, flags, config, launchedChrome) {
const fraggleRock = require('../lighthouse-core/fraggle-rock/api.js');
const puppeteer = require('puppeteer');
const fraggleRock = (await import('../lighthouse-core/fraggle-rock/api.js')).default;
const puppeteer = (await import('puppeteer')).default;
const browser = await puppeteer.connect({browserURL: `http://localhost:${launchedChrome.port}`});
const page = await browser.newPage();
flags.channel = 'fraggle-rock-cli';
Expand Down Expand Up @@ -272,8 +272,8 @@ async function runLighthouse(url, flags, config) {
}
}

module.exports = {
export {
parseChromeFlags,
saveResults,
runLighthouse,
runLighthouse
};
Loading