Consolidate viewer functionality into main report #1594

ebidel · 2017-02-01T04:12:41Z

R: all

This is the finale of #1169. But unlike that PR, this does not introduce a build step to lighthouse-core!

Now, all generated reports (cli/extension/viewer) have the full export button goodness:

The share button is still specific to viewer:

ebidel · 2017-02-01T04:13:51Z

lighthouse-core/report/scripts/lighthouse-report.js

+  /**
+   * Handler copy events.
+   */
+  onCopy(e) {


mostly moving code from viewer -> lighthouse-report.js

ebidel · 2017-02-01T04:15:33Z

lighthouse-core/report/scripts/lighthouse-report.js

-// If in Node, allow others to require us.  By default, browser code can juse
-// use the LighthouseReport class.
-if (typeof module !== 'undefined') {
+// Allow Node code to use require(). Browser code can use the LighthouseReport


Open to suggestions on this. There might be a Node-ier want to do this but I think it's simple enough.

Viewer still browserifies the code, so we keep requires for that and omit them for the browser context.

ebidel · 2017-02-01T04:16:03Z

lighthouse-core/report/scripts/logger.js

@@ -67,4 +67,6 @@ class Logger {
  }
 }

-module.exports = new Logger('#log'); // singleton


got rid of the nasty dom singleton. Viewer and the core report each defines their own logger now.

ebidel · 2017-02-01T04:17:30Z

lighthouse-core/report/templates/report-template.html

+'use strict';
+
+/**
+ * Note: getFilenamePrefix is duplicated from asset-saver.js


I'm up for suggestions on this too. We could factor out getFilenamePrefix from asset-saver.js and make it work in both Node and the browser.

+1 to this. Let's split into its own module or whatever?

Definitely shouldn't duplicate it here if we dont have to

ebidel · 2017-02-01T04:18:13Z

lighthouse-viewer/test/test-helpers.js

@@ -26,11 +26,14 @@ const PAGE = fs.readFileSync(path.join(__dirname, '../app/index.html'), 'utf8');
 function setupJsDomGlobals() {
  global.document = jsdom.jsdom(PAGE);
  global.window = global.document.defaultView;
+  global.logger = console;


ebidel · 2017-02-02T21:13:58Z

rebased

ebidel · 2017-02-03T23:19:25Z

rebased

paulirish · 2017-02-04T01:36:14Z

lighthouse-core/report/templates/report-template.html

+'use strict';
+
+/**
+ * Note: getFilenamePrefix is duplicated from asset-saver.js


+1 to this. Let's split into its own module or whatever?

Definitely shouldn't duplicate it here if we dont have to

paulirish · 2017-02-04T02:06:12Z

lighthouse-core/test/report/report-test.js

-    assert.ok(html.includes('printButton = document.querySelector'),
-                            'print button functionality attached');
-    assert.ok(html.includes('openButton = document.querySelector'),
-                            'open button functionality attached');
    assert.ok(html.includes('share js-share'), 'has share button');


i guess it'd be a pain to assert that the share icon has display:none in this case, yeah?

ah well.

ebidel · 2017-02-06T17:54:02Z

PTAL. Moved out getFilenamePrefix into a separate file so it could be require'd or used directly in the browser.

brendankenny · 2017-02-07T02:12:34Z

this might need a fix now that getFilenamePrefix is used in a few other places due to #1601, sorry :(

brendankenny

Sorry this didn't get looked at sooner.

This is looking good, and I really like the simplification it's doing, but I still feel like I need a diagram for our three(?) different reports, which scripts are included in each, and how they're each included.

Part of the issue is probably LighthouseViewerReport extending LighthouseReport but also creating a LighthouseReport to display it, just not including any of the script parts of it. I wonder if there's more we can do to tease this apart in the future.

brendankenny · 2017-02-07T02:17:17Z

lighthouse-core/lib/file-namer.js

+ */
+function getFilenamePrefix(results) {
+  // eslint-disable-next-line no-undef
+  const hostname = new (URLConstructor || URL)(results.url).hostname;


url-shim already guards against this. Any way we could rely on that and avoid this (and the require() below)? Since file-namer isn't included via script tag it seems like it's doable.

I couldn't see a way around it.

For node code, we need to require('url-shim') to get URL. But if you do this instead:

let URL; if (typeof module !== 'undefined' && module.exports) { URL = require('./url-shim'); ... }

You'll end up with URL === undefined in the browser. filer-namer.js gets <script> included in the main report here: https://github.com/GoogleChrome/lighthouse/pull/1594/files#diff-6a4779f17aa030f3223e4de92085eda9R227

brendankenny · 2017-02-07T02:19:02Z

lighthouse-core/report/scripts/.eslintrc.js

+module.exports = {
+  "extends": "../../../.eslintrc.js",
+  "env": {
+    "browser": true


👍 good call

brendankenny · 2017-02-07T05:59:11Z

Part of the issue is probably LighthouseViewerReport extending LighthouseReport but also creating a LighthouseReport to display it, just not including any of the script parts of it. I wonder if there's more we can do to tease this apart in the future.

Thinking about this more, I realize part of this setup is precisely because we're trying to avoid a build step in lighthouse-core :)

ebidel · 2017-02-07T06:06:01Z

this might need a fix now that getFilenamePrefix is used in a few other places due to #1601

We should be covered. asset-saver.js imports require('file-namer.js').getFilenamePrefix and still exports it for backwards compatibility.

brendankenny · 2017-02-07T06:22:15Z

lighthouse-core/report/templates/report-template.html

@@ -247,8 +249,14 @@ <h2 class="config-section__title">Blocked URL Patterns</h2>
 {{/each}}

 <script>
-window.addEventListener('DOMContentLoaded', _ => {
-  new LighthouseReport(window.lhresults);
+(function(exports) {


what do you think about moving this into its own script (report-startup or whatever) and included in ReportGenerator.getReportJS if reportContext is cli or extension? That seems like it may be clearer it's just for that context, but I'm not sure

We could, but does it buy anything?

Viewer has similar init code and uses a main.js. But the only reason I put that a separate script was because the code uses require().

My thinking was that it may make the execution flow clearer since report-template.html is used for the report and viewer but this script is only for report, not viewer.

Mostly I'm trying to look at my own confusion when I come back to this split and trying to think of ways to make it easier :)

(come to think of it, none of the scripts injected by getReportJS are used by the viewer, right?)

Right. Nothing here ends up in Viewer. Because of that, we should avoid adding things to getReport* that are specific to the main report and never used elsewhere. This is a good example, where the init code is specific to the page. Otherwise the browserified bundle gets larger :\

our own conservation of mass!

okay on the one hand we have

but then here we have

so yeah im pretty confused as well.

okay here's how report-generator is consumed:

viewer/app/src/main and lighthouse-background are browserified and used in the viewer and extension/devtools, respectively.

question:

why is report-generator including the source of scripts/lighthouse-report into the report when that file was already part of the bundle that shipped with the viewer

also, can we rename some of these files, because they all sound like the same thing right now. :)

Is the confusion mostly about the inline <script> at the bottom of report-template.html (@paulirish's 2nd screenshot)? I can create a separate .js file with the DOMContentLoaded setup and load it in the getReportJS() array. Would that help?

why is report-generator including the source of scripts/lighthouse-report into the report...

report-generator.js has always included the ligthhouse-report.js source into the report. I didn't change that setup. The only difference in this PR is that we're inlining more JS into the report (logger.js + filer-namer.js + lighthouse-report.js).

....when that file was already part of the bundle that shipped with the viewer

Viewer bundles report-generator.js as-is because it needs all the mechanics that generate the HTML report. Viewer needs all of report-generator.js because it dynamically generates an HTML report when the user drops a json file, pastes a link, or deep links from github. The idea was that we have the same codepath (ReportGenerator.generateHTML()) generating HTML for all three: crx/cli/viewer.

also, can we rename some of these files, because they all sound like the same thing right now. :)

Partially why I'm advocating we don't create more micro files :) lighthouse-report.js and the viewer's subclass (lighthouse-report-viewer.js) make sense to me, but I'm open to renaming.

Thinking about this some more, I'm not stoked on moving this:

window.addEventListener('DOMContentLoaded', _ => { window.logger = new Logger('#log'); new LighthouseReport(window.lhresults); });

into a separate script. There's little value in doing that. Plus, it'll need a license header and a bunch of eslint ignore boilerplate. All of that will add to our browserified bundles.

Next step is to kill some of that extra stuff in #1670

brendankenny · 2017-02-07T06:24:30Z

We should be covered. asset-saver.js imports require('file-namer.js').getFilenamePrefix and still exports it for backwards compatibility.

Ah, I see. It seems like that should be dropped and the reference and tests moved from asset-saver-test to file-namer-test, though

ebidel · 2017-02-07T18:37:29Z

Ah, I see. It seems like that should be dropped and the reference and tests moved from asset-saver-test to file-namer-test, though

Done. PTAL

brendankenny

this is an improvement in every way over the current situation, so LGTM

(but I'll continue to lodge complaints that we need to keep simplifying this so I can actually understand what goes where without getting 11 comments into it :P)

brendankenny · 2017-02-08T02:18:56Z

lighthouse-core/report/templates/report-template.html

 </script>
-{{/if}}
+{{/if}}


brendankenny · 2017-02-08T02:20:09Z

lighthouse-core/report/styles/report.css

+/* app z-indexes */
+.log-wrapper {
+  z-index: 3;
+}


brendankenny · 2017-02-08T02:22:06Z

lighthouse-core/report/report-generator.js

@@ -222,7 +222,11 @@ class ReportGenerator {
    if (reportContext === 'devtools') {
      return [];
    } else {
-      return [fs.readFileSync(path.join(__dirname, './scripts/lighthouse-report.js'), 'utf8')];
+      return [
+        fs.readFileSync(path.join(__dirname, './scripts/logger.js'), 'utf8'),


nit: can drop the path.join and just fs.readFileSync(__dirname + '/scripts/logger.js'), 'utf8')

Yea, just stuck with what we had in getReportCSS and getExceptionTemplate

ebidel · 2017-02-08T06:29:02Z

Ready freddie.

ebidel · 2017-02-09T01:40:03Z

💥

paulirish · 2017-02-09T01:57:13Z

🎐 🚶 🥇

(edit: before you try to hurt your brain on this.. i just picked emojis that sounded nice :)

ebidel commented Feb 1, 2017

View reviewed changes

ebidel mentioned this pull request Feb 1, 2017

WIP: Move copy button to main report #1169

Closed

ebidel force-pushed the consolidatereport branch from 03dc485 to eef5958 Compare February 3, 2017 23:18

paulirish requested changes Feb 4, 2017

View reviewed changes

brendankenny reviewed Feb 7, 2017

View reviewed changes

ebidel force-pushed the consolidatereport branch from 20c12b2 to 65d2ead Compare February 7, 2017 06:48

ebidel added 10 commits February 7, 2017 09:19

Move viewer code into main report

2e62287

fixes after rebase

4170f3b

more rebase fixes

db64421

Move open in viewer button into export dropdown. Add download icon

0f9dcf5

update comment

30aa258

better line spacing

bb5cfad

Update tests

4046ff3

fix after rebase

9738821

Refactor getFilenamePrefix into separate file

49851ee

Remove assset-saver export of getFileNamePrefix

54a0678

ebidel force-pushed the consolidatereport branch from 65d2ead to 54a0678 Compare February 7, 2017 18:31

Move tests

e103e4a

ebidel added the rls blocker label Feb 7, 2017

brendankenny approved these changes Feb 8, 2017

View reviewed changes

newlines

dd810cf

ebidel force-pushed the consolidatereport branch from 4f15b5b to dd810cf Compare February 8, 2017 18:01

happy linting

9934c76

brendankenny merged commit 9ac57cb into master Feb 9, 2017

brendankenny deleted the consolidatereport branch February 9, 2017 01:37

Consolidate viewer functionality into main report #1594

Consolidate viewer functionality into main report #1594

Conversation

ebidel commented Feb 1, 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 commented Feb 2, 2017

ebidel commented Feb 3, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ebidel commented Feb 6, 2017

brendankenny commented Feb 7, 2017

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

brendankenny Feb 7, 2017 • edited

Choose a reason for hiding this comment

brendankenny commented Feb 7, 2017

ebidel commented Feb 7, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brendankenny Feb 7, 2017 • edited

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 Feb 7, 2017

ebidel commented Feb 7, 2017

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

ebidel commented Feb 8, 2017

ebidel commented Feb 9, 2017

paulirish commented Feb 9, 2017 • edited

brendankenny Feb 7, 2017 •

edited

brendankenny Feb 7, 2017 •

edited

paulirish commented Feb 9, 2017 •

edited