refactor(aggregations): switch usage of aggregations to categories#1935
refactor(aggregations): switch usage of aggregations to categories#1935patrickhulce merged 7 commits intomasterfrom
Conversation
|
MC 🔨 |
|
⛰ |
|
there's only so much more cryptic my emoji messages can get so PTAL :) |
paulirish
left a comment
There was a problem hiding this comment.
This PR currently does break the --perf config. I don't think that was intentional, was it?
|
🤦♂️ doh! it doesn't matter how many times this happens, I can't ever remember that there's a perf config... |
|
if only there were some automated way to get a machine to check if it's working.... :P |
hey I proposed adding to smoke and paul vetoed! fixed |
|
the :P wasn't aimed at you :) |
|
FYI The new perf smoketest PR (#1957) wouldn't actually catch the above bug, as the JSON results are fine.. The failure was more on the reportGenerator side. |
brendankenny
left a comment
There was a problem hiding this comment.
looking good. Lots of comments but most are about jsdocs :)
lighthouse-core/config/config.js
Outdated
| const config = JSON.parse(JSON.stringify(oldConfig)); | ||
| // 1. Filter to just the chosen aggregations | ||
| config.aggregations = config.aggregations.filter(agg => aggregationIDs.includes(agg.id)); | ||
| config = Object.assign({}, config); |
There was a problem hiding this comment.
do we need to drop the JSON.parse(JSON.stringify())? If this is still just for the extension/devtools we're never going to have a live gatherer or audit class.
As hacky as it looks, until we have a proper deep clone utility it does ensure we don't have to audit all the methods this function calls (and you don't have to keep a copy of default.js open for reference to make sure every part is properly Object.assign()ed); we know the code can't modify the original even if it wanted to
There was a problem hiding this comment.
I dislike the idea of random parts of config not supporting a consistent feature set a little more than being careful about mutation, but I'll revert for now
There was a problem hiding this comment.
I dislike the idea of random parts of config not supporting a consistent feature set a little more than being careful about mutation
yeah, I agree. I was thinking more of future changes to the config format and having to reverify this invariant every time. e.g. we add some object on the audit objects in the future and in that PR we forget to verify that we Object.assign() at that level as well, and then (maybe in some future PR after that) the report generator or whatever augments those audit objects, and then we have those extra properties on subsequent uses of the default config in the extension/tests.
Part of that is inevitable due to require()'s behavior, and maybe we should only be loading config files with fs.readFile(), but regardless it's technical debt which we noted we had when we first wrote JSON.parse(JSON.stringify()), so we really should have come up with a better solution before now :S
lighthouse-core/config/config.js
Outdated
| * @return {Object} new config | ||
| */ | ||
| static generateNewConfigOfAggregations(oldConfig, aggregationIDs) { | ||
| * @param {!Object} config |
There was a problem hiding this comment.
keep the function and param descriptions?
lighthouse-core/config/config.js
Outdated
| * @param {!Array<string>} categoryIds | ||
| * @return {!Object} | ||
| */ | ||
| static generateNewConfigOfCategories(config, categoryIds) { |
There was a problem hiding this comment.
I think I personally prefer the old way of differentiating between the parameter passed in and the config being operated on with different variable names, but maybe there's an argument I'm missing. It seems like you lose a clear definition point, especially because you lose the constness in this case
lighthouse-core/config/config.js
Outdated
| id: agg.id | ||
| })); | ||
| /** | ||
| * @param {!Object<{audits: !Array<{id: string}>}>} categories |
There was a problem hiding this comment.
learned recently that, from the Closure Compiler side of things, at least, we really should be adding the string for the object key type in these: @param {!Object<string, {audits: !Array<{id: string}>}>}, otherwise you don't get the parameterized type checking on the entire parameter. Unfortunate but not the worst thing ever
There was a problem hiding this comment.
you mean all this time you've misled us!? :P haha will do
There was a problem hiding this comment.
you mean all this time you've misled us!?
I know! Also, it's dumb that it doesn't default to string cause that's what you get no matter what you use anyways
lighthouse-core/config/config.js
Outdated
| * @param {!Object<{audits: !Array<{id: string}>}>} categories | ||
| * @param {Array<string>=} categoryIds | ||
| * @param {Array<string>=} auditIds | ||
| * @return {!Object<{audits: !Array<{id: string}>}>} |
There was a problem hiding this comment.
!Object<string, {audits: !Array<{id: string}>}>}
| } | ||
|
|
||
| /** | ||
| * Convert categories into old-school aggregations for old HTML report compat. |
There was a problem hiding this comment.
!Array<{name: string, description: string, id: string, score: number, audits: !Array}>? :)
| const haveName = aggs.every(agg => agg.name.length); | ||
| const haveID = aggs.every(agg => agg.id.length); | ||
| assert.equal(haveName === haveID === true, true, 'they dont have IDs and names'); | ||
| const categories = Config.getCategories(defaultConfig); |
There was a problem hiding this comment.
we could maybe move the JSON.parse(JSON.stringify(defaultConfig)) to a before() to make it more consistent per test?
| const origConfig = JSON.parse(JSON.stringify(defaultConfig)); | ||
| Config.generateNewConfigOfAggregations(defaultConfig, aggregationIDs); | ||
| assert.deepStrictEqual(origConfig, defaultConfig, 'Original config mutated'); | ||
| const origConfig = Object.freeze(JSON.parse(JSON.stringify(defaultConfig))); |
There was a problem hiding this comment.
need to do a deep freeze if we do the test this way, e.g. setting origConfig.categories.pwa = 5 wouldn't throw here
| describe('generateConfigOfAggregations', () => { | ||
| const aggregationIDs = ['perf']; | ||
| describe('generateConfigOfCategories', () => { | ||
| const categoryIds = ['performance']; |
There was a problem hiding this comment.
nit: inheriting this, but maybe just inline this in the three tests that use it? doesn't seem helpful to have on its own
| /** | ||
| * Generates a document fragment containing a list of checkboxes and labels | ||
| * for the aggregation categories. | ||
| * for the category categories. |
017d74a to
b7d46ba
Compare
brendankenny
left a comment
There was a problem hiding this comment.
LGTM!
👋 aggregations
|
arg, sorry, I thought I caught any potential conflicts with #1964 but looks like I failed. Just @paulirish do you want to take a look since you were in the filtering/config generation code last? |
| Config.generateNewConfigOfAggregations(defaultConfig, aggregationIDs); | ||
| assert.deepStrictEqual(origConfig, defaultConfig, 'Original config mutated'); | ||
| const configCopy = JSON.parse(JSON.stringify(origConfig)); | ||
| Config.generateNewConfigOfCategories(configCopy, ['performance']); |
There was a problem hiding this comment.
just as an FYI, the rename of "perf" to "performance" will break any saved checkboxes people have in audits 2.0.
(that's totally fine with me at this stage, but something to keep in mind going forward)
There was a problem hiding this comment.
failure mode will be if they have it unchecked it will now be checked, right? We'll definitely want to be additive only in the future, especially once your checkboxes in devtools are persisted
There was a problem hiding this comment.
yes correct. "break" very gently. :)
|
@patrickhulce going to rebase? |
|
Oh yep, sorry! |
replaces other major spot that needed aggregations
Paves the way for...
onlysetting in config to specify specific categories/audits to run