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

Report v2: check/x boolean audits, expand details failing audits, optimal val #1963

Merged
merged 10 commits into from
Apr 7, 2017
Merged
Show file tree
Hide file tree
Changes from 7 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
1 change: 0 additions & 1 deletion .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,6 @@ coverage

lighthouse-cli/results
lighthouse-cli/performance-experiment/experiment-database/experiment-data*
results
results.html
last-run-results.html

Expand Down
13 changes: 12 additions & 1 deletion docs/hacking-tips.md
Original file line number Diff line number Diff line change
Expand Up @@ -13,9 +13,20 @@ Use [`--trace-warnings`](https://medium.com/@jasnell/introducing-process-warning
node --trace-warnings lighthouse-cli http://example.com
```

## Updating fixture dumps

`lighthouse-core/test/results/samples_v2.json` is generated from running LH against
dbw_tester.html. To update this file, run:

```sh
npm run start -- --output=json --output-path=lighthouse-core/test/results/sample_v2.json http://localhost:8080/dobetterweb/dbw_tester.html
```
Copy link
Member

Choose a reason for hiding this comment

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

should we add something about consider deleting any irrelevant changes from the diff, like exact timings, etc?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done


After updating, consider deleting any irrelevant changes from the diff (exact timings, timestamps, etc). Be sure to run the tests.

## Iterating on the v2 report

This'll generate new reports from the same results json.
This will generate new reports from the same results json.
Copy link
Member

Choose a reason for hiding this comment

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

harsh


```sh
# capture some results first:
Expand Down
11 changes: 7 additions & 4 deletions lighthouse-core/report/v2/report-generator.js
Original file line number Diff line number Diff line change
Expand Up @@ -17,12 +17,12 @@
'use strict';

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

const REPORT_TEMPLATE = fs.readFileSync(path.join(__dirname, './report-template.html'), 'utf8');
const REPORT_TEMPLATE = fs.readFileSync(__dirname + '/report-template.html', 'utf8');
// TODO: Setup a gulp pipeline to concat and minify the renderer files?
const REPORT_JAVASCRIPT = fs.readFileSync(path.join(__dirname, './report-renderer.js'), 'utf8');
const REPORT_CSS = fs.readFileSync(path.join(__dirname, './report-styles.css'), 'utf8');
const REPORT_JAVASCRIPT = fs.readFileSync(__dirname + '/report-renderer.js', 'utf8');
const REPORT_CSS = fs.readFileSync(__dirname + '/report-styles.css', 'utf8');
const REPORT_TEMPLATES = fs.readFileSync(__dirname + '/templates.html', 'utf8');

class ReportGeneratorV2 {
/**
Expand Down Expand Up @@ -97,11 +97,14 @@ class ReportGeneratorV2 {
generateReportHtml(reportAsJson) {
const sanitizedJson = JSON.stringify(reportAsJson).replace(/</g, '\\u003c');
const sanitizedJavascript = REPORT_JAVASCRIPT.replace(/<\//g, '\\u003c/');
// Remove script in templates files just to be safe.
const sanitizedTemplates = REPORT_TEMPLATES.replace(/<script>([\s\S]*?)<\/script>/g, '');
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure how much we should emphasize this; it doesn't strip event handlers, for instance. It's all our own markup, so I vote for not worrying about sanitization of the templates (we don't sanitize report-template.html so why should we do that with these? :)

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 agree with you. I recall that el.innerHTML = '<div>just a string</div>'; was deemed unsafe and that's our own markup :) The argument from someonnnnne was that we may add things later and not realize the mistakes.

Copy link
Member

Choose a reason for hiding this comment

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

I think this is different enough, though, because it's not markup assembled in (potentially) multiple places, it's all in one spot. So I think it's better on that front. I also don't like calling it sanitized unless it is actually sanitized :)

Copy link
Member

Choose a reason for hiding this comment

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

really think we should remove this, or call it "templatesWithScriptTagsRemoves" or something. Current version is easily worked around with an attribute on the script tag...I don't think this is an arms race we want to even start.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done


return ReportGeneratorV2.replaceStrings(REPORT_TEMPLATE, [
{search: '%%LIGHTHOUSE_JSON%%', replacement: sanitizedJson},
{search: '%%LIGHTHOUSE_JAVASCRIPT%%', replacement: sanitizedJavascript},
{search: '/*%%LIGHTHOUSE_CSS%%*/', replacement: REPORT_CSS},
{search: '%%LIGHTHOUSE_TEMPLATES%%', replacement: sanitizedTemplates},
]);
}
}
Expand Down
200 changes: 143 additions & 57 deletions lighthouse-core/report/v2/report-renderer.js
Original file line number Diff line number Diff line change
Expand Up @@ -15,26 +15,31 @@
*/
'use strict';

/**
* @fileoverview The entry point for rendering the Lighthouse report based on the JSON output.
* This file is injected into the report HTML along with the JSON report.
*
* Dummy text for ensuring report robustness: </script> pre$`post %%LIGHTHOUSE_JSON%%
*/

/* eslint-env browser */
/* eslint indent: [2, 2, { "SwitchCase": 1, "outerIIFEBody": 0 }] */

(function() {
const RATINGS = {
GOOD: {label: 'good', minScore: 75},
PASS: {label: 'pass', minScore: 75},
AVERAGE: {label: 'average', minScore: 45},
POOR: {label: 'poor'}
FAIL: {label: 'fail'}
Copy link
Member

Choose a reason for hiding this comment

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

does PASS/FAIL work here? Seems weird to have a three state pass/fail system :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not really a pass/fail system. Just for labeling/coloring right now. We'll probably need to expand in the future if we support things like grades. Seems fine for now.

Copy link
Member

Choose a reason for hiding this comment

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

Not really a pass/fail system. Just for labeling/coloring right now.

yeah, I was mostly just commenting on the names since PASS/FAIL is usually a binary state. Fine for now

};

/**
* Convert a score to a rating label.
* @param {number} value
* @param {number} score
* @return {string}
*/
function calculateRating(value) {
let rating = RATINGS.POOR.label;
if (value >= RATINGS.GOOD.minScore) {
rating = RATINGS.GOOD.label;
} else if (value >= RATINGS.AVERAGE.minScore) {
function calculateRating(score) {
let rating = RATINGS.FAIL.label;
if (score >= RATINGS.PASS.minScore) {
rating = RATINGS.PASS.label;
} else if (score >= RATINGS.AVERAGE.minScore) {
rating = RATINGS.AVERAGE.label;
}
return rating;
Expand All @@ -49,13 +54,7 @@ function formatNumber(number) {
return number.toLocaleString(undefined, {maximumFractionDigits: 1});
}

/**
* @fileoverview The entry point for rendering the Lighthouse report based on the JSON output.
* This file is injected into the report HTML along with the JSON report.
*
* Dummy text for ensuring report robustness: </script> pre$`post %%LIGHTHOUSE_JSON%%
*/
window.ReportRenderer = class ReportRenderer {
class DOM {
Copy link
Member

Choose a reason for hiding this comment

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

feels like something I know ahead of time you're going to disagree with me on :) but I don't personally see the need for a separate DOM class (yet, at least), especially because the split with ReportRenderer doesn't seem completely clear. Anything wrong with these living there?

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 knew you were going to bring this up! I wasn't convinced adding a bunch of generic methods to ReportRenderer made sense either. They could live as standalone helper functions, but devtools may need to extend them later.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

removed

/**
* @param {!Document} document
*/
Expand All @@ -64,15 +63,31 @@ window.ReportRenderer = class ReportRenderer {
}

/**
* @param {!ReportJSON} report
* @return {!Element}
* Sets the text content of an element. If the element does not exist,
* results in a noop.
* @param {Element} element
* @param {string} text
* @return {Element}
*/
renderReport(report) {
try {
return this._renderReport(report);
} catch (e) {
return this._renderException(e);
static setText(element, text) {
if (element) {
Copy link
Member

Choose a reason for hiding this comment

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

if we're worried about these, should they just throw? Seems like it's definitely an error since we're creating the templates and the selectors looking within them

Copy link
Contributor Author

@ebidel ebidel Apr 6, 2017

Choose a reason for hiding this comment

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

@paulirish asked for them so that the report fails gracefully if a node cant be found. My pref would be to remove them altogether. Tests should really be catching missing nodes in templates.html (markup we control). It would also get rid of the DOM and simplify render methods a bit if we just set things directly (.classList, .textContent).

I'm also ok keeping.

Copy link
Member

Choose a reason for hiding this comment

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

@paulirish asked for them so that the report fails gracefully if a node cant be found.

I think @paulirish was asking before we had completely settled on the approach used here, so it may be less of an issue. Queries failing fast (as soon as we try to set textContent or add to the classList) is a good thing, I think. I also like the idea of moving some of the query verification to tests, but if setting element.textContent throws a TypeError, just testing e.g. _renderAuditScore will verify the relevant elements exist if it doesn't throw :)

Copy link
Member

Choose a reason for hiding this comment

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

still think we should remove. Earlier (and non-silent) failures are better failures.

Copy link
Contributor Author

@ebidel ebidel Apr 7, 2017

Choose a reason for hiding this comment

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

@paulirish says, "I am low stress on this. Do whateva :)" I'll remove the methods.

Note to future me: give PI 24hrs and he will be low stress.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

element.textContent = text;
}
return element;
}

/**
* Adds a class to an element. If the element does not exist, results
* in a noop.
* @param {Element} element
* @param {...string} classNames
* @return {Element}
*/
static addClass(element, ...classNames) {
if (element) {
element.classList.add(...classNames);
}
return element;
}

/**
Expand All @@ -81,38 +96,103 @@ window.ReportRenderer = class ReportRenderer {
* @param {!Object<string, string>=} attrs Attribute key/val pairs.
* @return {!Element}
*/
_createElement(name, className, attrs = {}) {
createElement(name, className, attrs = {}) {
const element = this._document.createElement(name);
if (className) {
element.className = className;
}
for (const [key, val] of Object.entries(attrs)) {
element.setAttribute(key, val);
}
Object.keys(attrs).forEach(key => {
Copy link
Member

Choose a reason for hiding this comment

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

I knew we'd be running this in node before too long :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

+1

element.setAttribute(key, attrs[key]);
});
return element;
}

/**
* @param {string} selector
* @return {!DocumentFragment} A clone of the template content.
Copy link
Member

Choose a reason for hiding this comment

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

should this be !Node (though we're kind of assuming it's an !Element?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Technically a doc frag, which is not a full element. We use it like an !Element though.

Copy link
Member

Choose a reason for hiding this comment

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

But it isn't just grabbing the content, it also calls document.importNode which appears to return a Node.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

TBH, it's a terrible API and name. importNode() returns the node you import. Since we're importing a DocumentFragment, we get back a Node of that type.

screen shot 2017-04-06 at 12 35 37 pm

Copy link
Member

Choose a reason for hiding this comment

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

importNode() returns the node you import

ah, I see. Should _populateResult take a !DocumentFragment, then?

* @throws {Error}
*/
cloneTemplate(selector) {
const template = this._document.querySelector(selector);
if (!template) {
throw new Error(`Template not found: template${selector}`);
}
return this._document.importNode(template.content, true);
}
}

class ReportRenderer {
/**
* @param {!Document} document
*/
constructor(document) {
this._dom = new DOM(document);
}

/**
* @param {!ReportJSON} report
* @return {!Element}
*/
renderReport(report) {
try {
return this._renderReport(report);
} catch (e) {
return this._renderException(e);
}
}

/**
* @param {!DocumentFragment|Element} element DOM node to populate with values.
Copy link
Member

Choose a reason for hiding this comment

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

this is only ever called with a DocumentFragment, though

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yep, but it support both

Copy link
Member

Choose a reason for hiding this comment

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

{!DocumentFragment|!Element} then

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

* @param {number} score
* @param {string} scoringMode
* @param {string} title
* @param {string} description
* @return {!Element}
*/
_renderScore(score, title, description) {
const element = this._createElement('div', 'lighthouse-score');

const value = element.appendChild(this._createElement('div', 'lighthouse-score__value'));
value.textContent = formatNumber(score);
value.classList.add(`lighthouse-score__value--${calculateRating(score)}`);
_populateResult(element, score, scoringMode, title, description) {
// Fill in the blanks.
const valueEl = element.querySelector('.lighthouse-score__value');
DOM.setText(valueEl, formatNumber(score));
DOM.addClass(valueEl, `lighthouse-score__value--${calculateRating(score)}`,
`lighthouse-score__value--${scoringMode}`);

const column = element.appendChild(this._createElement('div', 'lighthouse-score__text'));
column.appendChild(this._createElement('div', 'lighthouse-score__title')).textContent = title;
column.appendChild(
this._createElement('div', 'lighthouse-score__description')).textContent = description;
DOM.setText(element.querySelector('.lighthouse-score__title'), title);
DOM.setText(element.querySelector('.lighthouse-score__description'), description);

return element;
}

/**
* @param {!AuditJSON} audit
* @return {!Element}
*/
_renderAuditScore(audit) {
const tmpl = this._dom.cloneTemplate('#tmpl-lighthouse-audit-score');

const scoringMode = audit.result.scoringMode;
Copy link
Member

Choose a reason for hiding this comment

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

How we've gotten here has made sense, but it is weird that scoringMode is on audit.result but the score is on audit directly

const description = audit.result.helpText;
let title = audit.result.description;

if (audit.result.displayValue) {
title += `: ${audit.result.displayValue}`;
}
if (audit.result.optimalValue) {
title += ` (target: ${audit.result.optimalValue})`;
}

return this._populateResult(tmpl, audit.score, scoringMode, title, description);
}

/**
* @param {!CategoryJSON} category
* @return {!Element}
*/
_renderCategoryScore(category) {
Copy link
Member

Choose a reason for hiding this comment

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

should these also be _renderAuditResult and _renderCategoryResult?

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'm actually going to change "_populateResult" back to _populateScore. It's populating the score dom with values.

Copy link
Member

Choose a reason for hiding this comment

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

I'm actually going to change _populateResult back to _populateScore. It's populating the score dom with values

my issue was that if someone was going into the codebase to see where audits' titles and descriptions are set, they probably wouldn't think it would be in _populateScore. If the issue is the template's id, that can change too :P

This is all going to change a lot in the near future, so if you feel strongly we can defer

const tmpl = this._dom.cloneTemplate('#tmpl-lighthouse-category-score');
const score = Math.round(category.score);
return this._populateResult(tmpl, score, 'numeric', category.name, category.description);
}

/**
* @param {!DetailsJSON} details
* @return {!Element}
Expand All @@ -135,7 +215,7 @@ window.ReportRenderer = class ReportRenderer {
* @return {!Element}
*/
_renderText(text) {
const element = this._createElement('div', 'lighthouse-text');
const element = this._dom.createElement('div', 'lighthouse-text');
element.textContent = text.text;
return element;
}
Expand All @@ -145,7 +225,7 @@ window.ReportRenderer = class ReportRenderer {
* @return {!Element}
*/
_renderBlock(block) {
const element = this._createElement('div', 'lighthouse-block');
const element = this._dom.createElement('div', 'lighthouse-block');
for (const item of block.items) {
element.appendChild(this._renderDetails(item));
}
Expand All @@ -157,14 +237,14 @@ window.ReportRenderer = class ReportRenderer {
* @return {!Element}
*/
_renderList(list) {
const element = this._createElement('details', 'lighthouse-list');
const element = this._dom.createElement('details', 'lighthouse-list');
if (list.header) {
const summary = this._createElement('summary', 'lighthouse-list__header');
const summary = this._dom.createElement('summary', 'lighthouse-list__header');
summary.textContent = list.header.text;
element.appendChild(summary);
}

const items = this._createElement('div', 'lighthouse-list__items');
const items = this._dom.createElement('div', 'lighthouse-list__items');
for (const item of list.items) {
items.appendChild(this._renderDetails(item));
}
Expand All @@ -177,7 +257,7 @@ window.ReportRenderer = class ReportRenderer {
* @return {!Element}
*/
_renderException(e) {
const element = this._createElement('div', 'lighthouse-exception');
const element = this._dom.createElement('div', 'lighthouse-exception');
element.textContent = String(e.stack);
return element;
}
Expand All @@ -187,7 +267,7 @@ window.ReportRenderer = class ReportRenderer {
* @return {!Element}
*/
_renderReport(report) {
const element = this._createElement('div', 'lighthouse-report');
const element = this._dom.createElement('div', 'lighthouse-report');
for (const category of report.reportCategories) {
element.appendChild(this._renderCategory(category));
}
Expand All @@ -199,8 +279,8 @@ window.ReportRenderer = class ReportRenderer {
* @return {!Element}
*/
_renderCategory(category) {
const element = this._createElement('div', 'lighthouse-category');
element.appendChild(this._renderScore(category.score, category.name, category.description));
const element = this._dom.createElement('div', 'lighthouse-category');
element.appendChild(this._renderCategoryScore(category));
for (const audit of category.audits) {
element.appendChild(this._renderAudit(audit));
}
Expand All @@ -212,19 +292,25 @@ window.ReportRenderer = class ReportRenderer {
* @return {!Element}
*/
_renderAudit(audit) {
const element = this._createElement('div', 'lighthouse-audit');
let title = audit.result.description;
if (audit.result.displayValue) {
title += ': ' + audit.result.displayValue;
}
element.appendChild(this._renderScore(audit.score, title, audit.result.helpText));
if (audit.result.details) {
element.appendChild(this._renderDetails(audit.result.details));
const element = this._dom.createElement('div', 'lighthouse-audit');
element.appendChild(this._renderAuditScore(audit));

// Append audit details to to header section so the entire audit is within one <details>.
const header = element.querySelector('.lighthouse-score__header');
Copy link
Member

Choose a reason for hiding this comment

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

since this depends on markup introduced by _renderAuditScore, move header/details stuff in there?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

if (header) {
Copy link
Member

Choose a reason for hiding this comment

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

in what cases might there not be a header?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it was meant to protect against null querySelector results like methods. But removed since we don't care about that anymore.

header.open = audit.score < 100; // expand failed audits
if (audit.result.details) {
header.appendChild(this._renderDetails(audit.result.details));
}
}

return element;
}
};
})();
}

// Exports
window.ReportRenderer = ReportRenderer;
window.DOM = DOM;
Copy link
Member

Choose a reason for hiding this comment

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

not needed anymore since you removed the IIFE?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Mocha needs it, but I've switched to proper module exports for Node env.


/** @typedef {{type: string, text: string|undefined, header: DetailsJSON|undefined, items: Array<DetailsJSON>|undefined}} */
let DetailsJSON; // eslint-disable-line no-unused-vars
Expand Down
Loading