Viewer: Lighthouse 2.0 support #2521

brendankenny · 2017-06-16T23:50:09Z

Mostly porting, all the features should be exactly the same in practice, but minor refactoring so viewer only adds functionality around the report rather than being a version of the report with different build options. This separation mostly existed before, this just makes it explicit.

LighthouseReportViewer is the host page. It has a placeholder on screen and it takes all sorts of input to get a json report (as before: paste, drag-and-drop, post message, gist url, gist id in the query string). Notably it only deals with loading a json report from somewhere.
viewer has a copy of the v2 report renderer classes. When a new json is to be loaded, viewer's _replaceReportHtml() creates a new copy of these and passes the json in, just like the regular report does. The only difference is that it uses a ViewerUiFeatures, a subclass of ReportUiFeatures, which disables the ability to send to viewer (since it already is in viewer), adds the option to save to gist, and has a custom "save as html" option.

viewer's report JS, CSS, and <template>s are pulled from the static strings in ReportGeneratorV2, which means when new files are added to the report we don't have to add them to the viewer build step too. The template file is a bit weird because it's inlined in viewer's index.html. We could pull it out into its own file, but then would either need to be put in the page with innerHTML or with an XHR with responseType=document. Open to options there.
To save the report from viewer, like before viewer needs to keep a copy of ReportGenerator around because it can't just copy itself (you'd get a copy of viewer, not just the report). ReportGeneratorV2 is browserified and also included in the JS bundle.

The downside of this is that all the report rendering scripts are included in the page twice, once in a string and once as code, and the ones in strings aren't minified (adding about 100KB to the bundle). We could minifiy with a smarter transform than brfs, or we could do something tricky to dedupe them like have the report generator use the scripts that the page includes instead of the fs.readFileSync versions. All the solutions I've explored so far ended up being pretty awkward, so, happy to figure that out in a followup.

brendankenny · 2017-06-16T23:57:21Z

lighthouse-core/closure/typedefs/viewer-externs.js

+ * @struct
+ * @constructor
+ */
+function ReportGenerator() {}


rather than trying to compile ReportGenerator (with its fs use), just use externs for the one method we care about

brendankenny · 2017-06-16T23:58:59Z

lighthouse-core/report/v2/renderer/report-renderer.js

   */
-  constructor(dom, categoryRenderer, uiFeatures = null) {
+  constructor(dom, categoryRenderer) {


reportUIFeatures is removed from the report-renderer constructor, which should make devtools subclassing without it slightly easier cc @paulirish

brendankenny · 2017-06-17T00:01:14Z

lighthouse-viewer/app/src/drag-and-drop.js

+/**
+ * Manages drag and drop file input for the page.
+ */
+class DragAndDrop {


slightly simpler than the old FileUploader. Handles only drag and dropping of files (the hidden input is now just in the page and its handler and readFile are now in the viewer main class)

brendankenny · 2017-06-17T00:03:02Z

lighthouse-viewer/app/src/firebase-auth.js

+   * returns null (and will not trigger sign in).
+   * @return {!Promise<?string>}
+   */
+  getAccessTokenIfLoggedIn() {


I believe this is a somewhat more explicit version of the old ready promise. Having a firebase user also seemed only to be an implementation detail, so it's now entirely internal to the class

I think there were cases where you'd get a user but they wouldn't have a token. Or there would be a user but the token wouldn't be refreshed or something. Can't remember exactly.

ebidel · 2017-06-17T00:26:19Z

Checking functionality. This is mostly working great!

Some things I found:

Pasting a json file doesnt seem to work. Pasting a gist url does :)
Save as HTML -> open that HTML file in a new tab -> blank tab.
Can't remember if v1 did this, but dragging in a new json file should nuke ?gist=ID if it's in the URL.
If you try to drop in a new json file on top of an existing repo, there's a layering issue with the left nav:

patrickhulce · 2017-06-17T00:33:30Z

lighthouse-viewer/app/src/drag-and-drop.js

+
+    document.addEventListener('dragover', e => {
+      e.stopPropagation();
+      e.preventDefault();


as an aside, what do each of these e.stopProgagation/preventDefaults do/why are they necessary? I built another drag/drop recently and I got away without some of them, but I don't know what exactly is required to pull this off...

cc: @ebidel for the original impl :)

This dude told me to do it: https://www.html5rocks.com/en/tutorials/dnd/basics/

TBH, I don't think we need them anymore in a dragover handler. Back in the day, there were browser quirks and I noted that e.preventDefault() was needed to even start a drag. But we're also dragging files (as opposed to links or something in-page) and at the document level. MDN mentions e.preventDefault in dragover, but I betcha that's not accurate.

Anyways, doesn't hurt to be explicit the method is handing the event.

patrickhulce · 2017-06-17T00:37:02Z

lighthouse-viewer/app/src/viewer-ui-features.js

+    if (this._saveGistCallback) {
+      this._saveGistCallback(this.json);
+    } else {
+      throw new Error('Cannot save gist from gist');


this error message doesn't make as much sense to me...

the gist also gazes into you (done)

patrickhulce · 2017-06-17T00:38:59Z

lighthouse-viewer/gulpfile.js

+  const reportCss = streamFromString(ReportGenerator.reportCss, 'report-styles.css');
+  const viewerCss = gulp.src('app/styles/viewer.css');
+
+  return streamqueue({objectMode: true}, reportCss, viewerCss)


I don't really know what all is happening here, but I'm sure it's all necessary and makes sense to you :)

I can add some comments. It's getting the report CSS from ReportGenerator.reportCss instead of from file, so that we don't have to keep up to date references to report resources here, we can just defer to whatever ReportGenerator includes.

Gulp makes it a little unwieldy to just insert a string into the object stream, of course.

is most of this to get around require() usage? Is there a reason you got ride of require? Kinda nice to have explicit deps defined. It was a pain to get working, but it was working :)

is most of this to get around require() usage? Is there a reason you got ride of require? Kinda nice to have explicit deps defined. It was a pain to get working, but it was working :)

I guess it sort of is to get around it, but (I believe) it's necessary now. Viewer code is mixed with report renderer code in Viewer, so require() is a non-starter unless we want to convert all the report renderer code to using it too (which I don't believe we can).

The downside is, yeah, you have to look at ReportGenerator and this gulpfile to check what's included. On the other hand, we have less fragility in files trying to depend on things in node code, browserified node code, and/or native browser APIs.

Biggest complexity introduced in this file is that it delegates to ReportGenerator to pull in the report dependencies. e.g. instead of the old version including '../lighthouse-core/report/styles/report.css' in the gulp.src, it pulls it in as a string from ReportGenerator.reportCss. It makes the gulp stream nonsense more complicated, but it's nice from a DRY/maintenance perspective since any changes to report dependencies will automatically be pulled in here.

brendankenny · 2017-06-17T01:12:31Z

Pasting a json file doesnt seem to work. Pasting a gist url does :)

done. Those no-op catches make that tricky to spot :)

Save as HTML -> open that HTML file in a new tab -> blank tab.

done

Can't remember if v1 did this, but dragging in a new json file should nuke ?gist=ID if it's in the URL.

yeah, it was preserved from v1 :P Fixed now though

If you try to drop in a new json file on top of an existing repo, there's a layering issue with the left nav

everybody loves z-index! done.

codecov · 2017-06-20T20:24:56Z

Codecov Report

Merging #2521 into master will decrease coverage by 0.48%.
The diff coverage is 29.82%.

@@            Coverage Diff             @@
##           master    #2521      +/-   ##
==========================================
- Coverage   84.52%   84.03%   -0.49%     
==========================================
  Files         169      169              
  Lines        5614     5613       -1     
  Branches      774      776       +2     
==========================================
- Hits         4745     4717      -28     
- Misses        855      882      +27     
  Partials       14       14

Impacted Files	Coverage Δ
...ouse-core/report/v2/renderer/report-ui-features.js	`1.41% <0%> (-0.25%)`	⬇️
lighthouse-core/report/v2/report-generator.js	`86.04% <0%> (-13.96%)`	⬇️
...hthouse-core/report/v2/renderer/report-renderer.js	`98.46% <100%> (+1.4%)`	⬆️
lighthouse-viewer/app/src/drag-and-drop.js	`59.25% <59.25%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update cea443e...7d363f6. Read the comment docs.

ebidel · 2017-06-21T20:19:59Z

lighthouse-viewer/app/src/drag-and-drop.js

@@ -0,0 +1,63 @@
+/**
+ * @license Copyright 2016 Google Inc. All Rights Reserved.


ebidel · 2017-06-21T20:31:31Z

lighthouse-viewer/app/src/firebase-auth.js

-      });
+    /**
+     * Promise which resolves after the first check of auth state. After this,
+     * _accessToken will be set if user logged in and has access token.


if the user is logged in....

ebidel · 2017-06-21T20:36:35Z

lighthouse-viewer/app/src/firebase-auth.js

+   * returns null (and will not trigger sign in).
+   * @return {!Promise<?string>}
+   */
+  getAccessTokenIfLoggedIn() {


I think there were cases where you'd get a user but they wouldn't have a token. Or there would be a user but the token wouldn't be refreshed or something. Can't remember exactly.

ebidel · 2017-06-21T20:37:22Z

lighthouse-viewer/app/src/firebase-auth.js

-      return idb.set('accessToken', this.accessToken).then(_ => {
-        return this.accessToken;
+      return idbKeyval.set('accessToken', this._accessToken).then(_ => {
+        return this._accessToken;


we can go 1 line on this

ebidel · 2017-06-21T20:40:35Z

lighthouse-viewer/app/src/github-api.js

 */
-class GithubAPI {
+class GithubApi {


ebidel · 2017-06-21T21:00:30Z

lighthouse-viewer/app/src/viewer-ui-features.js

+   * @param {?function(!ReportRenderer.ReportJSON)} saveGistCallback
+   */
+  constructor(dom, saveGistCallback) {
+    super(dom);


dom is so super 💪

ebidel · 2017-06-21T21:02:04Z

lighthouse-viewer/app/src/viewer-ui-features.js

+  }
+
+  /**
+   * Returns the html that recreates this report. Uses ReportGenerator


Uses ReportGenerator....?

ebidel · 2017-06-21T21:03:34Z

lighthouse-viewer/app/src/viewer-ui-features.js

+   * @override
+   */
+  getReportHtml() {
+    return new ReportGenerator().generateReportHtml(this.json);


would be be able to just use this in main report? Instead of pulling the outerHTML. That would allow us to get rid of _resetUIState() too.

would be be able to just use this in main report? Instead of pulling the outerHTML. That would allow us to get rid of _resetUIState() too.

We could consider that in a follow up. I avoided it here because it means ReportGenerator has to be browserified before Lighthouse can be run, taking us back to the report build step we just ditched by getting rid of pre-compiled handlebars templates.

I do have a separate idea for getting rid of _resetUIState(), though: basically we'd track which templates had been imported in a JS variable instead of an attribute. outerHTML wouldn't care anymore, and the equivalent of dom.resetTemplates() could be called in renderReport at the same time it does container.textContent = ''; so that they'd be cloned correctly again.

(though I am personally digging the fact that all our uses of the v2 report just builds the report rendering classes new each time. If we only want to go that way, no need for resetting the Dom class, it'll just be a new instance every time)

ebidel · 2017-06-21T21:08:08Z

lighthouse-viewer/gulpfile.js

-    .pipe(license())  // Add license to top.
-    .pipe(gulp.dest(`${config.dist}/src`));
+gulp.task('compile-js', () => {
+  const filename = __dirname + '/../lighthouse-core/report/v2/report-generator.js';


how big is the viewer bundle now? without fb and such.

how big is the viewer bundle now? without fb and such.

183KB

(About 100KB of that is the unminified js pulled in by ReportGenerator as a string, which can be brought down to about 25K with a simple uglify step, but we'd need to replace brfs with something that can chain transforms like that...good fodder for a followup)

ebidel · 2017-06-21T21:09:03Z

lighthouse-viewer/gulpfile.js

+    'node_modules/idb-keyval/dist/idb-keyval-min.js',
+  ]);
+
+  const versionStr = `window.LH_CURRENT_VERSION = '${lighthousePackage.version}';`;


add comment

disable save-as-gist based on providing a callback for saving

brendankenny · 2017-06-21T23:27:59Z

I think there were cases where you'd get a user but they wouldn't have a token. Or there would be a user but the token wouldn't be refreshed or something. Can't remember exactly.

yeah, this should maintain the earlier behavior where if both aren't present it requires reauthorizing.

brendankenny · 2017-06-21T23:38:37Z

updated!

ebidel · 2017-06-22T22:55:09Z

https://www.youtube.com/watch?v=9D-QD_HIfjA

brendankenny · 2017-06-22T23:02:01Z

https://googlechrome.github.io/lighthouse/viewer/?gist=c2df424d87feb2ceac02d9d16c0c602e

brendankenny commented Jun 16, 2017

View reviewed changes

brendankenny commented Jun 17, 2017

View reviewed changes

ebidel added the viewer label Jun 17, 2017

ebidel changed the title ~~port viewer to use report v2~~ Viewer: Lighthouse 2.0 support Jun 17, 2017

patrickhulce reviewed Jun 17, 2017

View reviewed changes

pgsgoossens approved these changes Jun 17, 2017

View reviewed changes

brendankenny mentioned this pull request Jun 19, 2017

extract logger from lighthouse #2528

Merged

brendankenny force-pushed the viewer branch 2 times, most recently from 2e57b13 to 7d363f6 Compare June 20, 2017 19:47

ebidel suggested changes Jun 21, 2017

View reviewed changes

brendankenny added 12 commits June 21, 2017 15:33

basic viewer working

563ea11

most features working

b30ec9f

reenable remaining features

ab22bbc

cleanup

4b52655

fix filename

b4d31ab

logger, ga, and version check

b1d3df4

make closure happy

2039219

pull report-ui-features out of report-renderer

3358aa2

disable save-as-gist based on providing a callback for saving

gulp watch

2b11d9e

add back drag-and-drop unit test

0166be4

actually make closure happy

6dfeb40

review fixes

edf7f70

review feedback

ae2ed6c

brendankenny force-pushed the viewer branch from 7d363f6 to ae2ed6c Compare June 21, 2017 23:38

ebidel approved these changes Jun 22, 2017

View reviewed changes

ebidel merged commit 2b31a44 into master Jun 22, 2017

ebidel deleted the viewer branch June 22, 2017 22:55

This was referenced Jun 22, 2017

make save-as-gist css more specific #2574

Merged

Unknown formatter: user-timings in exported lighthouse report in viewer #1985

Closed

		@@ -0,0 +1,63 @@
		/**
		* @license Copyright 2016 Google Inc. All Rights Reserved.

Viewer: Lighthouse 2.0 support #2521

Viewer: Lighthouse 2.0 support #2521

Conversation

brendankenny commented Jun 16, 2017 • edited by ebidel 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

ebidel commented Jun 17, 2017

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

ebidel Jun 21, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brendankenny commented Jun 17, 2017

codecov bot commented Jun 20, 2017 • edited Loading

Codecov Report

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

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

Choose a reason for hiding this comment

brendankenny commented Jun 21, 2017

brendankenny commented Jun 21, 2017

ebidel commented Jun 22, 2017

brendankenny commented Jun 22, 2017 • edited Loading

brendankenny commented Jun 16, 2017 •

edited by ebidel

Loading

ebidel Jun 21, 2017 •

edited

Loading

codecov bot commented Jun 20, 2017 •

edited

Loading

brendankenny commented Jun 22, 2017 •

edited

Loading