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

lobsterkatie · web-flow · commit 50c84c6a0b46 · 2022-09-09T07:00:03.000-07:00
While working on adding a `RequestData` integration to the nextjs SDK, I found that the functions we use to set default integrations in the SDK were a little hard to parse, primarily because of a docstring that didn't make any sense and vague and/or slightly inaccurate function and variable names. We also have been making things more complicated than necessary, in that

a) The existing functions in `utils/userIntegrations.ts` use the given default integration instance if no user instance is found. Meanwhile, the two index files do the same in the case that no user integrations are provided at all. But those two conditions aren't logically independent, so by simply providing a default (empty) value for the second case, we can rely on the first check to ensure the default instance is used. (See the now-removed `if (options.integrations)` check in both of the index files.)

b) The options we want to force onto any user instance we find have been living in a nested object which really doesn’t need to be nested. (See the `Options` - now `ForcedIntegrationOptions` - type in `utils/userIntegrations.ts`.)

c) We've essentially been manually implementing `Array.find`, which there's no reason for us to do. (See the `for`-loop in `addIntegrationToArray` - now `addOrUpdateIntegrationInArray` - in `utils/userIntegrations.ts`.)

This fixes those problems, rewrites the docstring, and renames a bunch of variables. It also

- Reorganizes the tests testing the adding of default integrations and refactors them to stop depending on the order or size of the eventual `integrations` array when testing an individual integration type.
- Fixes a small regex bug in `setNestedKey`.
diff --git a/packages/nextjs/src/index.client.ts b/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,
+  });
 }
diff --git a/packages/nextjs/src/index.server.ts b/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';
diff --git a/packages/nextjs/src/utils/userIntegrations.ts b/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;
 }
diff --git a/packages/nextjs/test/index.client.test.ts b/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,
+            }),
+          }),
+        );
+      });
     });
   });
 });
diff --git a/packages/nextjs/test/index.server.test.ts b/packages/nextjs/test/index.server.test.ts
diff --git a/packages/nextjs/test/utils/userIntegrations.test.ts b/packages/nextjs/test/utils/userIntegrations.test.ts
