search

GoogleChrome / web-vitals

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

Identical abstract selectors can be returned as distinct strings #514

Open vanderhoop opened 3 months ago

vanderhoop commented 3 months ago

The Issue

At the moment, the web-vitals attribution builds can return distinct selector strings for elements that have semantically identical selectors. For example, in the image below, you’ll see two selectors that are identical in the abstract/query sense, but distinct as strings:

exact_same_selector_but_with_different_class_order

A Proposal

In the construction of the selector string, sort the element class names before joining them. This sorting would ensure that the selectors reported above are consolidated and identical. The drawback is that the as-is order of classes could (in theory) help a developer delineate between elements that have the exact same classes but in a different order, but my guess is that that drawback is outweighed by the benefit of cleaner selector aggregation.

mmocny commented 3 months ago

Here it is, I think?

Seems easy enough to take a stab, and honestly might be improved in a few other ways. Do you think this is something you would be interesting in submitting a PR for?

vanderhoop commented 3 months ago

Do you think this is something you would be interesting in submitting a PR for?

Yeah, I'm happy to. I've noticed that the repo leans heavily into webdriver tests. You cool with unit tests for getSelector, or should I alter/augment some webdriver assertions?

philipwalton commented 3 months ago

I'm not necessarily opposed to changing this, though I do want to point out that any change to the selector generation logic would mess up people's existing reports. E.g. the situation that @vanderhoop is describing in his screenshot would start happening to everyone using the attribution build of this library whose HTML classes are not already in alphabetical order.

So this is something we'd want to run by RUM providers using the library to make sure they'd be OK with, and it would also need to be released with a major version bump (since it's a breaking change, for the reason I just mentioned).

vanderhoop commented 3 months ago

I do want to point out that any change to the selector generation logic would mess up people's existing reports. E.g. the situation that @vanderhoop is describing in his screenshot would start happening to everyone using the attribution build of this library whose HTML classes are not already in alphabetical order... ...so this is something we'd want to run by RUM providers using the library to make sure they'd be OK with

That's a great callout, and should be weighed against the existing selector inconsistency that library consumers are currently subject to. The optimistic view (that others might not share) is that this change in the selector data coming over the wire would end up causing a diff between the aggregated selectors in navigations ingested before/after upgrade, but that diff would be flushed within months for the common RUM use cases. Maybe @ErwinHofmanRV has thoughts, as I believe he's leveraging web-vitals under the hood?

it would also need to be released with a major version bump (since it's a breaking change, for the reason I just mentioned).

I don't see this change necessitating a new SEMVER major, as none of the APIs are changing, nor is the overall semantic structure of the returned result.

One option (which I don't love, but which scratches all the itches) is to make this sorting functionality opt-in. getSelector already has an optional maxLen(th) arg, though it doesn't actually appear to be leveraged in the current lib.

mmocny commented 3 months ago

make this sorting functionality opt-in

Although adding flags for everything can be a sign of poor design, I think this is a reasonable path forward. Default disabled in current version, default enabled in next version.

The opt-out could be a perf benefit for any provider that prefers to do it server-side.

ErwinHofmanRV commented 3 months ago

+1 for sorting/'uniqueifying' selectors

In general, the amount of distinct results/selectors would reduce. And the appearances/events-rate of selectors would improve, making it easier to grasp which with selector users should continue debugging in DevTools.

I do agree that sites that would be doing periodic comparisons (either manually or via alerts) might see a weird change in this number and might suddenly receive notifications if alerts were configured for the selector-dimension. But that will be phased out over time and all for the benefit of RUM users I would say.

So based on the way we use this selector data (basically the same as shown in the screenshot as provided by @vanderhoop ), I don't see a risk in a sudden change.