core(audit-mode): do not require a URL #5495
Conversation
lighthouse-cli/cli-flags.js
Outdated
| @@ -138,7 +138,9 @@ function getFlags(manualArgv) { | |||
| .check(/** @param {!LH.Flags} argv */ (argv) => { | |||
| // Make sure lighthouse has been passed a url, or at least one of --list-all-audits | |||
| // or --list-trace-categories. If not, stop the program and ask for a url | |||
| if (!requestedUrl) { | ||
| throw new Error('Cannot run audit mode on empty URL'); | ||
| } | ||
| if (opts.url && opts.url !== requestedUrl) { |
There was a problem hiding this comment.
might throw a wrench in the type checking works, but opts.url should now be optional in the function params?
lighthouse-core/runner.js
Outdated
| } else { | ||
| // save the requestedUrl provided by the user |
There was a problem hiding this comment.
we can ditch this comment now since we don't actually save it anymore (can also just drop it and use opts.url in its place)
lighthouse-core/runner.js
Outdated
| log.warn('Lighthouse', 'Performance stats might be skewed redirecting to HTTPS.'); | ||
| } | ||
|
|
||
| // canonicalize URL with any trailing slashes neccessary |
There was a problem hiding this comment.
this comment is just weird now. Add an if? Drop the necessary? Something else?
There was a problem hiding this comment.
how about .. move this comment to L77 and drop the use of parsedURL and s/parsedURL/requestedUrl/ ?
lighthouse-core/runner.js
Outdated
|
|
||
| let parsedURL; | ||
| try { | ||
| parsedURL = new URL(opts.url); |
lighthouse-core/runner.js
Outdated
| } | ||
|
|
||
| // If the URL isn't https and is also not localhost complain to the user. | ||
| if (parsedURL.protocol !== 'https:' && parsedURL.hostname !== 'localhost') { |
There was a problem hiding this comment.
I was gonna say two things...
- we could use
is-on-https'sisSecureRecordmethod here.. - lets merge the strings and just say
'The URL provided should be on HTTPS. Performance stats might be skewed redirecting to HTTPS.'
but tbh..
how about we nuke this check? Our redirects audit already catches perf cost of redirects. and if someone wants to audit a http page, they should be able to.
There was a problem hiding this comment.
also, in the future, i'd like to support example.com and just toss https:// in front of it. psi supports this. i think wpt, too.
There was a problem hiding this comment.
a bigger issue is service workers and how do you notice you aren't getting one because you typed in http, but agree there could be a better flow (we have top level warnings for that kind of thing now)
lighthouse-core/runner.js
Outdated
| log.warn('Lighthouse', 'Performance stats might be skewed redirecting to HTTPS.'); | ||
| } | ||
|
|
||
| // canonicalize URL with any trailing slashes neccessary |
There was a problem hiding this comment.
how about .. move this comment to L77 and drop the use of parsedURL and s/parsedURL/requestedUrl/ ?
| const opts = {url: 'https://example.com', config: generateConfig(settings), driverMock}; | ||
| try { | ||
| await Runner.run(null, opts); | ||
| assert.fail('should have thrown'); |
There was a problem hiding this comment.
won't this be caught by the try/catch and so never fail?
There was a problem hiding this comment.
yeah but the message won't match, it's the same weird pattern observed elsewhere in the tests :) jest has some much niftier matchers FWIW ;)
There was a problem hiding this comment.
oh right. That's confusing but ok :)
patrickhulce
changed the title
realized in #5493 that requiring a URL in audit mode is silly, this removes that requirement