Integrations setup rework and tests

Author: kamilogorek

packages/core/src/baseclient.ts

@@ -145,8 +145,6 @@ export abstract class BaseClient<B extends Backend, O extends Options> implement
     if (options.dsn) {
       this.dsn = new Dsn(options.dsn);
     }
-
-    this.integrations = setupIntegrations(options);
   }
 
   /**
@@ -157,6 +155,8 @@ export abstract class BaseClient<B extends Backend, O extends Options> implement
       return (this.installed = false);
     }
 
+    this.integrations = setupIntegrations(this.options);
+
     const backend = this.getBackend();
     if (!this.installed && backend.install) {
       backend.install();
@@ -427,6 +427,14 @@ export abstract class BaseClient<B extends Backend, O extends Options> implement
    * @inheritDoc
    */
   public getIntegration(name: string): Integration | null {
-    return (this.integrations && this.integrations[name]) || null;
+    const integration = this.integrations && this.integrations[name];
+
+    if (integration) {
+      return integration;
+    } else {
+      // logger.error(`No ${name} integration available on the current client`);
+      // debugger;
+      return null;
+    }
   }
 }

packages/core/src/integrations/index.ts

@@ -10,12 +10,74 @@ export { InboundFilters } from './inboundfilters';
 export { Debug } from './pluggable/debug';
 export { RewriteFrames } from './pluggable/rewriteframes';
 
-const installedIntegrations: string[] = [];
+export const installedIntegrations: string[] = [];
 
+/** Map of integrations assigned to a client */
 export interface IntegrationIndex {
   [key: string]: Integration;
 }
 
+/** Gets integration to install */
+export function getIntegrationsToSetup(options: Options): Integration[] {
+  const defaultIntegrations = (options.defaultIntegrations && [...options.defaultIntegrations]) || [];
+  const userIntegrations = options.integrations;
+  let integrations: Integration[] = [];
+
+  if (Array.isArray(userIntegrations)) {
+    const userIntegrationsNames = userIntegrations.map(i => i.name);
+    const pickedIntegrationsNames = [];
+
+    // Leave only unique default integrations, that were not overridden with provided user integrations
+    for (const defaultIntegration of defaultIntegrations) {
+      if (
+        userIntegrationsNames.indexOf(defaultIntegration.name) === -1 &&
+        pickedIntegrationsNames.indexOf(defaultIntegration.name) === -1
+      ) {
+        integrations.push(defaultIntegration);
+        pickedIntegrationsNames.push(defaultIntegration.name);
+      }
+    }
+
+    // Don't add same user integration twice
+    for (const userIntegration of userIntegrations) {
+      if (pickedIntegrationsNames.indexOf(userIntegration.name) === -1) {
+        integrations.push(userIntegration);
+        pickedIntegrationsNames.push(userIntegration.name);
+      }
+    }
+  } else if (typeof userIntegrations === 'function') {
+    integrations = userIntegrations(defaultIntegrations);
+    integrations = Array.isArray(integrations) ? integrations : [integrations];
+  } else {
+    return [...defaultIntegrations];
+  }
+
+  return integrations;
+}
+
+/** Setup given integration */
+export function setupIntegration(integration: Integration, options: Options): void {
+  if (installedIntegrations.indexOf(integration.name) !== -1) {
+    return;
+  }
+
+  try {
+    if (integration.setupOnce) {
+      integration.setupOnce(options);
+    }
+  } catch (_Oo) {
+    // TODO: Remove in v5
+    logger.warn(`Integration ${integration.name}: The install method is deprecated. Use "setupOnce".`);
+
+    if (integration.install) {
+      integration.install(options);
+    }
+  }
+
+  installedIntegrations.push(integration.name);
+  logger.log(`Integration installed: ${integration.name}`);
+}
+
 /**
  * Given a list of integration instances this installs them all. When `withDefaults` is set to `true` then all default
  * integrations are added unless they were already provided before.
@@ -24,40 +86,9 @@ export interface IntegrationIndex {
  */
 export function setupIntegrations<O extends Options>(options: O): IntegrationIndex {
   const integrations: IntegrationIndex = {};
-  let integrationsToInstall = (options.defaultIntegrations && [...options.defaultIntegrations]) || [];
-  if (Array.isArray(options.integrations)) {
-    const providedIntegrationsNames = options.integrations.map(i => i.name);
-    integrationsToInstall = [
-      // Leave only unique integrations, that were not overridden with provided integrations with the same name
-      ...integrationsToInstall.filter(integration => providedIntegrationsNames.indexOf(integration.name) === -1),
-      ...options.integrations,
-    ];
-  } else if (typeof options.integrations === 'function') {
-    integrationsToInstall = options.integrations(integrationsToInstall);
-  }
-
-  // Just in case someone will return non-array from a `itegrations` callback
-  if (Array.isArray(integrationsToInstall)) {
-    integrationsToInstall.forEach(integration => {
-      integrations[name] = integration;
-      if (installedIntegrations.indexOf(integration.name) !== -1) {
-        return;
-      }
-      try {
-        if (integration.setupOnce) {
-          // TODO remove
-          integration.setupOnce(options);
-        }
-      } catch (_Oo) {
-        logger.warn(`Integration ${integration.name}: The install method is deprecated. Use "setupOnce".`);
-        if (integration.install) {
-          // TODO remove if
-          integration.install(options);
-        }
-      }
-      installedIntegrations.push(integration.name);
-      logger.log(`Integration installed: ${integration.name}`);
-    });
-  }
+  getIntegrationsToSetup(options).forEach(integration => {
+    integrations[integration.name] = integration;
+    setupIntegration(integration, options);
+  });
   return integrations;
 }

packages/core/src/sdk.ts

@@ -25,7 +25,6 @@ export function initAndBind<F extends Client, O extends Options>(clientClass: Cl
   }
 
   const client = new clientClass(options);
-  client.install();
-
   getCurrentHub().bindClient(client);
+  client.install();
 }

packages/core/test/lib/base.test.ts

@@ -3,6 +3,7 @@ import { Breadcrumb, Status } from '@sentry/types';
 import { SentryError } from '../../src/error';
 import { TestBackend } from '../mocks/backend';
 import { TestClient } from '../mocks/client';
+import { TestIntegration } from '../mocks/integration';
 
 const PUBLIC_DSN = 'https://username@domain/path';
 
@@ -405,4 +406,16 @@ describe('BaseClient', () => {
       // TODO: Test rate limiting queues here
     });
   });
+
+  describe('integrations', () => {
+    test('setup each one of them on install', () => {
+      const client = new TestClient({
+        dsn: PUBLIC_DSN,
+        integrations: [new TestIntegration()],
+      });
+      expect(client.getIntegration('TestIntegration')).toBeFalsy();
+      client.install();
+      expect(client.getIntegration('TestIntegration')).toBeTruthy();
+    });
+  });
 });

packages/core/test/lib/integrations/inboundfilters.test.ts

@@ -65,28 +65,28 @@ describe('InboundFilters', () => {
     };
 
     it('string filter with partial match', () => {
-      inboundFilters.install({
+      inboundFilters.setupOnce({
         ignoreErrors: ['capture'],
       });
       expect(inboundFilters.isIgnoredError(messageEvent)).toBe(true);
     });
 
     it('string filter with exact match', () => {
-      inboundFilters.install({
+      inboundFilters.setupOnce({
         ignoreErrors: ['captureMessage'],
       });
       expect(inboundFilters.isIgnoredError(messageEvent)).toBe(true);
     });
 
     it('regexp filter with partial match', () => {
-      inboundFilters.install({
+      inboundFilters.setupOnce({
         ignoreErrors: [/capture/],
       });
       expect(inboundFilters.isIgnoredError(messageEvent)).toBe(true);
     });
 
     it('regexp filter with exact match', () => {
-      inboundFilters.install({
+      inboundFilters.setupOnce({
         ignoreErrors: [/^captureMessage$/],
       });
       expect(inboundFilters.isIgnoredError(messageEvent)).toBe(true);

packages/core/test/lib/integrations/index.test.ts

@@ -0,0 +1,120 @@
+import { Integration } from '@sentry/types';
+import { getIntegrationsToSetup } from '../../../src/integrations';
+
+/** JSDoc */
+class MockIntegration implements Integration {
+  public constructor(name: string) {
+    this.name = name;
+  }
+  public name: string;
+}
+
+describe('getIntegrationsToSetup', () => {
+  it('works with empty array', () => {
+    const integrations = getIntegrationsToSetup({
+      integrations: [],
+    });
+
+    expect(integrations.map(i => i.name)).toEqual([]);
+  });
+
+  it('works with single item', () => {
+    const integrations = getIntegrationsToSetup({
+      integrations: [new MockIntegration('foo')],
+    });
+
+    expect(integrations.map(i => i.name)).toEqual(['foo']);
+  });
+
+  it('works with multiple items', () => {
+    const integrations = getIntegrationsToSetup({
+      integrations: [new MockIntegration('foo'), new MockIntegration('bar')],
+    });
+
+    expect(integrations.map(i => i.name)).toEqual(['foo', 'bar']);
+  });
+
+  it('filter duplicated items', () => {
+    const integrations = getIntegrationsToSetup({
+      integrations: [new MockIntegration('foo'), new MockIntegration('foo'), new MockIntegration('bar')],
+    });
+
+    expect(integrations.map(i => i.name)).toEqual(['foo', 'bar']);
+  });
+
+  it('filter duplicated items and always let first win', () => {
+    const first = new MockIntegration('foo');
+    (first as any).order = 'first';
+    const second = new MockIntegration('foo');
+    (second as any).order = 'second';
+
+    const integrations = getIntegrationsToSetup({
+      integrations: [first, second, new MockIntegration('bar')],
+    });
+
+    expect(integrations.map(i => i.name)).toEqual(['foo', 'bar']);
+    expect((integrations[0] as any).order).toEqual('first');
+  });
+
+  it('work with empty defaults', () => {
+    const integrations = getIntegrationsToSetup({
+      defaultIntegrations: [],
+    });
+
+    expect(integrations.map(i => i.name)).toEqual([]);
+  });
+
+  it('work with single defaults', () => {
+    const integrations = getIntegrationsToSetup({
+      defaultIntegrations: [new MockIntegration('foo')],
+    });
+
+    expect(integrations.map(i => i.name)).toEqual(['foo']);
+  });
+
+  it('work with multiple defaults', () => {
+    const integrations = getIntegrationsToSetup({
+      defaultIntegrations: [new MockIntegration('foo'), new MockIntegration('bar')],
+    });
+
+    expect(integrations.map(i => i.name)).toEqual(['foo', 'bar']);
+  });
+
+  it('work with user integrations and defaults and pick defaults first', () => {
+    const integrations = getIntegrationsToSetup({
+      defaultIntegrations: [new MockIntegration('foo')],
+      integrations: [new MockIntegration('bar')],
+    });
+
+    expect(integrations.map(i => i.name)).toEqual(['foo', 'bar']);
+  });
+
+  it('work with user integrations and defaults and filter duplicates', () => {
+    const integrations = getIntegrationsToSetup({
+      defaultIntegrations: [new MockIntegration('foo'), new MockIntegration('foo')],
+      integrations: [new MockIntegration('bar'), new MockIntegration('bar')],
+    });
+
+    expect(integrations.map(i => i.name)).toEqual(['foo', 'bar']);
+  });
+
+  it('user integrations override defaults', () => {
+    const firstDefault = new MockIntegration('foo');
+    (firstDefault as any).order = 'firstDefault';
+    const secondDefault = new MockIntegration('bar');
+    (secondDefault as any).order = 'secondDefault';
+    const firstUser = new MockIntegration('foo');
+    (firstUser as any).order = 'firstUser';
+    const secondUser = new MockIntegration('bar');
+    (secondUser as any).order = 'secondUser';
+
+    const integrations = getIntegrationsToSetup({
+      defaultIntegrations: [firstDefault, secondDefault],
+      integrations: [firstUser, secondUser],
+    });
+
+    expect(integrations.map(i => i.name)).toEqual(['foo', 'bar']);
+    expect((integrations[0] as any).order).toEqual('firstUser');
+    expect((integrations[1] as any).order).toEqual('secondUser');
+  });
+});