report: sort audits by weight

[connorjclark]

Sort auditRefs by weight to influence render order in report.

Maybe useful diff in auditRefs order: https://www.diffchecker.com/NmdJHTbN

ref #12996

[brendankenny]

this might be a better fit in the category renderer since we're really using it for rendering side effects. That might(?) also allow easier splitting on category type, since it seems like we shouldn't be doing this for the perf category, at least

[connorjclark]

hadn't realized it changed the order of the metrics in the metric table. But the new order seems fine? is there any intention behind the current order?

this might be a better fit in the category renderer

lighthouse/report/renderer/category-renderer.js

Line 516 in 344a156

for (const [clumpId, auditRefs] of clumps) {

a) make a new array here const auditRefsByWeight = [...auditRefs].sort(...),
b) or mutate the array (mutating was the only thing I had considered, which is why I thought to do it in the only place we mutate the LHR)?

[brendankenny]

make a new array here const auditRefsByWeight = [...auditRefs].sort(...),

this one sounds good to me

[brendankenny]

hadn't realized it changed the order of the metrics in the metric table. But the new order seems fine? is there any intention behind the current order?

I believe there is an intentional order (or maybe there was an intentional order and we've organically evolved since then?), but we also don't necessarily want to rearrange things on people (at least in a minor release) since you get pretty used to the metric order

[connorjclark]

v9 then?

at the moment, LCP isn't even next to FCP... + things aren't sorted by weight ... so no idea if there is any logical framework to the order right now.

[paulirish]

(aside from my above comments.. i think this definitely improves the accessibility category) 👍

[brendankenny]

connorjclark added the 9.0 label yesterday

If we don't change the perf audit ordering (which I (personally) don't think we should do anyways), I think it'd be fine to ship sorting the accessibility, etc audits without a breaking change. It's definitely an improvement and not a disruptive one.

[brendankenny]

i dont care too much about placement.. this location also seems fine.

I really think it should be in category-renderer:

It's category rendering, not preparing data for compatibility. If you go looking for what's ordering audits in a category in the report, the most logical place is in CategoryRenderer.render() (or LHR generation, but agreed with #13053 (comment)). It's easy to unit test that way. We get abstaining from performance category renderer changes automatically. win-win-win-win :)

[brendankenny]

And it would parallel the equivalent functionality in performanceCategoryRenderer for opportunities

lighthouse/report/renderer/performance-category-renderer.js

Lines 214 to 216 in 1e22548

	const opportunityAudits = category.auditRefs
	.filter(audit => audit.group === 'load-opportunities' && !Util.showAsPassed(audit.result))
	.sort((auditA, auditB) => this._getWastedMs(auditB) - this._getWastedMs(auditA));

and diagnostics

lighthouse/report/renderer/performance-category-renderer.js

Lines 247 to 253 in 1e22548

	const diagnosticAudits = category.auditRefs
	.filter(audit => audit.group === 'diagnostics' && !Util.showAsPassed(audit.result))
	.sort((a, b) => {
	const scoreA = a.result.scoreDisplayMode === 'informative' ? 100 : Number(a.result.score);
	const scoreB = b.result.scoreDisplayMode === 'informative' ? 100 : Number(b.result.score);
	return scoreA - scoreB;
	});

[connorjclark]

I agree, I just haven't gotten around to it yet.

[connorjclark]

Oh, and I chose to sort in the report to 1) fix all rendering all previous LHRs 2) we don't necessarily want to sort audit refs in the default config