core(audit): add options param to make{Table,Opportunity}Details

[alexnj]

Might make sense to include in 10.0 per our discussion re: stabilizing API changes.

Quoting @connorjclark:

If we consider Audit.makeTableDetails part of our stable API (we probably should), then to defer this change post-10.0 we will need to support both all the params one-by-one, plus a single options object (or maybe headings, results, opts for just the optionals, but at that point should probably just collapse all of them into the object). Checking at runtime the number of parameters (or whatever) wouldn't be a big deal, and actually doing a breaking change here will just needlessly break custom audits, so I can see us doing that indefinitely.

[alexnj]

We're mandating the time savings for opportunities. I've maintained previous expectation now, but may be this is something to consider. We could relax this and default to 0 when savings aren't supplied.

[brendankenny]

overallSavingsMs should be required because opportunities have to be able to quantify their savings to be opportunities. We should maybe leave a comment on it to that effect so it's clearer.

(uses-rel-preload is the only exception because it always marks itself as not applicable to remove itself from the report while it's buggy and needs to be fixed)

[connorjclark]

1. For backcompat, I think we should still support the old interface. No need to break custom audits when we can do a minor thing to maintain it. This would look like using arguments.length to interpret the args in the old way, while using the new way as the actual TS interface

2. we should probably do makeTableDetails(options) instead of makeTableDetails(headings, results, options = {}), so that adding another required parameter doesn't require another change to the argument length.

[brendankenny]

for future blame/debugging, it would be helpful to include the object shape this is preparing for and/or link to discussion of that in the other PRs

[brendankenny]

One interesting thing is that LH.Audit.Details.Table['summary'] (wastedMs/wastedBytes) has been unused in the report for a long while now, so I'm not sure if it has much value anymore.

LH.Audit.Details.Opportunity['overallSavingsBytes'] is similarly unused, but I definitely use it all the time in http archive queries since it's more or less standardized in generation since it's currently only created by ByteEfficiencyAudit, so really prefer we not ditch it :)

--
If we're interested in maintaining backwards compatibility without anyone having to update their custom audit code (seems good in this case since they won't gain anything from requiring them to change code), we should limit extra support code as much as possible (if we don't make people update now, it won't make sense to make people update in 11, 12, etc, so this code will likely be forever).

  /**
   * @param {LH.Audit.Details.Table['headings']} headings
   * @param {LH.Audit.Details.Table['items']} results
   * @param {{wastedMs?: number, wastedBytes?: number}} [options]
   * @return {LH.Audit.Details.Table}
   */
  static makeTableDetails(headings, results, options = {}) {}

won't move everything to a single options object, but it would be fully compatible with any current calls (vs nested options.summary) and be future expandable.

Meanwhile technically anyone can use makeOpportunityDetails, but type: 'opportunity' only renders differently in the perf category (which is the only place overallSavingsMs is ever used), otherwise they're just treated as a table alias. In order for someone to have a custom audit calling makeOpportunityDetails separate from table behavior, they'd have to use a custom config to extend the performance category, at which point we're already asking them to make a bunch of changes for 10.0....I think we should just make a clean break. (and FWIW, there's no usage of makeOpportunityDetails on github outside of Lighthouse core).

[connorjclark]

Looks good, thanks for iterating!