ref(nextjs): Simplify adding default integrations when initializing SDK

packages/nextjs/src/index.client.ts

@@ -5,7 +5,7 @@ import { EventProcessor } from '@sentry/types';
 import { nextRouterInstrumentation } from './performance/client';
 import { buildMetadata } from './utils/metadata';
 import { NextjsOptions } from './utils/nextjsOptions';
-import { addIntegration, UserIntegrations } from './utils/userIntegrations';
+import { addOrUpdateIntegration, UserIntegrations } from './utils/userIntegrations';
 
 export * from '@sentry/react';
 export { nextRouterInstrumentation } from './performance/client';
@@ -57,17 +57,13 @@ export function init(options: NextjsOptions): void {
   });
 }
 
-function createClientIntegrations(integrations?: UserIntegrations): UserIntegrations {
+function createClientIntegrations(userIntegrations: UserIntegrations = []): UserIntegrations {
   const defaultBrowserTracingIntegration = new BrowserTracing({
     tracingOrigins: [...defaultRequestInstrumentationOptions.tracingOrigins, /^(api\/)/],
     routingInstrumentation: nextRouterInstrumentation,
   });
 
-  if (integrations) {
-    return addIntegration(defaultBrowserTracingIntegration, integrations, {
-      BrowserTracing: { keyPath: 'options.routingInstrumentation', value: nextRouterInstrumentation },
-    });
-  } else {
-    return [defaultBrowserTracingIntegration];
-  }
+  return addOrUpdateIntegration(defaultBrowserTracingIntegration, userIntegrations, {
+    'options.routingInstrumentation': nextRouterInstrumentation,
+  });
 }

packages/nextjs/src/index.server.ts

@@ -10,7 +10,7 @@ import * as path from 'path';
 import { isBuild } from './utils/isBuild';
 import { buildMetadata } from './utils/metadata';
 import { NextjsOptions } from './utils/nextjsOptions';
-import { addIntegration } from './utils/userIntegrations';
+import { addOrUpdateIntegration } from './utils/userIntegrations';
 
 export * from '@sentry/node';
 export { captureUnderscoreErrorException } from './utils/_error';
@@ -93,6 +93,8 @@ function sdkAlreadyInitialized(): boolean {
 }
 
 function addServerIntegrations(options: NextjsOptions): void {
+  let integrations = options.integrations || [];
+
   // This value is injected at build time, based on the output directory specified in the build config. Though a default
   // is set there, we set it here as well, just in case something has gone wrong with the injection.
   const distDirName = (global as GlobalWithDistDir).__rewriteFramesDistDir__ || '.next';
@@ -107,19 +109,16 @@ function addServerIntegrations(options: NextjsOptions): void {
       return frame;
     },
   });
-
-  if (options.integrations) {
-    options.integrations = addIntegration(defaultRewriteFramesIntegration, options.integrations);
-  } else {
-    options.integrations = [defaultRewriteFramesIntegration];
-  }
+  integrations = addOrUpdateIntegration(defaultRewriteFramesIntegration, integrations);
 
   if (hasTracingEnabled(options)) {
     const defaultHttpTracingIntegration = new Integrations.Http({ tracing: true });
-    options.integrations = addIntegration(defaultHttpTracingIntegration, options.integrations, {
-      Http: { keyPath: '_tracing', value: true },
+    integrations = addOrUpdateIntegration(defaultHttpTracingIntegration, integrations, {
+      _tracing: true,
     });
   }
+
+  options.integrations = integrations;
 }
 
 export type { SentryWebpackPluginOptions } from './config/types';

packages/nextjs/src/utils/userIntegrations.ts

@@ -1,15 +1,10 @@
 import { Integration } from '@sentry/types';
 
-export type UserFunctionIntegrations = (integrations: Integration[]) => Integration[];
-export type UserIntegrations = Integration[] | UserFunctionIntegrations;
+export type UserIntegrationsFunction = (integrations: Integration[]) => Integration[];
+export type UserIntegrations = Integration[] | UserIntegrationsFunction;
 
-type Options = {
-  [integrationName: string]:
-    | {
-        keyPath: string;
-        value: unknown;
-      }
-    | undefined;
+type ForcedIntegrationOptions = {
+  [keyPath: string]: unknown;
 };
 
 /**
@@ -24,70 +19,66 @@ type Options = {
 // eslint-disable-next-line @typescript-eslint/no-explicit-any
 function setNestedKey(obj: Record<string, any>, keyPath: string, value: unknown): void {
   // Ex. foo.bar.zoop will extract foo and bar.zoop
-  const match = keyPath.match(/([a-z]+)\.(.*)/i);
+  const match = keyPath.match(/([a-z_]+)\.(.*)/i);
+  // The match will be null when there's no more recursing to do, i.e., when we've reached the right level of the object
   if (match === null) {
     obj[keyPath] = value;
   } else {
-    setNestedKey(obj[match[1]], match[2], value);
+    // `match[1]` is the initial segment of the path, and `match[2]` is the remainder of the path
+    const innerObj = obj[match[1]];
+    setNestedKey(innerObj, match[2], value);
   }
 }
 
 /**
- * Retrieves the patched integrations with the provided integration.
+ * Enforces inclusion of a given integration with specified options in an integration array originally determined by the
+ * user, by either including the given default instance or by patching an existing user instance with the given options.
  *
- * The integration must be present in the final user integrations, and they are compared
- * by integration name. If the user has defined one, there's nothing to patch; if not,
- * the provided integration is added.
+ * Ideally this would happen when integrations are set up, but there isn't currently a mechanism there for merging
+ * options from a default integration instance with those from a user-provided instance of the same integration, only
+ * for allowing the user to override a default instance entirely. (TODO: Fix that.)
  *
- * @param integration The integration to patch, if necessary.
+ * @param defaultIntegrationInstance An instance of the integration with the correct options already set
  * @param userIntegrations Integrations defined by the user.
- * @param options options to update for a particular integration
- * @returns Final integrations, patched if necessary.
+ * @param forcedOptions Options with which to patch an existing user-derived instance on the integration.
+ * @returns A final integrations array.
  */
-export function addIntegration(
-  integration: Integration,
+export function addOrUpdateIntegration(
+  defaultIntegrationInstance: Integration,
   userIntegrations: UserIntegrations,
-  options: Options = {},
+  forcedOptions: ForcedIntegrationOptions = {},
 ): UserIntegrations {
-  if (Array.isArray(userIntegrations)) {
-    return addIntegrationToArray(integration, userIntegrations, options);
-  } else {
-    return addIntegrationToFunction(integration, userIntegrations, options);
-  }
+  return Array.isArray(userIntegrations)
+    ? addOrUpdateIntegrationInArray(defaultIntegrationInstance, userIntegrations, forcedOptions)
+    : addOrUpdateIntegrationInFunction(defaultIntegrationInstance, userIntegrations, forcedOptions);
 }
 
-function addIntegrationToArray(
-  integration: Integration,
+function addOrUpdateIntegrationInArray(
+  defaultIntegrationInstance: Integration,
   userIntegrations: Integration[],
-  options: Options,
+  forcedOptions: ForcedIntegrationOptions,
 ): Integration[] {
-  let includesName = false;
-  // eslint-disable-next-line @typescript-eslint/prefer-for-of
-  for (let x = 0; x < userIntegrations.length; x++) {
-    if (userIntegrations[x].name === integration.name) {
-      includesName = true;
-    }
+  const userInstance = userIntegrations.find(integration => integration.name === defaultIntegrationInstance.name);
 
-    const op = options[userIntegrations[x].name];
-    if (op) {
-      setNestedKey(userIntegrations[x], op.keyPath, op.value);
+  if (userInstance) {
+    for (const [keyPath, value] of Object.entries(forcedOptions)) {
+      setNestedKey(userInstance, keyPath, value);
     }
-  }
 
-  if (includesName) {
     return userIntegrations;
   }
-  return [...userIntegrations, integration];
+
+  return [...userIntegrations, defaultIntegrationInstance];
 }
 
-function addIntegrationToFunction(
-  integration: Integration,
-  userIntegrationsFunc: UserFunctionIntegrations,
-  options: Options,
-): UserFunctionIntegrations {
-  const wrapper: UserFunctionIntegrations = defaultIntegrations => {
+function addOrUpdateIntegrationInFunction(
+  defaultIntegrationInstance: Integration,
+  userIntegrationsFunc: UserIntegrationsFunction,
+  forcedOptions: ForcedIntegrationOptions,
+): UserIntegrationsFunction {
+  const wrapper: UserIntegrationsFunction = defaultIntegrations => {
     const userFinalIntegrations = userIntegrationsFunc(defaultIntegrations);
-    return addIntegrationToArray(integration, userFinalIntegrations, options);
+    return addOrUpdateIntegrationInArray(defaultIntegrationInstance, userFinalIntegrations, forcedOptions);
   };
   return wrapper;
 }

packages/nextjs/test/index.client.test.ts

@@ -7,7 +7,7 @@ import { getGlobalObject, logger } from '@sentry/utils';
 import { JSDOM } from 'jsdom';
 
 import { init, Integrations, nextRouterInstrumentation } from '../src/index.client';
-import { NextjsOptions } from '../src/utils/nextjsOptions';
+import { UserIntegrationsFunction } from '../src/utils/userIntegrations';
 
 const { BrowserTracing } = TracingIntegrations;
 
@@ -32,6 +32,10 @@ afterAll(() => {
   Object.defineProperty(global, 'location', { value: originalGlobalLocation });
 });
 
+function findIntegrationByName(integrations: Integration[] = [], name: string): Integration | undefined {
+  return integrations.find(integration => integration.name === name);
+}
+
 describe('Client init()', () => {
   afterEach(() => {
     jest.clearAllMocks();
@@ -95,84 +99,101 @@ describe('Client init()', () => {
   });
 
   describe('integrations', () => {
-    it('does not add BrowserTracing integration by default if tracesSampleRate is not set', () => {
-      init({});
-
-      const reactInitOptions: NextjsOptions = reactInit.mock.calls[0][0];
-      expect(reactInitOptions.integrations).toBeUndefined();
-    });
+    // Options passed by `@sentry/nextjs`'s `init` to `@sentry/react`'s `init` after modifying them
+    type ModifiedInitOptionsIntegrationArray = { integrations: Integration[] };
+    type ModifiedInitOptionsIntegrationFunction = { integrations: UserIntegrationsFunction };
 
-    it('adds BrowserTracing integration by default if tracesSampleRate is set', () => {
-      init({ tracesSampleRate: 1.0 });
+    it('supports passing unrelated integrations through options', () => {
+      init({ integrations: [new Integrations.Breadcrumbs({ console: false })] });
 
-      const reactInitOptions: NextjsOptions = reactInit.mock.calls[0][0];
-      expect(reactInitOptions.integrations).toHaveLength(1);
+      const reactInitOptions = reactInit.mock.calls[0][0] as ModifiedInitOptionsIntegrationArray;
+      const breadcrumbsIntegration = findIntegrationByName(reactInitOptions.integrations, 'Breadcrumbs');
 
-      const integrations = reactInitOptions.integrations as Integration[];
-      expect(integrations[0]).toEqual(expect.any(BrowserTracing));
-      // eslint-disable-next-line @typescript-eslint/unbound-method
-      expect((integrations[0] as InstanceType<typeof BrowserTracing>).options.routingInstrumentation).toEqual(
-        nextRouterInstrumentation,
-      );
+      expect(breadcrumbsIntegration).toBeDefined();
     });
 
-    it('adds BrowserTracing integration by default if tracesSampler is set', () => {
-      init({ tracesSampler: () => true });
+    describe('`BrowserTracing` integration', () => {
+      it('adds `BrowserTracing` integration if `tracesSampleRate` is set', () => {
+        init({ tracesSampleRate: 1.0 });
+
+        const reactInitOptions = reactInit.mock.calls[0][0] as ModifiedInitOptionsIntegrationArray;
+        const browserTracingIntegration = findIntegrationByName(reactInitOptions.integrations, 'BrowserTracing');
+
+        expect(browserTracingIntegration).toBeDefined();
+        expect(browserTracingIntegration).toEqual(
+          expect.objectContaining({
+            options: expect.objectContaining({
+              routingInstrumentation: nextRouterInstrumentation,
+            }),
+          }),
+        );
+      });
 
-      const reactInitOptions: NextjsOptions = reactInit.mock.calls[0][0];
-      expect(reactInitOptions.integrations).toHaveLength(1);
+      it('adds `BrowserTracing` integration if `tracesSampler` is set', () => {
+        init({ tracesSampler: () => true });
 
-      const integrations = reactInitOptions.integrations as Integration[];
-      expect(integrations[0]).toEqual(expect.any(BrowserTracing));
-      // eslint-disable-next-line @typescript-eslint/unbound-method
-      expect((integrations[0] as InstanceType<typeof BrowserTracing>).options.routingInstrumentation).toEqual(
-        nextRouterInstrumentation,
-      );
-    });
+        const reactInitOptions = reactInit.mock.calls[0][0] as ModifiedInitOptionsIntegrationArray;
+        const browserTracingIntegration = findIntegrationByName(reactInitOptions.integrations, 'BrowserTracing');
 
-    it('supports passing integration through options', () => {
-      init({ tracesSampleRate: 1.0, integrations: [new Integrations.Breadcrumbs({ console: false })] });
-      const reactInitOptions: NextjsOptions = reactInit.mock.calls[0][0];
-      expect(reactInitOptions.integrations).toHaveLength(2);
+        expect(browserTracingIntegration).toBeDefined();
+        expect(browserTracingIntegration).toEqual(
+          expect.objectContaining({
+            options: expect.objectContaining({
+              routingInstrumentation: nextRouterInstrumentation,
+            }),
+          }),
+        );
+      });
 
-      const integrations = reactInitOptions.integrations as Integration[];
-      expect(integrations).toEqual([expect.any(Integrations.Breadcrumbs), expect.any(BrowserTracing)]);
-    });
+      it('does not add `BrowserTracing` integration if tracing not enabled in SDK', () => {
+        init({});
 
-    it('uses custom BrowserTracing with array option with nextRouterInstrumentation', () => {
-      init({
-        tracesSampleRate: 1.0,
-        integrations: [new BrowserTracing({ idleTimeout: 5000, startTransactionOnLocationChange: false })],
-      });
+        const reactInitOptions = reactInit.mock.calls[0][0] as ModifiedInitOptionsIntegrationArray;
+        const browserTracingIntegration = findIntegrationByName(reactInitOptions.integrations, 'BrowserTracing');
 
-      const reactInitOptions: NextjsOptions = reactInit.mock.calls[0][0];
-      expect(reactInitOptions.integrations).toHaveLength(1);
-      const integrations = reactInitOptions.integrations as Integration[];
-      expect((integrations[0] as InstanceType<typeof BrowserTracing>).options).toEqual(
-        expect.objectContaining({
-          idleTimeout: 5000,
-          startTransactionOnLocationChange: false,
-          routingInstrumentation: nextRouterInstrumentation,
-        }),
-      );
-    });
+        expect(browserTracingIntegration).toBeUndefined();
+      });
 
-    it('uses custom BrowserTracing with function option with nextRouterInstrumentation', () => {
-      init({
-        tracesSampleRate: 1.0,
-        integrations: () => [new BrowserTracing({ idleTimeout: 5000, startTransactionOnLocationChange: false })],
+      it('forces correct router instrumentation if user provides `BrowserTracing` in an array', () => {
+        init({
+          tracesSampleRate: 1.0,
+          integrations: [new BrowserTracing({ startTransactionOnLocationChange: false })],
+        });
+
+        const reactInitOptions = reactInit.mock.calls[0][0] as ModifiedInitOptionsIntegrationArray;
+        const browserTracingIntegration = findIntegrationByName(reactInitOptions.integrations, 'BrowserTracing');
+
+        expect(browserTracingIntegration).toEqual(
+          expect.objectContaining({
+            options: expect.objectContaining({
+              routingInstrumentation: nextRouterInstrumentation,
+              // This proves it's still the user's copy
+              startTransactionOnLocationChange: false,
+            }),
+          }),
+        );
       });
 
-      const reactInitOptions: NextjsOptions = reactInit.mock.calls[0][0];
-      const integrationFunc = reactInitOptions.integrations as () => Integration[];
-      const integrations = integrationFunc();
-      expect((integrations[0] as InstanceType<typeof BrowserTracing>).options).toEqual(
-        expect.objectContaining({
-          idleTimeout: 5000,
-          startTransactionOnLocationChange: false,
-          routingInstrumentation: nextRouterInstrumentation,
-        }),
-      );
+      it('forces correct router instrumentation if user provides `BrowserTracing` in a function', () => {
+        init({
+          tracesSampleRate: 1.0,
+          integrations: defaults => [...defaults, new BrowserTracing({ startTransactionOnLocationChange: false })],
+        });
+
+        const reactInitOptions = reactInit.mock.calls[0][0] as ModifiedInitOptionsIntegrationFunction;
+        const materializedIntegrations = reactInitOptions.integrations(SentryReact.defaultIntegrations);
+        const browserTracingIntegration = findIntegrationByName(materializedIntegrations, 'BrowserTracing');
+
+        expect(browserTracingIntegration).toEqual(
+          expect.objectContaining({
+            options: expect.objectContaining({
+              routingInstrumentation: nextRouterInstrumentation,
+              // This proves it's still the user's copy
+              startTransactionOnLocationChange: false,
+            }),
+          }),
+        );
+      });
     });
   });
 });