report: autogenerate components.js from templates.html #12803

connorjclark · 2021-07-19T21:46:31Z

This autogenerates JS functions that make HTML components from the templates.html. This simplifies the renderer code (no need to bundle the HTML / ship a HTML file) and removes some global state management (we stored global state in the DOM via data-stamped).

The main motivation here is to satisfy constraints for using the renderer in a new internal google3 project.

keep templates.html indefinitely–it remains our source of truth for these "components"
rename template ids to be camelCase. These ids are used in dom.createComponent
dom.createComponent acts like dom.cloneTemplate–gets the template, caches it, and strips style tag if necessary
delete templateContext
move nested templates to be their own thing. simplifies the component generation

(still left to do):

Fix jest tests
Update build steps for CDT/PSI

…house into make-component-script

patrickhulce

a few outstanding questions, but LGTM!

patrickhulce · 2021-07-24T13:38:50Z

package.json

@@ -23,7 +23,7 @@
    "build-smokehouse-bundle": "node ./build/build-smokehouse-bundle.js",
    "build-lr": "yarn reset-link && node ./build/build-lightrider-bundles.js",
    "build-pack": "bash build/build-pack.sh",
-    "build-report": "node build/build-report.js",
+    "build-report": "node build/build-report-components.js && yarn eslint --fix report/renderer/components.js && node build/build-report.js",


so now every yarn start we're running eslint? 😞

reminder me again why a watch is terrible? 😅

running eslint on one file, it just takes a second

why a watch is terrible?

Extra hoops to jump through for development is never good. running yarn start ... is simpler than remembering to open a new terminal and run a watch command

don't feel strongly. i say we vote on it.

The watch question is a good one (as someone who generally dislikes watch commands :) and we should definitely look at what we're doing in all our build steps and the efficiency of them, but this seems ok until we have #12689 mostly settled and know more of the lay of the land?

Personally I've been spoiled by esbuild and would love to get building everything down to like < 1s total, but we'll see what's possible :)

build/build-report-components.js

patrickhulce · 2021-07-26T03:21:01Z

report/assets/styles.css

@@ -1168,16 +1168,14 @@
 }

 .lh-gauge-base {
-    opacity: 0.1;
-    stroke: var(--circle-background);


these intentionally removed? having trouble seeing how these are related

these variables do not exist. not related, just found during debugging.

patrickhulce · 2021-07-26T03:27:28Z

report/renderer/dom.js

-    if (template.hasAttribute('data-stamped')) {
-      this.findAll('style', clone).forEach(style => style.remove());
+  createComponent(componentName) {
+    let component = this._componentCache.get(componentName);


patrickhulce · 2021-07-26T03:28:55Z

report/report-generator.js

@@ -37,12 +37,14 @@ class ReportGenerator {
      .replace(/</g, '\\u003c') // replaces opening script tags
      .replace(/\u2028/g, '\\u2028') // replaces line separators ()
      .replace(/\u2029/g, '\\u2029'); // replaces paragraph separators
+    // terser does its own sanitization, but keep this basic replace for when


build/build-report-components.js

report/renderer/components.js

build/build-dt-report-resources.js

report/test/renderer/components-test.js

brendankenny · 2021-07-26T20:48:07Z

build/build-report-components.js

+      previousSiblingElement && nextSiblingElement &&
+      (previousSiblingElement.tagName === 'SPAN' || nextSiblingElement.tagName === 'SPAN')
+    );
+    if (!allowJustWhitespace) return;


is there any harm in always collapsing whitespace to a single ' ' (except for <pre> , etc) and not keeping the heuristic? A longer components.js?

much longer ... although I last checked that before aggregating appending nodes to a single .append. Can check again.

much longer ... although I last checked that before aggregating appending nodes to a single .append. Can check again.

yeah, I can see that. Reading https://developer.mozilla.org/en-US/docs/Web/API/Document_Object_Model/Whitespace, though, it's a lot more complicated than I thought it was, and it seems like detecting span is not sufficient, though maybe it is for the current form of our templates. It does seem easier to collapse all whitespace regardless of context (at a loss of file size, but maybe not too much?) and not have to figure it all out/test every case.

Latest commit has this change.

report/test/renderer/components-test.js

lighthouse-core/gather/driver/execution-context.js

connorjclark · 2021-08-06T19:07:45Z

I think this is landable.

We may come back to address the "watch/no watch" thing later down the line, once we've more time with the new esmodules/report must be built world.
There's minor contention regarding if caching is necessary: the complexity is scoped very narrowly to just one function/a couple lines in dom.js, at worst it is of zero benefit, and it's persisting the previous caching behavior, so I think it's fine to land now and if either of these assumptions prove to be wrong we can remove later on.

Anything else?

patrickhulce · 2021-08-06T23:48:21Z

SGTM @connorjclark !

brendankenny · 2021-08-07T00:12:21Z

The caching question was resolved well enough for me too, sorry if that wasn't clear. SGTM as well.

I did mean to ask if we should still be checking in report/renderer/components.js since it's part of yarn build-report now? Should we treat it like the other built report artifact(s) instead? That would avoid having to remember to run build-report after any changes to templates.html before committing/pushing.

We can also decide that later if need be. I can imagine you're eager to land and no longer maintain this branch :)

connorjclark · 2021-08-09T20:04:42Z

For me, the git check we do in CI is enough. I do prefer being able to see diffs of generated code, as it makes reviewing changes to the generator easier. We also have the "check in generated files" pattern for the CDT SourceMap.js, although in that case we decided to put it in a folder called generated.

brendankenny · 2021-08-09T20:39:30Z

For me, the git check we do in CI is enough. I do prefer being able to see diffs of generated code, as it makes reviewing changes to the generator easier. We also have the "check in generated files" pattern for the CDT SourceMap.js, although in that case we decided to put it in a folder called generated.

yeah, I'm wondering if it's going to be the next "person x, you have to run yarn update:sample-json to get rid of those CI errors in your PR", but I'm good with this way for now. We don't update templates.html very often, so it may be totally fine.

connorjclark and others added 30 commits July 15, 2021 16:13

wip

bc277e3

start using dom stuff

3018199

git rid of createTextNode

4ace55d

tweaks

8944fdd

better args serialz

ec87013

load templates file

4d1f12c

fragment

da5ac10

lol

f3cc791

attributeNS

3f473cf

lol

de88af4

add a innerHTML diff check

2df95e3

bring in DOM library

189c120

create good function name

f9649c8

move fn

f3e1069

some tlc

ae4551e

mostly working

0afbe2b

async main

00ccf4f

fix NS class

f9fd13d

lol

f3b4495

rm empty div

7a56d4a

create components.js

024a55a

reorder attributes

daccad9

actually need to do createTextNode bc textContent blows away children

7ffdfaa

Merge branch 'make-component-script' of github.com:GoogleChrome/light…

3a2566d

…house into make-component-script

fix

bac811b

fix

25aefd5

minor

9c1a41b

rm fallback

8afdcff

better component names.

f2653ca

snippets. refactor script a bit

b91c79e

patrickhulce approved these changes Jul 26, 2021

View reviewed changes

patrickhulce added waiting4committer and removed waiting4reviewer labels Jul 26, 2021

paulirish reviewed Jul 26, 2021

View reviewed changes

build/build-report-components.js Outdated Show resolved Hide resolved

build/build-report-components.js Outdated Show resolved Hide resolved

report/renderer/components.js Outdated Show resolved Hide resolved

brendankenny reviewed Jul 26, 2021

View reviewed changes

connorjclark added 2 commits July 30, 2021 11:56

Merge remote-tracking branch 'origin/master' into components

31fbea0

more ws

50faaa6

vercel bot deployed to Preview July 30, 2021 19:04 View deployment

update

7099549

vercel bot deployed to Preview July 30, 2021 21:02 View deployment

connorjclark added 3 commits August 2, 2021 16:55

Merge remote-tracking branch 'origin/master' into components

ae0f3d4

some comments

85a46a8

comments

7241e8d

vercel bot deployed to Preview August 3, 2021 00:12 View deployment

rm

fe66b3b

vercel bot deployed to Preview August 3, 2021 18:17 View deployment

fix

127b554

vercel bot deployed to Preview August 3, 2021 20:37 View deployment

brendankenny mentioned this pull request Aug 4, 2021

report(flow): standalone shell report with preact #12850

Merged

connorjclark added waiting4reviewer and removed waiting4committer labels Aug 6, 2021

connorjclark merged commit 800ffd6 into master Aug 9, 2021

connorjclark deleted the components branch August 9, 2021 21:59

connorjclark mentioned this pull request Aug 10, 2021

misc(build): remove template.html from devtools report resources #12891

Merged

connorjclark mentioned this pull request Sep 2, 2021

report(flow): sidebar mocks #13002

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

report: autogenerate components.js from templates.html #12803

report: autogenerate components.js from templates.html #12803

connorjclark commented Jul 19, 2021 •

edited

Loading

patrickhulce left a comment

patrickhulce Jul 24, 2021

connorjclark Jul 26, 2021

brendankenny Jul 26, 2021

patrickhulce Jul 26, 2021

connorjclark Jul 26, 2021

patrickhulce Jul 26, 2021

patrickhulce Jul 26, 2021

brendankenny Jul 26, 2021

connorjclark Jul 26, 2021

brendankenny Jul 26, 2021

connorjclark Jul 30, 2021

connorjclark commented Aug 6, 2021 •

edited

Loading

patrickhulce commented Aug 6, 2021

brendankenny commented Aug 7, 2021 •

edited

Loading

connorjclark commented Aug 9, 2021

brendankenny commented Aug 9, 2021

report: autogenerate components.js from templates.html #12803

report: autogenerate components.js from templates.html #12803

Conversation

connorjclark commented Jul 19, 2021 • edited Loading

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

connorjclark commented Aug 6, 2021 • edited Loading

patrickhulce commented Aug 6, 2021

brendankenny commented Aug 7, 2021 • edited Loading

connorjclark commented Aug 9, 2021

brendankenny commented Aug 9, 2021

connorjclark commented Jul 19, 2021 •

edited

Loading

connorjclark commented Aug 6, 2021 •

edited

Loading

brendankenny commented Aug 7, 2021 •

edited

Loading