ref: Inject Transports into Client (#4921)

Changes the way we initialize Transports. Previously, the Transport was initialized within the Client constructor (by calling setupTransport()). Change this so that the Transport is initialized beforehand and then injected into the client. This is currently (i.e. with this PR) happening via two additional arguments in the Client class constructors. The reason for two arguments is that we're still using both, the old `Transport` classes and the `NewTransport` interface. In the future, `NewTransport` will replace `Transport` and (once the old Transport is removed) the transport is then passed into the Client constructor as a property of the options object.

* add the injection logic
* initialize the transports in Browser and Node and pass them to the `Browser/NodeClient` initialization
* add client-external transport setup functions (extracted from setupTransport())
* add basic tests for these transport setup functions
* delete the client-internal setupTransport methods
* fixe a bunch of tests that are initializing clients

Author: Lms24

packages/browser/src/client.ts

@@ -1,12 +1,11 @@
-import { BaseClient, getEnvelopeEndpointWithUrlEncodedAuth, initAPIDetails, Scope, SDK_VERSION } from '@sentry/core';
-import { Event, EventHint, Options, Severity, SeverityLevel, Transport, TransportOptions } from '@sentry/types';
-import { getGlobalObject, logger, stackParserFromOptions, supportsFetch } from '@sentry/utils';
+import { BaseClient, NewTransport, Scope, SDK_VERSION } from '@sentry/core';
+import { Event, EventHint, Options, Severity, SeverityLevel, Transport } from '@sentry/types';
+import { getGlobalObject, logger, stackParserFromOptions } from '@sentry/utils';
 
 import { eventFromException, eventFromMessage } from './eventbuilder';
 import { IS_DEBUG_BUILD } from './flags';
 import { injectReportDialog, ReportDialogOptions } from './helpers';
 import { Breadcrumbs } from './integrations';
-import { FetchTransport, makeNewFetchTransport, makeNewXHRTransport, XHRTransport } from './transports';
 
 /**
  * Configuration options for the Sentry Browser SDK.
@@ -40,7 +39,7 @@ export class BrowserClient extends BaseClient<BrowserOptions> {
    *
    * @param options Configuration options for this SDK.
    */
-  public constructor(options: BrowserOptions = {}) {
+  public constructor(options: BrowserOptions = {}, transport: Transport, newTransport?: NewTransport) {
     options._metadata = options._metadata || {};
     options._metadata.sdk = options._metadata.sdk || {
       name: 'sentry.javascript.browser',
@@ -53,7 +52,7 @@ export class BrowserClient extends BaseClient<BrowserOptions> {
       version: SDK_VERSION,
     };
 
-    super(options);
+    super(options, transport, newTransport);
   }
 
   /**
@@ -122,40 +121,4 @@ export class BrowserClient extends BaseClient<BrowserOptions> {
     }
     super._sendEvent(event);
   }
-
-  /**
-   * @inheritDoc
-   */
-  protected _setupTransport(): Transport {
-    if (!this._options.dsn) {
-      // We return the noop transport here in case there is no Dsn.
-      return super._setupTransport();
-    }
-
-    const transportOptions: TransportOptions = {
-      ...this._options.transportOptions,
-      dsn: this._options.dsn,
-      tunnel: this._options.tunnel,
-      sendClientReports: this._options.sendClientReports,
-      _metadata: this._options._metadata,
-    };
-
-    const api = initAPIDetails(transportOptions.dsn, transportOptions._metadata, transportOptions.tunnel);
-    const url = getEnvelopeEndpointWithUrlEncodedAuth(api.dsn, api.tunnel);
-
-    if (this._options.transport) {
-      return new this._options.transport(transportOptions);
-    }
-    if (supportsFetch()) {
-      const requestOptions: RequestInit = { ...transportOptions.fetchParameters };
-      this._newTransport = makeNewFetchTransport({ requestOptions, url });
-      return new FetchTransport(transportOptions);
-    }
-
-    this._newTransport = makeNewXHRTransport({
-      url,
-      headers: transportOptions.headers,
-    });
-    return new XHRTransport(transportOptions);
-  }
 }

packages/browser/src/sdk.ts

@@ -7,6 +7,7 @@ import { IS_DEBUG_BUILD } from './flags';
 import { ReportDialogOptions, wrap as internalWrap } from './helpers';
 import { Breadcrumbs, Dedupe, GlobalHandlers, LinkedErrors, TryCatch, UserAgent } from './integrations';
 import { defaultStackParsers } from './stack-parsers';
+import { setupBrowserTransport } from './transports/setup';
 
 export const defaultIntegrations = [
   new CoreIntegrations.InboundFilters(),
@@ -97,7 +98,8 @@ export function init(options: BrowserOptions = {}): void {
     options.stackParser = defaultStackParsers;
   }
 
-  initAndBind(BrowserClient, options);
+  const { transport, newTransport } = setupBrowserTransport(options);
+  initAndBind(BrowserClient, options, transport, newTransport);
 
   if (options.autoSessionTracking) {
     startSessionTracking();

packages/browser/src/transports/index.ts

@@ -4,3 +4,5 @@ export { XHRTransport } from './xhr';
 
 export { makeNewFetchTransport } from './new-fetch';
 export { makeNewXHRTransport } from './new-xhr';
+
+export { setupBrowserTransport } from './setup';

packages/browser/src/transports/setup.ts

@@ -0,0 +1,68 @@
+import {
+  BaseTransportOptions,
+  getEnvelopeEndpointWithUrlEncodedAuth,
+  initAPIDetails,
+  NewTransport,
+  NoopTransport,
+} from '@sentry/core';
+import { Transport, TransportOptions } from '@sentry/types';
+import { supportsFetch } from '@sentry/utils';
+
+import { BrowserOptions } from '../client';
+import { FetchTransport } from './fetch';
+import { makeNewFetchTransport } from './new-fetch';
+import { makeNewXHRTransport } from './new-xhr';
+import { XHRTransport } from './xhr';
+
+export interface BrowserTransportOptions extends BaseTransportOptions {
+  // options to pass into fetch request
+  fetchParams: Record<string, string>;
+  headers?: Record<string, string>;
+  sendClientReports?: boolean;
+}
+
+/**
+ * Sets up Browser transports based on the passed `options`. If available, the returned
+ * transport will use the fetch API. In case fetch is not supported, an XMLHttpRequest
+ * based transport is created.
+ *
+ * @returns an object currently still containing both, the old `Transport` and
+ * `NewTransport` which will eventually replace `Transport`. Once this is replaced,
+ * this function will return a ready to use `NewTransport`.
+ */
+// TODO(v7): Adjust return value when NewTransport is the default
+export function setupBrowserTransport(options: BrowserOptions): { transport: Transport; newTransport?: NewTransport } {
+  if (!options.dsn) {
+    // We return the noop transport here in case there is no Dsn.
+    return { transport: new NoopTransport() };
+  }
+
+  const transportOptions: TransportOptions = {
+    ...options.transportOptions,
+    dsn: options.dsn,
+    tunnel: options.tunnel,
+    sendClientReports: options.sendClientReports,
+    _metadata: options._metadata,
+  };
+
+  const api = initAPIDetails(transportOptions.dsn, transportOptions._metadata, transportOptions.tunnel);
+  const url = getEnvelopeEndpointWithUrlEncodedAuth(api.dsn, api.tunnel);
+
+  if (options.transport) {
+    return { transport: new options.transport(transportOptions) };
+  }
+
+  if (supportsFetch()) {
+    const requestOptions: RequestInit = { ...transportOptions.fetchParameters };
+    const newTransport = makeNewFetchTransport({ requestOptions, url });
+    const fetchTransport = new FetchTransport(transportOptions);
+    return { transport: fetchTransport, newTransport };
+  }
+
+  const newTransport = makeNewXHRTransport({
+    url,
+    headers: transportOptions.headers,
+  });
+  const transport = new XHRTransport(transportOptions);
+  return { transport, newTransport };
+}

packages/browser/test/unit/index.test.ts

@@ -75,7 +75,7 @@ describe('SentryBrowser', () => {
   describe('showReportDialog', () => {
     describe('user', () => {
       const EX_USER = { email: '[email protected]' };
-      const client = new BrowserClient({ dsn });
+      const client = new BrowserClient({ dsn }, new SimpleTransport({ dsn }));
       const reportDialogSpy = jest.spyOn(client, 'showReportDialog');
 
       beforeEach(() => {
@@ -140,42 +140,51 @@ describe('SentryBrowser', () => {
 
     it('should capture a message', done => {
       getCurrentHub().bindClient(
-        new BrowserClient({
-          beforeSend: (event: Event): Event | null => {
-            expect(event.message).toBe('test');
-            expect(event.exception).toBeUndefined();
-            done();
-            return event;
+        new BrowserClient(
+          {
+            beforeSend: (event: Event): Event | null => {
+              expect(event.message).toBe('test');
+              expect(event.exception).toBeUndefined();
+              done();
+              return event;
+            },
+            dsn,
           },
-          dsn,
-        }),
+          new SimpleTransport({ dsn }),
+        ),
       );
       captureMessage('test');
     });
 
     it('should capture an event', done => {
       getCurrentHub().bindClient(
-        new BrowserClient({
-          beforeSend: (event: Event): Event | null => {
-            expect(event.message).toBe('event');
-            expect(event.exception).toBeUndefined();
-            done();
-            return event;
+        new BrowserClient(
+          {
+            beforeSend: (event: Event): Event | null => {
+              expect(event.message).toBe('event');
+              expect(event.exception).toBeUndefined();
+              done();
+              return event;
+            },
+            dsn,
           },
-          dsn,
-        }),
+          new SimpleTransport({ dsn }),
+        ),
       );
       captureEvent({ message: 'event' });
     });
 
     it('should not dedupe an event on bound client', async () => {
       const localBeforeSend = jest.fn();
       getCurrentHub().bindClient(
-        new BrowserClient({
-          beforeSend: localBeforeSend,
-          dsn,
-          integrations: [],
-        }),
+        new BrowserClient(
+          {
+            beforeSend: localBeforeSend,
+            dsn,
+            integrations: [],
+          },
+          new SimpleTransport({ dsn }),
+        ),
       );
 
       captureMessage('event222');
@@ -189,11 +198,14 @@ describe('SentryBrowser', () => {
     it('should use inboundfilter rules of bound client', async () => {
       const localBeforeSend = jest.fn();
       getCurrentHub().bindClient(
-        new BrowserClient({
-          beforeSend: localBeforeSend,
-          dsn,
-          integrations: [new Integrations.InboundFilters({ ignoreErrors: ['capture'] })],
-        }),
+        new BrowserClient(
+          {
+            beforeSend: localBeforeSend,
+            dsn,
+            integrations: [new Integrations.InboundFilters({ ignoreErrors: ['capture'] })],
+          },
+          new SimpleTransport({ dsn }),
+        ),
       );
 
       captureMessage('capture');
@@ -248,16 +260,16 @@ describe('SentryBrowser initialization', () => {
 
       const sdkData = (getCurrentHub().getClient() as any).getTransport()._api.metadata?.sdk;
 
-      expect(sdkData.name).toBe('sentry.javascript.browser');
-      expect(sdkData.packages[0].name).toBe('npm:@sentry/browser');
-      expect(sdkData.packages[0].version).toBe(SDK_VERSION);
-      expect(sdkData.version).toBe(SDK_VERSION);
+      expect(sdkData?.name).toBe('sentry.javascript.browser');
+      expect(sdkData?.packages[0].name).toBe('npm:@sentry/browser');
+      expect(sdkData?.packages[0].version).toBe(SDK_VERSION);
+      expect(sdkData?.version).toBe(SDK_VERSION);
     });
 
     it('should set SDK data when instantiating a client directly', () => {
-      const client = new BrowserClient({ dsn });
+      const client = new BrowserClient({ dsn }, new SimpleTransport({ dsn }));
 
-      const sdkData = (client as any).getTransport()._api.metadata?.sdk;
+      const sdkData = (client.getTransport() as any)._api.metadata?.sdk;
 
       expect(sdkData.name).toBe('sentry.javascript.browser');
       expect(sdkData.packages[0].name).toBe('npm:@sentry/browser');
@@ -298,15 +310,18 @@ describe('SentryBrowser initialization', () => {
 describe('wrap()', () => {
   it('should wrap and call function while capturing error', done => {
     getCurrentHub().bindClient(
-      new BrowserClient({
-        beforeSend: (event: Event): Event | null => {
-          expect(event.exception!.values![0].type).toBe('TypeError');
-          expect(event.exception!.values![0].value).toBe('mkey');
-          done();
-          return null;
+      new BrowserClient(
+        {
+          beforeSend: (event: Event): Event | null => {
+            expect(event.exception!.values![0].type).toBe('TypeError');
+            expect(event.exception!.values![0].value).toBe('mkey');
+            done();
+            return null;
+          },
+          dsn,
         },
-        dsn,
-      }),
+        new SimpleTransport({ dsn }),
+      ),
     );
 
     try {

packages/browser/test/unit/integrations/linkederrors.test.ts

@@ -4,6 +4,7 @@ import { createStackParser } from '@sentry/utils';
 import { BrowserClient } from '../../../src/client';
 import * as LinkedErrorsModule from '../../../src/integrations/linkederrors';
 import { defaultStackParsers } from '../../../src/stack-parsers';
+import { setupBrowserTransport } from '../../../src/transports';
 
 const parser = createStackParser(...defaultStackParsers);
 
@@ -38,7 +39,8 @@ describe('LinkedErrors', () => {
       one.cause = two;
 
       const originalException = one;
-      const client = new BrowserClient({ stackParser: parser });
+      const options = { stackParser: parser };
+      const client = new BrowserClient(options, setupBrowserTransport(options).transport);
       return client.eventFromException(originalException).then(event => {
         const result = LinkedErrorsModule._handler(parser, 'cause', 5, event, {
           originalException,
@@ -68,7 +70,8 @@ describe('LinkedErrors', () => {
       one.reason = two;
 
       const originalException = one;
-      const client = new BrowserClient({ stackParser: parser });
+      const options = { stackParser: parser };
+      const client = new BrowserClient(options, setupBrowserTransport(options).transport);
       return client.eventFromException(originalException).then(event => {
         const result = LinkedErrorsModule._handler(parser, 'reason', 5, event, {
           originalException,
@@ -94,7 +97,8 @@ describe('LinkedErrors', () => {
       one.cause = two;
       two.cause = three;
 
-      const client = new BrowserClient({ stackParser: parser });
+      const options = { stackParser: parser };
+      const client = new BrowserClient(options, setupBrowserTransport(options).transport);
       const originalException = one;
       return client.eventFromException(originalException).then(event => {
         const result = LinkedErrorsModule._handler(parser, 'cause', 2, event, {