plots: better support power use cases #2464

wwwillchen · 2017-06-07T22:34:04Z

This closes #2412

I'm copying the charts folder into each out directory because:

Needed a dynamic way to load generatedResults.js since the out folder can be an arbitrary name and I wanted to reduce the boilerplate of maintaining multiple charts page
I can't open a web page with query params using opn, so the initial load of charts/index.html has to bootstrap properly without any query params [0]
This makes it simpler to view older generated results, since each out directory has its own version of charts. This is helpful if you have old out dirs and the charts API has changed.

[0] Filed an issue here: sindresorhus/open#55

…nto plots-better-out

patrickhulce · 2017-06-08T19:08:43Z

lighthouse-core/config/plots.json

@@ -10,7 +10,6 @@
    "first-meaningful-paint",
    "speed-index-metric",
    "estimated-input-latency",
-    "time-to-interactive",


oops sorry :) **/*.js strikes again

true. @wwwillchen since you'll have to sort out this conflict anyhow...

can you change the plots.json to plots-config.js?

nice to have comments. ;)

patrickhulce

are there back compat considerations? I'm guessing our existing plots data will need to be regenerated

patrickhulce · 2017-06-08T20:21:44Z

plots/charts/chart-loader.js

+
+/* eslint-env browser */
+
+function parseQueryParameters(queryParams) {


use URLSearchParams instead?

wwwillchen · 2017-06-08T20:40:08Z

@patrickhulce for backwards compatibility:

yeah, existing out directories will need to be re-generated. It's a little dicey if it will work since, new metrics such as first-interactive cause analyze script to run into errors.
gh-pages plots source specific commits for charts so they should be OK

I think we can focus on backwards compat once plots over time (#2413) is implemented since it'll be vital then. For now, there's workarounds (e.g. checkout older lighthouse commit) to view old plots. Does that work for the team?

patrickhulce · 2017-06-08T20:42:50Z

I think we can focus on backwards compat once plots over time (#2413) is implemented since it'll be vital then. For now, there's workarounds (e.g. checkout older lighthouse commit) to view old plots. Does that work for the team?

Yeah sgtm, not a big deal until we have the historical view we'll want to preserve.

paulirish · 2017-06-13T19:12:54Z

lighthouse-core/config/plots.json

@@ -10,7 +10,6 @@
    "first-meaningful-paint",
    "speed-index-metric",
    "estimated-input-latency",
-    "time-to-interactive",


true. @wwwillchen since you'll have to sort out this conflict anyhow...

can you change the plots.json to plots-config.js?

nice to have comments. ;)

paulirish · 2017-06-13T21:10:25Z

plots/charts/index.html

@@ -16,14 +16,10 @@
 </head>

 <body>
-  <nav>
+  <nav id="nav">


u got some .html files in /plots to delete still.

paulirish · 2017-06-13T21:11:29Z

plots/measure.js

+ * @return {string}
+ */
+function generateOutPath() {
+  if (args.out) {


missing readme docs on this

paulirish · 2017-06-13T21:13:09Z

plots/utils.js

+ * @param {string} src
+ * @param {string} dest
+ */
+function copyRecursive(src, dest) {


lets use a module for this :)

paulirish · 2017-06-13T21:30:40Z

plots/charts/chart-loader.js

+const queryParams = new URLSearchParams(window.location.search);
+const chartParam = queryParams.get('chart') || './grouped-by-metric.js';
+
+const chartScript = document.createElement('script');


this is a bit clunky, plus the three metrics files are already overlapping pretty hard.

let's merge all of charts/*.js into one file.

you can hook up generateBloxPlotPerSite(), or generateBoxPlotChartPerMetric(); generateLinePlotChartPerMetric(); or generateGroupedBarChart(); depending on which view we're in.

would rather have a simple slug (e.g. 'by-metric') in the query param or hash instead of a file or function name.

paulirish · 2017-06-13T21:31:19Z

plots/utils.js

+ * @param {string} filePath
+ */
+function removeRecursive(filePath) {
+  try {


u can use rimraf

I've removed this (I think I can do away with it since I'm just copying the files individually into the correct out folder).

paulirish · 2017-06-13T21:33:03Z

plots/charts/index.html

  </nav>
-  <script src="./out/generatedResults.js"></script>
-  <script src="./bars-per-site.js"></script>
+  <script src="../generated-results.js"></script>


IMO if you're going to leave gen-results in the root of out then you should put index.html there as well.

paulirish · 2017-06-13T21:35:29Z

plots/analyze.js

@@ -48,17 +57,17 @@ main();
 function analyzeSite(sitePath) {
  console.log('Analyzing', sitePath); // eslint-disable-line no-console
  const runResults = [];
-  fs.readdirSync(sitePath).forEach(runDir => {
+  fs.readdirSync(sitePath).sort().forEach(runDir => {


dunno if it's useful but you might want to use localeCompare instead..

.sort((a, b) => a.localeCompare(b))

…nto plots-better-out

wwwillchen

@paulirish - please take another look.

wwwillchen · 2017-06-13T22:28:29Z

lighthouse-core/config/plots.json

@@ -10,7 +10,6 @@
    "first-meaningful-paint",
    "speed-index-metric",
    "estimated-input-latency",
-    "time-to-interactive",


wwwillchen · 2017-06-13T22:29:07Z

plots/analyze.js

@@ -48,17 +57,17 @@ main();
 function analyzeSite(sitePath) {
  console.log('Analyzing', sitePath); // eslint-disable-line no-console
  const runResults = [];
-  fs.readdirSync(sitePath).forEach(runDir => {
+  fs.readdirSync(sitePath).sort().forEach(runDir => {


wwwillchen · 2017-06-13T22:52:34Z

plots/charts/chart-loader.js

+const queryParams = new URLSearchParams(window.location.search);
+const chartParam = queryParams.get('chart') || './grouped-by-metric.js';
+
+const chartScript = document.createElement('script');


wwwillchen · 2017-06-13T22:53:44Z

plots/charts/index.html

@@ -16,14 +16,10 @@
 </head>

 <body>
-  <nav>
+  <nav id="nav">


wwwillchen · 2017-06-14T01:17:40Z

plots/utils.js

+ * @param {string} filePath
+ */
+function removeRecursive(filePath) {
+  try {


I've removed this (I think I can do away with it since I'm just copying the files individually into the correct out folder).

wwwillchen · 2017-06-14T01:17:48Z

plots/utils.js

+ * @param {string} src
+ * @param {string} dest
+ */
+function copyRecursive(src, dest) {


wwwillchen · 2017-06-14T01:19:40Z

plots/measure.js

+ * @return {string}
+ */
+function generateOutPath() {
+  if (args.out) {


wwwillchen · 2017-06-14T01:19:46Z

plots/charts/index.html

  </nav>
-  <script src="./out/generatedResults.js"></script>
-  <script src="./bars-per-site.js"></script>
+  <script src="../generated-results.js"></script>


paulirish · 2017-06-14T02:21:49Z

plots/measure.js

+  if (args.out) {
+    return path.resolve(__dirname, args.out);
+  }
+  const date = new Date().toISOString();


you'll need to replace colons in the ISOString

googlebot · 2017-06-14T02:44:38Z

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored by someone other than the pull request submitter. We need to confirm that they're okay with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of the commit author(s) and merge this pull request when appropriate.

paulirish · 2017-06-14T02:46:33Z

added 3 commits that fix my remaining concerns. :)

wwwillchen · 2017-06-14T17:58:52Z

@paulirish - thanks for the changes! let's merge?

wwwillchen added 4 commits June 7, 2017 14:51

up

8b5ec74

Merge branch 'master' of https://github.com/GoogleChrome/lighthouse i…

7813584

…nto plots-better-out

fix lint errors but not working

0ca2b21

fix

f364a20

patrickhulce reviewed Jun 8, 2017

View reviewed changes

patrickhulce suggested changes Jun 8, 2017

View reviewed changes

use URLSearchParams

e7b5bb2

paulirish requested changes Jun 13, 2017

View reviewed changes

wwwillchen added 8 commits June 13, 2017 15:25

Merge branch 'master' of https://github.com/GoogleChrome/lighthouse i…

a5fd9ac

…nto plots-better-out

update

2ae5d6a

should be done with edits

929f680

fixes

0def3bf

clean up

1e7f198

fix

cfbde4c

nits

9c0da0e

removed dead code

19e0d64

wwwillchen commented Jun 14, 2017

View reviewed changes

paulirish reviewed Jun 14, 2017

View reviewed changes

paulirish added 3 commits June 13, 2017 19:43

pretty print when logging metrics to stdout.

ff45a3a

route charts with function refs instead. clean query param. typo fixes.

688c8a3

sanitize folder paths

4e293d4

paulirish approved these changes Jun 14, 2017

View reviewed changes

fix typo

e7e2151

paulirish merged commit 3ccd170 into GoogleChrome:master Jun 14, 2017


		/* eslint-env browser */

		function parseQueryParameters(queryParams) {

plots: better support power use cases #2464

plots: better support power use cases #2464

Conversation

wwwillchen commented Jun 7, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

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

wwwillchen commented Jun 8, 2017

patrickhulce commented Jun 8, 2017

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wwwillchen 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

googlebot commented Jun 14, 2017

paulirish commented Jun 14, 2017

wwwillchen commented Jun 14, 2017

wwwillchen commented Jun 7, 2017 •

edited

Loading