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

exterkamp · 2019-04-11T22:06:21Z

Summary
Add new build script to clients/ to build a report-generator for LR ~~and override ui-features~~ to allow LR to use new report-generator.

Also change returns -> return in report-generator b/c linter.

…rates a report-generator for lightrider.

brendankenny · 2019-04-11T22:52:23Z

lighthouse-core/report/report-generator.js

@@ -57,14 +57,14 @@ class ReportGenerator {
   *  - the score value of the audit
   *
   * @param {LH.Result} lhr
-   * @returns {string}
+   * @return {string}


clients/lightrider/lightrider-ui-features.js

clients/lightrider/lightrider-file-namer.js

clients/lightrider/lightrider-ui-features.js

build/build-lr-files.js

patrickhulce

maybe when the dust settles, a tincy bit more background on why we're doing this would be helpful :)

I vaguely remember a discussion on a prior PR where we needed this for devtools about how the splitting of report generator would help LR maybe?

patrickhulce · 2019-04-12T03:37:49Z

clients/lightrider/lightrider-ui-features.js

@@ -0,0 +1,25 @@
+/**
+ * Copyright 2019 Google Inc. All Rights Reserved.


patrickhulce · 2019-04-12T03:40:13Z

clients/lightrider/lightrider-ui-features.js

+   * @override
+   */
+  getReportHtml() {
+    // @ts-ignore This is only called in browser in Lightrider where this exists.


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?

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.

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

patrickhulce · 2019-04-12T03:41:08Z

clients/lightrider/lightrider-ui-features.js

+/* global ReportUIFeatures */
+
+/**
+ * Extends ReportUIFeatures to use the saved report from a browserified ReportGenerator.


"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 :)

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.

lighthouse-core/lib/file-namer.js

brendankenny · 2019-04-12T18:40:45Z

I vaguely remember a discussion on a prior PR where we needed this for devtools about how the splitting of report generator would help LR maybe?

my understanding is @exterkamp needed a standalone report generator independent of the PSI UI so that it could render saved reports after the fact. He can correct me/fill in any other details, though.

I was hoping the devtools and this one could be shared, but devtools wanted it to use the cached resources internal to devtools rather than bundle the whole thing. Ideally we could share something, but the files are so short and simple, it feels like anything we come up with is just going to be longer and more complicated than duplicating the file.

A test isn't the worst idea, though, @exterkamp. Otherwise we could break you and we wouldn't find out until much later when you do the next import. Maybe we should file an issue?

We've had an issue (in our heads, not filed, I believe) for a while now that we should take the puppeteer tests that validate that the extension and viewer can render a report correctly by having them run and then verifying the report is correct. Each does this separately; we could pull out the report verification part so it can be shared between them and then add the lr report generator to the mix.

brendankenny · 2019-04-12T18:53:56Z

build/build-lightrider.js

+async function copyAssets() {
+  return cpy([
+    'clients/lightrider/lightrider-ui-features.js',
+    'lighthouse-core/lib/file-namer.js',


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 :)

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.

clients/lightrider/lightrider-ui-features.js

brendankenny · 2019-04-13T00:01:57Z

can you give us an update on what this is blocked on @exterkamp

paulirish · 2019-04-13T00:27:32Z

clients/lightrider/lightrider-ui-features.js

+   */
+  getReportHtml() {
+    // @ts-ignore ReportGenerator is put on window by the host page.
+    return window.ReportGenerator.generateReportHtml(this.json);


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.

exterkamp · 2019-04-15T22:36:08Z

So based on the changes in #8228 I have modified the internal renderer to use that & the report-generator and therefore lgithrider-ui-features and file-namer are not needed at this time. So this should only make a report-generator for lr, and that should be all we need at this point in time.

I was blocked by confusion around how we should render in Lightrider. But Paul cleared it up with his PR.

exterkamp · 2019-04-16T05:47:07Z

Okay so I think I finally have this organized:

report-generator only:

report-generator.js

new™️ psi renderer uses these files:

util.js
dom.js
details-renderer.js
crc-details-renderer.js
category-renderer.js
pwa-category-renderer.js
performance-category-renderer.js
report-renderer.js
psi.js
snippet-renderer.js
(notice lack of report-ui-features, as it doesn't need it)

current psi renderer uses these files:

util.js
dom.js
details-renderer.js
crc-details-renderer.js
category-renderer.js
pwa-category-renderer.js
performance-category-renderer.js
report-renderer.js
psi.js
report-ui-features.js
file-namer.js
lightrider-ui-features.js
report-generator.js

The method of report-generating for Lightrider will transition to using this new LR report-generator for now. In the future, we may re-add a lightrider-ui-features + a new api to render reports more easily.

PSI will move to a new psi renderer package that will include psi.js and not include report-ui-features.

This will allow PSI and Lightrider to render reports simultaneously with 1 set of lib files, using 2 subsets of files.

build/build-lightrider.js

package.json

exterkamp added 2 commits April 11, 2019 14:55

Made /lightrider client folder. Added lightrider-ui-features and gene…

fc147c1

…rates a report-generator for lightrider.

Removed blank overrides.

d7064da

exterkamp requested review from paulirish and brendankenny April 11, 2019 22:06

exterkamp requested a review from patrickhulce as a code owner April 11, 2019 22:06

Added lr file namer.

c09d109

brendankenny reviewed Apr 11, 2019

View reviewed changes

exterkamp added 4 commits April 11, 2019 17:45

Brendan's foodback.

02b3b3a

I read all the feedback b4 submitting, sure.

9bf50ba

ts-ignore window.ReportGen

b2b95f9

fix test to point to new entry location.

066d323

patrickhulce reviewed Apr 12, 2019

View reviewed changes

exterkamp added 2 commits April 12, 2019 10:19

Removed comment about node testing.

fffc38b

Add @license and new comment.

e12115d

brendankenny reviewed Apr 12, 2019

View reviewed changes

Better comments.

319dd94

exterkamp changed the title ~~client(lr): add lightrider report-generator and ui-features~~ [DNM] client(lr): add lightrider report-generator and ui-features Apr 12, 2019

paulirish approved these changes Apr 13, 2019

View reviewed changes

Merge branch 'master' into lr-report

b1463a9

vercel bot deployed to staging April 15, 2019 21:45 View deployment

Removed unecessary files from report generator.

9c17270

vercel bot deployed to staging April 15, 2019 22:31 View deployment

exterkamp changed the title ~~[DNM] client(lr): add lightrider report-generator and ui-features~~ client(lr): add lightrider report-generator Apr 15, 2019

exterkamp added the waiting4reviewer label Apr 15, 2019

removed unused lines.

27b07e9

vercel bot deployed to staging April 15, 2019 23:06 View deployment

exterkamp changed the title ~~client(lr): add lightrider report-generator~~ [DNM again :(] client(lr): add lightrider report-generator Apr 16, 2019

exterkamp changed the title ~~[DNM again :(] client(lr): add lightrider report-generator~~ client(lr): add lightrider report-generator Apr 16, 2019

paulirish mentioned this pull request Apr 16, 2019

[meta] 2019 I/O Burndown #8331

Closed

67 tasks

paulirish reviewed Apr 16, 2019

View reviewed changes

build/build-lightrider.js Outdated Show resolved Hide resolved

paulirish reviewed Apr 16, 2019

View reviewed changes

package.json Outdated Show resolved Hide resolved

Renames

390f543

vercel bot deployed to staging April 16, 2019 21:17 View deployment

paulirish merged commit 81a8a5c into master Apr 16, 2019

paulirish deleted the lr-report branch April 16, 2019 21:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

exterkamp commented Apr 11, 2019 •

edited

Loading

brendankenny Apr 11, 2019

patrickhulce left a comment

patrickhulce Apr 12, 2019

patrickhulce Apr 12, 2019

exterkamp Apr 12, 2019

patrickhulce Apr 12, 2019

patrickhulce Apr 12, 2019

exterkamp Apr 12, 2019

brendankenny commented Apr 12, 2019

brendankenny Apr 12, 2019

exterkamp Apr 12, 2019

brendankenny commented Apr 13, 2019

paulirish Apr 13, 2019

exterkamp commented Apr 15, 2019

exterkamp commented Apr 16, 2019 •

edited by paulirish

Loading

		@@ -0,0 +1,25 @@
		/**
		* Copyright 2019 Google Inc. All Rights Reserved.

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

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

Conversation

exterkamp commented Apr 11, 2019 • edited Loading

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

brendankenny commented Apr 12, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brendankenny commented Apr 13, 2019

Choose a reason for hiding this comment

exterkamp commented Apr 15, 2019

exterkamp commented Apr 16, 2019 • edited by paulirish Loading

exterkamp commented Apr 11, 2019 •

edited

Loading

exterkamp commented Apr 16, 2019 •

edited by paulirish

Loading