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

Ensure one-time event listeners only run once #538

Merged
merged 4 commits into from
Oct 3, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 7 additions & 9 deletions src/attribution/onINP.ts
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ import {
longestInteractionMap,
} from '../lib/interactions.js';
import {observe} from '../lib/observe.js';
import {whenIdle} from '../lib/whenIdle.js';
import {whenIdleOrHidden} from '../lib/whenIdleOrHidden.js';
import {onINP as unattributedOnINP} from '../onINP.js';
import {
INPAttribution,
Expand Down Expand Up @@ -76,10 +76,8 @@ const entryToEntriesGroupMap: WeakMap<
// A mapping of interactionIds to the target Node.
export const interactionTargetMap: Map<number, Node> = new Map();

// A reference to the idle task used to clean up entries from the above
// variables. If the value is -1 it means no task is queue, and if it's
// greater than -1 the value corresponds to the idle callback handle.
let idleHandle: number = -1;
// A boolean flag indicating whether or not a cleanup task has been queued.
let cleanupPending = false;

/**
* Adds new LoAF entries to the `pendingLoAFs` list.
Expand Down Expand Up @@ -159,8 +157,9 @@ const groupEntriesByRenderTime = (entry: PerformanceEventTiming) => {

const queueCleanup = () => {
// Queue cleanup of entries that are not part of any INP candidates.
if (idleHandle < 0) {
idleHandle = whenIdle(cleanupEntries);
if (!cleanupPending) {
whenIdleOrHidden(cleanupEntries);
cleanupPending = true;
}
};

Expand Down Expand Up @@ -206,8 +205,7 @@ const cleanupEntries = () => {
return loafsToKeep.has(loaf);
});

// Reset the idle callback handle so it can be queued again.
idleHandle = -1;
cleanupPending = false;
};

entryPreProcessingCallbacks.push(
Expand Down
23 changes: 0 additions & 23 deletions src/lib/onHidden.ts

This file was deleted.

12 changes: 3 additions & 9 deletions src/lib/whenIdle.ts → src/lib/whenIdleOrHidden.ts
Original file line number Diff line number Diff line change
Expand Up @@ -14,25 +14,19 @@
* limitations under the License.
*/

import {onHidden} from './onHidden.js';
import {runOnce} from './runOnce.js';

/**
* Runs the passed callback during the next idle period, or immediately
* if the browser's visibility state is (or becomes) hidden.
*/
export const whenIdle = (cb: () => void): number => {
export const whenIdleOrHidden = (cb: () => void) => {
const rIC = globalThis.requestIdleCallback || setTimeout;

let handle = -1;
cb = runOnce(cb);
// If the document is hidden, run the callback immediately, otherwise
// race an idle callback with the next `visibilitychange` event.
if (document.visibilityState === 'hidden') {
cb();
} else {
handle = rIC(cb);
onHidden(cb);
rIC(cb);
document.addEventListener('visibilitychange', cb, {once: true});

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

【discuss】why the cb should be called two times?

  • when it's hidden:call it once,immediately
  • when it's visible:call it once with rIC and another time when change to hidden

before the change,it‘s wrapped with runOnce will only call once

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for catching this! I've created to #541 to address this problem.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. Very nice catch!

}
return handle;
};
9 changes: 5 additions & 4 deletions src/onCLS.ts
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,6 @@ import {initMetric} from './lib/initMetric.js';
import {observe} from './lib/observe.js';
import {bindReporter} from './lib/bindReporter.js';
import {doubleRAF} from './lib/doubleRAF.js';
import {onHidden} from './lib/onHidden.js';
import {runOnce} from './lib/runOnce.js';
import {onFCP} from './onFCP.js';
import {CLSMetric, MetricRatingThresholds, ReportOpts} from './types.js';
Expand Down Expand Up @@ -107,9 +106,11 @@ export const onCLS = (
opts!.reportAllChanges,
);

onHidden(() => {
handleEntries(po.takeRecords() as CLSMetric['entries']);
report(true);
document.addEventListener('visibilitychange', () => {
if (document.visibilityState === 'hidden') {
handleEntries(po.takeRecords() as CLSMetric['entries']);
report(true);
}
});

// Only report after a bfcache restore if the `PerformanceObserver`
Expand Down
13 changes: 7 additions & 6 deletions src/onINP.ts
Original file line number Diff line number Diff line change
Expand Up @@ -24,10 +24,9 @@ import {
resetInteractions,
} from './lib/interactions.js';
import {observe} from './lib/observe.js';
import {onHidden} from './lib/onHidden.js';
import {initInteractionCountPolyfill} from './lib/polyfills/interactionCountPolyfill.js';
import {whenActivated} from './lib/whenActivated.js';
import {whenIdle} from './lib/whenIdle.js';
import {whenIdleOrHidden} from './lib/whenIdleOrHidden.js';

import {INPMetric, MetricRatingThresholds, ReportOpts} from './types.js';

Expand Down Expand Up @@ -89,7 +88,7 @@ export const onINP = (
// have been dispatched. Note: there is currently an experiment
// running in Chrome (EventTimingKeypressAndCompositionInteractionId)
// 123+ that if rolled out fully may make this no longer necessary.
whenIdle(() => {
whenIdleOrHidden(() => {
for (const entry of entries) {
processInteractionEntry(entry);
}
Expand Down Expand Up @@ -126,9 +125,11 @@ export const onINP = (
// where the first interaction is less than the `durationThreshold`.
po.observe({type: 'first-input', buffered: true});

onHidden(() => {
handleEntries(po.takeRecords() as INPMetric['entries']);
report(true);
document.addEventListener('visibilitychange', () => {
if (document.visibilityState === 'hidden') {
handleEntries(po.takeRecords() as INPMetric['entries']);
report(true);
}
});

// Only report after a bfcache restore if the `PerformanceObserver`
Expand Down
45 changes: 25 additions & 20 deletions src/onLCP.ts
Original file line number Diff line number Diff line change
Expand Up @@ -21,10 +21,9 @@ import {getActivationStart} from './lib/getActivationStart.js';
import {getVisibilityWatcher} from './lib/getVisibilityWatcher.js';
import {initMetric} from './lib/initMetric.js';
import {observe} from './lib/observe.js';
import {onHidden} from './lib/onHidden.js';
import {runOnce} from './lib/runOnce.js';
import {whenActivated} from './lib/whenActivated.js';
import {whenIdle} from './lib/whenIdle.js';
import {whenIdleOrHidden} from './lib/whenIdleOrHidden.js';
import {LCPMetric, MetricRatingThresholds, ReportOpts} from './types.js';

/** Thresholds for LCP. See https://web.dev/articles/lcp#what_is_a_good_lcp_score */
Expand Down Expand Up @@ -85,27 +84,33 @@ export const onLCP = (
opts!.reportAllChanges,
);

const stopListening = runOnce(() => {
if (!reportedMetricIDs[metric.id]) {
handleEntries(po!.takeRecords() as LCPMetric['entries']);
po!.disconnect();
reportedMetricIDs[metric.id] = true;
report(true);
}
});
// Ensure this logic only runs once, and wrap it in an idle callback
// so the callback is run in a separate task to avoid extending the
// keyboard/click handler to reduce INP impact.
// https://github.com/GoogleChrome/web-vitals/issues/383
const stopListening = () =>
whenIdleOrHidden(
runOnce(() => {
if (!reportedMetricIDs[metric.id]) {
handleEntries(po!.takeRecords() as LCPMetric['entries']);
po!.disconnect();
reportedMetricIDs[metric.id] = true;
report(true);
}
}),
);

// Stop listening after input. Note: while scrolling is an input that
// stops LCP observation, it's unreliable since it can be programmatically
// generated. See: https://github.com/GoogleChrome/web-vitals/issues/75
for (const type of ['keydown', 'click']) {
// Wrap in a setTimeout so the callback is run in a separate task
// to avoid extending the keyboard/click handler to reduce INP impact
// https://github.com/GoogleChrome/web-vitals/issues/383
addEventListener(type, () => whenIdle(stopListening), true);
// Stop listening after input or visibilitychange.
// Note: while scrolling is an input that stops LCP observation, it's
// unreliable since it can be programmatically generated.
// See: https://github.com/GoogleChrome/web-vitals/issues/75
for (const type of ['keydown', 'click', 'visibilitychange']) {
addEventListener(type, () => stopListening(), {
capture: true,
once: true,
});
}

onHidden(stopListening);

// Only report after a bfcache restore if the `PerformanceObserver`
// successfully registered.
onBFCacheRestore((event) => {
Expand Down