feat(core): allow unregistering callback through on

This commit updates the return signature of the client's `on` function to be a void function,
which, when executed, unregisters a callback. This adjustment is necessary for managing instances
where objects are created and destroyed, ensuring that callbacks are properly unregistered to
prevent self-referencing in callback closures and facilitate proper garbage collection.

Typically, changing a type from `void` to `() => void` (or `VoidFunction`) shouldn't be considered
a breaking change because `void` signifies the absence of a return value, implying that the return
value of the `on` function should never be used by consumers.

Opting for the `on` approach, which returns a cleanup function, "seems" simpler because having another
function called `off` requires saving the callback reference for later removal. With our pattern,
we encapsulate both the registration and removal of event listeners within a single function call.

Author: arturovt

packages/core/src/baseclient.ts

@@ -390,37 +390,40 @@ export abstract class BaseClient<O extends ClientOptions> implements Client<O> {
   /* eslint-disable @typescript-eslint/unified-signatures */
 
   /** @inheritdoc */
-  public on(hook: 'spanStart', callback: (span: Span) => void): void;
+  public on(hook: 'spanStart', callback: (span: Span) => void): () => void;
 
   /** @inheritdoc */
-  public on(hook: 'spanEnd', callback: (span: Span) => void): void;
+  public on(hook: 'spanEnd', callback: (span: Span) => void): () => void;
 
   /** @inheritdoc */
-  public on(hook: 'idleSpanEnableAutoFinish', callback: (span: Span) => void): void;
+  public on(hook: 'idleSpanEnableAutoFinish', callback: (span: Span) => void): () => void;
 
   /** @inheritdoc */
-  public on(hook: 'beforeEnvelope', callback: (envelope: Envelope) => void): void;
+  public on(hook: 'beforeEnvelope', callback: (envelope: Envelope) => void): () => void;
 
   /** @inheritdoc */
-  public on(hook: 'beforeSendEvent', callback: (event: Event, hint?: EventHint) => void): void;
+  public on(hook: 'beforeSendEvent', callback: (event: Event, hint?: EventHint) => void): () => void;
 
   /** @inheritdoc */
-  public on(hook: 'preprocessEvent', callback: (event: Event, hint?: EventHint) => void): void;
+  public on(hook: 'preprocessEvent', callback: (event: Event, hint?: EventHint) => void): () => void;
 
   /** @inheritdoc */
-  public on(hook: 'afterSendEvent', callback: (event: Event, sendResponse: TransportMakeRequestResponse) => void): void;
+  public on(
+    hook: 'afterSendEvent',
+    callback: (event: Event, sendResponse: TransportMakeRequestResponse) => void,
+  ): () => void;
 
   /** @inheritdoc */
-  public on(hook: 'beforeAddBreadcrumb', callback: (breadcrumb: Breadcrumb, hint?: BreadcrumbHint) => void): void;
+  public on(hook: 'beforeAddBreadcrumb', callback: (breadcrumb: Breadcrumb, hint?: BreadcrumbHint) => void): () => void;
 
   /** @inheritdoc */
-  public on(hook: 'createDsc', callback: (dsc: DynamicSamplingContext, rootSpan?: Span) => void): void;
+  public on(hook: 'createDsc', callback: (dsc: DynamicSamplingContext, rootSpan?: Span) => void): () => void;
 
   /** @inheritdoc */
   public on(
     hook: 'beforeSendFeedback',
     callback: (feedback: FeedbackEvent, options?: { includeReplay: boolean }) => void,
-  ): void;
+  ): () => void;
 
   /** @inheritdoc */
   public on(
@@ -443,23 +446,41 @@ export abstract class BaseClient<O extends ClientOptions> implements Client<O> {
       options: StartSpanOptions,
       traceOptions?: { sentryTrace?: string | undefined; baggage?: string | undefined },
     ) => void,
-  ): void;
+  ): () => void;
 
   /** @inheritdoc */
-  public on(hook: 'startNavigationSpan', callback: (options: StartSpanOptions) => void): void;
+  public on(hook: 'startNavigationSpan', callback: (options: StartSpanOptions) => void): () => void;
 
-  public on(hook: 'flush', callback: () => void): void;
+  public on(hook: 'flush', callback: () => void): () => void;
 
-  public on(hook: 'close', callback: () => void): void;
+  public on(hook: 'close', callback: () => void): () => void;
 
   /** @inheritdoc */
-  public on(hook: string, callback: unknown): void {
+  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] = [];
     }
 
     // @ts-expect-error We assue the types are correct
     this._hooks[hook].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);
+        hooks.splice(cbIndex, 1);
+      }
+    };
   }
 
   /** @inheritdoc */

packages/core/test/lib/base.test.ts

@@ -2014,4 +2014,59 @@ describe('BaseClient', () => {
       });
     });
   });
+
+  describe('hook removal with `on`', () => {
+    it('should return a cleanup function that, when executed, unregisters a hook', async () => {
+      expect.assertions(8);
+
+      const client = new TestClient(
+        getDefaultTestClientOptions({
+          dsn: PUBLIC_DSN,
+          enableSend: true,
+        }),
+      );
+
+      // @ts-expect-error Accessing private transport API
+      const mockSend = jest.spyOn(client._transport, 'send').mockImplementation(() => {
+        return Promise.resolve({ statusCode: 200 });
+      });
+
+      const errorEvent: Event = { message: 'error' };
+
+      const callback = jest.fn();
+      const removeAfterSendEventListenerFn = client.on('afterSendEvent', callback);
+
+      // @ts-expect-error Accessing private client API
+      expect(client._hooks['afterSendEvent']).toEqual([callback]);
+
+      client.sendEvent(errorEvent);
+      jest.runAllTimers();
+      // Wait for two ticks
+      // note that for whatever reason, await new Promise(resolve => setTimeout(resolve, 0)) causes the test to hang
+      await undefined;
+      await undefined;
+
+      expect(mockSend).toBeCalledTimes(1);
+      expect(callback).toBeCalledTimes(1);
+      expect(callback).toBeCalledWith(errorEvent, { statusCode: 200 });
+
+      // Should unregister `afterSendEvent` callback.
+      removeAfterSendEventListenerFn();
+      // @ts-expect-error Accessing private client API
+      expect(client._hooks['afterSendEvent']).toEqual([]);
+
+      client.sendEvent(errorEvent);
+      jest.runAllTimers();
+      // Wait for two ticks
+      // note that for whatever reason, await new Promise(resolve => setTimeout(resolve, 0)) causes the test to hang
+      await undefined;
+      await undefined;
+
+      expect(mockSend).toBeCalledTimes(2);
+      // Note that the `callback` has still been called only once and not twice,
+      // because we unregistered it.
+      expect(callback).toBeCalledTimes(1);
+      expect(callback).toBeCalledWith(errorEvent, { statusCode: 200 });
+    });
+  });
 });

packages/types/src/client.ts

@@ -181,12 +181,14 @@ export interface Client<O extends ClientOptions = ClientOptions> {
   /**
    * Register a callback for whenever a span is started.
    * Receives the span as argument.
+   * @returns A function that, when executed, removes the registered callback.
    */
-  on(hook: 'spanStart', callback: (span: Span) => void): void;
+  on(hook: 'spanStart', callback: (span: Span) => void): () => void;
 
   /**
    * Register a callback before span sampling runs. Receives a `samplingDecision` object argument with a `decision`
    * property that can be used to make a sampling decision that will be enforced, before any span sampling runs.
+   * @returns A function that, when executed, removes the registered callback.
    */
   on(
     hook: 'beforeSampling',
@@ -204,83 +206,96 @@ export interface Client<O extends ClientOptions = ClientOptions> {
   /**
    * Register a callback for whenever a span is ended.
    * Receives the span as argument.
+   * @returns A function that, when executed, removes the registered callback.
    */
-  on(hook: 'spanEnd', callback: (span: Span) => void): void;
+  on(hook: 'spanEnd', callback: (span: Span) => void): () => void;
 
   /**
    * Register a callback for when an idle span is allowed to auto-finish.
+   * @returns A function that, when executed, removes the registered callback.
    */
-  on(hook: 'idleSpanEnableAutoFinish', callback: (span: Span) => void): void;
+  on(hook: 'idleSpanEnableAutoFinish', callback: (span: Span) => void): () => void;
 
   /**
    * Register a callback for transaction start and finish.
+   * @returns A function that, when executed, removes the registered callback.
    */
-  on(hook: 'beforeEnvelope', callback: (envelope: Envelope) => void): void;
+  on(hook: 'beforeEnvelope', callback: (envelope: Envelope) => void): () => void;
 
   /**
    * Register a callback for before sending an event.
    * This is called right before an event is sent and should not be used to mutate the event.
    * Receives an Event & EventHint as arguments.
+   * @returns A function that, when executed, removes the registered callback.
    */
-  on(hook: 'beforeSendEvent', callback: (event: Event, hint?: EventHint | undefined) => void): void;
+  on(hook: 'beforeSendEvent', callback: (event: Event, hint?: EventHint | undefined) => void): () => void;
 
   /**
    * Register a callback for preprocessing an event,
    * before it is passed to (global) event processors.
    * Receives an Event & EventHint as arguments.
+   * @returns A function that, when executed, removes the registered callback.
    */
-  on(hook: 'preprocessEvent', callback: (event: Event, hint?: EventHint | undefined) => void): void;
+  on(hook: 'preprocessEvent', callback: (event: Event, hint?: EventHint | undefined) => void): () => void;
 
   /**
    * Register a callback for when an event has been sent.
+   * @returns A function that, when executed, removes the registered callback.
    */
-  on(hook: 'afterSendEvent', callback: (event: Event, sendResponse: TransportMakeRequestResponse) => void): void;
+  on(hook: 'afterSendEvent', callback: (event: Event, sendResponse: TransportMakeRequestResponse) => void): () => void;
 
   /**
    * Register a callback before a breadcrumb is added.
+   * @returns A function that, when executed, removes the registered callback.
    */
-  on(hook: 'beforeAddBreadcrumb', callback: (breadcrumb: Breadcrumb, hint?: BreadcrumbHint) => void): void;
+  on(hook: 'beforeAddBreadcrumb', callback: (breadcrumb: Breadcrumb, hint?: BreadcrumbHint) => void): () => void;
 
   /**
    * Register a callback when a DSC (Dynamic Sampling Context) is created.
+   * @returns A function that, when executed, removes the registered callback.
    */
-  on(hook: 'createDsc', callback: (dsc: DynamicSamplingContext, rootSpan?: Span) => void): void;
+  on(hook: 'createDsc', callback: (dsc: DynamicSamplingContext, rootSpan?: Span) => void): () => void;;
 
   /**
    * Register a callback when a Feedback event has been prepared.
    * This should be used to mutate the event. The options argument can hint
    * about what kind of mutation it expects.
+   * @returns A function that, when executed, removes the registered callback.
    */
   on(
     hook: 'beforeSendFeedback',
     callback: (feedback: FeedbackEvent, options?: { includeReplay?: boolean }) => void,
-  ): void;
+  ): () => void;
 
   /**
    * A hook for the browser tracing integrations to trigger a span start for a page load.
+   * @returns A function that, when executed, removes the registered callback.
    */
   on(
     hook: 'startPageLoadSpan',
     callback: (
       options: StartSpanOptions,
       traceOptions?: { sentryTrace?: string | undefined; baggage?: string | undefined },
     ) => void,
-  ): void;
+  ): () => void;
 
   /**
    * A hook for browser tracing integrations to trigger a span for a navigation.
+   * @returns A function that, when executed, removes the registered callback.
    */
-  on(hook: 'startNavigationSpan', callback: (options: StartSpanOptions) => void): void;
+  on(hook: 'startNavigationSpan', callback: (options: StartSpanOptions) => void): () => void;
 
   /**
    * A hook that is called when the client is flushing
+   * @returns A function that, when executed, removes the registered callback.
    */
-  on(hook: 'flush', callback: () => void): void;
+  on(hook: 'flush', callback: () => void): () => void;
 
   /**
    * A hook that is called when the client is closing
+   * @returns A function that, when executed, removes the registered callback.
    */
-  on(hook: 'close', callback: () => void): void;
+  on(hook: 'close', callback: () => void): () => void;
 
   /** Fire a hook whener a span starts. */
   emit(hook: 'spanStart', span: Span): void;