feat(core): Add client.init() to replace `client.setupIntegrations(…

…)` (#10118) This adds a new `client.init()` method to be called instead of `client.setupIntegrations()`. Note that this method simply initializes the integrations always, and depends on the check that we don't add integrations multiple times. This also has a bit of a different semantic, dropping the `force` argument in favor of just calling `addIntegration()` again - this depends on #10116 to really work.
getsentry · Jan 11, 2024 · 7ab0fe8 · 7ab0fe8
1 parent 324e5bf
commit 7ab0fe8
Show file tree

Hide file tree

Showing 10 changed files with 134 additions and 27 deletions.
diff --git a/MIGRATION.md b/MIGRATION.md
@@ -64,6 +64,10 @@ If you are using the `Hub` right now, see the following table on how to migrate
 | endSession()           | `Sentry.endSession()`                                                                |
 | shouldSendDefaultPii() | REMOVED - The closest equivalent is `Sentry.getClient().getOptions().sendDefaultPii` |
 
+## 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()`. Setting a span on the scope happens

diff --git 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
@@ -314,12 +314,19 @@ export abstract class BaseClient<O extends ClientOptions> implements Client<O> {
   }
 
   /**
-   * 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<O extends ClientOptions> implements Client<O> {
 
   /* 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
@@ -173,7 +173,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
@@ -681,7 +681,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' });
 
@@ -695,7 +695,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'));
@@ -716,7 +716,7 @@ describe('BaseClient', () => {
         integrations: [new TestIntegration(), null, undefined],
       });
       const client = new TestClient(options);
-      client.setupIntegrations();
+      client.init();
 
       client.captureEvent({ message: 'message' });
 
@@ -1492,24 +1492,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({
@@ -1518,12 +1542,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);
 
@@ -1533,17 +1573,41 @@ 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);
       expect(client.getIntegration(TestIntegration)).toBeTruthy();
       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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -151,9 +151,18 @@ export interface Client<O extends ClientOptions = ClientOptions> {
    * */
   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<Event>;