Skip to content

Commit

Permalink
feat(core): allow unregistering callback through on
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
arturovt committed Apr 28, 2024
1 parent d381ace commit 5a2041b
Show file tree
Hide file tree
Showing 3 changed files with 120 additions and 29 deletions.
51 changes: 36 additions & 15 deletions packages/core/src/baseclient.ts
Original file line number Diff line number Diff line change
Expand Up @@ -388,37 +388,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) => void): void;
public on(hook: 'createDsc', callback: (dsc: DynamicSamplingContext) => void): () => void;

/** @inheritdoc */
public on(
hook: 'beforeSendFeedback',
callback: (feedback: FeedbackEvent, options?: { includeReplay: boolean }) => void,
): void;
): () => void;

/** @inheritdoc */
public on(
Expand All @@ -441,23 +444,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 */
Expand Down
55 changes: 55 additions & 0 deletions packages/core/test/lib/base.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1880,4 +1880,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 });
});
});
});
43 changes: 29 additions & 14 deletions packages/types/src/client.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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',
Expand All @@ -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) => void): void;
on(hook: 'createDsc', callback: (dsc: DynamicSamplingContext) => 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;
Expand Down

0 comments on commit 5a2041b

Please sign in to comment.