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

Experimental Soft Navigation support #308

Draft
wants to merge 65 commits into
base: main
Choose a base branch
from
Draft

Experimental Soft Navigation support #308

wants to merge 65 commits into from

Conversation

tunetheweb
Copy link
Member

@tunetheweb tunetheweb commented Jan 19, 2023

Warning: This branch is likey to stay in draft for some time, and not to be merged into main until the Soft Navs experiment completes

Fixes #119

Implements experimental support of Soft Navigations to the library:

  • - Updated README
  • - TTFB
  • - FCP
  • - LCP
  • - CLS
  • - FID (dependent on 1407656)
  • - INP
  • - Attribution (particularly navigationTiming)
  • - Test Cases (needs to wait for a supporting version of Chrome to be available in WDIO)

Copy link
Member

@mmocny mmocny left a comment

Choose a reason for hiding this comment

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

Hey, this is excellent work. Quite a bit to get right, thanks for the effort!

I only review the CLS changes, and I think there's a potentially cleaner way to do it. I didn't review the specifics of the other metrics since I the change would alter a lot there too, so wanted to discuss that first.

Basically:

  • I think the observe() helper, which all metrics use, should be the only place where we look at navigationID and split entries whenever it changes. See: https://github.com/GoogleChrome/web-vitals/blob/main/src/lib/observe.ts#L52
  • Whenever navigationID changes (or, the first time for initial nav), that helper can look up pageURL, etc, that way we have shared cache for all metrics
  • Each metric implementation right now already uses helpers like onFCP, onHidden, onBFCacheRestore... in order to "finalize" and/or "reset" its metrics. I think we could have another helper like onNavigate(), which can be used to flush/reset metrics.

One tricky bit may be that the onNavigate() helper needs to be called before any new Entry that uses that navigationID, and you could have entries with different navigationID in a single PerformanceObserver callbacks, especially when using buffered entries.

That said, I think all this code made be fragile to such cases and needs careful testing.

Keep at it!

src/onCLS.ts Outdated Show resolved Hide resolved
entries.forEach((entry) => {
if (
Copy link
Member

Choose a reason for hiding this comment

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

Perhaps a cleaner way to do it is to have the helper class that calls handleEntries automatically split entries based on navigationID?

Then, you dont' need to do this once per entry.

Also, that helper that does the split could call something like "finalizeAndReset", so we wouldn't need to do these tests here at all. The metric specific handlers would just get a URL and maintain a score...

WDYT?

Copy link
Member Author

@tunetheweb tunetheweb Jan 20, 2023

Choose a reason for hiding this comment

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

While that may look like cleaner code, I wonder about the performance implications of splitting up the array like that versus doing this check every time? In effect you’d need to do that check anyway for each entry AND copy the entries to a new array. Which sounds like a net negative to me.

WDYT?

src/onCLS.ts Outdated
};

if (softNavs(opts)) {
observe('soft-navigation', reportSoftNavCLS);
Copy link
Member

Choose a reason for hiding this comment

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

Followup from comment above... I think we should just automatically do this from inside the observe() helper, shared by all metrics

Copy link
Member Author

Choose a reason for hiding this comment

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

Not all metrics need the soft-navigation observer (FCP and FID don’t for example as they can finalise immediately).

For those that do need this to finalise metrics that may not be finalised before then, there are different steps to make so it’s not generic across all the metrics. We could pass in a callback function to handle that, but that’s exactly what this currently does.

Also currently the observe function is for a single observer with a call back on what to do with that. I’m not sure it’s right to change that to also call another observer for some metrics, with a different callback function.

But maybe I’m just not seeing the way to do this? If you have a look at some of the other metrics and still think there’s an easier/cleaner way to do this, then definitely open to it, if you could give me a little nudge as to how you think it could be structure.

@tunetheweb
Copy link
Member Author

That’s for the review @mmocny ! I’ve cleaned it up quite a bit further so it’s a bit smaller now. Have another look if you get a chance and see what you think now.

@alekseykulikov
Copy link

Hey! I'm super excited about this work. Could you republish the branch?
The current 3.1.1-soft-navs doesn't report LCP and instead prints BARRY.
Screen Shot 2023-02-08 at 22 30 06

@tunetheweb
Copy link
Member Author

Well that's embarrassing! Republished to the soft-navs tag (which is an alias to 3.1.1-soft-navs-1 now).

@alekseykulikov
Copy link

Thanks, that was fast.
Maybe, I'm doing something wrong, but there's still no LCP report, despite actual values are available (Chrome 110):
Screen Shot 2023-02-08 at 23 00 17

@karreiro
Copy link

karreiro commented Jan 8, 2024

Hi @tunetheweb,

Thanks a lot for introducing support for Soft Navigations in the library!

While exploring the new features, I noticed something interesting. It seems that not all changes are being reported when reportAllChanges is activated, as the onINP module is only tracking the longest interactions.

To handle that scenario, I've slightly updated the onINP module (#411) to ensure that all changes are reported when reportAllChanges is turned on (demo). I've directed my PR to your branch, as my changes are somewhat related to soft navs. I'd be glad to hear your thoughts on this.

@tunetheweb
Copy link
Member Author

As discussed in #411 that is working as intended. But I raised #413 to make it clearer what exactly that intention is.

@tunetheweb
Copy link
Member Author

Updated this branch to v4 code.

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.

Web Vital Metrics for Single Page Applications
4 participants