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

client(lr): add lightrider report-generator #8197

Merged
merged 14 commits into from
Apr 16, 2019
70 changes: 70 additions & 0 deletions build/build-lightrider.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,70 @@
/**
* @license Copyright 2019 Google Inc. All Rights Reserved.
* Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except in compliance with the License. You may obtain a copy of the License at http://www.apache.org/licenses/LICENSE-2.0
* Unless required by applicable law or agreed to in writing, software distributed under the License is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the License for the specific language governing permissions and limitations under the License.
*/
'use strict';

const browserify = require('browserify');
const cpy = require('cpy');
const fs = require('fs');
const path = require('path');
const makeDir = require('make-dir');
const bundleBuilder = require('./build-bundle.js');

const LHROOT = path.join(__dirname, '..');
const distDir = path.join(__dirname, '..', 'dist', 'lightrider');
const sourceDir = __dirname + '/../clients/lightrider';

const bundleOutFile = `${distDir}/report-generator.js`;
exterkamp marked this conversation as resolved.
Show resolved Hide resolved
const generatorFilename = `./lighthouse-core/report/report-generator.js`;

const entrySourceName = 'lightrider-entry.js';
const entryDistName = 'lighthouse-lr-bundle.js';

makeDir.sync(path.dirname(distDir));

/**
* @return {Promise<void>}
*/
async function copyAssets() {
return cpy([
'clients/lightrider/lightrider-ui-features.js',
'lighthouse-core/lib/file-namer.js',
Copy link
Member

Choose a reason for hiding this comment

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

do you also want to copy all the other renderer assets LR has copied out manually right now? Or wait on this because you don't want to rewrite the matching bash script on the other side :)

Copy link
Member Author

Choose a reason for hiding this comment

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

I think this works for now, because that whole file portion is copied wholesale. Simplifying all of this syncing should def be a bug to track.

], distDir, {
cwd: LHROOT,
parents: false,
});
}

/**
* Browserify and minify entry point.
*/
function buildEntryPoint() {
const inFile = `${sourceDir}/${entrySourceName}`;
const outFile = `${distDir}/${entryDistName}`;
return bundleBuilder.build(inFile, outFile);
}

/**
* Browserify and minify the LR report generator.
*/
function buildReportGenerator() {
browserify(generatorFilename, {standalone: 'ReportGenerator'})
// Transform the fs.readFile etc into inline strings.
.transform('brfs', {global: true, parserOpts: {ecmaVersion: 10}})
.bundle((err, src) => {
if (err) throw err;
fs.writeFileSync(bundleOutFile, src.toString());
});
}

async function run() {
await Promise.all([
buildEntryPoint(),
copyAssets(),
buildReportGenerator(),
]);
}

run();
Original file line number Diff line number Diff line change
Expand Up @@ -5,19 +5,19 @@
*/
'use strict';

const lighthouse = require('../lighthouse-core/index.js');
const lighthouse = require('../../lighthouse-core/index.js');

const assetSaver = require('../lighthouse-core/lib/asset-saver.js');
const LHError = require('../lighthouse-core/lib/lh-error.js');
const preprocessor = require('../lighthouse-core/lib/proto-preprocessor.js');
const assetSaver = require('../../lighthouse-core/lib/asset-saver.js');
const LHError = require('../../lighthouse-core/lib/lh-error.js');
const preprocessor = require('../../lighthouse-core/lib/proto-preprocessor.js');

/** @type {Record<'mobile'|'desktop', LH.Config.Json>} */
const LR_PRESETS = {
mobile: require('../lighthouse-core/config/lr-mobile-config.js'),
desktop: require('../lighthouse-core/config/lr-desktop-config.js'),
mobile: require('../../lighthouse-core/config/lr-mobile-config.js'),
desktop: require('../../lighthouse-core/config/lr-desktop-config.js'),
};

/** @typedef {import('../lighthouse-core/gather/connections/connection.js')} Connection */
/** @typedef {import('../../lighthouse-core/gather/connections/connection.js')} Connection */

/**
* Run lighthouse for connection and provide similar results as in CLI.
Expand Down
25 changes: 25 additions & 0 deletions clients/lightrider/lightrider-ui-features.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
/**
* Copyright 2019 Google Inc. All Rights Reserved.
Copy link
Collaborator

Choose a reason for hiding this comment

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

* Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except in compliance with the License. You may obtain a copy of the License at http://www.apache.org/licenses/LICENSE-2.0
* Unless required by applicable law or agreed to in writing, software distributed under the License is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the License for the specific language governing permissions and limitations under the License.
*/
'use strict';

/* global ReportUIFeatures */

/**
* Extends ReportUIFeatures to use the saved report from a browserified ReportGenerator.
Copy link
Collaborator

Choose a reason for hiding this comment

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

"saved report" meaning the generic report assets, right? not like a saved report instance of a particular run that's somehow bundled in a way I don't know about :)

Copy link
Member Author

Choose a reason for hiding this comment

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

Uh I think I get what you're asking? It is just me trying to say that this is used to get report HTML via a report generator and this.json instead of outerHTML like a reg report.

*/
class LightriderUIFeatures extends ReportUIFeatures {
/**
* Uses ReportGenerator to create the html that recreates this report.
* @return {string}
* @override
*/
getReportHtml() {
// @ts-ignore This is only called in browser in Lightrider where this exists.
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not sure I understand this comment. It's called in the browser as in the WRS devtools script? Or it's called in the browser as-in PSI front-end?

Copy link
Member Author

Choose a reason for hiding this comment

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

It's called in the browser as-in something similar to the PSI front-end.

This ui-features is packaged with a report-generator for anyone trying to render a report internally on a front-end. In order to do that they need this overrided getReportHtml since we can't just outerHTML.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ahhhhhh OK, the "in Lightrider" made that a bit confusing for me :)

this is happening in the consumers of the LR response just like in DevTools

exterkamp marked this conversation as resolved.
Show resolved Hide resolved
return window.ReportGenerator.generateReportHtml(this.json);
Copy link
Member

Choose a reason for hiding this comment

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

in the long term i'm interested in changing ReportUIFeatures.getReportHtml() to be this instead. and we'd always ship reportgenerator inside of the HTML report.

but that requires some work. so it'll wait.

}
}

module.exports = LightriderUIFeatures;
2 changes: 1 addition & 1 deletion clients/test/lightrider-entry-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@
'use strict';

const assert = require('assert');
const lhBackground = require('../lightrider-entry.js');
const lhBackground = require('../lightrider/lightrider-entry.js');
const Runner = require('../../lighthouse-core/runner.js');
const LHError = require('../../lighthouse-core/lib/lh-error.js');

Expand Down
10 changes: 1 addition & 9 deletions lighthouse-core/lib/file-namer.js
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@
* @return {string}
*/
function getFilenamePrefix(lhr) {
const hostname = new (getUrlConstructor())(lhr.finalUrl).hostname;
const hostname = new URL(lhr.finalUrl).hostname;
patrickhulce marked this conversation as resolved.
Show resolved Hide resolved
const date = (lhr.fetchTime && new Date(lhr.fetchTime)) || new Date();

const timeStr = date.toLocaleTimeString('en-US', {hour12: false});
Expand All @@ -36,14 +36,6 @@ function getFilenamePrefix(lhr) {
return filenamePrefix.replace(/[/?<>\\:*|":]/g, '-');
}

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

// don't attempt to export in the browser.
if (typeof module !== 'undefined' && module.exports) {
module.exports = {getFilenamePrefix};
Expand Down
4 changes: 2 additions & 2 deletions lighthouse-core/report/report-generator.js
Original file line number Diff line number Diff line change
Expand Up @@ -57,14 +57,14 @@ class ReportGenerator {
* - the score value of the audit
*
* @param {LH.Result} lhr
* @returns {string}
* @return {string}
Copy link
Member

Choose a reason for hiding this comment

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

👍

*/
static generateReportCSV(lhr) {
// To keep things "official" we follow the CSV specification (RFC4180)
// The document describes how to deal with escaping commas and quotes etc.
const CRLF = '\r\n';
const separator = ',';
/** @param {string} value @returns {string} */
/** @param {string} value @return {string} */
const escape = value => `"${value.replace(/"/g, '""')}"`;

// Possible TODO: tightly couple headers and row values
Expand Down
2 changes: 1 addition & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@
"build-all:task:windows": "yarn build-extension && yarn build-devtools && yarn build-lr && yarn build-viewer",
"build-extension": "node ./build/build-extension.js",
"build-devtools": "node ./build/build-bundle.js clients/devtools-entry.js dist/lighthouse-dt-bundle.js && node ./build/build-dt-report-resources.js",
"build-lr": "node ./build/build-bundle.js clients/lightrider-entry.js dist/lighthouse-lr-bundle.js",
"build-lr": "node ./build/build-lightrider.js",
exterkamp marked this conversation as resolved.
Show resolved Hide resolved
"build-viewer": "node ./build/build-viewer.js",
"clean": "rimraf dist proto/scripts/*.json proto/scripts/*_pb2.* proto/scripts/*_pb.* proto/scripts/__pycache__ proto/scripts/*.pyc *.report.html *.report.dom.html *.report.json *.devtoolslog.json *.trace.json || true",
"lint": "[ \"$CI\" = true ] && eslint --quiet -f codeframe . || eslint .",
Expand Down