-
Notifications
You must be signed in to change notification settings - Fork 9.3k
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
Allow Perf-X server Rerun lighthouse on POST request. #1393
Allow Perf-X server Rerun lighthouse on POST request. #1393
Conversation
Looks like your rebase messed up, as the rename from a previous PR is reverted here. Can you take a look? |
Do you want to also add a button to the report to rerun? That way we can test out this flow easily. |
cde16c8
to
eef6acb
Compare
Yes, I will do that. But probably in another PR. Because I think it's not just a rerun button. We also need to pass additional cli-flags (bolckedUrlPatterns and probably If we want something in this PR to ease the test process, I think we can include a function to trigger rerun in report page, which is not visible to users but can be manually invoked in dev-tool console. |
Actually, it's intentional. Because I thought the server now does more than just serve and open report. But yeah, rerun is not used anywhere yet. I have renamed the function to |
that sgtm. let's do that. |
For a possible followup to this: One thing this makes me think of is since there's a |
Agree. I was thinking about renaming |
Will come back later to address some coding style issues. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ready for review again. PTAL. :)
@@ -100,9 +102,10 @@ function rerunRequestHandler(request, response) { | |||
|
|||
// Add more to flags without changing the original flags | |||
const flags = Object.assign({}, lhParams.flags, additionalFlags); | |||
lighthouse(lhParams.url, flags, lhParams.configs).then(results => { | |||
lighthouse(lhParams.url, flags, perfOnlyConfig).then(results => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When we rerun tests with configurations (e.g. URL blocking patterns), we only care about performance. So use perfOnlyConfig
here to get faster result.
@@ -0,0 +1,7 @@ | |||
module.exports = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Allow us to use /* exported rerunLighthouse */
in perf-x-api.js
the more I think about it the more i like the Ideally, I'd like it in this PR, but to keep this straightforward for review, I'm okay with splitting it into its own PR. (We can still review them simultaneously, even with the dependency) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
mostly good. I am liking this direction...
Right now rerunlighthouse
just logs the new results to the console. Don't we want to update the report?
scriptList.push(fs.readFileSync(path.join(__dirname, './scripts/perf-x-api.js'))); | ||
} | ||
|
||
scriptList.push(fs.readFileSync(path.join(__dirname, './scripts/lighthouse-report.js'))); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
awkward logic here.
probably better to do a `reportContext !== 'devtools' conditional around this last push(), and nuke the early exit on 211
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah. That's much better. Fixed.
if (request.method === 'GET') { | ||
if (pathname === '/') { | ||
reportRequestHandler(request, response); | ||
} else if (pathname === '/rerun') { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let's nuke the GET handler for rerun completely, yah?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, didn't notice that it's not deleted yet. Fixed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right now rerunlighthouse just logs the new results to the console. Don't we want to update the report?
In my original plan, this PR will only include back-end support for rerunning lighthouse but not front-end blocking patterns input interface or report update. Because the way to surface results and to configure blocking patterns is not decided yet.
Ideally, I'd like it in this PR, but to keep this straightforward for review, I'm okay with splitting it into its own PR
Hmm. I think if we add support for users to input blocking patterns in the report page in this PR, then we can rename --view
in this PR. Otherwise, we'd better stick with --view
for now because this mode is not interactive for users yet.
if (request.method === 'GET') { | ||
if (pathname === '/') { | ||
reportRequestHandler(request, response); | ||
} else if (pathname === '/rerun') { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, didn't notice that it's not deleted yet. Fixed.
scriptList.push(fs.readFileSync(path.join(__dirname, './scripts/perf-x-api.js'))); | ||
} | ||
|
||
scriptList.push(fs.readFileSync(path.join(__dirname, './scripts/lighthouse-report.js'))); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah. That's much better. Fixed.
Ah.. I was thinking just UI to trigger a re-run, and viewing of those new results. That wouldn't include configuration of blocked URLs or comparing different runs. Those two are separate and something i know our designer Chris has been thinking about, as well. |
} | ||
|
||
if (reportContext !== 'devtools') { | ||
scriptList.push(fs.readFileSync(path.join(__dirname, './scripts/lighthouse-report.js'))); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why was the 'utf8' removed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry. I didn't add this when I rewrite this method because I thought 'utf8' is default anyway : (
I will add it back in the next commit. Would you mind to tell me why it's necessary though?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think null is the default. https://nodejs.org/api/fs.html#fs_fs_readfilesync_file_options
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My bad. Fixed in the next update! Thanks for pointing out!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added a button in the top bar of the report page (only in --interactive
mode).
Currently, clicking rerun button will just give user a new link to the new report. This will be changed later in the project.
@@ -83,18 +83,17 @@ const cliFlags = yargs | |||
'port': 'The port to use for the debugging protocol. Use 0 for a random port', | |||
'skip-autolaunch': 'Skip autolaunch of Chrome when already running instance is not found', | |||
'select-chrome': 'Interactively choose version of Chrome to use when multiple installations are found', | |||
'interactive': 'Open Lighthouse in interactive mode' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Guess it's time to change --view
to --interactive
now. Becuase report page now is somewhat interactive.
const lighthouse = require('../../lighthouse-core'); | ||
const perfOnlyConfig = require('../../lighthouse-core/config/perf.json'); | ||
|
||
const hostedExperiments = []; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right now the server stores all the results / configs here (in the memory). I'm aware of the potential memory issue. After we have a front end that can compare performance data, we can directly forward the results to that front end page and don't need to store them in memory (at least not in cli).
Edit: Now only stores one instance of rerun results.
module.exports = { | ||
serveAndOpenReport | ||
hostExperiment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now this function does more than just opening report. Guess it's time to change this name as well.
@@ -151,6 +151,11 @@ body { | |||
display: none; | |||
} | |||
|
|||
.report-body__icon.rerun-button { | |||
background-image: url('data:image/svg+xml;utf8,<svg xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink" width="24px" height="24px" viewBox="0 0 24 24" fill="#000000"><defs><path id="a" d="M0 0h24v24H0V0z"/></defs><clipPath id="b"><use xlink:href="#a" overflow="visible"/></clipPath><path clip-path="url(#b)" d="M21 10.12h-6.78l2.74-2.82c-2.73-2.7-7.15-2.8-9.88-.1-2.73 2.71-2.73 7.08 0 9.79 2.73 2.71 7.15 2.71 9.88 0C18.32 15.65 19 14.08 19 12.1h2c0 1.98-.88 4.55-2.64 6.29-3.51 3.48-9.21 3.48-12.72 0-3.5-3.47-3.53-9.11-.02-12.58 3.51-3.47 9.14-3.47 12.65 0L21 3v7.12zM12.5 8v4.25l3.5 2.08-.72 1.21L11 13V8h1.5z"/></svg>'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not sure about this icon choice. Haven't found a better one yet.
Ready for review again. PTAL |
076c925
to
571724c
Compare
@@ -0,0 +1,57 @@ | |||
/** |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
does it make sense to move this under lighthouse-cli/performance-experiment/
to keep everything in one place?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmm. What about this approach:
- Move
perf-x.js
tolighthouse-cli/performance-experiment/
- In
lighthouse-cli/performance-experiment/server.js
, overridereportGenerator.getReportJS
.
So we can keepperf-x.js
inlighthouse-cli/performance-experiment/
while do not need to import something outside of core toreport-generator.js
const additionalFlags = JSON.parse(message); | ||
const flags = Object.assign({}, hostedExperiments.original.params.flags, additionalFlags); | ||
|
||
lighthouse(url, flags, perfOnlyConfig).then(results => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let's assume that other clients (like devtools) will also want to rerun lighthouse. in that case, can we split out this bit into a separate file?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you explain in a little bit more detail? I don't know how to make it reusable by extension or devtools. Thanks : )
<button class="report-body__icon rerun-button js-rerun-button"></button> | ||
<div class="rerun-popup js-rerun-popup" status="idle"> | ||
<p class="rerun-message">Rerunning Lighthouse</p> | ||
<a class="rerun-report-link js-rerun-report-link" href="/?id=rerun">New Report</a> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this UX is a little funky. maybe @samuelli has some ideas.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's just go with the button rotating while its running. After its finished, load the new report in the same page. Also add a title to the button.
Optionally, gray out the background of the report while its running.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the advice. The button now rotates while rerunning. Also tried graying out the background, but that does not look very nice.
rerun button now reruns lighthouse with the same config and refresh the page after rerun is completed\n
7094039
to
0b65e48
Compare
Ready for review again. PTAL. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There will be more churn here, but this is a solid start. let's get this in place so we can iterate on the UX.
) * perf-x server now can rerun lighouse on request * support post request to rerun lighthouse with addtional flags * Added a rerun button on report page * moved perf-x.js to perf-x project folder * changed UX design rerun button now reruns lighthouse with the same config and refresh the page after rerun is completed
) * perf-x server now can rerun lighouse on request * support post request to rerun lighthouse with addtional flags * Added a rerun button on report page * moved perf-x.js to perf-x project folder * changed UX design rerun button now reruns lighthouse with the same config and refresh the page after rerun is completed
Make Perf-X server be able to handle lighthouse rerun request. After receiving the request, Perf-X server will run lighthouse with same parameters plus some additional flags provided in the POST request body and then responds with generated results.
This will allow us to rerun lighthouse with some URLblocking patterns within lighthouse report in the future.
Part of Perf-X project #1143