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.

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?

done

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

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.

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

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?

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?

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.

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.

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?

sure

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 :)

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

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

sure

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 :)

already have....?

fixed

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

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

shall we alphabetize all of these listings?

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.

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.

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

done