ref(types): Avoid some any type casting around wrap code (#14463)

We use a heavy dose of `any`, that I would like to reduce.

So I started out to look into `wrap()`, which made heave use of this,
and tried to rewrite it to avoid `any` as much as possible. This
required some changes around it, but should now have much better type
inferrence etc. than before, and be more "realistic" in what it tells
you.

While at this, I also removed the `before` argument that we were not
using anymore - `wrap` is not exported anymore, so this is purely
internal.

Author: mydea

dev-packages/browser-integration-tests/suites/public-api/instrumentation/eventListener/event-target/subject.js

@@ -0,0 +1,11 @@
+const btn = document.createElement('button');
+btn.id = 'btn';
+document.body.appendChild(btn);
+
+const functionListener = function () {
+  throw new Error('event_listener_error');
+};
+
+btn.addEventListener('click', functionListener);
+
+btn.click();

dev-packages/browser-integration-tests/suites/public-api/instrumentation/eventListener/event-target/test.ts

@@ -0,0 +1,29 @@
+import { expect } from '@playwright/test';
+import type { Event } from '@sentry/types';
+
+import { sentryTest } from '../../../../../utils/fixtures';
+import { getFirstSentryEnvelopeRequest } from '../../../../../utils/helpers';
+
+sentryTest('should capture target name in mechanism data', async ({ getLocalTestUrl, page }) => {
+  const url = await getLocalTestUrl({ testDir: __dirname });
+
+  const eventData = await getFirstSentryEnvelopeRequest<Event>(page, url);
+
+  expect(eventData.exception?.values).toHaveLength(1);
+  expect(eventData.exception?.values?.[0]).toMatchObject({
+    type: 'Error',
+    value: 'event_listener_error',
+    mechanism: {
+      type: 'instrument',
+      handled: false,
+      data: {
+        function: 'addEventListener',
+        handler: 'functionListener',
+        target: 'EventTarget',
+      },
+    },
+    stacktrace: {
+      frames: expect.any(Array),
+    },
+  });
+});

packages/browser/src/helpers.ts

@@ -31,6 +31,21 @@ export function ignoreNextOnError(): void {
   });
 }
 
+// eslint-disable-next-line @typescript-eslint/ban-types
+type WrappableFunction = Function;
+
+export function wrap<T extends WrappableFunction>(
+  fn: T,
+  options?: {
+    mechanism?: Mechanism;
+  },
+): WrappedFunction<T>;
+export function wrap<NonFunction>(
+  fn: NonFunction,
+  options?: {
+    mechanism?: Mechanism;
+  },
+): NonFunction;
 /**
  * Instruments the given function and sends an event to Sentry every time the
  * function throws an exception.
@@ -40,29 +55,31 @@ export function ignoreNextOnError(): void {
  * @returns The wrapped function.
  * @hidden
  */
-export function wrap(
-  fn: WrappedFunction,
+export function wrap<T extends WrappableFunction, NonFunction>(
+  fn: T | NonFunction,
   options: {
     mechanism?: Mechanism;
   } = {},
-  before?: WrappedFunction,
-  // eslint-disable-next-line @typescript-eslint/no-explicit-any
-): any {
+): NonFunction | WrappedFunction<T> {
   // for future readers what this does is wrap a function and then create
   // a bi-directional wrapping between them.
   //
   // example: wrapped = wrap(original);
   //  original.__sentry_wrapped__ -> wrapped
   //  wrapped.__sentry_original__ -> original
 
-  if (typeof fn !== 'function') {
+  function isFunction(fn: T | NonFunction): fn is T {
+    return typeof fn === 'function';
+  }
+
+  if (!isFunction(fn)) {
     return fn;
   }
 
   try {
     // if we're dealing with a function that was previously wrapped, return
     // the original wrapper.
-    const wrapper = fn.__sentry_wrapped__;
+    const wrapper = (fn as WrappedFunction<T>).__sentry_wrapped__;
     if (wrapper) {
       if (typeof wrapper === 'function') {
         return wrapper;
@@ -84,18 +101,12 @@ export function wrap(
     return fn;
   }
 
-  /* eslint-disable prefer-rest-params */
+  // Wrap the function itself
   // It is important that `sentryWrapped` is not an arrow function to preserve the context of `this`
-  const sentryWrapped: WrappedFunction = function (this: unknown): void {
-    const args = Array.prototype.slice.call(arguments);
-
+  const sentryWrapped = function (this: unknown, ...args: unknown[]): unknown {
     try {
-      if (before && typeof before === 'function') {
-        before.apply(this, arguments);
-      }
-
-      // eslint-disable-next-line @typescript-eslint/no-explicit-any, @typescript-eslint/no-unsafe-member-access
-      const wrappedArguments = args.map((arg: any) => wrap(arg, options));
+      // Also wrap arguments that are themselves functions
+      const wrappedArguments = args.map(arg => wrap(arg, options));
 
       // Attempt to invoke user-land function
       // NOTE: If you are a Sentry user, and you are seeing this stack frame, it
@@ -125,18 +136,19 @@ export function wrap(
 
       throw ex;
     }
-  };
-  /* eslint-enable prefer-rest-params */
+  } as unknown as WrappedFunction<T>;
 
-  // Accessing some objects may throw
-  // ref: https://github.com/getsentry/sentry-javascript/issues/1168
+  // Wrap the wrapped function in a proxy, to ensure any other properties of the original function remain available
   try {
     for (const property in fn) {
       if (Object.prototype.hasOwnProperty.call(fn, property)) {
-        sentryWrapped[property] = fn[property];
+        sentryWrapped[property as keyof T] = fn[property as keyof T];
       }
     }
-  } catch (_oO) {} // eslint-disable-line no-empty
+  } catch {
+    // Accessing some objects may throw
+    // ref: https://github.com/getsentry/sentry-javascript/issues/1168
+  }
 
   // Signal that this function has been wrapped/filled already
   // for both debugging and to prevent it to being wrapped/filled twice
@@ -146,16 +158,19 @@ export function wrap(
 
   // Restore original function name (not all browsers allow that)
   try {
-    const descriptor = Object.getOwnPropertyDescriptor(sentryWrapped, 'name') as PropertyDescriptor;
+    // eslint-disable-next-line @typescript-eslint/no-non-null-assertion
+    const descriptor = Object.getOwnPropertyDescriptor(sentryWrapped, 'name')!;
     if (descriptor.configurable) {
       Object.defineProperty(sentryWrapped, 'name', {
         get(): string {
           return fn.name;
         },
       });
     }
-    // eslint-disable-next-line no-empty
-  } catch (_oO) {}
+  } catch {
+    // This may throw if e.g. the descriptor does not exist, or a browser does not allow redefining `name`.
+    // to save some bytes we simply try-catch this
+  }
 
   return sentryWrapped;
 }

packages/browser/src/integrations/browserapierrors.ts

@@ -96,8 +96,7 @@ const _browserApiErrorsIntegration = ((options: Partial<BrowserApiErrorsOptions>
 export const browserApiErrorsIntegration = defineIntegration(_browserApiErrorsIntegration);
 
 function _wrapTimeFunction(original: () => void): () => number {
-  // eslint-disable-next-line @typescript-eslint/no-explicit-any
-  return function (this: any, ...args: any[]): number {
+  return function (this: unknown, ...args: unknown[]): number {
     const originalCallback = args[0];
     args[0] = wrap(originalCallback, {
       mechanism: {
@@ -110,11 +109,8 @@ function _wrapTimeFunction(original: () => void): () => number {
   };
 }
 
-// eslint-disable-next-line @typescript-eslint/no-explicit-any
-function _wrapRAF(original: any): (callback: () => void) => any {
-  // eslint-disable-next-line @typescript-eslint/no-explicit-any
-  return function (this: any, callback: () => void): () => void {
-    // eslint-disable-next-line @typescript-eslint/no-unsafe-member-access
+function _wrapRAF(original: () => void): (callback: () => void) => unknown {
+  return function (this: unknown, callback: () => void): () => void {
     return original.apply(this, [
       wrap(callback, {
         mechanism: {
@@ -131,16 +127,14 @@ function _wrapRAF(original: any): (callback: () => void) => any {
 }
 
 function _wrapXHR(originalSend: () => void): () => void {
-  // eslint-disable-next-line @typescript-eslint/no-explicit-any
-  return function (this: XMLHttpRequest, ...args: any[]): void {
+  return function (this: XMLHttpRequest, ...args: unknown[]): void {
     // eslint-disable-next-line @typescript-eslint/no-this-alias
     const xhr = this;
     const xmlHttpRequestProps: XMLHttpRequestProp[] = ['onload', 'onerror', 'onprogress', 'onreadystatechange'];
 
     xmlHttpRequestProps.forEach(prop => {
       if (prop in xhr && typeof xhr[prop] === 'function') {
-        // eslint-disable-next-line @typescript-eslint/no-explicit-any
-        fill(xhr, prop, function (original: WrappedFunction): () => any {
+        fill(xhr, prop, function (original) {
           const wrapOptions = {
             mechanism: {
               data: {
@@ -169,30 +163,21 @@ function _wrapXHR(originalSend: () => void): () => void {
 }
 
 function _wrapEventTarget(target: string): void {
-  // eslint-disable-next-line @typescript-eslint/no-explicit-any
-  const globalObject = WINDOW as { [key: string]: any };
-  // eslint-disable-next-line @typescript-eslint/no-unsafe-member-access
-  const proto = globalObject[target] && globalObject[target].prototype;
+  const globalObject = WINDOW as unknown as Record<string, { prototype?: object }>;
+  const targetObj = globalObject[target];
+  const proto = targetObj && targetObj.prototype;
 
-  // eslint-disable-next-line @typescript-eslint/no-unsafe-member-access, no-prototype-builtins
+  // eslint-disable-next-line no-prototype-builtins
   if (!proto || !proto.hasOwnProperty || !proto.hasOwnProperty('addEventListener')) {
     return;
   }
 
   fill(proto, 'addEventListener', function (original: VoidFunction,): (
-    eventName: string,
-    fn: EventListenerObject,
-    options?: boolean | AddEventListenerOptions,
-  ) => void {
-    return function (
-      // eslint-disable-next-line @typescript-eslint/no-explicit-any
-      this: any,
-      eventName: string,
-      fn: EventListenerObject,
-      options?: boolean | AddEventListenerOptions,
-    ): (eventName: string, fn: EventListenerObject, capture?: boolean, secure?: boolean) => void {
+    ...args: Parameters<typeof WINDOW.addEventListener>
+  ) => ReturnType<typeof WINDOW.addEventListener> {
+    return function (this: unknown, eventName, fn, options): VoidFunction {
       try {
-        if (typeof fn.handleEvent === 'function') {
+        if (isEventListenerObject(fn)) {
           // ESlint disable explanation:
           //  First, it is generally safe to call `wrap` with an unbound function. Furthermore, using `.bind()` would
           //  introduce a bug here, because bind returns a new function that doesn't have our
@@ -211,14 +196,13 @@ function _wrapEventTarget(target: string): void {
             },
           });
         }
-      } catch (err) {
+      } catch {
         // can sometimes get 'Permission denied to access property "handle Event'
       }
 
       return original.apply(this, [
         eventName,
-        // eslint-disable-next-line @typescript-eslint/no-explicit-any
-        wrap(fn as any as WrappedFunction, {
+        wrap(fn, {
           mechanism: {
             data: {
               function: 'addEventListener',
@@ -234,48 +218,41 @@ function _wrapEventTarget(target: string): void {
     };
   });
 
-  fill(
-    proto,
-    'removeEventListener',
-    function (
-      originalRemoveEventListener: () => void,
-      // eslint-disable-next-line @typescript-eslint/no-explicit-any
-    ): (this: any, eventName: string, fn: EventListenerObject, options?: boolean | EventListenerOptions) => () => void {
-      return function (
-        // eslint-disable-next-line @typescript-eslint/no-explicit-any
-        this: any,
-        eventName: string,
-        fn: EventListenerObject,
-        options?: boolean | EventListenerOptions,
-      ): () => void {
-        /**
-         * There are 2 possible scenarios here:
-         *
-         * 1. Someone passes a callback, which was attached prior to Sentry initialization, or by using unmodified
-         * method, eg. `document.addEventListener.call(el, name, handler). In this case, we treat this function
-         * as a pass-through, and call original `removeEventListener` with it.
-         *
-         * 2. Someone passes a callback, which was attached after Sentry was initialized, which means that it was using
-         * our wrapped version of `addEventListener`, which internally calls `wrap` helper.
-         * This helper "wraps" whole callback inside a try/catch statement, and attached appropriate metadata to it,
-         * in order for us to make a distinction between wrapped/non-wrapped functions possible.
-         * If a function was wrapped, it has additional property of `__sentry_wrapped__`, holding the handler.
-         *
-         * When someone adds a handler prior to initialization, and then do it again, but after,
-         * then we have to detach both of them. Otherwise, if we'd detach only wrapped one, it'd be impossible
-         * to get rid of the initial handler and it'd stick there forever.
-         */
-        const wrappedEventHandler = fn as unknown as WrappedFunction;
-        try {
-          const originalEventHandler = wrappedEventHandler && wrappedEventHandler.__sentry_wrapped__;
-          if (originalEventHandler) {
-            originalRemoveEventListener.call(this, eventName, originalEventHandler, options);
-          }
-        } catch (e) {
-          // ignore, accessing __sentry_wrapped__ will throw in some Selenium environments
+  fill(proto, 'removeEventListener', function (originalRemoveEventListener: VoidFunction,): (
+    this: unknown,
+    ...args: Parameters<typeof WINDOW.removeEventListener>
+  ) => ReturnType<typeof WINDOW.removeEventListener> {
+    return function (this: unknown, eventName, fn, options): VoidFunction {
+      /**
+       * There are 2 possible scenarios here:
+       *
+       * 1. Someone passes a callback, which was attached prior to Sentry initialization, or by using unmodified
+       * method, eg. `document.addEventListener.call(el, name, handler). In this case, we treat this function
+       * as a pass-through, and call original `removeEventListener` with it.
+       *
+       * 2. Someone passes a callback, which was attached after Sentry was initialized, which means that it was using
+       * our wrapped version of `addEventListener`, which internally calls `wrap` helper.
+       * This helper "wraps" whole callback inside a try/catch statement, and attached appropriate metadata to it,
+       * in order for us to make a distinction between wrapped/non-wrapped functions possible.
+       * If a function was wrapped, it has additional property of `__sentry_wrapped__`, holding the handler.
+       *
+       * When someone adds a handler prior to initialization, and then do it again, but after,
+       * then we have to detach both of them. Otherwise, if we'd detach only wrapped one, it'd be impossible
+       * to get rid of the initial handler and it'd stick there forever.
+       */
+      try {
+        const originalEventHandler = (fn as WrappedFunction).__sentry_wrapped__;
+        if (originalEventHandler) {
+          originalRemoveEventListener.call(this, eventName, originalEventHandler, options);
         }
-        return originalRemoveEventListener.call(this, eventName, wrappedEventHandler, options);
-      };
-    },
-  );
+      } catch (e) {
+        // ignore, accessing __sentry_wrapped__ will throw in some Selenium environments
+      }
+      return originalRemoveEventListener.call(this, eventName, fn, options);
+    };
+  });
+}
+
+function isEventListenerObject(obj: unknown): obj is EventListenerObject {
+  return typeof (obj as EventListenerObject).handleEvent === 'function';
 }