Skip to content

feat(core): Add type & utility for function-based integrations #9818

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 12 commits into from
Dec 18, 2023
7 changes: 6 additions & 1 deletion packages/core/src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,12 @@ export { createTransport } from './transports/base';
export { makeOfflineTransport } from './transports/offline';
export { makeMultiplexedTransport } from './transports/multiplexed';
export { SDK_VERSION } from './version';
export { getIntegrationsToSetup, addIntegration } from './integration';
export {
getIntegrationsToSetup,
addIntegration,
// eslint-disable-next-line deprecation/deprecation
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we are using deprecated functions, I think we should disable this rule instead of overriding it everywhere

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The idea here was to make it easier to spot that this should be removed in v8 🤔 no strong feelings, I just wanted to make it clear that this is a temporary thing that will be removed in v8 again - generally, we should avoid using deprecated stuff ourselves, so I think the rule is good.

convertIntegrationFnToClass,
} from './integration';
export { FunctionToString, InboundFilters, LinkedErrors } from './integrations';
export { prepareEvent } from './utils/prepareEvent';
export { createCheckInEnvelope } from './checkin';
Expand Down
25 changes: 24 additions & 1 deletion packages/core/src/integration.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import type { Client, Event, EventHint, Integration, Options } from '@sentry/types';
import type { Client, Event, EventHint, Integration, IntegrationClass, IntegrationFn, Options } from '@sentry/types';
import { arrayify, logger } from '@sentry/utils';

import { DEBUG_BUILD } from './debug-build';
Expand Down Expand Up @@ -155,3 +155,26 @@ function findIndex<T>(arr: T[], callback: (item: T) => boolean): number {

return -1;
}

/**
* Convert a new integration function to the legacy class syntax.
* In v8, we can remove this and instead export the integration functions directly.
*
* @deprecated This will be removed in v8!
*/
export function convertIntegrationFnToClass<Fn extends IntegrationFn>(
name: string,
fn: Fn,
): IntegrationClass<Integration> {
return Object.assign(
// eslint-disable-next-line @typescript-eslint/no-explicit-any
function ConvertedIntegration(...rest: any[]) {
return {
// eslint-disable-next-line @typescript-eslint/no-empty-function
setupOnce: () => {},
...fn(...rest),
};
},
{ id: name },
) as unknown as IntegrationClass<Integration>;
}
55 changes: 18 additions & 37 deletions packages/core/src/integrations/inboundfilters.ts
Original file line number Diff line number Diff line change
@@ -1,7 +1,8 @@
import type { Client, Event, EventHint, Integration, StackFrame } from '@sentry/types';
import type { Event, IntegrationFn, StackFrame } from '@sentry/types';
import { getEventDescription, logger, stringMatchesSomePattern } from '@sentry/utils';

import { DEBUG_BUILD } from '../debug-build';
import { convertIntegrationFnToClass } from '../integration';

// "Script error." is hard coded into browsers for errors that it can't read.
// this is the result of a script being pulled in from an external domain and CORS.
Expand All @@ -28,42 +29,23 @@ export interface InboundFiltersOptions {
disableTransactionDefaults: boolean;
}

/** Inbound filters configurable by the user */
export class InboundFilters implements Integration {
/**
* @inheritDoc
*/
public static id: string = 'InboundFilters';

/**
* @inheritDoc
*/
public name: string;

private readonly _options: Partial<InboundFiltersOptions>;

public constructor(options: Partial<InboundFiltersOptions> = {}) {
this.name = InboundFilters.id;
this._options = options;
}

/**
* @inheritDoc
*/
public setupOnce(_addGlobalEventProcessor: unknown, _getCurrentHub: unknown): void {
// noop
}
const INTEGRATION_NAME = 'InboundFilters';
const inboundFiltersIntegration: IntegrationFn = (options: Partial<InboundFiltersOptions>) => {
return {
name: INTEGRATION_NAME,
processEvent(event, _hint, client) {
const clientOptions = client.getOptions();
const mergedOptions = _mergeOptions(options, clientOptions);
return _shouldDropEvent(event, mergedOptions) ? null : event;
},
};
};

/** @inheritDoc */
public processEvent(event: Event, _eventHint: EventHint, client: Client): Event | null {
const clientOptions = client.getOptions();
const options = _mergeOptions(this._options, clientOptions);
return _shouldDropEvent(event, options) ? null : event;
}
}
/** Inbound filters configurable by the user */
// eslint-disable-next-line deprecation/deprecation
export const InboundFilters = convertIntegrationFnToClass(INTEGRATION_NAME, inboundFiltersIntegration);

/** JSDoc */
export function _mergeOptions(
function _mergeOptions(
internalOptions: Partial<InboundFiltersOptions> = {},
clientOptions: Partial<InboundFiltersOptions> = {},
): Partial<InboundFiltersOptions> {
Expand All @@ -84,8 +66,7 @@ export function _mergeOptions(
};
}

/** JSDoc */
export function _shouldDropEvent(event: Event, options: Partial<InboundFiltersOptions>): boolean {
function _shouldDropEvent(event: Event, options: Partial<InboundFiltersOptions>): boolean {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why does this function start with underscore?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

they are "private" - we are not super consistent there, sometimes we prefix them and sometimes not. but specifically here I did not change that, just removed the export as we don't actually use it anywhere!

if (options.ignoreInternal && _isSentryError(event)) {
DEBUG_BUILD &&
logger.warn(`Event dropped due to being internal Sentry Error.\nEvent: ${getEventDescription(event)}`);
Expand Down
56 changes: 55 additions & 1 deletion packages/core/test/lib/integration.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,13 @@ import type { Integration, Options } from '@sentry/types';
import { logger } from '@sentry/utils';

import { Hub, makeMain } from '../../src/hub';
import { addIntegration, getIntegrationsToSetup, installedIntegrations, setupIntegration } from '../../src/integration';
import {
addIntegration,
convertIntegrationFnToClass,
getIntegrationsToSetup,
installedIntegrations,
setupIntegration,
} from '../../src/integration';
import { TestClient, getDefaultTestClientOptions } from '../mocks/client';

function getTestClient(): TestClient {
Expand Down Expand Up @@ -647,3 +653,51 @@ describe('addIntegration', () => {
expect(warnings).toHaveBeenCalledWith('Cannot add integration "test" because no SDK Client is available.');
});
});

describe('convertIntegrationFnToClass', () => {
/* eslint-disable deprecation/deprecation */
it('works with a minimal integration', () => {
const integrationFn = () => ({ name: 'testName' });

const IntegrationClass = convertIntegrationFnToClass('testName', integrationFn);

expect(IntegrationClass.id).toBe('testName');

const integration = new IntegrationClass();
expect(integration).toEqual({
name: 'testName',
setupOnce: expect.any(Function),
});
});

it('works with integration hooks', () => {
const setup = jest.fn();
const setupOnce = jest.fn();
const processEvent = jest.fn();
const preprocessEvent = jest.fn();

const integrationFn = () => {
return {
name: 'testName',
setup,
setupOnce,
processEvent,
preprocessEvent,
};
};

const IntegrationClass = convertIntegrationFnToClass('testName', integrationFn);

expect(IntegrationClass.id).toBe('testName');

const integration = new IntegrationClass();
expect(integration).toEqual({
name: 'testName',
setupOnce,
setup,
processEvent,
preprocessEvent,
});
});
/* eslint-enable deprecation/deprecation */
});
2 changes: 1 addition & 1 deletion packages/types/src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,7 @@ export type { EventProcessor } from './eventprocessor';
export type { Exception } from './exception';
export type { Extra, Extras } from './extra';
export type { Hub } from './hub';
export type { Integration, IntegrationClass } from './integration';
export type { Integration, IntegrationClass, IntegrationFn, IntegrationFnResult } from './integration';
export type { Mechanism } from './mechanism';
export type { ExtractedNodeRequestData, HttpHeaderValue, Primitive, WorkerLocation } from './misc';
export type { ClientOptions, Options } from './options';
Expand Down
41 changes: 41 additions & 0 deletions packages/types/src/integration.ts
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,47 @@ export interface IntegrationClass<T> {
new (...args: any[]): T;
}

/**
* An integration in function form.
* This is expected to return an integration result,
*/
export type IntegrationFn = (...rest: any[]) => IntegrationFnResult;

export interface IntegrationFnResult {
/**
* The name of the integration.
*/
name: string;

/**
* This hook is only called once, even if multiple clients are created.
* It does not receives any arguments, and should only use for e.g. global monkey patching and similar things.
*/
setupOnce?(): void;

/**
* Set up an integration for the given client.
* Receives the client as argument.
*
* Whenever possible, prefer this over `setupOnce`, as that is only run for the first client,
* whereas `setup` runs for each client. Only truly global things (e.g. registering global handlers)
* should be done in `setupOnce`.
*/
setup?(client: Client): void;

/**
* An optional hook that allows to preprocess an event _before_ it is passed to all other event processors.
*/
preprocessEvent?(event: Event, hint: EventHint | undefined, client: Client): void;

/**
* An optional hook that allows to process an event.
* Return `null` to drop the event, or mutate the event & return it.
* This receives the client that the integration was installed for as third argument.
*/
processEvent?(event: Event, hint: EventHint, client: Client): Event | null | PromiseLike<Event | null>;
}

/** Integration interface */
export interface Integration {
/**
Expand Down