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

Add eventListener to reset CLS & INP metrics #433

Closed
wants to merge 3 commits into from

Conversation

spetroll
Copy link

This PR enables manual resetting of CLS & INP metrics through dispatching a reset-web-vital-metrics event. It allows single-page applications to trigger this event during page transitions and measure these long-running metric measurement for each page. While it differs from Google's use of the metrics, it greatly enhances the ability of SPAs to assess the performance of individual pages.

Currently, as a fallback, we've enabled reportAllChanges for these metrics, buffer the measurements and report the latest before a page transition. However, this approach skews the results of later pages in a user journey because the metric is only reported again when it's worse than before. This PR aims to provide a more accurate understanding of page performance in the context of SPA pages.

This is similar to the soft-navigate branch but can be used today already. It could also serve as a fallback method in case the soft-navigate heuristics don't work particularly well with certain applications.

Relevant issue: #119


export const onManualSoftNavigation = (cb: () => void) => {
document.addEventListener('reset-web-vital-metrics', cb);
};
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I could also easily export something like

export const triggerManualSoftNavigation = () => {
  document.dispatchEvent(new Event('reset-web-vital-metrics'));
}

but I'm not sure how you feel about exposing API besides onXXX from this package.

* limitations under the License.
*/

export const onManualSoftNavigation = (cb: () => void) => {
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Naming probably isn't the best, I defer to your judgement ;)

@tunetheweb
Copy link
Member

tunetheweb commented Feb 23, 2024

@spetroll thank you very much for working on this and submitting this PR.

However, while we appreciate the potential usefulness of this change to allow SPAs to manually measure CLS and INP per route, we do have some concerns with accepting this into the web-vitals library:

First up, the intent of the library (as given in the Overview) is for:

measuring all the Web Vitals metrics on real users, in a way that accurately matches how they're measured by Chrome and reported to other Google tools (e.g. Chrome User Experience Report, Page Speed Insights, Search Console's Speed Report).

By reseting CLS and INP you will affect the overall "hard-nav" CLS and INP that can be reported.

This is also part of why the soft branch support has not been merged either.

Secondly, the "measuring all the Web Vitals metrics" is a key point. By only making CLS and INP available (which I agree is all that CAN be made available given Chrome itself will not reset LCP or FCP), we think this will lead to confusion, or a certain expectation that they will be available.

So, given all that, we are unlikely to merge this PR, at least without a lot of further discussion. So wanted to let you know that, before you worked on it more.

@spetroll
Copy link
Author

@tunetheweb Thanks for the feedback. I assumed as much by myself and understand how our needs might differ. Thanks for letting me know :)

We'll probably maintain our own fork in the foreseeable future, as many changes we have added on top to this library are much easier to implement from "within", but aren't relevant for most of the target audience or the original intent of this library.

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

Successfully merging this pull request may close these issues.

2 participants