Better attribution type

[tunetheweb]

Fixes #408

[brendankenny]

This works for the issue in #408, but the better solution would be some variation on

export type MetricWithAttribution =
  | CLSMetricWithAttribution
  | FCPMetricWithAttribution
  | FIDMetricWithAttribution
  | INPMetricWithAttribution
  | LCPMetricWithAttribution
  | TTFBMetricWithAttribution;

I think we were considering that with the Metric interface as well, but settled in doing that in a separate MetricType. I don't totally remember why, though. Documentation? Or to avoid a breaking change?

If it's a top-level union, then it both fixes the type widening in #408, but also lets you narrow without an assertion, e.g. if you have a generic metricWithAttribution you can check the name so the compiler knows which metric it's dealing with

if (metricWithAttribution.name === 'LCP') {
  const lcpEntry = metricWithAttribution.attribution.lcpEntry; // Property known to be present.
  // ...
}

nbd if this isn't a path we want to go down today, but both Metric and MetricWithAttribution feel like they're leaving typescript functionality behind and making them slightly harder to use than they need to be.

[tunetheweb]

Alternatively we could introduce a new MetricWithAttributionType with that definition (which would be consistent with MetricType) and leave MetricWithAttribution alone for now. Then make the MetricWithAttribution change in v4, to avoid a breaking change for now incase we have any more v3 releases.

It does seem odd to have both interfaces and types defined though but maybe that's OK in this case?

@mcanu any thoughts here?

[tunetheweb]

Anyone any more thoughts on this?

@brendankenny @mcanu @philipwalton

[philipwalton]

@brendankenny can you take a look at this and make a recommendation? I think any change here might be a breaking change, so it'd be good to get this in the v4 release if possible.

[philipwalton]

Also, once we have a decision we can update this PR's base branch to the v4 branch.

[brendankenny]

If there's going to be a breaking change, definitely switching to the unions at the top level would be ideal (e.g. get rid of Metric and have MetricType be the real thing, similar with MetricWithAttribution), then move the types used for documentation to just be documentation.

I would love to give all the types a little refresh but I don't know if I'll have the time in the next ~24 hours if that's the goal for v4.

[tunetheweb]

CLosing in favour of #471