Simplify report generation: no pretty print, no formatter classes #1838

brendankenny · 2017-03-08T22:58:55Z

This ended up being something of a monster since formatters have grown to touch so many parts of the codebase. This removes formatters completely, setting the stage for much simpler report refactors in the future.

The only remaining "formatting" is done with handlebar partials, and all that functionality now lives entirely with lighthouse-core/report/. The only thing any code outside that directory needs to know about formatting is the string specified by audits in their extendedInfo.formatter results (this hasn't changed at all from before, still on the Formatter.SUPPORTED_FORMATS object).

The individual commits stand on their own, but the last moves enough around that I'm not sure if going through them individually really helps with review. As mentioned in the commit messages, the PR:

removes pretty printing on the command line (fixes Is it time to deprecate the CLI report output? #1739)
simplifies the --output and --output-path options a bit since we got rid of pretty (see bin.ts/readme changes for details)
moves all handlebar helpers from getHelpers() in individual formatter files to the central handlebar-helpers.js. This is somewhat questionable organizationally, but they all live in a global namespace when they're registered anyways, and having them in one place will aid with future refactoring.
move partial html and css files from formatters/partials/ to report/partials/
eliminates the Formatter subclasses for each formatter since after Adopt handlebars precompiled templates #1752 the partial sources are retrieved from the precompiled partials anyway and with all the changes from above, they no longer have a use
keep formatter unit tests but instead have them compile and test the html produced by the partials directly

In addition, Formatter.SUPPORTED_FORMATS is now auto-generated from the available partial files and report-generator.js now throws on unknown formatters, so it should cut down on issues like #1820

Tested in CLI and extension, no visible changes

brendankenny · 2017-03-08T23:02:06Z

lighthouse-core/audits/critical-request-chains.js

+   * Get stats about the longest initiator chain (as determined by time duration)
+   * @return {{duration: number, length: number, transferSize: number}}
+   */
+  static _getLongestChain(tree) {


move from the formatter to the audit. I think it was only in the formatter because we didn't have extendedInfo when this was first written

brendankenny · 2017-03-08T23:03:20Z

lighthouse-core/audits/critical-request-chains.js

-          value: chains
+          value: {
+            chains,
+            longestChain: CriticalRequestChains._getLongestChain(chains)


the change to the CriticalRequestChains extendedInfo is the only real externally-noticeable change. Added longestChain since it's used several times in the report, so no reason not to just precompute it

patrickhulce

death to the pretty print, long live the age of the new report! 💯 🥇

patrickhulce · 2017-03-08T23:03:21Z

lighthouse-core/audits/critical-request-chains.js

@@ -40,6 +40,58 @@ class CriticalRequestChains extends Audit {
    };
  }

+  static _traverse(tree, cb) {


did this used to live in the formatter?

already answered, ignore

patrickhulce · 2017-03-08T23:05:31Z

lighthouse-core/report/formatter.js

+fs.readdirSync(__dirname + '/partials')
+  .filter(filename => filename.endsWith('.html'))
+  .forEach(filename => {
+    const baseName = filename.replace(/\.html$/, '');


all of this is still lines up with eric's fix?

not entirely, it changes somewhat, but it is all working.

I moved us from three possible cases (caps for the SUPPORTED_FORMATS name, camel for the actual value of that variable, and kebab for the partial name, so we had to convert from camel to kebab to map from formatter to partial), to just caps and kebab (so the value of the formatter property is the same as the name of the partial)

so the value of the formatter property is the same as the name of the partial

(and that's now checked by a test)

Tested the PR and everything looks good.

patrickhulce · 2017-03-08T23:06:51Z

lighthouse-core/report/report-generator.js

+      const partialName = audit.extendedInfo.formatter;
+      const partial = reportPartials.report.partials[partialName];
+      if (!partial) {
+        throw new Error(`${auditName} requested unknown partial for formatting`);


paulirish · 2017-03-09T03:31:56Z

lighthouse-core/report/formatter.js

+   * (e.g. `CRITICAL_REQUEST_CHAINS` to `critical-request-chains).
+   * @return {!Object<string, string>}
+   */
+  static get SUPPORTED_FORMATS() {


does this need to be in caps?

does this need to be in caps?

no, I just left it how it was before so that audits don't have to change the formatter: Formatter.SUPPORTED_FORMATS.USER_TIMINGS (or whatever) line in extendedInfo.

Selecting the partial to use is a little awkward in general. It could also have been a string (like e.g requiredArtifacts is) since that's all that variable really is, but there is the advantage of if your editor does support code completion you get suggestions there. The SUPPORTED_FORMATS could have lived somewhere else, too (like on ReportGenerator).

Basically I didn't see an option that was legitimately better, so I just left it the same until we do come up with something :)

ebidel · 2017-03-09T23:39:26Z

readme.md

@@ -34,7 +34,7 @@ Kick off a run by passing `lighthouse` the URL to audit:
 lighthouse https://airhorner.com/
 ```

-Lighthouse will prettyprint a report to CLI. You can control the output format by passing flags.
+Lighthouse will write an HTML report to file. You can control the output format by passing flags.


"write the report to an HTML file"?

"write the report to an HTML file"?

I mean, the report is the html, it's not writing it to an HTML file...I don't know.

wording was a bit funny to me. "write your report to a file..."?

I think the point of the original statement is to point out what you get by default and call attention to the fact that there are flags to change that.

Agree it is a bit awkward. "Lighthouse will write an HTML report to file by default." to call attention to the fact that it's talking about the default behavior?

Also we could delete the line :)

"write an HTML report to file" sounds incomplete. "to a file", "to an HTML file"....

What about " By default, Lighthouse writes the report to an HTML file. You can control the output format by passing flags."

ebidel · 2017-03-09T23:39:49Z

readme.md

@@ -66,8 +66,13 @@ Configuration:

 Output:
  --output       Reporter for the results, supports multiple values
-                         [choices: "none", "pretty", "json", "html"]               [default: "none"]
-  --output-path  The file path to output the results
+                                    [choices: "json", "html"]                    [default: "none"]


[default: "html"] right?

[default: "html"] right?

whoops, yes. done.

ebidel · 2017-03-09T23:43:05Z

lighthouse-core/report/handlebar-helpers.js

- * Figures out the total score for an aggregation
- * @param {{total: number}} aggregation
- * @return {number}
+ * Format number.


formatNumber formaters a number...

formatNumber formaters a number

I got less creative as I went along and just stole it from the also excellent // format time

ebidel · 2017-03-09T23:46:54Z

lighthouse-core/report/handlebar-helpers.js

@@ -161,7 +195,7 @@ const handlebarHelpers = {
   * @param {!Object} opts
   * @return {string}
   */
-  createTreeRenderContext(tree, opts) {
+  createTreeRenderContext(tree = {}, opts) {


doc the default val?

this was to work around a wonky unit test in development, but no longer necessary \o/ removed.

ebidel · 2017-03-09T23:49:32Z

lighthouse-core/report/formatter.js

+fs.readdirSync(__dirname + '/partials')
+  .filter(filename => filename.endsWith('.html'))
+  .forEach(filename => {
+    const baseName = filename.replace(/\.html$/, '');


Tested the PR and everything looks good.

ebidel · 2017-03-09T23:51:04Z

lighthouse-core/report/report-generator.js

 const fs = require('fs');
 const path = require('path');

-function toKebabCase(string) {


expand critical request chain extendedInfo so less is computed in its formatter

brendankenny · 2017-03-10T04:36:49Z

merged! looking at the open PRs, I only see #1847 and #1840 needing to be updated after this, and that should just require const Formatter = require('../formatters/formatter'); -> const Formatter = require('../report/formatter');

paulirish · 2017-03-11T00:21:07Z

@brendankenny thanks for checking! i've updated those PRs

brendankenny changed the title ~~Simplify report generation -- no pretty print, no formatter classes~~ Simplify report generation: no pretty print, no formatter classes Mar 8, 2017

brendankenny commented Mar 8, 2017

View reviewed changes

patrickhulce reviewed Mar 8, 2017

View reviewed changes

paulirish approved these changes Mar 9, 2017

View reviewed changes

ebidel suggested changes Mar 9, 2017

View reviewed changes

ebidel approved these changes Mar 9, 2017

View reviewed changes

brendankenny force-pushed the noformatter branch from 1bdcde4 to d4b612d Compare March 10, 2017 00:19

brendankenny added 5 commits March 9, 2017 16:34

remove pretty printing from CLI

fe908c1

doc and organize handlebars helpers

9dce18c

centralize all handlebar helper methods

42fcbea

expand critical request chain extendedInfo so less is computed in its formatter

remove pretty formatter from formatters

6e5be5e

remove formatters

79ff1b0

brendankenny force-pushed the noformatter branch from d4b612d to 79ff1b0 Compare March 10, 2017 00:34

brendankenny merged commit b5afd60 into master Mar 10, 2017

brendankenny deleted the noformatter branch March 10, 2017 04:33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Simplify report generation: no pretty print, no formatter classes #1838

Simplify report generation: no pretty print, no formatter classes #1838

brendankenny commented Mar 8, 2017

brendankenny Mar 8, 2017

brendankenny Mar 8, 2017

patrickhulce left a comment

patrickhulce Mar 8, 2017

patrickhulce Mar 8, 2017

patrickhulce Mar 8, 2017

brendankenny Mar 9, 2017

brendankenny Mar 9, 2017

ebidel Mar 9, 2017

patrickhulce Mar 8, 2017

paulirish Mar 9, 2017

brendankenny Mar 9, 2017

ebidel Mar 9, 2017

brendankenny Mar 10, 2017

ebidel Mar 10, 2017

brendankenny Mar 10, 2017

ebidel Mar 10, 2017

brendankenny Mar 10, 2017

ebidel Mar 9, 2017

brendankenny Mar 10, 2017

ebidel Mar 9, 2017

brendankenny Mar 10, 2017 •

edited

ebidel Mar 9, 2017

brendankenny Mar 10, 2017

ebidel Mar 9, 2017

ebidel Mar 9, 2017

brendankenny commented Mar 10, 2017

paulirish commented Mar 11, 2017

Simplify report generation: no pretty print, no formatter classes #1838

Simplify report generation: no pretty print, no formatter classes #1838

Conversation

brendankenny commented Mar 8, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

patrickhulce 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

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 Mar 10, 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

brendankenny commented Mar 10, 2017

paulirish commented Mar 11, 2017

brendankenny Mar 10, 2017 •

edited