Skip to content

Commit e2ef681

Browse files
authored
fix(browser): Reduce number of visibilitystate and pagehide listeners (#18581)
Previously, we called `onHidden` in our slightly modified version of the vendored in `whenIdleOrHidden` API from web-vitals. This caused 2 additional event listeners to be registered with each `whenIdleOrHidden` invocation, which also didn't get cleaned up properly. To fix this, and prevent similar situations, this PR: - inlines the `pagehide` event listener registration in `whenIdleOrHidden` so that we can remove the `onHidden` call - adds `once: true` to the listener registration so that the callback is only invoked once - deprecates `onHidden` because IMHO we should remove it in v11 and replace the one remaining use of it with a direct `visibilityChange` subscription. Closes #18584 (added automatically) closes https://linear.app/getsentry/issue/JS-1339/investigate-multiple-event-listeners-in-nextjs-sdk
1 parent aaa0fea commit e2ef681

File tree

3 files changed

+32
-17
lines changed

3 files changed

+32
-17
lines changed

packages/browser-utils/src/metrics/utils.ts

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -218,6 +218,7 @@ export function listenForWebVitalReportEvents(
218218
collected = true;
219219
}
220220

221+
// eslint-disable-next-line deprecation/deprecation
221222
onHidden(() => {
222223
_runCollectorCallbackOnce('pagehide');
223224
});

packages/browser-utils/src/metrics/web-vitals/lib/onHidden.ts

Lines changed: 24 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -21,25 +21,37 @@ export interface OnHiddenCallback {
2121
(event: Event): void;
2222
}
2323

24-
// Sentry-specific change:
25-
// This function's logic was NOT updated to web-vitals 4.2.4 or 5.x but we continue
26-
// to use the web-vitals 3.5.2 versiondue to us having stricter browser support.
27-
// PR with context that made the changes: https://github.com/GoogleChrome/web-vitals/pull/442/files#r1530492402
28-
// The PR removed listening to the `pagehide` event, in favour of only listening to `visibilitychange` event.
29-
// This is "more correct" but some browsers we still support (Safari <14.4) don't fully support `visibilitychange`
30-
// or have known bugs w.r.t the `visibilitychange` event.
31-
// TODO (v11): If we decide to drop support for Safari 14.4, we can use the logic from web-vitals 4.2.4
32-
// In this case, we also need to update the integration tests that currently trigger the `pagehide` event to
33-
// simulate the page being hidden.
24+
/**
25+
* Sentry-specific change:
26+
*
27+
* This function's logic was NOT updated to web-vitals 4.2.4 or 5.x but we continue
28+
* to use the web-vitals 3.5.2 version due to having stricter browser support.
29+
*
30+
* PR with context that made the changes:
31+
* https://github.com/GoogleChrome/web-vitals/pull/442/files#r1530492402
32+
*
33+
* The PR removed listening to the `pagehide` event, in favour of only listening to
34+
* the `visibilitychange` event. This is "more correct" but some browsers we still
35+
* support (Safari <14.4) don't fully support `visibilitychange` or have known bugs
36+
* with respect to the `visibilitychange` event.
37+
*
38+
* TODO (v11): If we decide to drop support for Safari 14.4, we can use the logic
39+
* from web-vitals 4.2.4. In this case, we also need to update the integration tests
40+
* that currently trigger the `pagehide` event to simulate the page being hidden.
41+
*
42+
* @param {OnHiddenCallback} cb - Callback to be executed when the page is hidden or unloaded.
43+
*
44+
* @deprecated use `whenIdleOrHidden` or `addPageListener('visibilitychange')` instead
45+
*/
3446
export const onHidden = (cb: OnHiddenCallback) => {
3547
const onHiddenOrPageHide = (event: Event) => {
3648
if (event.type === 'pagehide' || WINDOW.document?.visibilityState === 'hidden') {
3749
cb(event);
3850
}
3951
};
4052

41-
addPageListener('visibilitychange', onHiddenOrPageHide, true);
53+
addPageListener('visibilitychange', onHiddenOrPageHide, { capture: true, once: true });
4254
// Some browsers have buggy implementations of visibilitychange,
4355
// so we use pagehide in addition, just to be safe.
44-
addPageListener('pagehide', onHiddenOrPageHide, true);
56+
addPageListener('pagehide', onHiddenOrPageHide, { capture: true, once: true });
4557
};

packages/browser-utils/src/metrics/web-vitals/lib/whenIdleOrHidden.ts

Lines changed: 7 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -16,7 +16,6 @@
1616

1717
import { WINDOW } from '../../../types.js';
1818
import { addPageListener, removePageListener } from './globalListeners.js';
19-
import { onHidden } from './onHidden.js';
2019
import { runOnce } from './runOnce.js';
2120

2221
/**
@@ -34,15 +33,18 @@ export const whenIdleOrHidden = (cb: () => void) => {
3433
// eslint-disable-next-line no-param-reassign
3534
cb = runOnce(cb);
3635
addPageListener('visibilitychange', cb, { once: true, capture: true });
36+
// sentry: we use pagehide instead of directly listening to visibilitychange
37+
// because some browsers we still support (Safari <14.4) don't fully support
38+
// `visibilitychange` or have known bugs w.r.t the `visibilitychange` event.
39+
// TODO(v11): remove this once we drop support for Safari <14.4
40+
addPageListener('pagehide', cb, { once: true, capture: true });
3741
rIC(() => {
3842
cb();
3943
// Remove the above event listener since no longer required.
4044
// See: https://github.com/GoogleChrome/web-vitals/issues/622
4145
removePageListener('visibilitychange', cb, { capture: true });
46+
// TODO(v11): remove this once we drop support for Safari <14.4
47+
removePageListener('pagehide', cb, { capture: true });
4248
});
43-
// sentry: we use onHidden instead of directly listening to visibilitychange
44-
// because some browsers we still support (Safari <14.4) don't fully support
45-
// `visibilitychange` or have known bugs w.r.t the `visibilitychange` event.
46-
onHidden(cb);
4749
}
4850
};

0 commit comments

Comments
 (0)