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

Extending the NavigationTimingType #184

Open
tunetheweb opened this issue Nov 8, 2022 · 5 comments
Open

Extending the NavigationTimingType #184

tunetheweb opened this issue Nov 8, 2022 · 5 comments

Comments

@tunetheweb
Copy link
Member

tunetheweb commented Nov 8, 2022

Related to the discussion on #179

There are a few uses cases where the NavigationTimingType does not cover the type:

  • Restores - can be detected with document.wasDiscarded. Should we add a restore navigation type rather than reuse the original navigation type?
  • BFcache usage - it's arguable whether this is a "navigation", but given pages that are not in bfcache will reload, and that counts as back_forward navigation, I argue BFcache restores should be a navigation. Currently this can be detected with the persisted flag on the pageshow event. Should we add a back_forward_cache navigation type, and treat this is as a true navigation, to explicitly make it easier to identify these?
  • Prerender - this is explicitly noted in the spec as for the Resource Hint, but is currently being reimplemented in Chrome using the Speculation rules API, and the omnibox which does not fit this definition. Should the wording be relaxed to include all prerenders?
  • Additionally the prerender option is not listed in the interace, when it should be shouldn't it?

These types, can have very different performance measurements compared to other navigations and it is often recommended to measure them separately, but at the moment, this results in lots of extra code for (for example) and understanding of all these nuances to enable this.

There's a lot of different issues in this one, so happy to split this out, or continue some of the discussion in #179 but thought I'd start with the one issue for now in case we wanted to tackle together.

@verlok-cn
Copy link

Should we add a back_forward_cache navigation type, and treat this is as a true navigation, to explicitly make it easier to identify these?

I'm in favour 👍

@Krinkle
Copy link
Member

Krinkle commented Dec 8, 2022

Should we add a back_forward_cache navigation type, and treat this is as a true navigation, to explicitly make it easier to identify these?

A possible reason against, might be that it could be confusing (and possibly break compat) if we retroactively change the value of performance.navigation.type, performance.timing, and performance.getEntriesByType('navigation') during a bfcache restore that includes the original JS heap as these would already have been consumed.

I notice that in Firefox today, it already does this. The original data in performance.timing is preserved as-is with navigationStart holding the timestamp of when the original navigation started, but performance.navigation.type changes during the bfcache restore from TYPE_NAVIGATE to TYPE_BACK_FORWARD. I realize that I had incorrectly assumed that this navigation type would only be set when bfcache isn't leveraged, since no navigation got rendered. It's restored instantly from memory with no new navigation timing data being provided. I guess it doesn't cause double-counting of the original navigation, since the already-executed JS code isn't forced to consume it a second time, but it also makes it unclear what the intended use case is for seeing a new navigation type without seeing new navigation timing data.

@yoavweiss
Copy link
Contributor

RE BFCache specifically, there's a proposal for dedicated entries for it, to avoid the issues @Krinkle pointed out.
It was discussed a while back (recording)

/cc @clelland

@tunetheweb
Copy link
Member Author

I guess it doesn't cause double-counting of the original navigation, since the already-executed JS code isn't forced to consume it a second time but it also makes it unclear what the intended use case is for seeing a new navigation type without seeing new navigation timing data.

In part it's to avoid me (and others!) having to write code like this:

https://github.com/GoogleChrome/web-vitals/blob/39f178242afbb96dca3d48b216d60e7cd4cfa633/src/lib/initMetric.ts#L26-L34

I feel it would make it more obvious to anyone firing events (e.g. LCP), without having to be NavigationTimingType experts and know ALL the edge case you need to consider (3 currently!).

For restores and prerender it does show new navigational timing data, so this is more about categorising that data appriopriately.

For bfcache, I agree it does not fire new data. There's a question if it should? Which seems to be discussed prior to me joining this WG as per Yoav's comment. Seems like from a review of that, that there was some agreement, but needs the work to be done.

Maybe we drop bfcache for now, and limit scope of my ask here to restores and prerender, since they are simpler?

Bfcache could be revisited later (perhaps with soft nav as mentioned in the discussions Yoav's linked?)

@fergald
Copy link

fergald commented Oct 20, 2023

Tab duplication/cloning has the same issues. Maybe RESTORE can be extended to cover both (maybe with a name change) or we would have a new entry for it.

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

5 participants