ref(utils): Change addInstrumentationHandler to take reg args (#4309)

Change addInstrumentationHandler to take reg args

Passing in an object is not necessary as there is always going to be two
arguments required. We can just convert this to explicitly passing the
options, it saves on bundle size.

In addition, we can remove the guards around `type` and `callback`
because addInstrumentationHandler is an internally used function, so
Typescript and our test suite should handle the function getting called
with arguments of the correct type.

Author: AbhiPrasad

packages/browser/src/integrations/breadcrumbs.ts

@@ -85,34 +85,19 @@ export class Breadcrumbs implements Integration {
    */
   public setupOnce(): void {
     if (this._options.console) {
-      addInstrumentationHandler({
-        callback: _consoleBreadcrumb,
-        type: 'console',
-      });
+      addInstrumentationHandler('console', _consoleBreadcrumb);
     }
     if (this._options.dom) {
-      addInstrumentationHandler({
-        callback: _domBreadcrumb(this._options.dom),
-        type: 'dom',
-      });
+      addInstrumentationHandler('dom', _domBreadcrumb(this._options.dom));
     }
     if (this._options.xhr) {
-      addInstrumentationHandler({
-        callback: _xhrBreadcrumb,
-        type: 'xhr',
-      });
+      addInstrumentationHandler('xhr', _xhrBreadcrumb);
     }
     if (this._options.fetch) {
-      addInstrumentationHandler({
-        callback: _fetchBreadcrumb,
-        type: 'fetch',
-      });
+      addInstrumentationHandler('fetch', _fetchBreadcrumb);
     }
     if (this._options.history) {
-      addInstrumentationHandler({
-        callback: _historyBreadcrumb,
-        type: 'history',
-      });
+      addInstrumentationHandler('history', _historyBreadcrumb);
     }
   }
 }

packages/browser/src/integrations/globalhandlers.ts

@@ -74,9 +74,10 @@ export class GlobalHandlers implements Integration {
 
 /** JSDoc */
 function _installGlobalOnErrorHandler(): void {
-  addInstrumentationHandler({
+  addInstrumentationHandler(
+    'error',
     // eslint-disable-next-line @typescript-eslint/no-explicit-any
-    callback: (data: { msg: any; url: any; line: any; column: any; error: any }) => {
+    (data: { msg: any; url: any; line: any; column: any; error: any }) => {
       const [hub, attachStacktrace] = getHubAndAttachStacktrace();
       if (!hub.getIntegration(GlobalHandlers)) {
         return;
@@ -101,15 +102,15 @@ function _installGlobalOnErrorHandler(): void {
 
       addMechanismAndCapture(hub, error, event, 'onerror');
     },
-    type: 'error',
-  });
+  );
 }
 
 /** JSDoc */
 function _installGlobalOnUnhandledRejectionHandler(): void {
-  addInstrumentationHandler({
+  addInstrumentationHandler(
+    'unhandledrejection',
     // eslint-disable-next-line @typescript-eslint/no-explicit-any
-    callback: (e: any) => {
+    (e: any) => {
       const [hub, attachStacktrace] = getHubAndAttachStacktrace();
       if (!hub.getIntegration(GlobalHandlers)) {
         return;
@@ -151,8 +152,7 @@ function _installGlobalOnUnhandledRejectionHandler(): void {
       addMechanismAndCapture(hub, error, event, 'onunhandledrejection');
       return;
     },
-    type: 'unhandledrejection',
-  });
+  );
 }
 
 /**

packages/browser/src/sdk.ts

@@ -226,15 +226,12 @@ function startSessionTracking(): void {
   hub.captureSession();
 
   // We want to create a session for every navigation as well
-  addInstrumentationHandler({
-    callback: ({ from, to }) => {
-      // Don't create an additional session for the initial route or if the location did not change
-      if (from === undefined || from === to) {
-        return;
-      }
-      hub.startSession({ ignoreDuration: true });
-      hub.captureSession();
-    },
-    type: 'history',
+  addInstrumentationHandler('history', ({ from, to }) => {
+    // Don't create an additional session for the initial route or if the location did not change
+    if (from === undefined || from === to) {
+      return;
+    }
+    hub.startSession({ ignoreDuration: true });
+    hub.captureSession();
   });
 }

packages/tracing/src/browser/request.ts

@@ -117,20 +117,14 @@ export function instrumentOutgoingRequests(_options?: Partial<RequestInstrumenta
   const spans: Record<string, Span> = {};
 
   if (traceFetch) {
-    addInstrumentationHandler({
-      callback: (handlerData: FetchData) => {
-        fetchCallback(handlerData, shouldCreateSpan, spans);
-      },
-      type: 'fetch',
+    addInstrumentationHandler('fetch', (handlerData: FetchData) => {
+      fetchCallback(handlerData, shouldCreateSpan, spans);
     });
   }
 
   if (traceXHR) {
-    addInstrumentationHandler({
-      callback: (handlerData: XHRData) => {
-        xhrCallback(handlerData, shouldCreateSpan, spans);
-      },
-      type: 'xhr',
+    addInstrumentationHandler('xhr', (handlerData: XHRData) => {
+      xhrCallback(handlerData, shouldCreateSpan, spans);
     });
   }
 }

packages/tracing/src/browser/router.ts

@@ -24,33 +24,30 @@ export function instrumentRoutingWithDefaults<T extends Transaction>(
   }
 
   if (startTransactionOnLocationChange) {
-    addInstrumentationHandler({
-      callback: ({ to, from }: { to: string; from?: string }) => {
-        /**
-         * This early return is there to account for some cases where a navigation transaction starts right after
-         * long-running pageload. We make sure that if `from` is undefined and a valid `startingURL` exists, we don't
-         * create an uneccessary navigation transaction.
-         *
-         * This was hard to duplicate, but this behavior stopped as soon as this fix was applied. This issue might also
-         * only be caused in certain development environments where the usage of a hot module reloader is causing
-         * errors.
-         */
-        if (from === undefined && startingUrl && startingUrl.indexOf(to) !== -1) {
-          startingUrl = undefined;
-          return;
-        }
+    addInstrumentationHandler('history', ({ to, from }: { to: string; from?: string }) => {
+      /**
+       * This early return is there to account for some cases where a navigation transaction starts right after
+       * long-running pageload. We make sure that if `from` is undefined and a valid `startingURL` exists, we don't
+       * create an uneccessary navigation transaction.
+       *
+       * This was hard to duplicate, but this behavior stopped as soon as this fix was applied. This issue might also
+       * only be caused in certain development environments where the usage of a hot module reloader is causing
+       * errors.
+       */
+      if (from === undefined && startingUrl && startingUrl.indexOf(to) !== -1) {
+        startingUrl = undefined;
+        return;
+      }
 
-        if (from !== to) {
-          startingUrl = undefined;
-          if (activeTransaction) {
-            logger.log(`[Tracing] Finishing current transaction with op: ${activeTransaction.op}`);
-            // If there's an open transaction on the scope, we need to finish it before creating an new one.
-            activeTransaction.finish();
-          }
-          activeTransaction = customStartTransaction({ name: global.location.pathname, op: 'navigation' });
+      if (from !== to) {
+        startingUrl = undefined;
+        if (activeTransaction) {
+          logger.log(`[Tracing] Finishing current transaction with op: ${activeTransaction.op}`);
+          // If there's an open transaction on the scope, we need to finish it before creating an new one.
+          activeTransaction.finish();
         }
-      },
-      type: 'history',
+        activeTransaction = customStartTransaction({ name: global.location.pathname, op: 'navigation' });
+      }
     });
   }
 }

packages/tracing/src/errors.ts

@@ -7,14 +7,8 @@ import { getActiveTransaction } from './utils';
  * Configures global error listeners
  */
 export function registerErrorInstrumentation(): void {
-  addInstrumentationHandler({
-    callback: errorCallback,
-    type: 'error',
-  });
-  addInstrumentationHandler({
-    callback: errorCallback,
-    type: 'unhandledrejection',
-  });
+  addInstrumentationHandler('error', errorCallback);
+  addInstrumentationHandler('unhandledrejection', errorCallback);
 }
 
 /**

packages/tracing/test/browser/browsertracing.test.ts

@@ -23,7 +23,7 @@ jest.mock('@sentry/utils', () => {
   const actual = jest.requireActual('@sentry/utils');
   return {
     ...actual,
-    addInstrumentationHandler: ({ callback, type }: any): void => {
+    addInstrumentationHandler: (type, callback): void => {
       if (type === 'history') {
         // rather than actually add the navigation-change handler, grab a reference to it, so we can trigger it manually
         mockChangeHistory = callback;

packages/tracing/test/browser/request.test.ts

@@ -26,26 +26,20 @@ describe('instrumentOutgoingRequests', () => {
   it('instruments fetch and xhr requests', () => {
     instrumentOutgoingRequests();
 
-    expect(addInstrumentationHandler).toHaveBeenCalledWith({
-      callback: expect.any(Function),
-      type: 'fetch',
-    });
-    expect(addInstrumentationHandler).toHaveBeenCalledWith({
-      callback: expect.any(Function),
-      type: 'xhr',
-    });
+    expect(addInstrumentationHandler).toHaveBeenCalledWith('fetch', expect.any(Function));
+    expect(addInstrumentationHandler).toHaveBeenCalledWith('xhr', expect.any(Function));
   });
 
   it('does not instrument fetch requests if traceFetch is false', () => {
     instrumentOutgoingRequests({ traceFetch: false });
 
-    expect(addInstrumentationHandler).not.toHaveBeenCalledWith({ callback: expect.any(Function), type: 'fetch' });
+    expect(addInstrumentationHandler).not.toHaveBeenCalledWith('fetch', expect.any(Function));
   });
 
   it('does not instrument xhr requests if traceXHR is false', () => {
     instrumentOutgoingRequests({ traceXHR: false });
 
-    expect(addInstrumentationHandler).not.toHaveBeenCalledWith({ callback: expect.any(Function), type: 'xhr' });
+    expect(addInstrumentationHandler).not.toHaveBeenCalledWith('xhr', expect.any(Function));
   });
 });
 

packages/tracing/test/browser/router.test.ts

@@ -8,7 +8,7 @@ jest.mock('@sentry/utils', () => {
   const actual = jest.requireActual('@sentry/utils');
   return {
     ...actual,
-    addInstrumentationHandler: ({ callback, type }: any): void => {
+    addInstrumentationHandler: (type, callback): void => {
       addInstrumentationHandlerType = type;
       mockChangeHistory = callback;
     },

packages/tracing/test/errors.test.ts

@@ -11,15 +11,15 @@ jest.mock('@sentry/utils', () => {
   const actual = jest.requireActual('@sentry/utils');
   return {
     ...actual,
-    addInstrumentationHandler: ({ callback, type }: any) => {
+    addInstrumentationHandler: (type, callback) => {
       if (type === 'error') {
         mockErrorCallback = callback;
       }
       if (type === 'unhandledrejection') {
         mockUnhandledRejectionCallback = callback;
       }
       if (typeof mockAddInstrumentationHandler === 'function') {
-        return mockAddInstrumentationHandler({ callback, type });
+        return mockAddInstrumentationHandler(type, callback);
       }
     },
   };
@@ -44,11 +44,8 @@ describe('registerErrorHandlers()', () => {
   it('registers error instrumentation', () => {
     registerErrorInstrumentation();
     expect(mockAddInstrumentationHandler).toHaveBeenCalledTimes(2);
-    expect(mockAddInstrumentationHandler).toHaveBeenNthCalledWith(1, { callback: expect.any(Function), type: 'error' });
-    expect(mockAddInstrumentationHandler).toHaveBeenNthCalledWith(2, {
-      callback: expect.any(Function),
-      type: 'unhandledrejection',
-    });
+    expect(mockAddInstrumentationHandler).toHaveBeenNthCalledWith(1, 'error', expect.any(Function));
+    expect(mockAddInstrumentationHandler).toHaveBeenNthCalledWith(2, 'unhandledrejection', expect.any(Function));
   });
 
   it('does not set status if transaction is not on scope', () => {

packages/utils/src/instrument.ts

@@ -12,11 +12,6 @@ import { supportsHistory, supportsNativeFetch } from './supports';
 
 const global = getGlobalObject<Window>();
 
-/** Object describing handler that will be triggered for a given `type` of instrumentation */
-interface InstrumentHandler {
-  type: InstrumentHandlerType;
-  callback: InstrumentHandlerCallback;
-}
 type InstrumentHandlerType =
   | 'console'
   | 'dom'
@@ -82,13 +77,10 @@ function instrument(type: InstrumentHandlerType): void {
  * Use at your own risk, this might break without changelog notice, only used internally.
  * @hidden
  */
-export function addInstrumentationHandler(handler: InstrumentHandler): void {
-  if (!handler || typeof handler.type !== 'string' || typeof handler.callback !== 'function') {
-    return;
-  }
-  handlers[handler.type] = handlers[handler.type] || [];
-  (handlers[handler.type] as InstrumentHandlerCallback[]).push(handler.callback);
-  instrument(handler.type);
+export function addInstrumentationHandler(type: InstrumentHandlerType, callback: InstrumentHandlerCallback): void {
+  handlers[type] = handlers[type] || [];
+  (handlers[type] as InstrumentHandlerCallback[]).push(callback);
+  instrument(type);
 }
 
 /** JSDoc */