From f62ee103aa6d2d3afda0112b49ab3e98f3248eb6 Mon Sep 17 00:00:00 2001 From: Francesco Novy Date: Tue, 9 Jan 2024 15:52:25 +0100 Subject: [PATCH] feat(core): Add `client.init()` to replace `client.setupIntegrations()` --- MIGRATION.md | 4 + packages/browser/test/unit/sdk.test.ts | 2 +- packages/core/src/baseclient.ts | 20 ++++- packages/core/src/hub.ts | 2 + packages/core/test/lib/base.test.ts | 74 +++++++++++++++++-- .../lib/integrations/inboundfilters.test.ts | 2 +- packages/node-experimental/src/sdk/init.ts | 16 +++- packages/node/src/sdk.ts | 6 +- .../test/integrations/localvariables.test.ts | 24 +++--- packages/types/src/client.ts | 11 ++- 10 files changed, 134 insertions(+), 27 deletions(-) diff --git a/MIGRATION.md b/MIGRATION.md index c7587fc10c76..00a81c25f19a 100644 --- a/MIGRATION.md +++ b/MIGRATION.md @@ -8,6 +8,10 @@ npx @sentry/migr8@latest This will let you select which updates to run, and automatically update your code. Make sure to still review all code changes! +## Deprecate `client.setupIntegrations()` + +Instead, use the new `client.init()` method. You should probably not use this directly and instead use `Sentry.init()`, which calls this under the hood. But if you have a special use case that requires that, you can call `client.init()` instead now. + ## Deprecate `scope.getSpan()` and `scope.setSpan()` Instead, you can get the currently active span via `Sentry.getActiveSpan()`. diff --git a/packages/browser/test/unit/sdk.test.ts b/packages/browser/test/unit/sdk.test.ts index fe977707920b..75cb238d35bb 100644 --- a/packages/browser/test/unit/sdk.test.ts +++ b/packages/browser/test/unit/sdk.test.ts @@ -42,7 +42,7 @@ jest.mock('@sentry/core', () => { return new Scope(); }, bindClient(client: Client): boolean { - client.setupIntegrations(); + client.init!(); return true; }, }; diff --git a/packages/core/src/baseclient.ts b/packages/core/src/baseclient.ts index 628df591248d..83615b5c8564 100644 --- a/packages/core/src/baseclient.ts +++ b/packages/core/src/baseclient.ts @@ -314,12 +314,19 @@ export abstract class BaseClient implements Client { } /** - * Sets up the integrations + * This is an internal function to setup all integrations that should run on the client. + * @deprecated Use `client.init()` instead. */ public setupIntegrations(forceInitialize?: boolean): void { if ((forceInitialize && !this._integrationsInitialized) || (this._isEnabled() && !this._integrationsInitialized)) { - this._integrations = setupIntegrations(this, this._options.integrations); - this._integrationsInitialized = true; + this._setupIntegrations(); + } + } + + /** @inheritdoc */ + public init(): void { + if (this._isEnabled()) { + this._setupIntegrations(); } } @@ -512,6 +519,13 @@ export abstract class BaseClient implements Client { /* eslint-enable @typescript-eslint/unified-signatures */ + /** Setup integrations for this client. */ + protected _setupIntegrations(): void { + this._integrations = setupIntegrations(this, this._options.integrations); + // TODO v8: We don't need this flag anymore + this._integrationsInitialized = true; + } + /** Updates existing session based on the provided event */ protected _updateSessionFromEvent(session: Session, event: Event): void { let crashed = false; diff --git a/packages/core/src/hub.ts b/packages/core/src/hub.ts index 0787f7c164ed..e4e7bc8bf499 100644 --- a/packages/core/src/hub.ts +++ b/packages/core/src/hub.ts @@ -167,7 +167,9 @@ export class Hub implements HubInterface { const top = this.getStackTop(); top.client = client; top.scope.setClient(client); + // eslint-disable-next-line deprecation/deprecation if (client && client.setupIntegrations) { + // eslint-disable-next-line deprecation/deprecation client.setupIntegrations(); } } diff --git a/packages/core/test/lib/base.test.ts b/packages/core/test/lib/base.test.ts index 0b39216e19a5..078cd04f63e8 100644 --- a/packages/core/test/lib/base.test.ts +++ b/packages/core/test/lib/base.test.ts @@ -671,7 +671,7 @@ describe('BaseClient', () => { test('adds installed integrations to sdk info', () => { const options = getDefaultTestClientOptions({ dsn: PUBLIC_DSN, integrations: [new TestIntegration()] }); const client = new TestClient(options); - client.setupIntegrations(); + client.init(); client.captureEvent({ message: 'message' }); @@ -685,7 +685,7 @@ describe('BaseClient', () => { const options = getDefaultTestClientOptions({ dsn: PUBLIC_DSN, integrations: [new TestIntegration()] }); const client = new TestClient(options); - client.setupIntegrations(); + client.init(); client.addIntegration(new AdHocIntegration()); client.captureException(new Error('test exception')); @@ -706,7 +706,7 @@ describe('BaseClient', () => { integrations: [new TestIntegration(), null, undefined], }); const client = new TestClient(options); - client.setupIntegrations(); + client.init(); client.captureEvent({ message: 'message' }); @@ -1482,24 +1482,48 @@ describe('BaseClient', () => { const options = getDefaultTestClientOptions({ dsn: PUBLIC_DSN, integrations: [new TestIntegration()] }); const client = new TestClient(options); + // eslint-disable-next-line deprecation/deprecation client.setupIntegrations(); expect(Object.keys((client as any)._integrations).length).toEqual(1); expect(client.getIntegration(TestIntegration)).toBeTruthy(); }); - test('skips installation if DSN is not provided', () => { + test('sets up each integration on `init` call', () => { + expect.assertions(2); + + const options = getDefaultTestClientOptions({ dsn: PUBLIC_DSN, integrations: [new TestIntegration()] }); + const client = new TestClient(options); + client.init(); + + expect(Object.keys((client as any)._integrations).length).toEqual(1); + expect(client.getIntegration(TestIntegration)).toBeTruthy(); + }); + + test('skips installation for `setupIntegrations()` if DSN is not provided', () => { expect.assertions(2); const options = getDefaultTestClientOptions({ integrations: [new TestIntegration()] }); const client = new TestClient(options); + // eslint-disable-next-line deprecation/deprecation client.setupIntegrations(); expect(Object.keys((client as any)._integrations).length).toEqual(0); expect(client.getIntegration(TestIntegration)).toBeFalsy(); }); - test('skips installation if `enabled` is set to `false`', () => { + test('skips installation for `init()` if DSN is not provided', () => { + expect.assertions(2); + + const options = getDefaultTestClientOptions({ integrations: [new TestIntegration()] }); + const client = new TestClient(options); + client.init(); + + expect(Object.keys((client as any)._integrations).length).toEqual(0); + expect(client.getIntegration(TestIntegration)).toBeFalsy(); + }); + + test('skips installation for `setupIntegrations()` if `enabled` is set to `false`', () => { expect.assertions(2); const options = getDefaultTestClientOptions({ @@ -1508,12 +1532,28 @@ describe('BaseClient', () => { integrations: [new TestIntegration()], }); const client = new TestClient(options); + // eslint-disable-next-line deprecation/deprecation client.setupIntegrations(); expect(Object.keys((client as any)._integrations).length).toEqual(0); expect(client.getIntegration(TestIntegration)).toBeFalsy(); }); + test('skips installation for `init()` if `enabled` is set to `false`', () => { + expect.assertions(2); + + const options = getDefaultTestClientOptions({ + dsn: PUBLIC_DSN, + enabled: false, + integrations: [new TestIntegration()], + }); + const client = new TestClient(options); + client.init(); + + expect(Object.keys((client as any)._integrations).length).toEqual(0); + expect(client.getIntegration(TestIntegration)).toBeFalsy(); + }); + test('skips installation if integrations are already installed', () => { expect.assertions(4); @@ -1523,6 +1563,7 @@ describe('BaseClient', () => { const setupIntegrationsHelper = jest.spyOn(integrationModule, 'setupIntegrations'); // it should install the first time, because integrations aren't yet installed... + // eslint-disable-next-line deprecation/deprecation client.setupIntegrations(); expect(Object.keys((client as any)._integrations).length).toEqual(1); @@ -1530,10 +1571,33 @@ describe('BaseClient', () => { expect(setupIntegrationsHelper).toHaveBeenCalledTimes(1); // ...but it shouldn't try to install a second time + // eslint-disable-next-line deprecation/deprecation client.setupIntegrations(); expect(setupIntegrationsHelper).toHaveBeenCalledTimes(1); }); + + test('does not add integrations twice when calling `init` multiple times', () => { + const options = getDefaultTestClientOptions({ dsn: PUBLIC_DSN, integrations: [new TestIntegration()] }); + const client = new TestClient(options); + // note: not the `Client` method `setupIntegrations`, but the free-standing function which that method calls + const setupIntegrationsHelper = jest.spyOn(integrationModule, 'setupIntegrations'); + + // it should install the first time, because integrations aren't yet installed... + client.init(); + + expect(Object.keys((client as any)._integrations).length).toEqual(1); + expect(client.getIntegration(TestIntegration)).toBeTruthy(); + expect(setupIntegrationsHelper).toHaveBeenCalledTimes(1); + + client.init(); + + // is called again... + expect(setupIntegrationsHelper).toHaveBeenCalledTimes(2); + + // but integrations are only added once anyhow! + expect(client['_integrations']).toEqual({ TestIntegration: expect.any(TestIntegration) }); + }); }); describe('flush/close', () => { diff --git a/packages/core/test/lib/integrations/inboundfilters.test.ts b/packages/core/test/lib/integrations/inboundfilters.test.ts index c8df79cd22fe..b49b4b18671f 100644 --- a/packages/core/test/lib/integrations/inboundfilters.test.ts +++ b/packages/core/test/lib/integrations/inboundfilters.test.ts @@ -37,7 +37,7 @@ function createInboundFiltersEventProcessor( }), ); - client.setupIntegrations(); + client.init(); const eventProcessors = client['_eventProcessors']; const eventProcessor = eventProcessors.find(processor => processor.id === 'InboundFilters'); diff --git a/packages/node-experimental/src/sdk/init.ts b/packages/node-experimental/src/sdk/init.ts index 0f60bccd343c..ce22aa109339 100644 --- a/packages/node-experimental/src/sdk/init.ts +++ b/packages/node-experimental/src/sdk/init.ts @@ -6,7 +6,7 @@ import { getSentryRelease, makeNodeTransport, } from '@sentry/node'; -import type { Integration } from '@sentry/types'; +import type { Client, Integration } from '@sentry/types'; import { consoleSandbox, dropUndefinedKeys, @@ -67,7 +67,9 @@ export function init(options: NodeExperimentalOptions | undefined = {}): void { // unless somebody specifically sets a different one on a scope/isolations cope getGlobalScope().setClient(client); - client.setupIntegrations(); + if (isEnabled(client)) { + client.init(); + } if (options.autoSessionTracking) { startSessionTracking(); @@ -79,7 +81,11 @@ export function init(options: NodeExperimentalOptions | undefined = {}): void { const client = getClient(); if (client.addIntegration) { // force integrations to be setup even if no DSN was set - client.setupIntegrations(true); + // If they have already been added before, they will be ignored anyhow + const integrations = client.getOptions().integrations; + for (const integration of integrations) { + client.addIntegration(integration); + } client.addIntegration( new Integrations.Spotlight({ sidecarUrl: typeof options.spotlight === 'string' ? options.spotlight : undefined, @@ -213,3 +219,7 @@ function startSessionTracking(): void { } }); } + +function isEnabled(client: Client): boolean { + return client.getOptions().enabled !== false && client.getTransport() !== undefined; +} diff --git a/packages/node/src/sdk.ts b/packages/node/src/sdk.ts index 59a895b0809c..00091f6192c7 100644 --- a/packages/node/src/sdk.ts +++ b/packages/node/src/sdk.ts @@ -183,7 +183,11 @@ export function init(options: NodeOptions = {}): void { const client = getClient(); if (client && client.addIntegration) { // force integrations to be setup even if no DSN was set - client.setupIntegrations(true); + // If they have already been added before, they will be ignored anyhow + const integrations = client.getOptions().integrations; + for (const integration of integrations) { + client.addIntegration(integration); + } client.addIntegration( new Spotlight({ sidecarUrl: typeof options.spotlight === 'string' ? options.spotlight : undefined }), ); diff --git a/packages/node/test/integrations/localvariables.test.ts b/packages/node/test/integrations/localvariables.test.ts index e592c90a3a86..b0fc6094e6a5 100644 --- a/packages/node/test/integrations/localvariables.test.ts +++ b/packages/node/test/integrations/localvariables.test.ts @@ -162,11 +162,11 @@ describeIf(NODE_VERSION.major >= 18)('LocalVariables', () => { const options = getDefaultNodeClientOptions({ stackParser: defaultStackParser, includeLocalVariables: true, - integrations: [localVariables], + integrations: [], }); const client = new NodeClient(options); - client.setupIntegrations(true); + client.addIntegration(localVariables); const eventProcessors = client['_eventProcessors']; const eventProcessor = eventProcessors.find(processor => processor.id === 'LocalVariables'); @@ -253,11 +253,11 @@ describeIf(NODE_VERSION.major >= 18)('LocalVariables', () => { const options = getDefaultNodeClientOptions({ stackParser: defaultStackParser, includeLocalVariables: true, - integrations: [localVariables], + integrations: [], }); const client = new NodeClient(options); - client.setupIntegrations(true); + client.addIntegration(localVariables); await session.runPause(exceptionEvent100Frames); @@ -278,11 +278,11 @@ describeIf(NODE_VERSION.major >= 18)('LocalVariables', () => { const options = getDefaultNodeClientOptions({ stackParser: defaultStackParser, includeLocalVariables: true, - integrations: [localVariables], + integrations: [], }); const client = new NodeClient(options); - client.setupIntegrations(true); + client.addIntegration(localVariables); const nonExceptionEvent = { method: exceptionEvent.method, @@ -299,11 +299,11 @@ describeIf(NODE_VERSION.major >= 18)('LocalVariables', () => { const localVariables = new LocalVariablesSync({}, session); const options = getDefaultNodeClientOptions({ stackParser: defaultStackParser, - integrations: [localVariables], + integrations: [], }); const client = new NodeClient(options); - client.setupIntegrations(true); + client.addIntegration(localVariables); const eventProcessors = client['_eventProcessors']; const eventProcessor = eventProcessors.find(processor => processor.id === 'LocalVariables'); @@ -315,11 +315,11 @@ describeIf(NODE_VERSION.major >= 18)('LocalVariables', () => { const localVariables = new LocalVariablesSync({}, undefined); const options = getDefaultNodeClientOptions({ stackParser: defaultStackParser, - integrations: [localVariables], + integrations: [], }); const client = new NodeClient(options); - client.setupIntegrations(true); + client.addIntegration(localVariables); const eventProcessors = client['_eventProcessors']; const eventProcessor = eventProcessors.find(processor => processor.id === 'LocalVariables'); @@ -336,11 +336,11 @@ describeIf(NODE_VERSION.major >= 18)('LocalVariables', () => { const options = getDefaultNodeClientOptions({ stackParser: defaultStackParser, includeLocalVariables: true, - integrations: [localVariables], + integrations: [], }); const client = new NodeClient(options); - client.setupIntegrations(true); + client.addIntegration(localVariables); await session.runPause(exceptionEvent); await session.runPause(exceptionEvent); diff --git a/packages/types/src/client.ts b/packages/types/src/client.ts index c9c37349306d..bee40613bb8c 100644 --- a/packages/types/src/client.ts +++ b/packages/types/src/client.ts @@ -151,9 +151,18 @@ export interface Client { * */ addIntegration?(integration: Integration): void; - /** This is an internal function to setup all integrations that should run on the client */ + /** + * This is an internal function to setup all integrations that should run on the client. + * @deprecated Use `client.init()` instead. + */ setupIntegrations(forceInitialize?: boolean): void; + /** + * Initialize this client. + * Call this after the client was set on a scope. + */ + init?(): void; + /** Creates an {@link Event} from all inputs to `captureException` and non-primitive inputs to `captureMessage`. */ // eslint-disable-next-line @typescript-eslint/no-explicit-any eventFromException(exception: any, hint?: EventHint): PromiseLike;