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

feat: add basic lighthouse functionality #825

Merged
merged 2 commits into from
Mar 2, 2017

Conversation

patrickhulce
Copy link
Contributor

@patrickhulce patrickhulce commented Mar 2, 2017

cc: @paulirish @ebidel @brendankenny

The Lighthouse
http://g.recordit.co/jouv3SIcwe.gif

The Run
image

The Report (video is available on mobile)
image

this.lighthouseFile_ = path.join(this.runTempDir_, 'lighthouse.html');
this.app_.timeout(1000, 'Waiting for Chrome to be available...');
process_utils.scheduleExec(this.app_,
'lighthouse',
Copy link
Contributor

Choose a reason for hiding this comment

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

Probably up to Pat, but should we put the lighthouse install instructions in a readme?

Presumably it's npm install -g GoogleChrome/lighthouse + confirm npm globals are in your PATH?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah the most helpful instructions I actually found were over in the google sites page rather than any README so I figured we'd add it there, but even better to find a place here if there's one I missed!

Copy link
Contributor

Choose a reason for hiding this comment

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

How big is it and is it feasible to include directly? Worst case I'll add logic to the agent at startup to look for it and try to install if it isn't there (and automatically update if it is) but if we can eliminate the need for manually installing then all of the existing agents will pick up lighthouse support automatically.

Also, does running it as "lighthouse" work on Windows as well or does Node need to explicitly be referenced? Just checking since the Node agent runs across all platforms.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

IMO the maintenance burden of keeping it as a submodule or checking in the result of npm install here is more trouble than letting npm install -g handle it and adding an auto-update check, but it's a bit safer so I'm open to either.

I'm not on Windows anywhere with node, so I can't verify it, but we've recently had a contributor making sure our build runs on appveyor which I believe removed our *nix specific hashbangs and such. If your path is setup correctly it should, but I can validate on my machine at home tonight.

Copy link

@ebidel ebidel left a comment

Choose a reason for hiding this comment

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

PHP! It's been a while my friend.

'--output-path', this.lighthouseFile_,
this.task_.url,
], undefined, this.timeout_).then(function () {
var devtoolsLogFile = path.join(this.runTempDir_, 'lighthouse-0.devtoolslog.json');
Copy link

Choose a reason for hiding this comment

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

is this indentation intentional?

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 line yes :) though the above should be indented 2 more spaces. it's used elsewhere like this, will fix


// Cache for a year
header('Last-Modified: ' . gmdate('r'));
header('Expires: '.gmdate('r', time() + 31536000));
Copy link

Choose a reason for hiding this comment

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

space around .

Copy link

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

The WPT code is old so it's just left over :-) It's fine to remove it but it also doesn't hurt since cache-control takes precedence.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll remove

@ebidel
Copy link

ebidel commented Mar 2, 2017

Does our snazy LH animation come into the mix anywhere, or is that just for PR description fun?

Copy link
Contributor

@pmeenan pmeenan left a comment

Choose a reason for hiding this comment

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

No blockers so I'm happy to merge. My vote is to merge it as it is and iterate on the result UI and agent install bits. LMK if that sounds good and I'll pull it in.

this.lighthouseFile_ = path.join(this.runTempDir_, 'lighthouse.html');
this.app_.timeout(1000, 'Waiting for Chrome to be available...');
process_utils.scheduleExec(this.app_,
'lighthouse',
Copy link
Contributor

Choose a reason for hiding this comment

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

How big is it and is it feasible to include directly? Worst case I'll add logic to the agent at startup to look for it and try to install if it isn't there (and automatically update if it is) but if we can eliminate the need for manually installing then all of the existing agents will pick up lighthouse support automatically.

Also, does running it as "lighthouse" work on Windows as well or does Node need to explicitly be referenced? Just checking since the Node agent runs across all platforms.


// Cache for a year
header('Last-Modified: ' . gmdate('r'));
header('Expires: '.gmdate('r', time() + 31536000));
Copy link
Contributor

Choose a reason for hiding this comment

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

The WPT code is old so it's just left over :-) It's fine to remove it but it also doesn't hurt since cache-control takes precedence.

// create a friendlier (unique) name for the fi
$fileUrl = GetFileUrl($url);
if ( $test['testinfo']['lighthouse'] && $fvMedian )
{
echo "<a href=\"/lighthouse.php?test=$id&run=$fvMedian\">Lighthouse report</a><br>";
Copy link
Contributor

Choose a reason for hiding this comment

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

This works for now but I'd really like to give it much more prominent treatment. The grades section in the header is a perfect place since the grades won't be calculated in the lighthouse case. If the score can be pulled out of the HTML then the UI can have a large:

Lighthouse Score: 85/100 (click for full report)

And maybe also the logo ;-)

@ebidel
Copy link

ebidel commented Mar 2, 2017 via email

@patrickhulce
Copy link
Contributor Author

Does our snazy LH animation come into the mix anywhere, or is that just for PR description fun?

Just fun, but it will! That GIF was from my local custom LH-WPT launch page ;)

I also vote to iterate on the UI, adding the score, etc.

@pmeenan pmeenan merged commit 7ce8cd2 into catchpoint:master Mar 2, 2017
@ebidel
Copy link

ebidel commented Mar 2, 2017

Woot! cc @igrigorik

@patrickhulce patrickhulce deleted the lighthouse_run branch March 2, 2017 18:15
@igrigorik
Copy link

\o/ ... woot!

Pat: when might we see this live on webpagetest.org? :)

@pmeenan
Copy link
Contributor

pmeenan commented Mar 2, 2017

Probably next week. I want to do some testing first and then since I have ~60 mobile agents I really want to teach the agent code to install and update lighthouse automatically so I don't have to manually configure all of them.

@pmeenan
Copy link
Contributor

pmeenan commented Mar 3, 2017

There go my hopes for an easy install :( Apparently the Node version on the Pi's is 4.x (which is what is in the repo for jessie) so I need to go through and upgrade them all to 7.x anyway. Starting on the testing now and I still plan to have it up and running next week.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants