Update README to make it more clear that regardless of reportAllChanges the final reported metric is the same

[tunetheweb]

Fixes #597

[williazz]

Thank you very much for your collaboration

[williazz]

#597 (comment)

onINP already monitors all interactions across the page lifecycle and but then only reports it at the end of the page (or on tab switching...)

How is INP reported at "the end of the page"? From my limited of your codebase, I see that the maximum INP (ignoring outliers) is reported on visibility state changes to hidden. But I could not see where INP is reported at the end of the page.

The use case that I am concerned about is:

1. User lands on a website
2. User interacts with the website for 30 minutes without ever triggering a visibility state change to hidden (by hiding the window, switching tabs, etc.)
3. User closes the website.

[tunetheweb]

The visibilitychange event fires for both tab hides, and also navigations away. Check out the Page Lifecycle API docs.

[williazz]

https://developer.mozilla.org/en-US/docs/Web/API/Document/visibilitychange_event

This event fires with a visibilityState of hidden when a user navigates to a new page, switches tabs, closes the tab, minimizes or closes the browser, or, on mobile, switches from the browser to a different app.

Thanks I see it here as well now.

[philipwalton]

I know the current README says the reportAllChanges option is not recommended, but if/when the fetchLater() API becomes more widely available, I think there may be cases where it does make more sense to use reportAllChanges: true, to get the increased reliability of that API and not depend on the reliability of visibilitychange events firing.

I think rather than discouraging the use of reportAllChanges to address this confusion, we could just clarify that, regardless of whether reportAllChanges is true or false, the final reported value will always be the same.

[tunetheweb]

I know the current README says the reportAllChanges option is not recommended, but if/when the fetchLater() API becomes more widely available, I think there may be cases where it does make more sense to use reportAllChanges: true, to get the increased reliability of that API and not depend on the reliability of visibilitychange events firing.

I think there's still a risk of reportAllChanges resulting in too much work and causing perf issues depending on what developers do in their onXXX callbacks. Of course web-vitals.js does its own processing regardless of reportAllChanges but we try to minimise that to the minimum needed to accurately calculate the metrics and take take to yield where necessary. But anything devs do beyond that in the callbacks could be quite intensive so I do think we want to discourage that. This is especially the case for INP which can result in a lot of events (especially when typing for example), or when the default 40ms event timing threshold is reduced—cases I've seen examples of recently.

The fetchLater() API use case is interesting. I guess that could be used to queue up the beacon on each change, but only send it once (as in this example, but I do worry that, depending how much processing you need to do to prepare that beacon, you could fall into my concern above. Maybe we need some sort of debounce here to reduce any impact of this.

And I thought visibilitychange events were reliable? But it's just there's a short period from that to do any processing and get the beacon out. And so I could see fetchLater() being perhaps more reliable for the latter part, but if it's the former that's the issue then again doing that more frequently on every change, may still be a concern.

I think rather than discouraging the use of reportAllChanges to address this confusion, we could just clarify that, regardless of whether reportAllChanges is true or false, the final reported value will always be the same.

Happy to do that for now in this PR, and we can discuss all of the above more to decide on that.

[philipwalton]

And I thought visibilitychange events were reliable? But it's just there's a short period from that to do any processing and get the beacon out. And so I could see fetchLater() being perhaps more reliable for the latter part, but if it's the former that's the issue then again doing that more frequently on every change, may still be a concern.

The visibilitychange event is the most reliable lifecycle event we have, but it's still not 100% reliable and I suspect that's one of the reasons developers don't see 100% (or near 100%) beacon success rates.

Happy to do that for now in this PR, and we can discuss all of the above more to decide on that.

Yeah, that SGTM. Let's use this PR to clarify the confusion around reportAllChanges and then discuss whether we should recommend reportAllChanges separately.

[tunetheweb]

Happy to do that for now in this PR, and we can discuss all of the above more to decide on that.

Yeah, that SGTM. Let's use this PR to clarify the confusion around reportAllChanges and then discuss whether we should recommend reportAllChanges separately.

Done.