From 2c386274355065e5c03520b68517f9735153a8f4 Mon Sep 17 00:00:00 2001 From: arturovt Date: Mon, 22 Apr 2024 10:06:00 +0300 Subject: [PATCH 1/3] 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. --- packages/core/src/baseclient.ts | 51 ++++++++++++++++++-------- packages/core/test/lib/base.test.ts | 55 +++++++++++++++++++++++++++++ packages/types/src/client.ts | 43 ++++++++++++++-------- 3 files changed, 120 insertions(+), 29 deletions(-) diff --git a/packages/core/src/baseclient.ts b/packages/core/src/baseclient.ts index 59afda8dc43b..70c4261a6d3f 100644 --- a/packages/core/src/baseclient.ts +++ b/packages/core/src/baseclient.ts @@ -390,37 +390,40 @@ export abstract class BaseClient implements Client { /* 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 implements Client { 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 */ diff --git a/packages/core/test/lib/base.test.ts b/packages/core/test/lib/base.test.ts index d24dd42cb1c9..afce551c4cf0 100644 --- a/packages/core/test/lib/base.test.ts +++ b/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 }); + }); + }); }); diff --git a/packages/types/src/client.ts b/packages/types/src/client.ts index bb0ec4646211..eed9279352eb 100644 --- a/packages/types/src/client.ts +++ b/packages/types/src/client.ts @@ -181,12 +181,14 @@ export interface Client { /** * 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,60 +206,70 @@ export interface Client { /** * 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', @@ -265,22 +277,25 @@ export interface Client { 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; From cf9e279771a8dbaf448d7759e34ae7b3538f2168 Mon Sep 17 00:00:00 2001 From: Francesco Novy Date: Wed, 3 Jul 2024 10:37:18 +0200 Subject: [PATCH 2/3] Apply suggestions from code review --- packages/core/test/lib/base.test.ts | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/packages/core/test/lib/base.test.ts b/packages/core/test/lib/base.test.ts index afce551c4cf0..cc25811acd31 100644 --- a/packages/core/test/lib/base.test.ts +++ b/packages/core/test/lib/base.test.ts @@ -2026,8 +2026,7 @@ describe('BaseClient', () => { }), ); - // @ts-expect-error Accessing private transport API - const mockSend = jest.spyOn(client._transport, 'send').mockImplementation(() => { + const mockSend = jest.spyOn(client['_transport'], 'send').mockImplementation(() => { return Promise.resolve({ statusCode: 200 }); }); @@ -2036,8 +2035,7 @@ describe('BaseClient', () => { const callback = jest.fn(); const removeAfterSendEventListenerFn = client.on('afterSendEvent', callback); - // @ts-expect-error Accessing private client API - expect(client._hooks['afterSendEvent']).toEqual([callback]); + expect(client['_hooks']['afterSendEvent']).toEqual([callback]); client.sendEvent(errorEvent); jest.runAllTimers(); @@ -2052,8 +2050,7 @@ describe('BaseClient', () => { // Should unregister `afterSendEvent` callback. removeAfterSendEventListenerFn(); - // @ts-expect-error Accessing private client API - expect(client._hooks['afterSendEvent']).toEqual([]); + expect(client['_hooks']['afterSendEvent']).toEqual([]); client.sendEvent(errorEvent); jest.runAllTimers(); From d7c2fd48e52f0d6a7a1542e1b3253cd2c7392f25 Mon Sep 17 00:00:00 2001 From: Francesco Novy Date: Wed, 3 Jul 2024 11:54:22 +0200 Subject: [PATCH 3/3] fix it, oops... --- packages/core/test/lib/base.test.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/core/test/lib/base.test.ts b/packages/core/test/lib/base.test.ts index cc25811acd31..3fa10828e32a 100644 --- a/packages/core/test/lib/base.test.ts +++ b/packages/core/test/lib/base.test.ts @@ -2026,7 +2026,7 @@ describe('BaseClient', () => { }), ); - const mockSend = jest.spyOn(client['_transport'], 'send').mockImplementation(() => { + const mockSend = jest.spyOn(client.getTransport()!, 'send').mockImplementation(() => { return Promise.resolve({ statusCode: 200 }); });