Instrument back/forward cache navigations #70
Conversation
ajhyndman
force-pushed
the
handle-bfcache
branch
from
140129c
to
a47bdd1
Compare
| // Navigation is initiated by a prerender hint. | ||
| | 'prerender' | ||
| // Navigation was triggered with a script operation, e.g. in a single page application. | ||
| | 'script'; |
There was a problem hiding this comment.
Can we document that this is an extension of NavigationTimingType and script is the only additional value?
There was a problem hiding this comment.
Yeah, good call, updated.
| @@ -4,6 +4,18 @@ import {requestAllIdleCallback} from './requestAllIdleCallback'; | |||
| import {InViewportImageObserver} from './inViewportImageObserver'; | |||
| import {Logger} from './util/logger'; | |||
|
|
|||
| export type NavigationType = | |||
There was a problem hiding this comment.
Can this extend NavigationTimingType, so we only need to add script?
| ? 'back_forward' | ||
| : start !== 0 | ||
| ? 'script' | ||
| : navigationEntries[navigationEntries.length - 1].type; |
There was a problem hiding this comment.
I don't think it matters much either way since there is only 1 navigation entry, but maybe navigationEntries[0] is easier to read?
There was a problem hiding this comment.
I think I wasn't confident that there can only ever be one navigation entry.
It seems likely there are some edge cases where the navigation entry can be reported again, after a cache restore, for instance.
Do you know if the spec guarantees that there will only ever be one navigation entry?
There was a problem hiding this comment.
This says
Only the current document is included, so usually there is only one PerformanceNavigationTiming object to observe.
and web-vitals only takes the first navigation entry: https://github.com/GoogleChrome/web-vitals/blob/main/src/lib/getNavigationEntry.ts
| window.addEventListener('locationchange', () => void calculator.start(performance.now())); | ||
|
|
||
| // restart measurement for SPA navigation | ||
| window.addEventListener('locationchange', (event) => void calculator.start(event.timeStamp)); |
There was a problem hiding this comment.
Is there more documentation on the locationchange event? I can't find it on MDN or in our repo
There was a problem hiding this comment.
Nvm - just realized we have it documented in README already.
| window.addEventListener('pageshow', (event) => { | ||
| // abort if this is the initial pageload | ||
| if (!event.persisted) return; | ||
| void calculator.start(event.timeStamp, true); |
There was a problem hiding this comment.
Is it possible that pageshow and locationchange are both triggered somewhat simultaneously for a page, resulting in sometimes conflicting calls (in an unexpected order) to calculator.start?
There was a problem hiding this comment.
I don't think that's likely. This event should only be triggered either on initial pageload, in which case we ignore it, or on restore from the bfcache, in which case, nothing else on the page knows that something different from normal operation has occurred.
The design of @dropbox/ttvc also tries to ensure that duplicate, simultaneous calls to start() are effectively idempotent. i.e. the first measurement will be immediately thrown out and only the subsequent value will be used. If this does occur, it shouldn't impact results.
|
Thanks for the thorough reviews @hongrich and @umairnadeem! |
This change builds instrumentation for navigations enhanced by browser back/forward caches into the library.
This uses the
pageshowevent and it's.persistedproperty to identify a navigation and trigger a new measurement.NOTE: This change bakes into the library that we should start a new measurement for TTVC whenever a page is restored from the back/forward cache. I think this is almost always desirable, and therefore a good default, but I am not certain.
It's possible for a user to accomplish this today in user space, by importing the
start()method and calling it manually. It would also be possible to make this an opt-out configuration property.I think a related question is how or whether we should let the user know that the measurement was started via back/forward cache restore.Edit: I pushed another commit which reports navigationType in the detail object for each metric result. I think this addresses most of my own concerns. If someone wants to exclude back/forward measurements, they can do so by dropping values reported with the matching navigationType. Similarly, they can include that metadata as a tag when persisting that measurement anywhere.