GoogleChrome / web-vitals

Essential metrics for a healthy site.
https://web.dev/vitals
Apache License 2.0
7.47k stars 410 forks source link

Expose the target element in INP attribution #478

Closed philipwalton closed 3 months ago

philipwalton commented 3 months ago

Fixes #456.

This PR exposes the HTMLElement object in addition to a selector pointing to that object, to support use cases where additional information from the element itself is useful.

The following properties were added:

Breaking change:

In addition, since we realized we had inconsistent naming for LCP—where the element property was a CSS elector, whereas it's called "target" in every other attribution object—this PR renames element to target to be consistent:

-LCPAttribution.element
+LCPAttribution.target

Given that this will be released with v4 and there is already a breaking change in the LCPAttribution interface, it made sense to rename rather than alias and deprecate.

Tiggerito commented 3 months ago

I think the element->target means the code examples would need updating. e.g.

https://github.com/GoogleChrome/web-vitals?tab=readme-ov-file#send-attribution-data https://github.com/GoogleChrome/web-vitals?tab=readme-ov-file#lcp-attribution

philipwalton commented 3 months ago

However, at risk of bikeshedding:

Yeah, it's tricky because I think we want to balance three things:

  1. Clarity and intuitiveness of naming
  2. Consistency between metrics (despite the underlying APIs not being consistent)
  3. Minimal changes to existing users

And I'm honestly not sure what solution best address all three of these...

That said, if this is a "pick two" kinda situation, I'd probably pick (1) and (3) since there is already inconsistency, so maybe it's better to just leave LCP and CLS as they are, and only introduce this for INP.

Also, for LCP and CLS, there isn't actually a need for an element reference (yet), since the entries for those metrics will always contain a reference to the element. This is only needed for INP, since there are cases where entries[0].target will not match attribution.interactionTarget, and maybe that's a good enough reason to only update INP, and to pick the name that makes the most sense when having both.

Also, since we're already changing INP attribution names, we have more leway to pick the best name. And if we ever make a similar change for LCP and CLS (e.g. where we store a reference to the element ASAP to minimize null references), then we can similarly update those attribution interfaces. WDYT?

If we just change INP, how about using these names?

interface INPAttribution {
  interactionTargetSelector: string;
  interactionTargetElement: Node | undefined;
}
tunetheweb commented 3 months ago

That works for me!

philipwalton commented 3 months ago

That works for me!

Updated. I'll also update the PR description once we finalize what changes we're making.

philipwalton commented 3 months ago

cc: @just-jeb in case you have thoughts on this PR (either the naming, or in general).

philipwalton commented 3 months ago

We discussed this offline and decided to close this in favor of #479, which keeps the original name the same.