misc: minor cleanup of audit-details type names

[brendankenny]

see below for discussion of original PR and why it was dropped.

Instead, minor cleanup of audit-details types:

• ItemValueTypes should never have been plural
• Values moved to be in line with other table details names
• make OpportunityItem relationship with TableItem clearer
• drop table-unification TODO owner

breaking change: two property name changes in table audit details

follow up to #7177 and #7192 (apparently this was a good task for v5 :)

We have two table-like audit details -- table and opportunity -- that just barely differ in their table-ness:

• their column headings format have different names for two properties
• opportunity items (elements) are essentially table items with extra constraints
  (the other stuff on the two objects is fine and not involved in table details rendering)

To render the tables, #7192 introduced a function that turned table tables into opportunity tables so there could be a single rendering path.

So to unify things, it's just a simple rename of the properties in the tableheadings so everyone is happy and we can remove the old unification function (since grown to functions :). It's good housecleaning but also makes ongoing table rendering work simpler...e.g. subRows had to be added twice, in both table and opportunity tables.

There's a good bit of churn visually in this PR, but it's mostly s/itemType/valueType and s/text:/label:. No test or rendering changes. Significant code changes are in details-render.js and audit-details.d.ts.

[brendankenny]

OpportunityItem was already a TableItem in tsc's eyes (since it checks using structural typing), this just makes the relationship more explicit. OpportunityItem exists to let opportunity audits know what properties to include on their table items, while the more generic TableItem is what details-renderer knows how to render (regardless of specific keys).

[brendankenny]

giving it to you @connorjclark since you touched tables last :)

[patrickhulce]

I know you're not going to like this, but, I feel strongly that this type of change should go into the beta release of a major and not be snuck in in between (or at the very least flagged as an upcoming breaking change in the release notes of the beta as a warning to early testers).

It is a change to the structure of details that anyone implementing an alternative renderer (lhci, @mattzeunert @alekseykulikov @benschwarz ) will now have to test against with much less (potentially zero?) lead time.

Also if this gets surfaced in PSI (I think it does?) then that strongly warrants a major version breaking change there because this is the shape of the data changing, not just "sorry some performance metrics might change".

[paulirish]

Also if this gets surfaced in PSI (I think it does?) then that strongly warrants a major version breaking change

heyyyyyy now. slow down there 🤠 haha

---

but, aside from that I agree this is too 'breaking' of a change to introduce at this point in the dev cycle.

One alternative, if we want to land something now, is to add valueType and label props to the opportunityitem type. So we could do the non-breaking 1st half of this in 6, message the deprecation, and complete in 7.

[brendankenny]

I know you're not going to like this, but, I feel strongly that this type of change should go into the beta release of a major and not be snuck in in between (or at the very least flagged as an upcoming breaking change in the release notes of the beta as a warning to early testers).

That's fine, I was working on some i18n stuff and it felt stupid to be duplicating it across these two details types, so it seemed like a good idea to finally fix the breaking TODO before 6.0 :)

It's really good someone maintaining an alternative renderer is on the core team so these problems come up in review, but I guess the frustrating part is we don't have a process established, so e.g. it's not clear to me why it's not feasible to have another beta between now and...a month from now?...when we do the release.

I will, however, be bringing up this example forever whenever we discuss whether something should be added to our public interface :)

One alternative, if we want to land something now, is to add valueType and label props to the opportunityitem type. So we could do the non-breaking 1st half of this in 6, message the deprecation, and complete in 7.

This seemed like a good idea to me at first, but one thing that still had to be added to this PR was something in prepareReportResult to support LHRs with tables from Lighthouse < 6.0. If we're producing LHRs with the old properties and supporting rendering LHRs with the old properties, the special cases risk outweighing the code we have today (which already does both of those things), so we should probably just leave it as is and figure out timing on a different day.

[patrickhulce]

LGTM, thanks for the flexibility! :)