Skip to content

Add Lighthouse audit command #7990

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

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
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
9 changes: 7 additions & 2 deletions packages/react-scripts/bin/react-scripts.js
Original file line number Diff line number Diff line change
Expand Up @@ -19,12 +19,17 @@ const spawn = require('react-dev-utils/crossSpawn');
const args = process.argv.slice(2);

const scriptIndex = args.findIndex(
x => x === 'build' || x === 'eject' || x === 'start' || x === 'test'
x =>
x === 'build' ||
x === 'eject' ||
x === 'start' ||
x === 'test' ||
x === 'audit'
);
const script = scriptIndex === -1 ? args[0] : args[scriptIndex];
const nodeArgs = scriptIndex > 0 ? args.slice(0, scriptIndex) : [];

if (['build', 'eject', 'start', 'test'].includes(script)) {
if (['build', 'eject', 'start', 'test', 'audit'].includes(script)) {
const result = spawn.sync(
'node',
nodeArgs
Expand Down
2 changes: 2 additions & 0 deletions packages/react-scripts/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,7 @@
"jest-environment-jsdom-fourteen": "0.1.0",
"jest-resolve": "24.9.0",
"jest-watch-typeahead": "0.4.2",
"lighthouse": "^5.6.0",
"mini-css-extract-plugin": "0.8.0",
"optimize-css-assets-webpack-plugin": "5.0.3",
"pnp-webpack-plugin": "1.5.0",
Expand All @@ -71,6 +72,7 @@
"resolve": "1.12.0",
"resolve-url-loader": "3.1.1",
"sass-loader": "8.0.0",
"serve-handler": "^6.1.2",
"semver": "6.3.0",
"style-loader": "1.0.0",
"terser-webpack-plugin": "2.2.1",
Expand Down
75 changes: 75 additions & 0 deletions packages/react-scripts/scripts/audit.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,75 @@
// @remove-file-on-eject
/**
* Copyright (c) 2015-present, Facebook, Inc.
*
* This source code is licensed under the MIT license found in the
* LICENSE file in the root directory of this source tree.
*/
'use strict';

const { createServer } = require('http');
const { writeFileSync } = require('fs');
const { join } = require('path');
const { choosePort } = require('react-dev-utils/WebpackDevServerUtils');
const open = require('open');
const handler = require('serve-handler');
const lighthouse = require('lighthouse');
Copy link
Contributor

@ianschmitz ianschmitz Nov 17, 2019

Choose a reason for hiding this comment

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

Running chrome in headless would be amazing! I could see some cool CI use cases to fail when below a threshold. Potentially using puppeteer would make that trivial for us, removes the need to have chrome installed to use this functionality, and also provides the option to run headed if needed for debugging etc.

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, so there's an option for headless. I left it "headfull" as the default intentionally - I feel that for some people, it's nice to see how it works - I guess we can rely on a flag or config for opting out of that?

What do you think @ianschmitz?

Copy link
Contributor

Choose a reason for hiding this comment

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

On second thought, including puppeteer as a direct dependency would greatly increase the node_modules size which isn't great:

When you install Puppeteer, it downloads a recent version of Chromium (~170MB Mac, ~282MB Linux, ~280MB Win).

Documentation could likely handle this:

If you want to run this in CI, make sure Chrome/Chromium is installed... (or use puppeteer etc.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, and I think we should also handle this in the script... I'm sure we can.

const chromeLauncher = require('chrome-launcher');
const paths = require('../config/paths');

const DEFAULT_PORT = parseInt(process.env.PORT, 10) || 3000;
const HOST = process.env.HOST || '0.0.0.0';

// https://github.com/GoogleChrome/lighthouse/blob/master/docs/readme.md#using-programmatically
const launchChromeAndRunLighthouse = (url, opts) => {
return chromeLauncher
.launch({ chromeFlags: opts.chromeFlags })
.then(chrome => {
Copy link
Contributor

Choose a reason for hiding this comment

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

We should be able to use async/await if that helps clean it up.

Copy link
Contributor Author

@mrmckeb mrmckeb Nov 17, 2019

Choose a reason for hiding this comment

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

We don't support Node 6 anymore, right @ianschmitz? If so, I 1000% agree.

We have Node ">8.10" as our requirement for react-scripts, so I'll make this change.

opts.port = chrome.port;
return lighthouse(url, opts).then(results => {
Copy link
Contributor

@ianschmitz ianschmitz Nov 17, 2019

Choose a reason for hiding this comment

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

It would be nice to forward arguments to lighthouse. They have some cool customizations through their CLI.

For example --emulated-form-factor, the various --throttling flags or --chrome-flags

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed, this would make sense. Or better yet - or perhaps also - we could load a config file?

Copy link
Contributor

@ianschmitz ianschmitz Nov 17, 2019

Choose a reason for hiding this comment

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

async/await may make it a bit simpler to cover failure cases such as any of this process throwing in which case we should probably clean up the browser and server instances.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed. I just left this as per the docs for now - and forgot that we don't support Node 6.

return chrome.kill().then(() => results.report);
});
});
};

const server = createServer((request, response) =>
handler(request, response, {
renderSingle: true,
public: paths.appBuild,
})
);

choosePort(HOST, DEFAULT_PORT)
.then(() => choosePort(HOST, DEFAULT_PORT))
.then(port => {
if (port == null) {
console.log('Unable to find a free port');
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we do something like this in error cases for consistency elsewhere?

console.error(
  chalk.bold.red(...)
)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed, this should be polished - it also needs some line spacing.

process.exit(1);
}

server.listen(port);

console.log('Server started, beginning audit...');

return launchChromeAndRunLighthouse(`http://${HOST}:${port}`, {
output: 'html',
});
})
.then(report => {
console.log('Audit finished, writing report...');

const reportPath = join(paths.appPath, 'lighthouse-audit.html');
writeFileSync(reportPath, report);

console.log('Opening report in browser...');

open(reportPath, { url: true });

console.log('Exiting...');

server.close();
})
.catch(() => {
console.log('Something went wrong, exiting...');
server.close();
});
1 change: 1 addition & 0 deletions packages/react-scripts/scripts/init.js
Original file line number Diff line number Diff line change
Expand Up @@ -114,6 +114,7 @@ module.exports = function(
build: 'react-scripts build',
test: 'react-scripts test',
eject: 'react-scripts eject',
lighthouse: 'react-scripts audit',
Copy link
Contributor

Choose a reason for hiding this comment

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

What do you think of audit: 'react-scripts audit'?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is what I had... but it turns out it conflicts with yarn audit @ianschmitz. I didn't think of that initially, and didn't rename the other files yet, as I think the name would need discussion.

Maybe audit-app?

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah doh! Of course

};

// Setup the eslint config
Expand Down