misc(treemap): add placeholder, gist and file upload features #12511

connorjclark · 2021-05-19T00:08:34Z

Added placeholder div, just like viewer
Using logger
Load from ?gist=
Paste JSON/gist URL
Drag/drop/upload file
Using AbortController to clean up event listeners when treemapViewer is recreated. See https://css-tricks.com/using-abortcontroller-as-an-alternative-for-removing-event-listeners/ . Using polyfill: https://github.com/mysticatea/event-target-shim

https://gist.github.com/connorjclark/060f0322480cc9dbad793b8cea405e46

https://lighthouse-git-treemap-appify-googlechrome.vercel.app/gh-pages/treemap/?gist=060f0322480cc9dbad793b8cea405e46

TODO/follow up PR:

Loading indicator, like viewer
Options menu, "Save as gist" if not already, like viewer
"Back to placeholder" button from the treemap view

patrickhulce

WDYT about "misc(treemap): add placeholder, load from gist, and file drop features" for PR name? I was hunting around for the upload functionality until I saw the "gist creation" was a todo which was the only upload I could think of.

I wasn't able to build treemap locally either (complaining about umd.js of eventtargetshim) so I couldn't test anything out, but looks like a solid copy of viewer :)

patrickhulce · 2021-05-19T15:11:28Z

lighthouse-viewer/app/src/github-api.js

@@ -36,6 +36,7 @@ class GithubApi {
      return Promise.reject(new Error('Save already in progress'));
    }

+    // @ts-ignore


what's with all the explanationless ignores? :)

I have no idea...all the sudden tsc -p . fails because of these. but not in my editor...having weird issues lately :/

ok it's b/c the Strings import in build-treemap causes TSC to more than it should for tsc -p .

Have you tried replacing with window.logger? That's what's declared on the typedefs.

lighthouse-viewer/app/src/lighthouse-report-viewer.js

patrickhulce · 2021-05-19T15:14:27Z

lighthouse-viewer/app/src/drag-and-drop.js

+   * @param {File} file
+   * @return {Promise<string>}
+   */
+  readFile(file) {


seems like it should be static?

ah I see below that would be annoying for tests, alright

lighthouse-viewer/app/src/drag-and-drop.js

lighthouse-viewer/app/src/lighthouse-report-viewer.js

patrickhulce · 2021-05-19T15:25:26Z

lighthouse-treemap/app/index.html

+        <h1 class="treemap-placeholder__heading">Lighthouse Treemap</h1>
+        <div class="treemap-placeholder__help">To view a treemap: Paste the Lighthouse result json or a Gist URL.<br>
+        You can also drag 'n drop the file or click here to select it.<br><br>
+        To get a fresh report, use the Lighthouse browser extension (<a href="https://chrome.google.com/webstore/detail/lighthouse/blipmdconlkpinefehnmjammfjpmpbjk">Chrome</a> / <a href="https://addons.mozilla.org/en-US/firefox/addon/google-lighthouse/">Firefox</a>)<br>


The extension bit might be more confusing advice for treemap since they'll end up back over at viewer instead of the treemap and they may or may not have the "view treemap" button.

They should always have it after PSI upgrades. does that resolve your concern?

They should always have it after PSI upgrades. does that resolve your concern?

Not particularly. My concern is that if we didn't copy/paste from viewer it feels like we wouldn't have written this that paragraph this way and especially not encourage the extension (the reason we did so for viewer is because the extension is the entrypoint for most people using viewer).

The "may or may not have the view treemap button" was also a comment on the general applicability of this page compared to viewer and how appropriate it is to explain that. For example, should we use this space instead to link to a treemap / sourcemaps article on web.dev?

lighthouse-treemap/app/src/main.js

patrickhulce · 2021-05-19T15:45:35Z

lighthouse-treemap/app/src/main.js

+   */
+  convertToOptions(json) {
+    if (json instanceof Object) {
+      if ('lhr' in json) return json;


it's so messed up that TS thinks this is OK 😆

json is inferred to be never at this point, so we've basically silently devolved from unknown to any

Co-authored-by: Patrick Hulce <patrick.hulce@gmail.com>

connorjclark · 2021-05-19T16:24:37Z

I wasn't able to build treemap locally either (complaining about umd.js of eventtargetshim) so I couldn't test anything out, but looks like a solid copy of viewer :)

what's the error? I get no error.

patrickhulce · 2021-05-19T17:09:50Z

what's the error? I get no error.

Same error as CI. The issue is require.resolve('event-target-shim/umd.js').

In Node 10 this works as expected by requiring that file. In Node 12+ this looks for an export called umd.js which does not exist and throws instead of finding the file. require.resolve('event-target-shim/umd') works fine as there is an export called umd. Honestly I'm more confused why this works just fine on your end :)

connorjclark · 2021-05-19T17:12:08Z

Same error as CI. The issue is require.resolve('event-target-shim/umd.js').

woah... why does CI keep going after this..

idk i'm on v12.13.0

connorjclark · 2021-05-19T17:15:09Z

In Node 12+ this looks for an export called umd.js which does not exist and throws instead of finding the file.

what's that now? Then why do the following lines, which also import a file with the .js extension work? or is it different because it's not multiple path levels deep?

what is this node 12 feature/change to module resolution called?

EDIT: oh, I just noticed the exports field in package.json, guess that's it.

brendankenny · 2021-05-19T17:26:32Z

types/i18n.d.ts

+
+    export type LhlMessages = Record<string, {message: string}>;
+
+    export type Strings = Record<LH.Locale, LhlMessages>;


I'd really like to avoid making global types when they aren't needed, or at least scope these to treemap.d.ts. Defining LhlMessages in locales.js where the actual object is also defined was very intentional, and LH.Strings is a simple and can just be defined in place.

removed Strings. but LhlMessages is used in 6 different files, so despite intention it is being used globally just in the more cumbersome jsdoc import style.

so despite intention it is being used globally just in the more cumbersome jsdoc import style.

Strong disagree. At no point has anyone suggested setting global.i18n = require('./lighthouse-core/lib/i18n/i18n.js') so we can avoid require('../../../lib/i18n/i18n.js') in all the files that use it, for instance.

I think that it's not worth hashing out the usefulness of globals (whether in value- or type-space) in this PR, though, but more importantly I think there's enough going on in here without making changes to types in core. Can we revert and follow up later? Happy to discuss at another time.

paulirish · 2021-05-19T21:56:13Z

A few months ago we discussed an idea of merging these into a single webapp. The report viewer, the treemap, the calculator. At the time I think it was decided things were too early.

Seems like it's time to reopen that conversation? :)

connorjclark · 2021-05-20T01:37:29Z

At the time I think it was decided things were too early.
Seems like it's time to reopen that conversation? :)

Happy to discuss this, but it shouldn't block moving forward with this PR :)

wdyt of adding to placeholder: "What is this? Show me an example" text that links to a gist?

patrickhulce

It seemed like this wasn't actually waiting on committer, so I took another look.

WDYT about "misc(treemap): add placeholder UI, load from gist, and file drop features" for PR name? I was hunting around for the upload functionality until I saw the "gist creation" was a todo which was the only upload I could think of.

Bump :)

The treemap tests appear to be broken in CI, but I wasn't able to manually reproduce. Something else might have changed with the programmatic triggering?

build/build-treemap.js

patrickhulce · 2021-05-20T15:02:03Z

lighthouse-treemap/app/images/lighthouse-logo.svg

@@ -0,0 +1,116 @@
+<svg width="128" height="128" xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink">


I can't comment directly on them, but those favicon file renames seem like a mistake. I don't see a manifest.json for the treemap app that references them and I don't see the viewer references removed either.

oops... I copied the images folder, and accidently deleted excess images from the wrong folder. hence "rename"

lighthouse-treemap/app/styles/treemap.css

Co-authored-by: Patrick Hulce <patrick.hulce@gmail.com>

connorjclark · 2021-05-20T16:29:11Z

It seemed like this wasn't actually waiting on committer, so I took another look.

WDYT about "misc(treemap): add placeholder UI, load from gist, and file drop features" for PR name? I was hunting around for the upload functionality until I saw the "gist creation" was a todo which was the only upload I could think of.
Bump :)

I renamed the title: misc(treemap): add placeholder div, gist and file upload features. Does this not clear up ambiguity? maybe we need an oxford comma too :)

The treemap tests appear to be broken in CI, but I wasn't able to manually reproduce. Something else might have changed with the programmatic triggering?

Been poking at this. No idea why this is failing now.

patrickhulce · 2021-05-20T16:54:47Z

Does this not clear up ambiguity? maybe we need an oxford comma too :)

haha, oxford comma aside, the both upload + lack of "fetch only" from gist distinction in combination is still confusing to me, but the chance that someone reads this again to learn what it contained is pretty low so 🤷

connorjclark · 2021-05-20T17:25:36Z

lighthouse-treemap/test/treemap-test-pptr.js

@@ -77,9 +77,12 @@ describe('Lighthouse Treemap', () => {
      await new Promise(resolve => browser.on('targetcreated', resolve));
      const target = (await browser.targets()).find(target => target.url() === treemapUrl);
      page = await target.page();
-      await openerPage.close();


deleting await openerPage.close(); fixed the issue. shrug.

Co-authored-by: Patrick Hulce <patrick.hulce@gmail.com>

wip

6872f93

connorjclark requested a review from a team as a code owner May 19, 2021 00:08

connorjclark requested review from patrickhulce and removed request for a team May 19, 2021 00:08

google-cla bot added the cla: yes label May 19, 2021

devtools-bot assigned patrickhulce May 19, 2021

devtools-bot added the waiting4reviewer label May 19, 2021

uhhh

d9ecc0d

vercel bot deployed to Preview May 19, 2021 00:11 View deployment

fix test

45e953a

vercel bot deployed to Preview May 19, 2021 00:32 View deployment

update

46e599b

vercel bot deployed to Preview May 19, 2021 02:11 View deployment

patrickhulce reviewed May 19, 2021

View reviewed changes

patrickhulce added waiting4committer and removed waiting4reviewer labels May 19, 2021

Update lighthouse-viewer/app/src/lighthouse-report-viewer.js

f6e7eea

Co-authored-by: Patrick Hulce <patrick.hulce@gmail.com>

vercel bot deployed to Preview May 19, 2021 16:22 View deployment

connorjclark changed the title ~~misc(treemap): add placeholder div, gist and upload features~~ misc(treemap): add placeholder div, gist and file upload features May 19, 2021

pr

a0e2122

vercel bot deployed to Preview May 19, 2021 16:48 View deployment

update

c828f3e

vercel bot deployed to Preview May 19, 2021 17:00 View deployment

update

9b1faed

vercel bot deployed to Preview May 19, 2021 17:22 View deployment

brendankenny reviewed May 19, 2021

View reviewed changes

lolhtml

1ba79ce

vercel bot deployed to Preview May 19, 2021 17:37 View deployment

types

7b1198b

vercel bot deployed to Preview May 19, 2021 18:15 View deployment

fix

4797825

vercel bot deployed to Preview May 20, 2021 01:56 View deployment

morefix

6b5aca6

vercel bot deployed to Preview May 20, 2021 01:56 View deployment

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

396fa06

vercel bot deployed to Preview May 20, 2021 02:02 View deployment

oops

0e5e9fe

vercel bot deployed to Preview May 20, 2021 02:04 View deployment

patrickhulce reviewed May 20, 2021

View reviewed changes

pr

8c6e56e

vercel bot deployed to Preview May 20, 2021 16:26 View deployment

Update lighthouse-treemap/app/styles/treemap.css

0332c86

Co-authored-by: Patrick Hulce <patrick.hulce@gmail.com>

vercel bot deployed to Preview May 20, 2021 16:26 View deployment

patrickhulce approved these changes May 20, 2021

View reviewed changes

fixtest

df55b1e

vercel bot deployed to Preview May 20, 2021 17:23 View deployment

connorjclark commented May 20, 2021

View reviewed changes

Update build/build-treemap.js

316613b

Co-authored-by: Patrick Hulce <patrick.hulce@gmail.com>

vercel bot deployed to Preview May 20, 2021 18:53 View deployment

connorjclark changed the title ~~misc(treemap): add placeholder div, gist and file upload features~~ misc(treemap): add placeholder, gist and file upload features May 20, 2021

connorjclark merged commit 30b3e06 into master May 20, 2021

connorjclark deleted the treemap-appify branch May 20, 2021 22:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

misc(treemap): add placeholder, gist and file upload features #12511

misc(treemap): add placeholder, gist and file upload features #12511

connorjclark commented May 19, 2021 •

edited by patrickhulce

Loading

patrickhulce left a comment

patrickhulce May 19, 2021

connorjclark May 19, 2021

connorjclark May 19, 2021

patrickhulce May 19, 2021

patrickhulce May 19, 2021

patrickhulce May 19, 2021

patrickhulce May 19, 2021

connorjclark May 19, 2021

patrickhulce May 19, 2021

patrickhulce May 19, 2021

connorjclark commented May 19, 2021

patrickhulce commented May 19, 2021

connorjclark commented May 19, 2021 •

edited

Loading

connorjclark commented May 19, 2021 •

edited

Loading

brendankenny May 19, 2021

connorjclark May 19, 2021

brendankenny May 19, 2021

paulirish commented May 19, 2021

connorjclark commented May 20, 2021

patrickhulce left a comment

patrickhulce May 20, 2021

connorjclark May 20, 2021

connorjclark commented May 20, 2021

patrickhulce commented May 20, 2021

connorjclark May 20, 2021 •

edited

Loading


		export type LhlMessages = Record<string, {message: string}>;

		export type Strings = Record<LH.Locale, LhlMessages>;

		@@ -0,0 +1,116 @@
		<svg width="128" height="128" xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink">

misc(treemap): add placeholder, gist and file upload features #12511

misc(treemap): add placeholder, gist and file upload features #12511

Conversation

connorjclark commented May 19, 2021 • edited by patrickhulce 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

connorjclark commented May 19, 2021

patrickhulce commented May 19, 2021

connorjclark commented May 19, 2021 • edited Loading

connorjclark commented May 19, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paulirish commented May 19, 2021

connorjclark commented May 20, 2021

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

connorjclark commented May 20, 2021

patrickhulce commented May 20, 2021

connorjclark May 20, 2021 • edited Loading

Choose a reason for hiding this comment

connorjclark commented May 19, 2021 •

edited by patrickhulce

Loading

connorjclark commented May 19, 2021 •

edited

Loading

connorjclark commented May 19, 2021 •

edited

Loading

connorjclark May 20, 2021 •

edited

Loading