report: add gatherMode option to category score

[adamraine]

Addresses #13009 (comment)

Plan to merge before after #13009

[adamraine]

Would be nice to merge environment into options. Would this be a breaking change?

[patrickhulce]

We could do a environmentOrOptions and detect with typeof environmentOrOptions === 'string'?

[connorjclark]

I think at the moment our contract begins and ends with ReportGenerator.generateReportHtml, renderer.renderReport, DOM, and ReportUIFeatures. Even that is being generous, there's an argument that only lighthouse --output=html (and Node equivalent) is officially supported.

Changes to CategoryRenderer presently would only break our PSI integration, which can be easily fixed on the next roll to google3 (and tests will prevent a regression).

[connorjclark]

also web.dev, but i don't have any memory of how that works :)

[brendankenny]

yeah, you have to be calling PerformanceCategoryRenderer specifically, so I don't see a problem breaking it, but if we're going so far as preserving PwaCategoryRenderer.renderScoreGauge to avoid a breaking change it might be better to sniff for 'PSI' vs {environment: 'PSI'}.

[patrickhulce]

I'm fine with this, but I personally don't believe this purity of LH.ReportResult.Category is all that worth it given the current state of things.

• One can easily forget to pass gatherMode in options just as easily one can forget to add it to the category object/invoke prepareReportResult.
• This moves the decision of whether a category should be fractional in nature to the individual score component rather than carrying this information alongside the category itself. If any other display component uses this information in the future, we're duplicating logic and threading through gatherMode down to that new component too.
• The interface change around PSI environment touches a brittle and undertested integration area.
• The tech debt we all agreed isn't ideal at the time that sparked this has had very little visible impact in the past three years since its introduction, which tells me it's not all that important to invest around.

But again, the work here has been done, and I don't suspect the consequences either way will be particularly large, so I'm fine either way.

[patrickhulce]

We could do a environmentOrOptions and detect with typeof environmentOrOptions === 'string'?

[brendankenny]

I don't think it's worth going too deep here, but to get some thoughts out:

• One can easily forget to pass gatherMode in options just as easily one can forget to add it to the category object/invoke prepareReportResult.

Agreed that forgetting is less of an issue, it's more that in an ideal API you wouldn't need to call it.

• This moves the decision of whether a category should be fractional in nature to the individual score component rather than carrying this information alongside the category itself. If any other display component uses this information in the future, we're duplicating logic and threading through gatherMode down to that new component too.

Agreed it's not clear yet what the best parameterization here is, but we can totally make that switch in this PR. reportRenderer is acting on the gatherMode but could dispatch a category-rendering specific option (displayMode/gaugeMode/scoreMode-but-that's-getting-close-to-scoreDisplayMode) to the categoryRenderer.renderCategoryScore and categoryRenderer.render() calls. This was more a question of where the option is, not what it is.

• The interface change around PSI environment touches a brittle and undertested integration area.

You won't get an argument from me :) but it's been improving. It seems like for environment, specifically, the plan might be to get rid of it in #12772?

• The tech debt we all agreed isn't ideal at the time that sparked this has had very little visible impact in the past three years since its introduction, which tells me it's not all that important to invest around.

That TODO is somewhat of a running joke by this point (and @adamraine set me up too perfectly for it but maybe I should have resisted :). I think that is is a good reminder that "eh, let's just make it work" solutions often live forever precisely because their downsides ever don't rise to a sufficient priority level. Philosophically/purity-wise it is what it is, but it doesn't hurt to reevaluate when building new stuff on it, which is why I brought it up.

[brendankenny]

yeah, you have to be calling PerformanceCategoryRenderer specifically, so I don't see a problem breaking it, but if we're going so far as preserving PwaCategoryRenderer.renderScoreGauge to avoid a breaking change it might be better to sniff for 'PSI' vs {environment: 'PSI'}.

[brendankenny]

LGTM

[brendankenny]

better as

Suggested change

	* @param {{gatherMode: LH.Result.GatherMode}=} options
	* @param {{displayMode: 'gauge'|'fraction'}}=} options

(or whatever option name)?

[adamraine]

We might want to adjust the display mode based on gather mode and category at some point. Are you saying we should do this only for this function?

[brendankenny]

I meant for all of render/renderCategoryHeader/renderCategoryScore and have reportRenderer.render() decide which option to pass in for how to display the score, but we can also defer if you'd prefer (they're equivalent for now). Whatever you think works/is easier to move forward with.