misc(treemap): initialize app structure by connorjclark · Pull Request #11635 · GoogleChrome/lighthouse

connorjclark · 2020-11-05T23:31:56Z

ref #11254, split off from #11545

Basic app structure, including build and puppeteer tests. Simply prints the LHR to the page.

connorjclark · 2020-11-05T23:40:44Z

Should the commits for this be misc(treemap) or should we add treemap (and viewer?) to the PR title linter?

patrickhulce

thanks for the split up, much easier to follow! 🎉 💯

patrickhulce · 2020-11-05T23:38:46Z

+        m = s.getElementsByTagName(o)[0]; a.async = 1; a.src = g; m.parentNode.insertBefore(a, m)
+    })(window, document, 'script', 'https://www.google-analytics.com/analytics.js', 'ga');
+
+    ga('create', 'UA-85519014-2', 'auto');


is this the same analytics account as the viewer? I guess that's what we want when served over same domain anyway

patrickhulce · 2020-11-05T23:45:18Z

+ */
+function injectOptions() {
+  // @ts-expect-error
+  if (!window.__injected) return;


could this mean...?

Suggested change

if (!window.__injected) return;

// Don't inject again if we've already injected the options.

if (window.__injected) return;

if so my dream about syncing the UI state to this seems to be DOA :)

(I refactored this a bit, I think I was mistaken that a separated boolean was needed here)

What do you mean by syncing UI state? For the Ctrl-S document save? I suppose we could delete and keep replacing this script tag as the user changes the view, to facilitate that. Although I doubt many will know to do this, and a better future plan would be a gist-hosted solution.

patrickhulce · 2020-11-05T23:55:45Z

+    type Node = _Node;
+  }
+
+  interface WebTreeMapOptions {


these aren't used yet right?

nope. Just types for the webtreemap library. https://github.com/paulirish/webtreemap-cdt#options

patrickhulce · 2020-11-06T00:05:45Z

+
+async function main() {
+  if (window.__TREEMAP_OPTIONS) {
+    init(window.__TREEMAP_OPTIONS);


Suggested change

init(window.__TREEMAP_OPTIONS);

// Prefer the hardcoded options from a saved HTML file above all.

init(window.__TREEMAP_OPTIONS);

is this the only situation that this applies?

patrickhulce · 2020-11-06T00:08:25Z

+
+declare global {
+  module Treemap {
+    interface Options {


will this eventually have options in it too?

right now options feels like a bit of a misnomer, payload, initialization data, do any of those seem right?

I'm mostly pushing on this a bit because it feels like we are about to get some UI state for this app, which feels distinctly different from an initial payload. Maybe it's the intention to have all UI state configurable from the initial payload though?

Maybe it's the intention to have all UI state configurable from the initial payload though?

Yup. The UI state is called "Mode" in the WIP branch: https://github.com/GoogleChrome/lighthouse/compare/viewer-treemap-2#diff-c3c7c9ceaada02e42e7ba0b22d920ac101a4747c1742eae23000f6c7627c6f3bR13 The idea is that some bits of the LH report will set a mode to show a certain view.

patrickhulce · 2020-11-06T00:28:29Z

Should the commits for this be misc(treemap) or should we add treemap (and viewer?) to the PR title linter?

How active do you expect treemap viewer to be once the bulk is built? If the answer is "as active as viewer" I'd vote misc(treemap) :)

connorjclark · 2020-11-06T18:56:57Z

tsc -p lighthouse-viewer/ and tsc -p lighthouse-treemap/ completely fail, and I don't know how to resolve it yet.

Beytoven · 2020-11-06T21:34:58Z

+
+  <script src="src/bundled.js"></script>
+  <script>
+    (function (i, s, o, g, r, a, m) {


A comment here that summarizes what's happening could be helpful if it's not too much trouble. Personally, at first glance this bit threw me for a loop.

This is meant-to-be-unreadable code for google analytics :) it's what they give to you for copy/paste.

Beytoven

LGTM

connorjclark · 2020-11-13T01:19:47Z

@patrickhulce what further changes are you requesting?

connorjclark · 2020-11-13T01:43:58Z

@@ -0,0 +1,28 @@
+import '../../types/treemap';


I don't see any errors in vscode now, but yarn tsc -p lighthouse-treemap/ fails spectacularly. @brendankenny I'm quite lost here, do you have any advice?

I think typeRoots might be your problem

patrickhulce · 2020-11-13T01:53:34Z

@patrickhulce what further changes are you requesting?

I don't know yet I thought you were still working on this, sorry. Is this awaiting my feedback?

patrickhulce

pretty much LGTM, just the types issue and cleanup of outstanding GA question

patrickhulce · 2020-11-13T02:02:29Z

+ * @param {LH.Treemap.Options} options
+ */
+function injectOptions(options) {
+  if (window.__treemapOptions) return;


definitely fits now, so not worth discussing much, but can I reserve the right to object to options if this turns into state in future PRs? 😃

patrickhulce · 2020-11-13T02:12:02Z

+{
+  "extends": "../tsconfig",
+  "compilerOptions": {
+    "typeRoots": [


I don't think this actually includes the types like you want it to, does it? That might be your problem, most of our types aren't packages, they're just files that should be in include, no?

awesome, thanks!

patrickhulce · 2020-11-13T02:12:35Z

@@ -0,0 +1,28 @@
+import '../../types/treemap';


I think typeRoots might be your problem

treemap: initialize app structure

f8518de

connorjclark requested a review from a team as a code owner November 5, 2020 23:31

connorjclark requested review from Beytoven and removed request for a team November 5, 2020 23:31

google-cla Bot added the cla: yes label Nov 5, 2020

devtools-bot assigned Beytoven Nov 5, 2020

devtools-bot added the waiting4reviewer label Nov 5, 2020

connorjclark commented Nov 5, 2020

View reviewed changes

Comment thread lighthouse-treemap/types/treemap.d.ts Outdated

connorjclark mentioned this pull request Nov 5, 2020

Lighthouse Treemap ☂️ #11254

Closed

24 tasks

build-all

535422b

vercel Bot had a problem deploying to Preview November 6, 2020 00:06 Failure

patrickhulce suggested changes Nov 6, 2020

View reviewed changes

patrickhulce self-assigned this Nov 6, 2020

patrickhulce added waiting4committer and removed waiting4reviewer labels Nov 6, 2020

connorjclark added 2 commits November 5, 2020 18:54

patrick comments

6390fd1

__treemapOptions

a48402a

vercel Bot deployed to Preview November 6, 2020 01:06 View deployment

connorjclark changed the title ~~treemap: initialize app structure~~ misc(treemap): initialize app structure Nov 6, 2020

kinda fix types

d2ca505

vercel Bot deployed to Preview November 6, 2020 18:52 View deployment

yarn type-check but fails

59be9d0

vercel Bot deployed to Preview November 6, 2020 18:57 View deployment

didnt mean to delete that

61efa44

vercel Bot deployed to Preview November 6, 2020 20:18 View deployment

Beytoven reviewed Nov 6, 2020

View reviewed changes

Beytoven approved these changes Nov 6, 2020

View reviewed changes

comment

3a14210

vercel Bot deployed to Preview November 6, 2020 21:39 View deployment

connorjclark added 2 commits November 12, 2020 19:20

Merge remote-tracking branch 'origin/master' into treemap-basic

eadd41e

somewhat closer to fixed typing

29f9fa0

vercel Bot deployed to Preview November 13, 2020 01:27 View deployment

hmmm

bef0523

vercel Bot deployed to Preview November 13, 2020 01:37 View deployment

update

a44f9c9

vercel Bot deployed to Preview November 13, 2020 01:43 View deployment

connorjclark commented Nov 13, 2020

View reviewed changes

patrickhulce reviewed Nov 13, 2020

View reviewed changes

fix types

b5a8141

vercel Bot deployed to Preview November 13, 2020 20:41 View deployment

ga

d3432a9

vercel Bot deployed to Preview November 13, 2020 20:46 View deployment

connorjclark added land-when-ci-is-green waiting4reviewer and removed waiting4committer labels Nov 13, 2020

patrickhulce approved these changes Nov 13, 2020

View reviewed changes

devtools-bot merged commit fea7ffc into master Nov 13, 2020

devtools-bot deleted the treemap-basic branch November 13, 2020 21:30

devtools-bot removed the land-when-ci-is-green label Nov 13, 2020

	if (!window.__injected) return;
	// Don't inject again if we've already injected the options.
	if (window.__injected) return;

	init(window.__TREEMAP_OPTIONS);
	// Prefer the hardcoded options from a saved HTML file above all.
	init(window.__TREEMAP_OPTIONS);

Conversation

connorjclark commented Nov 5, 2020

Uh oh!

Uh oh!

connorjclark commented Nov 5, 2020

Uh oh!

patrickhulce left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

patrickhulce commented Nov 6, 2020

Uh oh!

connorjclark commented Nov 6, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Beytoven left a comment

Choose a reason for hiding this comment

Uh oh!

connorjclark commented Nov 13, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

patrickhulce commented Nov 13, 2020

Uh oh!

patrickhulce left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants