-
Notifications
You must be signed in to change notification settings - Fork 9.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
refactor(aggregations): switch usage of aggregations to categories #1935
Conversation
MC 🔨 |
⛰ |
there's only so much more cryptic my emoji messages can get so PTAL :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🤦♂️ 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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
keep the function and param descriptions?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
lighthouse-core/config/config.js
Outdated
* @param {!Array<string>} categoryIds | ||
* @return {!Object} | ||
*/ | ||
static generateNewConfigOfCategories(config, categoryIds) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 const
ness in this case
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
lighthouse-core/config/config.js
Outdated
id: agg.id | ||
})); | ||
/** | ||
* @param {!Object<{audits: !Array<{id: string}>}>} categories |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you mean all this time you've misled us!? :P haha will do
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
!Object<string, {audits: !Array<{id: string}>}>}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
@@ -61,6 +61,30 @@ class ReportGeneratorV2 { | |||
} | |||
|
|||
/** | |||
* Convert categories into old-school aggregations for old HTML report compat. | |||
* @param {!Array<!Object>} categories |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
!Array<{name: string, description: string, id: string, score: number, audits: !Array}>
? :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we could maybe move the JSON.parse(JSON.stringify(defaultConfig))
to a before()
to make it more consistent per test?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
need to do a deep freeze if we do the test this way, e.g. setting origConfig.categories.pwa = 5
wouldn't throw here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
reverted
describe('generateConfigOfAggregations', () => { | ||
const aggregationIDs = ['perf']; | ||
describe('generateConfigOfCategories', () => { | ||
const categoryIds = ['performance']; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: inheriting this, but maybe just inline this in the three tests that use it? doesn't seem helpful to have on its own
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
@@ -155,16 +155,16 @@ function onGenerateReportButtonClick(background, selectedAggregations) { | |||
|
|||
/** | |||
* Generates a document fragment containing a list of checkboxes and labels | |||
* for the aggregation categories. | |||
* for the category categories. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
delete category
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
017d74a
to
b7d46ba
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes correct. "break" very gently. :)
@patrickhulce going to rebase? |
Oh yep, sorry! |
replaces other major spot that needed aggregations
Paves the way for...
only
setting in config to specify specific categories/audits to run