core(runner): split lhr, artifacts return, respect output type #4999

patrickhulce · 2018-04-19T21:20:16Z

jumped on @brendankenny's suggestion to make {lhr, artifacts} and a breaking feature in the process (#4333), having node module respect the output=html setting

also I had a bug in config with default settings when undefined (mostly happens in tests, but could be user explicitly passing undefined) so fixing that was good 👍

brendankenny

probably easier to still have the report generated outside of runner so that the devtools bundle doesn't need it (arguably cleaner that way than pruning via browserify, otoh doing it the other way moves us closer to the various background scripts just using the node interface)

also the lighthouse-background side will currently be broken

brendankenny · 2018-04-19T21:31:13Z

also, FWIW, "core(runner): respect output setting" is definitely not the most important change happening here :P

patrickhulce · 2018-04-19T21:48:54Z

also, FWIW, "core(runner): respect output setting" is definitely not the most important change happening here :P

what's the most important change?

probably easier to still have the report generated outside of runner so that the devtools bundle doesn't need it

I see, so against the feature existing or just proposing index.js?

patrickhulce · 2018-04-19T21:49:41Z

also the lighthouse-background side will currently be broken

yes this was meant to be proposal, added [PROPOSAL]

paulirish · 2018-04-19T23:04:01Z

One option: add browserify ignore for lib/format-output.. and always check if it's truth before using it. But I should look closer before jumping to a solution

brendankenny · 2018-04-19T23:05:09Z

what's the most important change?

splitting up the return value from the module :)

I see, so against the feature existing or just proposing index.js?

oh, I think 👍 👍, just index.js or have to do something about the bundling

paulirish

seems like you can add '../report/v2/report-generator' to browserify ignore, and we should be good.

and update the lh-bg files to request their intended output

paulirish · 2018-04-20T00:14:42Z

lighthouse-core/index.js

@@ -46,10 +46,12 @@ function lighthouse(url, flags = {}, configJSON) {
    // kick off a lighthouse run
    return Runner.run(connection, {url, config})
      .then((lighthouseResults = {}) => {
+        lighthouseResults.lhr = lighthouseResults.lhr || {};


rename to runnerResult

paulirish · 2018-04-20T00:16:05Z

lighthouse-cli/run.js

-      const artifacts = results.artifacts;
-      delete results.artifacts;
-      return saveResults(results, artifacts, flags).then(_ => results);
+      return saveResults(results, flags).then(_ => results);


rename results to runnerResult

paulirish · 2018-04-20T00:16:08Z

lighthouse-cli/run.js

 * @param {!LH.Flags} flags
 * @return {Promise<void>}
 */
-function saveResults(results, artifacts, flags) {
+function saveResults(results, flags) {
+  const {lhr, artifacts} = results;


rename results to runnerResult

paulirish · 2018-04-20T00:16:25Z

lighthouse-core/runner.js

@@ -17,16 +17,15 @@ const fs = require('fs');
 const path = require('path');
 const URL = require('./lib/url-shim');
 const Sentry = require('./lib/sentry');
+const getFormattedReport = require('./lib/format-output').createOutput;


nearly all our source files are defined as nouns, so we should do lhr-formatter.js or something.

paulirish

I'm +1 on the proposal.

brendankenny · 2018-04-20T00:21:28Z

seems like you can add '../report/v2/report-generator' to browserify ignore, and we should be good.

extension needs it, though, it just (currently) uses it in lighthouse-ext-background.js, so would need to split on builds

brendankenny · 2018-04-20T00:23:54Z

lighthouse-core/lib/format-output.js

@@ -0,0 +1,72 @@
+/**


live under report/?

patrickhulce · 2018-04-20T20:08:42Z

aight folks ready for re-review!

brendankenny

implementation looks great! But bikeshedding...

do we want to refer to the final report string as the formattedReport instead of the generatedReport? That makes a certain amount of sense, but I'd like to get rid of the one we don't use because they sound very similar (e.g. if we use formattedReport, there shouldn't be a report-generator.js :)
one way of doing this would be to merge report-formatter.js and report-generator.js in this PR, a single file that makes reports, but kick out the fs.readFileSync() calls and reportJs/reportCss/reportTemplates getters to a different file that's just like html-report-assets.js or whatever and we can browserify.ignore() that file

brendankenny · 2018-04-23T18:50:10Z

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

+  /**
+   * Creates the results output in a format based on the `mode`.
+   * @param {LH.Result} lhr
+   * @param {string} outputMode


'html'|'csv'|'json'?

patrickhulce · 2018-04-23T20:02:52Z

do we want to refer to the final report string as the formattedReport instead of the generatedReport? That makes a certain amount of sense, but I'd like to get rid of the one we don't use because they sound very similar (e.g. if we use formattedReport, there shouldn't be a report-generator.js

resolved on .report 🎉

one way of doing this would be to merge report-formatter.js and report-generator.js in this PR, a single file that makes reports, but kick out the fs.readFileSync() calls and reportJs/reportCss/reportTemplates getters to a different file that's just like html-report-assets.js or whatever and we can browserify.ignore() that file

great idea, done! 👍

brendankenny

I love this

ebidel · 2018-04-24T00:06:18Z

It's a party.

…eChrome#4999)

core(runner): respect output setting

6c6b0b1

patrickhulce requested review from brendankenny and paulirish as code owners April 19, 2018 21:20

devtools-bot added the waiting4reviewer label Apr 19, 2018

brendankenny reviewed Apr 19, 2018

View reviewed changes

patrickhulce changed the title ~~core(runner): respect output setting~~ [WIP] core(runner): respect output setting Apr 19, 2018

patrickhulce changed the title ~~[WIP] core(runner): respect output setting~~ [PROPOSAL] core(runner): respect output setting Apr 19, 2018

patrickhulce changed the title ~~[PROPOSAL] core(runner): respect output setting~~ [PROPOSAL] core(runner): split lhr, artifacts return, respect output type Apr 19, 2018

paulirish requested changes Apr 20, 2018

View reviewed changes

paulirish reviewed Apr 20, 2018

View reviewed changes

brendankenny reviewed Apr 20, 2018

View reviewed changes

lighthouse-core/lib/format-output.js Outdated

@@ -0,0 +1,72 @@

/**

Copy link

Member

brendankenny Apr 20, 2018

Choose a reason for hiding this comment

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

live under report/?

patrickhulce added 5 commits April 20, 2018 09:48

feedback

774d3d3

feedback

f6e9dbc

Merge branch 'master' into move_output_to_core

e194afc

revert unnecessary

c7a5aec

move timing to runner

23cf111

patrickhulce requested review from ebidel and vinamratasingal-zz as code owners April 20, 2018 18:10

patrickhulce changed the title ~~[PROPOSAL] core(runner): split lhr, artifacts return, respect output type~~ core(runner): split lhr, artifacts return, respect output type Apr 20, 2018

paulirish mentioned this pull request Apr 20, 2018

[meta] 2018 I/O Burndown #5008

Closed

82 tasks

patrickhulce added the I/O-Expedite label Apr 23, 2018

brendankenny requested changes Apr 23, 2018

View reviewed changes

feedback

ced2c2c

paulirish approved these changes Apr 23, 2018

View reviewed changes

patrickhulce assigned brendankenny Apr 23, 2018

brendankenny approved these changes Apr 23, 2018

View reviewed changes

patrickhulce merged commit 711ca6e into master Apr 23, 2018

patrickhulce deleted the move_output_to_core branch April 23, 2018 23:55

brendankenny mentioned this pull request Apr 24, 2018

report: move html report from v2/ dir to html/ #5034

Merged

paulirish removed the waiting4reviewer label Apr 30, 2018

kdzwinel pushed a commit to kdzwinel/lighthouse that referenced this pull request Aug 16, 2018

core(runner): split lhr, artifacts return, respect output type (Googl…

1982548

…eChrome#4999)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

core(runner): split lhr, artifacts return, respect output type #4999

core(runner): split lhr, artifacts return, respect output type #4999

patrickhulce commented Apr 19, 2018

brendankenny left a comment

brendankenny commented Apr 19, 2018

patrickhulce commented Apr 19, 2018

patrickhulce commented Apr 19, 2018 •

edited

Loading

paulirish commented Apr 19, 2018

brendankenny commented Apr 19, 2018

paulirish left a comment

paulirish Apr 20, 2018

patrickhulce Apr 20, 2018

paulirish Apr 20, 2018

patrickhulce Apr 20, 2018

paulirish Apr 20, 2018

patrickhulce Apr 20, 2018

paulirish Apr 20, 2018

paulirish left a comment

brendankenny commented Apr 20, 2018

brendankenny Apr 20, 2018

patrickhulce commented Apr 20, 2018

brendankenny left a comment

brendankenny Apr 23, 2018

patrickhulce Apr 23, 2018

patrickhulce commented Apr 23, 2018

brendankenny left a comment

ebidel commented Apr 24, 2018

core(runner): split lhr, artifacts return, respect output type #4999

core(runner): split lhr, artifacts return, respect output type #4999

Conversation

patrickhulce commented Apr 19, 2018

brendankenny left a comment

Choose a reason for hiding this comment

brendankenny commented Apr 19, 2018

patrickhulce commented Apr 19, 2018

patrickhulce commented Apr 19, 2018 • edited Loading

paulirish commented Apr 19, 2018

brendankenny commented Apr 19, 2018

paulirish 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

paulirish left a comment

Choose a reason for hiding this comment

brendankenny commented Apr 20, 2018

Choose a reason for hiding this comment

patrickhulce commented Apr 20, 2018

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

patrickhulce commented Apr 23, 2018

brendankenny left a comment

Choose a reason for hiding this comment

ebidel commented Apr 24, 2018

patrickhulce commented Apr 19, 2018 •

edited

Loading