search

GoogleChrome / web-vitals

Essential metrics for a healthy site.
https://web.dev/vitals
Apache License 2.0
7.62k stars 418 forks source link

Refactor the `stopListening()` function to only run once #548

Closed philipwalton closed 1 month ago

philipwalton commented 1 month ago

This PR fixes a subtle logic difference introduced in #538.

Previously, the stopListening() function in onLCP.ts would only ever run once, but with the change in #538 it could run more than once. I don't think this would result in any real change in behavior (since the metric shouldn't have changed between invocations); however, I think it's still better to restore the previous behavior.

tunetheweb commented 1 month ago

Could you explain what the "subtle logic difference" is and why that means it could run more that once?

And won't the if (!reportedMetricIDs[metric.id]) { guard prevent it running more than once anyway?

philipwalton commented 1 month ago

Could you explain what the "subtle logic difference" is and why that means it could run more that once?

With the current logic, the stopListening() function could get invoked three times, once for each of the click, keydown, or visibilitychange events. Each time it gets invoked it creates a new instance of the function that gets passed to runOnce().

With my changes, the stopListening() function is assigned the result of what runOnce() returns, so there's only a single copy of that logic rather than potentially three copies.

And won't the if (!reportedMetricIDs[metric.id]) { guard prevent it running more than once anyway?

Yes, I think it will. I'd actually forgotten about that, so maybe the runOnce() is not even needed? Or perhaps the reportedMetricIDs object is not needed?

philipwalton commented 1 month ago

It looks like the runOnce() code was added in this commit https://github.com/GoogleChrome/web-vitals/commit/e5e4a6d188da2ecd2d78279e4c8e946408a743ca which addressed a different change and which doesn't seem to account for the reportedMetricIDs object, so maybe that can be removed?

tunetheweb commented 1 month ago

Cool. I figured it was the three click handlers, but with the guard I wasn't 100% sure.

Personally I have the if (!reportedMetricIDs[metric.id]) { simpler to grok. Especially as whenIdleOrHidden will use runOnce, but then there's another one in stopListening so called twice in some cases. Which also hurts my brain to think of all this...

philipwalton commented 1 month ago

I decided to remove the reportedMetricIDs object, since it was used it multiple places like the onBFCacheRestore(), which I don't think was needed? Also removing reportedMetricIDs resulted in a smaller bundle size than removing the runOnce().