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

Add --view flag which will start a server and host report.html #1130

Merged
merged 12 commits into from
Dec 15, 2016
Merged
Show file tree
Hide file tree
Changes from 6 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
27 changes: 21 additions & 6 deletions lighthouse-cli/bin.ts
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,8 @@ import * as Printer from './printer';
const lighthouse = require('../lighthouse-core');
const assetSaver = require('../lighthouse-core/lib/asset-saver.js');
const log = require('../lighthouse-core/lib/log');
const server = require('./server/server');
const opn = require('opn');
import {ChromeLauncher} from './chrome-launcher';
import * as Commands from './commands/commands';

Expand Down Expand Up @@ -90,12 +92,14 @@ const cliFlags = yargs

.group([
'output',
'output-path'
'output-path',
'view'
], 'Output:')
.describe({
'output': 'Reporter for the results',
'output-path': `The file path to output the results
Example: --output-path=./lighthouse-results.html`
Example: --output-path=./lighthouse-results.html`,
'view': 'Open report.html in browser'
})

// boolean values
Expand All @@ -112,7 +116,8 @@ Example: --output-path=./lighthouse-results.html`
'select-chrome',
'verbose',
'quiet',
'help'
'help',
'view'
])
.choices('output', Printer.GetValidOutputOptions())

Expand Down Expand Up @@ -198,7 +203,7 @@ function initPort(flags: {port: number}): Promise<undefined> {

function launchChromeAndRun(addresses: Array<string>,
config: Object,
flags: {port: number, selectChrome: boolean}) {
flags: {port: number, selectChrome: boolean, view: boolean}) {

return initPort(flags).then(() => {
const launcher = new ChromeLauncher({
Expand All @@ -219,7 +224,7 @@ function launchChromeAndRun(addresses: Array<string>,
})
}

function lighthouseRun(addresses: Array<string>, config: Object, flags: Object) {
function lighthouseRun(addresses: Array<string>, config: Object, flags: {view: boolean}) {
// Process URLs once at a time
const address = addresses.shift();
if (!address) {
Expand All @@ -229,11 +234,21 @@ function lighthouseRun(addresses: Array<string>, config: Object, flags: Object)
return lighthouse(address, flags, config)
.then((results: Results) => Printer.write(results, outputMode, outputPath))
.then((results: Results) => {
const filename = `./${assetSaver.getFilenamePrefix({url: address})}.report.html`;

Copy link

Choose a reason for hiding this comment

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

So --view overrides output pretty?

if (outputMode === Printer.OutputMode[Printer.OutputMode.pretty]) {
const filename = `./${assetSaver.getFilenamePrefix({url: address})}.report.html`;
Printer.write(results, 'html', filename);
}

Copy link

Choose a reason for hiding this comment

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

nit: remove blank line

// Generate report.html, host it and open it in the default browser
if (flags.view) {
server.listen().then((port: number) => {
const fileAbsPath = `${__dirname}/server/reports/${filename}`;
Printer.write(results, 'html', fileAbsPath);
Copy link
Member

Choose a reason for hiding this comment

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

We are already saving the file to disk in the default pretty case. Shall we just reuse that file and host that?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Currently, the report page is a static page. But the new report page should allow developers to interact with that. The server will then need to host a different report page (eventually). So I think we cannot use the same html file unless we want to update the old report page to the new one as well.

Copy link
Member

Choose a reason for hiding this comment

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

ah gotcha. now i see what you were thinking here.

one way we could handle this is append a <script>...</script> to the end of the response HTML as its being served. (that's also how livereload servers work, just injecting scripts into the end of the file)

generally this has some advantages, in that we can keep the reporting stuff really consistent between our static and interactive reports.

wdyt?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Wow I didn't know we could do it in that approach. Thanks for the suggestion!

Copy link
Contributor Author

@weiwei-lin weiwei-lin Dec 12, 2016

Choose a reason for hiding this comment

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

Already changed. Also, server.js now uses 'http' instead of 'express'.
Edit: It's not working properly. Will update it soon
Edit2: Updated

opn(`http://localhost:${port}/reports/${filename}`);
});
}

return lighthouseRun(addresses, config, flags);
});
}
Expand Down
4 changes: 4 additions & 0 deletions lighthouse-cli/server/reports/.gitignore
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
# Ignore everything in this directory
*
# Except this file
!.gitignore
38 changes: 38 additions & 0 deletions lighthouse-cli/server/server.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,38 @@
/**
Copy link
Member

Choose a reason for hiding this comment

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

is there a more descriptive directory name we can use here since this is going to become specifically the performance experiments server?

* @license
* Copyright 2016 Google Inc. All Rights Reserved.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/
'use strict';

const express = require('express');
Copy link
Member

Choose a reason for hiding this comment

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

can we try to do the server without express?

it's so simple I think we can get away with not using it and instead reusing https://github.com/GoogleChrome/lighthouse/blob/master/lighthouse-cli/test/fixtures/static-server.js

It might need a feature or two, but augmenting that seems better.

const app = express();
let portPromise;

app.use('/reports', express.static(`${__dirname}/reports`));

function listen() {
if (portPromise) {
portPromise = new Promise((resolve, reject) => {
const server = app.listen(0, () => {
resolve(server.address().port);
});
});
}
return portPromise;
}

module.exports = {
listen: listen
Copy link

Choose a reason for hiding this comment

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

listen: app.listen?

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 changed it to listen: app.listen.bind(app)

};
4 changes: 3 additions & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,9 @@
"semver": ">=4.3.3",
"speedline": "1.0.3",
"ws": "^1.1.1",
"yargs": "3.30.0"
"yargs": "3.30.0",
"express": "^4.14.0",
"opn": "^4.0.2"
},
"repository": "googlechrome/lighthouse",
"keywords": [
Expand Down