Add Lighthouse audit command #7990
Conversation
| @@ -114,6 +114,7 @@ module.exports = function( | |||
| build: 'react-scripts build', | |||
| test: 'react-scripts test', | |||
| eject: 'react-scripts eject', | |||
| lighthouse: 'react-scripts audit', | |||
There was a problem hiding this comment.
What do you think of audit: 'react-scripts audit'?
There was a problem hiding this comment.
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?
| .launch({ chromeFlags: opts.chromeFlags }) | ||
| .then(chrome => { | ||
| opts.port = chrome.port; | ||
| return lighthouse(url, opts).then(results => { |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Agreed, this would make sense. Or better yet - or perhaps also - we could load a config file?
| const launchChromeAndRunLighthouse = (url, opts) => { | ||
| return chromeLauncher | ||
| .launch({ chromeFlags: opts.chromeFlags }) | ||
| .then(chrome => { |
There was a problem hiding this comment.
We should be able to use async/await if that helps clean it up.
There was a problem hiding this comment.
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.
| .then(() => choosePort(HOST, DEFAULT_PORT)) | ||
| .then(port => { | ||
| if (port == null) { | ||
| console.log('Unable to find a free port'); |
There was a problem hiding this comment.
Should we do something like this in error cases for consistency elsewhere?
console.error(
chalk.bold.red(...)
)There was a problem hiding this comment.
Agreed, this should be polished - it also needs some line spacing.
| const { choosePort } = require('react-dev-utils/WebpackDevServerUtils'); | ||
| const open = require('open'); | ||
| const handler = require('serve-handler'); | ||
| const lighthouse = require('lighthouse'); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.)
There was a problem hiding this comment.
Yes, and I think we should also handle this in the script... I'm sure we can.
| .launch({ chromeFlags: opts.chromeFlags }) | ||
| .then(chrome => { | ||
| opts.port = chrome.port; | ||
| return lighthouse(url, opts).then(results => { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Agreed. I just left this as per the docs for now - and forgot that we don't support Node 6.
|
Good point @ianschmitz, what if it just checked for a Or it could build in the case that a build doesn't exist... |
Yeah something like that would probably be nice |
This PR adds a
lighthousescript, name TBD.This script should:
Known issues: