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

weiwei-lin · 2016-12-09T02:35:15Z

This server will allow to host trace file in the future. #616

googlebot · 2016-12-09T02:35:17Z

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed, please reply here (e.g. I signed it!) and we'll verify. Thanks.

If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
If you signed the CLA as a corporation, please let us know the company's name.

googlebot · 2016-12-09T02:40:31Z

CLAs look good, thanks!

samuelli · 2016-12-09T02:50:51Z

lighthouse-cli/server/server.js

+});
+
+function listen(port) {
+  return app.listen(port)


samuelli · 2016-12-09T02:52:02Z

lighthouse-cli/server/server.js

+}
+
+module.exports = {
+  listen: listen


listen: app.listen?

I changed it to listen: app.listen.bind(app)

samuelli · 2016-12-09T02:58:00Z

lighthouse-cli/bin.ts

+        const fileAbsPath = `${__dirname}/server/reports/${filename}`;
+        Printer.write(results, 'html', fileAbsPath);
+        opn(`http://localhost:${_REPORT_SERVER_PORT}/reports/${filename}`);
+


So --view overrides output pretty?

samuelli · 2016-12-09T02:58:09Z

lighthouse-cli/bin.ts

        const filename = `./${assetSaver.getFilenamePrefix({url: address})}.report.html`;
        Printer.write(results, 'html', filename);
+


nit: remove blank line

samuelli · 2016-12-09T02:59:40Z

Need to fix travis errors.

brendankenny · 2016-12-09T03:20:29Z

is it possible to post the plan for this to a public issue to give context to everyone who hasn't seen it?

brendankenny · 2016-12-09T03:28:40Z

lighthouse-cli/package.json

@@ -8,6 +8,8 @@
    "typescript": "^2.0.3"
  },
  "dependencies": {
-    "@types/node": "^6.0.45"
+    "@types/node": "^6.0.45",
+    "express": "^4.14.0",


until we need more advanced server features, could we limit this to a more simple server a la static-server to avoid the extensive dependencies express brings in until we need them?

IMO express is very minimalist already. I think the trade off is writing less code or less dependencies. Express lets us do simple routing and deals with basic security issues in single lines. (current code can be cut down further).

samuelli · 2016-12-09T04:05:29Z

lighthouse-cli/server/server.js

+const express = require('express');
+const app = express();
+
+app.get('/reports/:name', (request, response) => {


What about using express.static() here if its all in one directory?

samuelli · 2016-12-09T04:07:50Z

@brendankenny: Yup, in this particular case #616 is just about viewing trace results.

@WeiweiAtGit: can you make sure you summaries the experiments ideas in a separate issue?

weiwei-lin · 2016-12-09T04:12:56Z

@samuelli Sure. I will summarize the project and put a link to the document later.

…npendencies to top level

ebidel · 2016-12-09T17:44:05Z

lighthouse-cli/bin.ts

@@ -234,6 +240,14 @@ function lighthouseRun(addresses: Array<string>, config: Object, flags: Object)
        Printer.write(results, 'html', filename);
      }

+      // Generate report.html, host it and open it in the default browser
+      if (flags.view) {
+        const filename = `${assetSaver.getFilenamePrefix({url: address})}.report.html`;


maybe dry this up with 239-240.

Thanks for the suggestion

paulirish · 2016-12-12T00:01:42Z

lighthouse-cli/server/server.js

+ */
+'use strict';
+
+const express = require('express');


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.

paulirish · 2016-12-12T00:05:05Z

lighthouse-cli/bin.ts

+      if (flags.view) {
+        server.listen().then((port: number) => {
+          const fileAbsPath = `${__dirname}/server/reports/${filename}`;
+          Printer.write(results, 'html', fileAbsPath);


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

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.

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?

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

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

…ico to avoid error when loading report.html in a browser

Deleted unnecessary code in server.js.\nUpdated filename variable when file is not found.\nRearranged code to avoid using variable 'server' before defining

…generated in the past Now use symlink to access report files which are not in the server folder.\nAlso deny access to files outside server folder.\n

brendankenny

some more comments :)

brendankenny · 2016-12-13T01:40:52Z

lighthouse-cli/server/server.js

@@ -0,0 +1,94 @@
+/**


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

brendankenny · 2016-12-13T01:44:18Z

lighthouse-cli/server/server.js

+
+function getPort(port) {
+  if (!portPromise) {
+    portPromise = new Promise((reslove, reject) => {


s/reslove/resolve

brendankenny · 2016-12-13T01:50:48Z

lighthouse-cli/server/server.js

+const server = http.createServer(requestHandler);
+server.on('error', e => console.error(e.code, e));
+
+function getPort(port) {


should this be called something more like createServer?

(after the listen() callback the port query is just a sync operation)

brendankenny · 2016-12-13T01:59:01Z

lighthouse-cli/bin.ts

+      if (flags.view) {
+        server.getPort(0).then((port: number) => {
+          const filePath = `${server.FOLDERS.REPORTS}/${filename}`;
+          const htmlExist = outputMode === Printer.OutputMode[Printer.OutputMode.pretty]


Would it be better to just check if the file exists to avoid having two places to change any logic on when a file will be printed to file?

Thanks for your advice. I tried to follow your approach but I found that we still need to check the output mode because we only want to host HTML file (may change to only JSON file later to ease performance comparison). Besides, there may exist a random file named 'stdout' in CWD, considering that 'stdout' is a common name. Thus I think it's still better to check whether outputPath is 'stdout'.

brendankenny · 2016-12-13T02:03:43Z

lighthouse-cli/bin.ts

+          // If the HTML file is already generated, create a symlink to it
+          if (htmlExist) {
+            const targetPath = outputMode === Printer.OutputMode[Printer.OutputMode.pretty] ? filename : outputPath;
+            fs.symlinkSync(path.resolve(targetPath), filePath);


should we clear this directory on server close? On the one hand, it may be worth having older versions, but if you're deleting the original the symlink becomes useless and you probably won't know to delete the ones in the server directory.

Instead of deleting the entire folder, now the server will delete all broken symlinks on start. So we can keep the old reports and we don't need to worry about those broken symlinks.

…tart

weiwei-lin

Changed server folder name from 'server' to 'performance-experiment'.
Changed function name from 'listen' to 'startServer'.
Server now deletes broken symlinks on start.

weiwei-lin · 2016-12-13T03:11:37Z

lighthouse-cli/bin.ts

+      if (flags.view) {
+        server.getPort(0).then((port: number) => {
+          const filePath = `${server.FOLDERS.REPORTS}/${filename}`;
+          const htmlExist = outputMode === Printer.OutputMode[Printer.OutputMode.pretty]


Thanks for your advice. I tried to follow your approach but I found that we still need to check the output mode because we only want to host HTML file (may change to only JSON file later to ease performance comparison). Besides, there may exist a random file named 'stdout' in CWD, considering that 'stdout' is a common name. Thus I think it's still better to check whether outputPath is 'stdout'.

weiwei-lin · 2016-12-13T05:05:54Z

lighthouse-cli/bin.ts

+          // If the HTML file is already generated, create a symlink to it
+          if (htmlExist) {
+            const targetPath = outputMode === Printer.OutputMode[Printer.OutputMode.pretty] ? filename : outputPath;
+            fs.symlinkSync(path.resolve(targetPath), filePath);


Instead of deleting the entire folder, now the server will delete all broken symlinks on start. So we can keep the old reports and we don't need to worry about those broken symlinks.

brendankenny

a few more things

brendankenny · 2016-12-14T01:21:05Z

lighthouse-cli/bin.ts

+        performanceXServer.startServer(0).then((port: number) => {
+          const filePath = `${performanceXServer.FOLDERS.REPORTS}/${filename}`;
+          const htmlExist = outputMode === Printer.OutputMode[Printer.OutputMode.pretty]
+            || (outputPath !== 'stdout' && outputMode === Printer.OutputMode[Printer.OutputMode.html]);


for some conflicting advice here: I'm still not a fan of the fragility of this check. It may not be worth reusing the pretty report...I'd personally prefer just re-printing (~100ms for a decent report) and longer term we could do some kind of refactor like separating generation of the report html from saving to file

@paulirish for his opinion

Sure. I'm happy to regenerate the report rather than read it from where we just printed it on disk.

if --view becomes a default, then the automatic disk-save won't really make sense.

brendankenny · 2016-12-14T01:22:24Z

lighthouse-cli/performance-experiment/server.js

+};
+
+const server = http.createServer(requestHandler);
+server.on('error', e => console.error(e.code, e));


should use log.error here for logging

brendankenny · 2016-12-14T01:22:32Z

lighthouse-cli/performance-experiment/server.js

+        sendResponse(404, `404 - File not found. ${pathname}`);
+        return;
+      }
+      console.error(`Unable to read local file ${filePath}:`, err);


log.error here as well

Added a file overview and some comments for performance-experiment/server.js. No longer check if report page is already created. Now always create a new copy for report page. Removed symlink cleanup process on server start since it’s no longer needed.

brendankenny

LGTM

brendankenny · 2016-12-14T22:13:00Z

over to @paulirish

paulirish

Yup. This is looking ace. Thanks for kicking this awesome work off, @WeiweiAtGit :D

weiwei-lin · 2016-12-15T02:09:51Z

Thanks for all the advice!

…eChrome#1130)

add --view flag which will start a sever and host report.html

aff780a

weiwei-lin force-pushed the add-server branch from 9ac64ca to aff780a Compare December 9, 2016 02:40

samuelli reviewed Dec 9, 2016

View reviewed changes

lighthouse-cli/server/server.js

});

function listen(port) {

return app.listen(port)

Copy link

samuelli Dec 9, 2016

Choose a reason for hiding this comment

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

nit: ;

samuelli reviewed Dec 9, 2016

View reviewed changes

samuelli approved these changes Dec 9, 2016

View reviewed changes

brendankenny reviewed Dec 9, 2016

View reviewed changes

samuelli reviewed Dec 9, 2016

View reviewed changes

weiwei-lin added 2 commits December 9, 2016 16:26

simplifed server.js, --view no longer override pretty, moved added de…

06f884e

…npendencies to top level

add empty folder lighthouse-cli/server/reports

2e079bf

ebidel changed the title ~~Add --view flag which will start a sever and host report.html~~ Add --view flag which will start a server and host report.html Dec 9, 2016

ebidel reviewed Dec 9, 2016

View reviewed changes

weiwei-lin added 3 commits December 12, 2016 08:28

No longer calculate report filename separately

19a0b6e

report page server now use a dynamic port

bfa8235

No longer init portPromise in server.js to undefined

6d1bb36

paulirish requested changes Dec 12, 2016

View reviewed changes

weiwei-lin added 3 commits December 12, 2016 14:41

Use 'http' module instead of 'express' for server.js and add favicon.…

4aca0ab

…ico to avoid error when loading report.html in a browser

Fixed some bugs

1e20465

Deleted unnecessary code in server.js.\nUpdated filename variable when file is not found.\nRearranged code to avoid using variable 'server' before defining

Fixed creating duplicated html file and allow accessing report files …

4ab4a75

…generated in the past Now use symlink to access report files which are not in the server folder.\nAlso deny access to files outside server folder.\n

brendankenny requested changes Dec 13, 2016

View reviewed changes

Fixed coding style and report server now deletes broken symlinks on s…

59a178e

…tart

weiwei-lin force-pushed the add-server branch from f760d64 to 59a178e Compare December 13, 2016 05:22

weiwei-lin commented Dec 13, 2016

View reviewed changes

Added for-in guard in server.js

aaaf92d

brendankenny requested changes Dec 14, 2016

View reviewed changes

brendankenny approved these changes Dec 14, 2016

View reviewed changes

brendankenny assigned paulirish Dec 14, 2016

weiwei-lin mentioned this pull request Dec 14, 2016

Return artifacts from runner and move assets/artifacts saving to cli #1163

Merged

paulirish approved these changes Dec 15, 2016

View reviewed changes

paulirish merged commit dd6348a into GoogleChrome:master Dec 15, 2016

weiwei-lin deleted the add-server branch December 15, 2016 10:45

andrewrota pushed a commit to andrewrota/lighthouse that referenced this pull request Jan 13, 2017

Add --view flag which will start a server and host report.html (Googl…

8a17875

…eChrome#1130)

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

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

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

Conversation

weiwei-lin commented Dec 9, 2016

googlebot commented Dec 9, 2016

googlebot commented Dec 9, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

samuelli commented Dec 9, 2016

brendankenny commented Dec 9, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

samuelli commented Dec 9, 2016

weiwei-lin commented Dec 9, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

weiwei-lin Dec 12, 2016 • edited Loading

Choose a reason for hiding this comment

brendankenny left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

weiwei-lin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brendankenny left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brendankenny left a comment

Choose a reason for hiding this comment

brendankenny commented Dec 14, 2016

paulirish left a comment

Choose a reason for hiding this comment

weiwei-lin commented Dec 15, 2016

weiwei-lin commented Dec 9, 2016 •

edited

Loading

weiwei-lin Dec 12, 2016 •

edited

Loading