fix: Cleanup hooks when they are not used anymore (#12852)

Small optimization using the new hook cleanup capabilities to remove
unused hooks.

Ref PR to do this in angular:
#12786

Author: mydea

dev-packages/browser-integration-tests/suites/tracing/trace-lifetime/navigation/test.ts

@@ -74,6 +74,14 @@ sentryTest('error after navigation has navigation traceId', async ({ getLocalTes
     sentryTest.skip();
   }
 
+  await page.route('https://dsn.ingest.sentry.io/**/*', route => {
+    return route.fulfill({
+      status: 200,
+      contentType: 'application/json',
+      body: JSON.stringify({ id: 'test-id' }),
+    });
+  });
+
   const url = await getLocalTestUrl({ testDir: __dirname });
 
   // ensure pageload transaction is finished

packages/browser/src/tracing/browserTracingIntegration.ts

@@ -288,7 +288,7 @@ export const browserTracingIntegration = ((_options: Partial<BrowserTracingOptio
           return;
         }
 
-        if (activeSpan) {
+        if (activeSpan && !spanToJSON(activeSpan).timestamp) {
           DEBUG_BUILD && logger.log(`[Tracing] Finishing current root span with op: ${spanToJSON(activeSpan).op}`);
           // If there's an open transaction on the scope, we need to finish it before creating an new one.
           activeSpan.end();
@@ -304,7 +304,7 @@ export const browserTracingIntegration = ((_options: Partial<BrowserTracingOptio
           return;
         }
 
-        if (activeSpan) {
+        if (activeSpan && !spanToJSON(activeSpan).timestamp) {
           DEBUG_BUILD && logger.log(`[Tracing] Finishing current root span with op: ${spanToJSON(activeSpan).op}`);
           // If there's an open transaction on the scope, we need to finish it before creating an new one.
           activeSpan.end();

packages/core/src/baseclient.ts

@@ -459,27 +459,19 @@ export abstract class BaseClient<O extends ClientOptions> implements Client<O> {
 
   /** @inheritdoc */
   public on(hook: string, callback: unknown): () => void {
-    // Note that the code below, with nullish coalescing assignment,
-    // may reduce the code, so it may be switched to when Node 14 support
-    // is dropped (the `??=` operator is supported since Node 15).
-    // (this._hooks[hook] ??= []).push(callback);
-    if (!this._hooks[hook]) {
-      this._hooks[hook] = [];
-    }
+    const hooks = (this._hooks[hook] = this._hooks[hook] || []);
 
     // @ts-expect-error We assue the types are correct
-    this._hooks[hook].push(callback);
+    hooks.push(callback);
 
     // This function returns a callback execution handler that, when invoked,
     // deregisters a callback. This is crucial for managing instances where callbacks
     // need to be unregistered to prevent self-referencing in callback closures,
     // ensuring proper garbage collection.
     return () => {
-      const hooks = this._hooks[hook];
-
-      if (hooks) {
-        // @ts-expect-error We assue the types are correct
-        const cbIndex = hooks.indexOf(callback);
+      // @ts-expect-error We assue the types are correct
+      const cbIndex = hooks.indexOf(callback);
+      if (cbIndex > -1) {
         hooks.splice(cbIndex, 1);
       }
     };

packages/core/src/tracing/idleSpan.ts

@@ -96,6 +96,8 @@ export function startIdleSpan(startSpanOptions: StartSpanOptions, options: Parti
 
   let _autoFinishAllowed: boolean = !options.disableAutoFinish;
 
+  const _cleanupHooks: (() => void)[] = [];
+
   const {
     idleTimeout = TRACING_DEFAULTS.idleTimeout,
     finalTimeout = TRACING_DEFAULTS.finalTimeout,
@@ -240,6 +242,8 @@ export function startIdleSpan(startSpanOptions: StartSpanOptions, options: Parti
     _finished = true;
     activities.clear();
 
+    _cleanupHooks.forEach(cleanup => cleanup());
+
     _setSpanForScope(scope, previousActiveSpan);
 
     const spanJSON = spanToJSON(span);
@@ -298,41 +302,47 @@ export function startIdleSpan(startSpanOptions: StartSpanOptions, options: Parti
     }
   }
 
-  client.on('spanStart', startedSpan => {
-    // If we already finished the idle span,
-    // or if this is the idle span itself being started,
-    // or if the started span has already been closed,
-    // we don't care about it for activity
-    if (_finished || startedSpan === span || !!spanToJSON(startedSpan).timestamp) {
-      return;
-    }
+  _cleanupHooks.push(
+    client.on('spanStart', startedSpan => {
+      // If we already finished the idle span,
+      // or if this is the idle span itself being started,
+      // or if the started span has already been closed,
+      // we don't care about it for activity
+      if (_finished || startedSpan === span || !!spanToJSON(startedSpan).timestamp) {
+        return;
+      }
 
-    const allSpans = getSpanDescendants(span);
+      const allSpans = getSpanDescendants(span);
 
-    // If the span that was just started is a child of the idle span, we should track it
-    if (allSpans.includes(startedSpan)) {
-      _pushActivity(startedSpan.spanContext().spanId);
-    }
-  });
+      // If the span that was just started is a child of the idle span, we should track it
+      if (allSpans.includes(startedSpan)) {
+        _pushActivity(startedSpan.spanContext().spanId);
+      }
+    }),
+  );
 
-  client.on('spanEnd', endedSpan => {
-    if (_finished) {
-      return;
-    }
+  _cleanupHooks.push(
+    client.on('spanEnd', endedSpan => {
+      if (_finished) {
+        return;
+      }
 
-    _popActivity(endedSpan.spanContext().spanId);
-  });
+      _popActivity(endedSpan.spanContext().spanId);
+    }),
+  );
 
-  client.on('idleSpanEnableAutoFinish', spanToAllowAutoFinish => {
-    if (spanToAllowAutoFinish === span) {
-      _autoFinishAllowed = true;
-      _restartIdleTimeout();
+  _cleanupHooks.push(
+    client.on('idleSpanEnableAutoFinish', spanToAllowAutoFinish => {
+      if (spanToAllowAutoFinish === span) {
+        _autoFinishAllowed = true;
+        _restartIdleTimeout();
 
-      if (activities.size) {
-        _restartChildSpanTimeout();
+        if (activities.size) {
+          _restartChildSpanTimeout();
+        }
       }
-    }
-  });
+    }),
+  );
 
   // We only start the initial idle timeout if we are not delaying the auto finish
   if (!options.disableAutoFinish) {

packages/feedback/src/core/sendFeedback.ts

@@ -40,12 +40,13 @@ export const sendFeedback: SendFeedback = (
     // After 5s, we want to clear anyhow
     const timeout = setTimeout(() => reject('Unable to determine if Feedback was correctly sent.'), 5_000);
 
-    client.on('afterSendEvent', (event: Event, response: TransportMakeRequestResponse) => {
+    const cleanup = client.on('afterSendEvent', (event: Event, response: TransportMakeRequestResponse) => {
       if (event.event_id !== eventId) {
         return;
       }
 
       clearTimeout(timeout);
+      cleanup();
 
       // Require valid status codes, otherwise can assume feedback was not sent successfully
       if (

packages/react/src/errorboundary.tsx

@@ -77,6 +77,7 @@ class ErrorBoundary extends React.Component<ErrorBoundaryProps, ErrorBoundarySta
   private readonly _openFallbackReportDialog: boolean;
 
   private _lastEventId?: string;
+  private _cleanupHook?: () => void;
 
   public constructor(props: ErrorBoundaryProps) {
     super(props);
@@ -87,7 +88,7 @@ class ErrorBoundary extends React.Component<ErrorBoundaryProps, ErrorBoundarySta
     const client = getClient();
     if (client && props.showDialog) {
       this._openFallbackReportDialog = false;
-      client.on('afterSendEvent', event => {
+      this._cleanupHook = client.on('afterSendEvent', event => {
         if (!event.type && this._lastEventId && event.event_id === this._lastEventId) {
           showReportDialog({ ...props.dialogOptions, eventId: this._lastEventId });
         }
@@ -137,6 +138,11 @@ class ErrorBoundary extends React.Component<ErrorBoundaryProps, ErrorBoundarySta
     if (onUnmount) {
       onUnmount(error, componentStack, eventId);
     }
+
+    if (this._cleanupHook) {
+      this._cleanupHook();
+      this._cleanupHook = undefined;
+    }
   }
 
   public resetErrorBoundary: () => void = () => {