Refactor the stopListening() function to only run once

[philipwalton]

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]

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]

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]

It looks like the runOnce() code was added in this commit e5e4a6d which addressed a different change and which doesn't seem to account for the reportedMetricIDs object, so maybe that can be removed?

[tunetheweb]

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]

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().

[tunetheweb]

LGTM

However, I don't think this was really an issue. Click and Keyboard will prevent the browser from emitting a new LCP anyway so there shouldn't in theory be later LCP entries after those.

See: https://resource-loading.glitch.me/slow-lcp.html

But let's clean it up anyway!