[WIP] feat: base config v2 functionality #1880

patrickhulce · 2017-03-20T21:35:05Z

Implements the base functionality for the move to config v2 upon which all other features for the new format can be implemented.

design doc
related #1874 #1875 #1876 #1463 #1826

Missing because WIP (will be fixed before merge):

~~category description text~~
~~jsdocs~~
~~tests~~
consensus :)

Missing because MVP:

settings object
audit/gatherer options
customizable scoring functions (both proposed are already implemented just hardcoded atm)
handling of auditResults/artifacts/trace files, planned to be split from config entirely in v2

Current look:

paulirish

can you also hook up --perf to select the perf aggregation? would love to see that.

EDIT: ah, this is dependent on the settings object. That's coming, I see. :)

paulirish · 2017-03-20T22:11:09Z

lighthouse-core/config/v2/config.js

+    }
+
+    // Perform a shallow clone so we can adjust gatherers and audits
+    configJson = Object.assign({}, configJson);


I think you'll need to do parse/stringify otherwise the original configJson object can be mutated, right?

I do shallow copies on any children I mutate and whenever this is exposed to outside for potential mutation it's parsed/stringified in asJson, this was mainly to preserve the ability to call from javascript with audits: {custom: {implementation: MyAuditClass}}

or at least I try to do shallow copies everywhere, we'll see when I add tests :)

or at least I try to do shallow copies everywhere, we'll see when I add tests :)

test will be good :) assert.notDeepStrictEqual might be sufficient, but we could also do something with deep Object.freeze and see what throws (in strict mode)

Especially important for the browserified/lighthouse-as-a-module case since when loading the config json via require() any changes to the original object is persisted to the next use of it.

paulirish · 2017-03-21T18:20:21Z

lighthouse-core/runner.js

          artifacts: runResults.artifacts,
          runtimeConfig: Runner.getRuntimeConfig(opts.flags),
+          categories,


should we put this inside a report object on the result payload? or title it reportCategories to communicate clearly that it's just presentational?

Yeah I'm in favor of restructuring this object for v2, should probably add a section to the doc to discuss format

I was thinking something like the following?

{ report: { meta: {version, time, url}, // stuff thats currently top-level config, // full config used (after extension, cli overrides, etc) score, // final overall score categories, // heirarchy of computed scores/results audits // individual results by audit id } // the stuff that gets written to report.json and whose contents can be used to render the HTML report without any additional information artifacts: {gatherers, trace, perfLog/devtoolsMessages} // used to quickly re-run analysis/perform later deep dives/debug/etc }

agree that top level categories here is a bit confusing. Can we call it reportCategories until we settle on a result object shape?

paulirish · 2017-03-22T01:56:07Z

lighthouse-core/config/v2/config.js

+    }, []).sort((itemA, itemB) => itemA.order - itemB.order);
+  }
+
+  static getGathererImplementations(gatherersObject) {


this and getAuditImplementations appear to be the same. You leaving them WET for now expecting them to change in the future?

So they are, haha I'll DRY up. It was copypasta I expected to modify but didn't need to in the end

paulirish · 2017-03-22T02:00:57Z

lighthouse-core/config/v2/config.js

+
+    if (gathererIds.size !== usedGathererIds.size) {
+      const unused = _subtract(gathererIds, usedGathererIds);
+      log.warn('config', `Gatherers are unused: ${unused.join(', ')}`);


is this a bad thing? or just noting they are being culled from the working config based on the selection.

I'm fine with warn but the text could be more neutral to indicate its okay. (I'm expecting that this will log a lot when you do --perf)

Seemed like a good thing to warn about in case you misname or something, this function is likely to experience the most change when we add only/skip etc, so I'd say we can re-evaluate or just remove in the filtered case.

paulirish · 2017-03-22T02:42:18Z

Super excited about this work. :)

On the whole I'm super excited about the new declarative config. The v2/config is a big upgrade over the current config.

Let's get some tests in place. IMO Category text can wait for another PR.

I spoke with @brendankenny and we're thinking of him enabling the v2 flag which enables the DOM based report renderer. This will start as an empty page and we'll build it out from there.

brendankenny

mostly higher level feedback, with a few style things.

A lot of really good work here, it's going to make the config a lot cleaner.

brendankenny · 2017-03-22T03:39:18Z

lighthouse-core/config/v2/config.js

+const defaultConfigJson = require(defaultConfigPath);
+
+const _flatten = arr => [].concat(...arr);
+const _subtract = (setA, setB) => Array.from(setA).filter(x => !setB.has(x));


maybe _difference? also should this return a Set as well?

sure, and it did at first but it never gets used as a set, just as an array, I'll rename to _differenceAsArray

brendankenny · 2017-03-22T03:43:21Z

lighthouse-core/config/v2/default.json

@@ -0,0 +1,308 @@
+{
+  "version": 2,


share @ebidel's concern over the genericness of version. Personally I think configVersion works, but I also think we don't necessarily need anything here since we're planning on supporting just one type of config at a time, and we can just go with config verification with nice error messages instead (we could even include a check for "looks like you're using the old config format...here's a link").

Otherwise it's just a magic number users have to get right or lighthouse complains.

Agreed we should have verification of the config format but we explicitly are attempting to support two config versions at once to start as we iterate, so I think the version has value as a clue of which path to head toward. A try until it works approach with validation and error messages greatly complicates the MVP. How about configVersion until it becomes the default and we nuke the field entirely and rely on validation only?

brendankenny · 2017-03-22T03:45:21Z

lighthouse-core/config/v2/default.json

+    "styles": {},
+    "css-usage": {},
+    "all-event-listeners": {
+      "path": "dobetterweb/all-event-listeners"


as we were talking about, if some of the core gatherers can be referred to by their id they probably all should be. Having subdirectories is just an artifact of our repo organization process, not anything inherent to the gatherer

Not sure what you're trying to say here. IMO, this is actually a step in the direction of decoupling our arbitrary repo folder structure from magic happening in the config. What leads you to believe our repo organization process is now inherent to the gatherer now?

brendankenny · 2017-03-22T03:54:20Z

lighthouse-core/config/v2/default.json

+    "theme-color-meta": {},
+    "content-width": {},
+    "deprecations": {},
+    "aria-allowed-attr": {


what does the object key ("aria-allowed-attr" here) do functionally?

On current master, aggregations refer to audit.name values, while this string would be used only to look up the path to that audit. For configv2 the path property here is used to find the file, so is the object key used for nothing or is it now what's referred to by the category's children?

For audits, as with each of these toplevel objects, the keys are the id (currently audit.meta.name) of the audit (rather than have it redundantly specified inside the object as well). There's really no need for audits to have category or name in their implementation now either.

brendankenny · 2017-03-22T03:55:29Z

lighthouse-core/config/v2/default.json

+      "pwa": {
+        "name": "Progressive Web App",
+        "description": "",
+        "children": [


children seems like the wrong relationship here. Was audits bad? It does make it clear that this is a set of audits... :)

children was used forseeing the whole subcategory discussion occurring and the likely scenario that we will need to add categories as children of other categories. Keeping it generic with children, IMO, leaves open the possibility without a breaking change. In the proposal, each child has a type: "audit"|"category"|"some-newfangled-thing" but since all we use are audits today it seemed unnecessary.

I'm in favor of keeping "audits" for now. I read this block as "this category contains/runs these audits". That naming is consistent with the list of "audits" above and is probably easier for contributors to make the linkage when adding new audits.

brendankenny · 2017-03-22T03:56:32Z

lighthouse-core/gather/gather-runner.js

@@ -431,6 +431,11 @@ class GatherRunner {
  static instantiateGatherers(passes, rootPath) {
    return passes.map(pass => {
      pass.gatherers = pass.gatherers.map(gatherer => {
+        if (gatherer.implementation) {


here (and in a few other places (report-generator, runner) where there's a special conditional branch) can you make sure to add some sort of // for config v2 so it's clear and easy to spot?

brendankenny · 2017-03-22T04:01:48Z

lighthouse-core/report/v2/report-generator.js

+    }, {});
+
+    const score = this._geometricMean([
+      {score: scoreByCategory.pwa, weight: 5},


hard coding category names and weights here seems like a mistake. Were you waiting to add a scoring property? Waiting is good, but can you just add weight to the category objects and skip this? (I'll comment in the design doc)

I also think using _geometricMean here is premature until we've had a real discussion on scoring, but happy to have it here for now :P

Yeah I was waiting to introduce scoring methods and since nothing scoring wise has changed despite lots of talk the past few months I thought I'd poke the bear and see what came of it :P

Anything related to scoring does feel like a future PR. There's enough to bit off in here with all the other changes :)

brendankenny · 2017-03-22T04:03:06Z

lighthouse-core/runner.js

          artifacts: runResults.artifacts,
          runtimeConfig: Runner.getRuntimeConfig(opts.flags),
+          categories,


agree that top level categories here is a bit confusing. Can we call it reportCategories until we settle on a result object shape?

brendankenny · 2017-03-22T04:05:42Z

lighthouse-core/config/v2/default.json

+
+  "passes": {
+    "defaultPass": {
+      "order": 10,


can we kick order from the MVP so we can discuss it in a separate PR? I'm still not really a fan and with no motivating code that needs it yet there's no rush

brendankenny · 2017-03-22T04:15:23Z

lighthouse-core/config/v2/config.js

+    ]);
+
+    // Extend only after our paths have been resolved
+    configJson = ConfigV2.extendIfNecessary(configJson, configPath);


I kind of want to punt extensions from the MVP as well, that's going to take some thought to get it right, but it looks like you're just matching current behavior here?

it looks like you're just matching current behavior here?

The intent behind several of the config changes was specifically to enable clear and simple extension not needing logic or fancy behavior of any sort, so this is the final behavior as far as I'm concerned. That's what the structure of the new config enables :)

brendankenny · 2017-03-22T06:07:48Z

I'd really like to clear up the audit/gatherer id/name/classname/key-in-various-objects distinctions a bit more.

What is the relationship between the keys in the gatherers top level object and the gatherers object in each pass? Do the pass gatherer keys reference the keys in the top level object?
Same thing for the audits top-level object keys vs the id values in each category's children array (I believe I asked this in a comment)
Is there a way to use a gatherer twice with different input? This is hard since we use artifacts[classname] currently to identify the output of each gatherer, so we may want to table this for now.
Is there a way to use an audit twice? Since we're letting audits take input with this new version this is going to be a great thing. Ideally I could do something like

"time-to-interactive-cold": {
  "path": "time-to-interactive",
  "inputTrace": "firstPass"
},
"time-to-interactive-warm": {
  "path": "time-to-interactive",
  "inputTrace": "laterPass"
}

(modulo better naming/clearer handling of path or whatever, etc)
and then being able to refer to time-to-interactive-cold and time-to-interactive-warm as different things in my categories. This would require using the keys as ids though, and not something on the audit itself.

I haven't fully thought that through yet, so there may be issues with that :)

So, basically, it would be sweet to simplify the id/key/path thing a bit more and figure out for sure how we're going to allow single audits multiple times (bonus points for gatherers multiple times, though that seems a lot messier).

brendankenny · 2017-03-22T06:25:45Z

kind of shitty idea, everyone feel free to disagree: to iterate on some of this a bit more but not block the new report, we could make even more of an MVP by making the config v2 exactly the same as v1 except have report.categories instead of aggregations. I think everyone is already in agreement on the report.categories stuff here.

config.js probably wouldn't need to change much from master and the report generation/scoring could stay as it is in this PR. It's mostly v2/config.js that wouldn't be needed yet.

Just to be clear, this is all really good and would just get rolled over to the next change, I'm just thinking about this because I don't want to slow us way down by making us talk about all these things.

...? :)

patrickhulce · 2017-03-22T18:32:53Z

audit/gatherer id/name/classname/key-in-various-objects

I seem to have failed in config clarity given that I think the biggest win of config v2 is getting rid all of most of this nonsense and moving everything to IDs (apart from artifact name being class name...still need to hash that one out). The keys for all toplevel objects (gatherers, audits, passes) are IDs of that object. This ID should how everything else in the config refers to that particular object. Examples below.

What is the relationship between the keys in the gatherers top level object and the gatherers object in each pass? Do the pass gatherer keys reference the keys in the top level object?

The strings specified in a passes gatherers property are the IDs of gatherers. The long-term view was that these wouldn't need to be strings but potentially objects specifying a gatherer ID and potential options (for example collecting similar data across passes, or with slightly different settings).

Same thing for the audits top-level object keys vs the id values in each category's children array (I believe I asked this in a comment)

Responded in comment too, but same story. These strings are the IDs of the audits.

Is there a way to use a gatherer twice with different input? This is hard since we use artifacts[classname] currently to identify the output of each gatherer, so we may want to table this for now.

Correcto, as mentioned above still need to hash this out, but my big controversial proposal will be to start storing artifacts by gatherer ID, ignore class name completely, and move requiredArtifacts to an input property of an audit (with a defaults suggested by impl so config doesn't balloon with unnecessary wiring).....buuuuuuut one major change at a time :)

Is there a way to use an audit twice? Since we're letting audits take input with this new version this is going to be a great thing. Ideally I could do something like

Bingo :)

This would require using the keys as ids though, and not something on the audit itself.

yes! this is exactly what I'm trying to move to.

patrickhulce · 2017-03-22T18:35:51Z

making the config v2 exactly the same as v1 except have report.categories instead of aggregations

😢 😢

Seriously though, IMO there's no need to have these set in stone until we actually flip the switch and kill config v1 making v2 the default. If we have buyer's remorse about passes as object/whatever, I don't see why we couldn't change it all around tomorrow (though I obviously hope major points could be ironed out in doc stage 😄 )

ebidel · 2017-03-22T22:27:33Z

lighthouse-core/config/v2/config.js

+  static resolvePaths(object, configPath, searchPaths = []) {
+    object = Object.assign({}, object);
+    Object.keys(object).forEach(key => {
+      // We don't need to resolve objects that already have


already have....?

ebidel · 2017-03-22T22:31:30Z

lighthouse-core/config/v2/config.js

+    return object;
+  }
+
+  static objectToList(object) {


is there a reason to make this a static? Can it be a helper like _differenceAsArray?

mainly for testing since it did do fancier things like ordering, but that's been nixed for a future PR and didn't seem worth moving

ebidel · 2017-03-22T22:57:19Z

lighthouse-core/config/v2/default.json

+{
+  "configVersion": 2,
+
+  "gatherers": {


what is the top-level gatherers for? list all possible gatherers and allow options?

yep :) lists all gatherers with their ids and paths (if different from id) plus allowing options and multiple version of the same implementation later on down the road

ebidel · 2017-03-22T23:00:51Z

lighthouse-core/config/v2/default.json

+  },
+
+  "audits": {
+    "is-on-https": {},


shall we alphabetize all of these listings?

ebidel · 2017-03-22T23:08:06Z

lighthouse-core/config/v2/default.json

+      "pwa": {
+        "name": "Progressive Web App",
+        "description": "",
+        "children": [


I'm in favor of keeping "audits" for now. I read this block as "this category contains/runs these audits". That naming is consistent with the list of "audits" above and is probably easier for contributors to make the linkage when adding new audits.

ebidel · 2017-03-22T23:10:16Z

lighthouse-core/config/v2/default.json

+    }
+  },
+
+  "report": {


I still thinks it's worth exploring modularity but am alright with adding a report section.

IOW,report could be it's own file (config/report.json). In that way, we keep configuration separate from output generation. As as an example, I could see users wanting to use our settings.json with their own report.json to create a custom report. They wouldn't have to change a mega file. That said, maybe your extension model smooths over that nasty....?

I'm definitely not opposed to separating the presentation config from the runtime config at some point, easy extension eases the immediate pain IMO.

ebidel · 2017-03-22T23:11:52Z

lighthouse-core/config/v2/default.json

+      },
+      "performance": {
+        "name": "Performance",
+        "description": "",


Might as well port over the descriptions from v1 while we're here.

feat: base config v2 functionality

48498e9

paulirish reviewed Mar 20, 2017

View reviewed changes

paulirish reviewed Mar 21, 2017

View reviewed changes

paulirish mentioned this pull request Mar 21, 2017

[meta] V2 Report Burndown #1887

Closed

12 tasks

patrickhulce added 2 commits March 21, 2017 16:29

remove numericScore

9cb6206

more fixes

69e0557

paulirish reviewed Mar 22, 2017

View reviewed changes

dry up collect implementations

a6699a5

brendankenny requested changes Mar 22, 2017

View reviewed changes

patrickhulce added 2 commits March 22, 2017 11:41

brendan's feedback

33125de

added config tests

744b335

ebidel reviewed Mar 22, 2017

View reviewed changes

patrickhulce added 3 commits March 22, 2017 16:19

add descriptions

5d53ff2

update message

cbfb847

linting

3d754e8

patrickhulce mentioned this pull request Mar 23, 2017

feat(report): add v2 with categories #1896

Merged

patrickhulce closed this Mar 27, 2017

[WIP] feat: base config v2 functionality #1880

[WIP] feat: base config v2 functionality #1880

Conversation

patrickhulce commented Mar 20, 2017 • edited Loading

paulirish left a comment • edited Loading

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

paulirish commented Mar 22, 2017

brendankenny 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

brendankenny Mar 22, 2017 • edited Loading

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

brendankenny commented Mar 22, 2017

brendankenny commented Mar 22, 2017

patrickhulce commented Mar 22, 2017

patrickhulce commented Mar 22, 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

patrickhulce commented Mar 20, 2017 •

edited

Loading

paulirish left a comment •

edited

Loading

brendankenny Mar 22, 2017 •

edited

Loading