Add support for generating custom targets in the attribution build

[philipwalton]

Fixes #561 and #580.

This PR is an alternative approach to #562 (built on top of my proposed changes in #583) that also adds a new top-level generateTarget configuration option which allows developers to pass in their own function—as an alternative to the libraries internal getSelector() function.

All of the "target" attribution fields that previously reported the result of the internal getSelector() function will now report whatever value is returned from the passed generateTarget() function.

I believe there are two benefits to this approach:

1. Developers whose HTML markup does not use meaningful or predictable class names may find the current logic unhelpful and prefer to configure their own.
2. Developers who want a reference to the underlying DOM element can use this function to get it, without this library having to retain that reference for the majority of users who don't need it.

Breaking changes in this PR include (similar to #562):

1. LCPAttribution.element has been renamed to LCPAttribution.target (for consistency, similar to Save a reference to the LCP and CLS targets in case they are removed from the DOM #562).
2. LCPAttribution.interactionTargetElement has been removed (to address the issue in Keeping elements around for attribution can leak memory #580).

[tunetheweb]

Can you update $ to _?

And add examples to README?

Other than that LGTM.

[philipwalton]

Can you update $ to _?

I believe I already did. Are you still see examples of $ around?

[tunetheweb]

I believe I already did. Are you still see examples of $ around?

Yes. In LCP and CLS ones that were part of this PR not the last one. For example:

https://github.com/GoogleChrome/web-vitals/blob/custom-target/src/attribution/onCLS.ts#L72

[philipwalton]

Ahh, right. Updated!

[philipwalton]

@tunetheweb @brendankenny are you both happy with generateTarget as the config option name for the function?

[tunetheweb]

SGTM

My only slight concern is that typescript gets slightly messier for most users now that Target is an unknown? Or is it clever enough to realise that's a string when no generateTarget is passed?

Also if users return the Node instead then they may not consider this memory "leak" issue?

We could insist (especially given the generateTarget name) that this returns a string and we don't allow it to return the Node (or anything else).

Or we could always return the target using our string selector and have an additional, optional, return field for this (in which case generateTarget is obviously not a good name).

[philipwalton]

My only slight concern is that typescript gets slightly messier for most users now that Target is an unknown? Or is it clever enough to realise that's a string when no generateTarget is passed?

We could potentially type it as string | unknown, though not sure if that makes things easier. @brendankenny do you have any thoughts here?

Also if users return the Node instead then they may not consider this memory "leak" issue?

In all cases we only reference these values via a WeakMap keyed on performance entries, so in the worst case scenario the max number DOM nodes that would be "leaked" would be equal to the number of performance entries stored by the attribution object.

---

Another option would be to document that the generateTarget() function must return a string, and then if someone really really wanted a reference to the DOM node then they could create their own mapping however they want within the function logic that they pass. I don't think that would significantly change the available functionality, but it would make it slightly harder. WDYT?

[brendankenny]

My only slight concern is that typescript gets slightly messier for most users now that Target is an unknown? Or is it clever enough to realise that's a string when no generateTarget is passed?

We could potentially type it as string | unknown, though not sure if that makes things easier. @brendankenny do you have any thoughts here?

string | unknown will just collapse into unknown after a step or two of type checking (e.g. WeakMap<Interaction, string | unknown> is just WeakMap<Interaction, unknown>) because unknown includes string already.

If this was a strongly desired goal, the way to do this would probably be to parameterize the attribution objects with the target type (defaulting to unknown), eg

export interface INPAttribution<Target=unknown> {
  //...
  interactionTarget: Target;
  // ...
}

export interface INPMetricWithAttribution<Target=unknown> extends INPMetric {
  attribution: INPAttribution<Target>;
}

Then for onINP, the type signature would be something like

export const onINP = <Target>(
  onReport: (metric: INPMetricWithAttribution<NoInfer<Target>>) => void,
  opts: ReportOpts<Target> = {},
) => {

(NoInfer requires typescript 5.4 so the Target type is defined solely by ReportOpts).

There are probably some things I'm forgetting (there's trickiness between type parameters in function parameters vs the parameter of a callback which is itself a parameter...) but that should mostly work. Whether or not the extra complexity and maintenance is worth it is a judgement call though :)

[tunetheweb]

I'm not sure it IS worth that complexity.

So I'm back to:

1. Live with it as is.
2. Insist on a string return if people want to override the getSelector function.
3. Add an optional "return Node" config (defaulted to false) that can be used in addition to out TargetElement (in addition to, or instead of the previous option 2).

I'm starting to veer towards 3. Probably without the ability to override our getSelector function. That's kinda most like what we have now in v4, but without the extra memory usage by default.

WDYT?

[brendankenny]

There are probably some things I'm forgetting

oh, like defaulting to string as returned by getSelector. It should be done on the onINP level, maybe something like

export const onINP = <Target=ReturnType<typeof getSelector>>(?

But that's going to take some more work to get it in sync with the generateTarget defaulting behavior.

[philipwalton]

I think I'm leaning toward enforcing a string return type in TypeScript, just to simplify things. Of course, this is just what TypeScript enforces, so technically anyone could update the types themselves locally if they really wanted to return a Node (or whatever else).

At the end of the day, you can't send a DOM element to a server anyway, so at some point you're gonna have to convert it to a string regardless. The only exception to this would be things like DevTools or browser extensions, and I'm fine with having those consumers jump through extra hoops to get the node reference.

[tunetheweb]

philipwalton requested a review from tunetheweb 13 hours ago

LGTM but I was waiting for the README updates before giving the formal OK.

[philipwalton]

LGTM but I was waiting for the README updates before giving the formal OK.

Gah! Forgot to run git push.

PTAL.