Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Identical abstract selectors can be returned as distinct strings #514

Open
vanderhoop opened this issue Aug 8, 2024 · 6 comments
Open

Comments

@vanderhoop
Copy link
Contributor

vanderhoop commented Aug 8, 2024

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.

@vanderhoop vanderhoop changed the title Identical selectors are returned as distinct strings Identical selectors can be returned as distinct strings Aug 8, 2024
@mmocny
Copy link
Member

mmocny commented Aug 8, 2024

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
Copy link
Contributor Author

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
Copy link
Member

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
Copy link
Contributor Author

vanderhoop commented Aug 9, 2024

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.

@vanderhoop vanderhoop changed the title Identical selectors can be returned as distinct strings Identical abstract selectors can be returned as distinct strings Aug 9, 2024
@mmocny
Copy link
Member

mmocny commented Aug 9, 2024

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
Copy link

+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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants