diff --git a/dev-packages/browser-integration-tests/suites/public-api/instrumentation/eventListener/event-target/subject.js b/dev-packages/browser-integration-tests/suites/public-api/instrumentation/eventListener/event-target/subject.js new file mode 100644 index 000000000000..1f28a4f125e0 --- /dev/null +++ b/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(); diff --git a/dev-packages/browser-integration-tests/suites/public-api/instrumentation/eventListener/event-target/test.ts b/dev-packages/browser-integration-tests/suites/public-api/instrumentation/eventListener/event-target/test.ts new file mode 100644 index 000000000000..7900c774a914 --- /dev/null +++ b/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(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), + }, + }); +}); diff --git a/packages/browser/src/helpers.ts b/packages/browser/src/helpers.ts index 8871b1ba2c68..0cfaa2d710b7 100644 --- a/packages/browser/src/helpers.ts +++ b/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( + fn: T, + options?: { + mechanism?: Mechanism; + }, +): WrappedFunction; +export function wrap( + fn: NonFunction, + options?: { + mechanism?: Mechanism; + }, +): NonFunction; /** * Instruments the given function and sends an event to Sentry every time the * function throws an exception. @@ -40,14 +55,12 @@ export function ignoreNextOnError(): void { * @returns The wrapped function. * @hidden */ -export function wrap( - fn: WrappedFunction, +export function wrap( + fn: T | NonFunction, options: { mechanism?: Mechanism; } = {}, - before?: WrappedFunction, - // eslint-disable-next-line @typescript-eslint/no-explicit-any -): any { +): NonFunction | WrappedFunction { // for future readers what this does is wrap a function and then create // a bi-directional wrapping between them. // @@ -55,14 +68,18 @@ export function wrap( // 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).__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; - // 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,7 +158,8 @@ 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 { @@ -154,8 +167,10 @@ export function wrap( }, }); } - // 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; } diff --git a/packages/browser/src/integrations/browserapierrors.ts b/packages/browser/src/integrations/browserapierrors.ts index 2290c1a45d66..fffbcff0b09a 100644 --- a/packages/browser/src/integrations/browserapierrors.ts +++ b/packages/browser/src/integrations/browserapierrors.ts @@ -96,8 +96,7 @@ const _browserApiErrorsIntegration = ((options: Partial 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; + 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 + ) => ReturnType { + 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 + ) => ReturnType { + 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'; } diff --git a/packages/browser/test/integrations/helpers.test.ts b/packages/browser/test/helpers.test.ts similarity index 84% rename from packages/browser/test/integrations/helpers.test.ts rename to packages/browser/test/helpers.test.ts index ebfabd475e09..72bd9a392360 100644 --- a/packages/browser/test/integrations/helpers.test.ts +++ b/packages/browser/test/helpers.test.ts @@ -2,7 +2,7 @@ import { describe, expect, it, vi } from 'vitest'; import type { WrappedFunction } from '@sentry/types'; -import { wrap } from '../../src/helpers'; +import { wrap } from '../src/helpers'; describe('internal wrap()', () => { it('should wrap only functions', () => { @@ -13,16 +13,24 @@ describe('internal wrap()', () => { const num = 42; expect(wrap(fn)).not.toBe(fn); - // @ts-expect-error Issue with `WrappedFunction` type from wrap fn expect(wrap(obj)).toBe(obj); - // @ts-expect-error Issue with `WrappedFunction` type from wrap fn expect(wrap(arr)).toBe(arr); - // @ts-expect-error Issue with `WrappedFunction` type from wrap fn expect(wrap(str)).toBe(str); - // @ts-expect-error Issue with `WrappedFunction` type from wrap fn expect(wrap(num)).toBe(num); }); + it('correctly infers types', () => { + const a = wrap(42); + expect(a > 40).toBe(true); + + const b = wrap('42'); + expect(b.length).toBe(2); + + const c = wrap(() => 42); + expect(c()).toBe(42); + expect(c.__sentry_original__).toBeInstanceOf(Function); + }); + it('should preserve correct function name when accessed', () => { const namedFunction = (): number => 1337; expect(wrap(namedFunction)).not.toBe(namedFunction); @@ -56,16 +64,6 @@ describe('internal wrap()', () => { expect(wrap(wrapped)).toBe(wrapped); }); - it('calls "before" function when invoking wrapped function', () => { - const fn = (() => 1337) as WrappedFunction; - const before = vi.fn(); - - const wrapped = wrap(fn, {}, before); - wrapped(); - - expect(before).toHaveBeenCalledTimes(1); - }); - it('attaches metadata to original and wrapped functions', () => { const fn = (() => 1337) as WrappedFunction; @@ -78,10 +76,11 @@ describe('internal wrap()', () => { expect(wrapped.__sentry_original__).toBe(fn); }); - it('copies over original functions properties', () => { - const fn = (() => 1337) as WrappedFunction; - fn.some = 1337; - fn.property = 'Rick'; + it('keeps original functions properties', () => { + const fn = Object.assign(() => 1337, { + some: 1337, + property: 'Rick', + }); const wrapped = wrap(fn); @@ -105,7 +104,7 @@ describe('internal wrap()', () => { }); it('recrusively wraps arguments that are functions', () => { - const fn = (() => 1337) as WrappedFunction; + const fn = (_arg1: unknown, _arg2: unknown) => 1337; const fnArgA = (): number => 1337; const fnArgB = (): number => 1337; @@ -162,7 +161,7 @@ describe('internal wrap()', () => { }); it('internal flags shouldnt be enumerable', () => { - const fn = (() => 1337) as WrappedFunction; + const fn = () => 1337; const wrapped = wrap(fn); // Shouldn't show up in iteration @@ -172,7 +171,7 @@ describe('internal wrap()', () => { expect(Object.keys(wrapped)).toEqual(expect.not.arrayContaining(['__sentry_wrapped__'])); // But should be accessible directly expect(wrapped.__sentry_original__).toBe(fn); - expect(fn.__sentry_wrapped__).toBe(wrapped); + expect((fn as WrappedFunction).__sentry_wrapped__).toBe(wrapped); }); it('should only return __sentry_wrapped__ when it is a function', () => { diff --git a/packages/core/src/utils-hoist/object.ts b/packages/core/src/utils-hoist/object.ts index d3e785f7639d..cb6ca9813232 100644 --- a/packages/core/src/utils-hoist/object.ts +++ b/packages/core/src/utils-hoist/object.ts @@ -81,7 +81,8 @@ export function markFunctionWrapped(wrapped: WrappedFunction, original: WrappedF * @param func the function to unwrap * @returns the unwrapped version of the function if available. */ -export function getOriginalFunction(func: WrappedFunction): WrappedFunction | undefined { +// eslint-disable-next-line @typescript-eslint/ban-types +export function getOriginalFunction(func: WrappedFunction): T | undefined { return func.__sentry_original__; } diff --git a/packages/types/src/wrappedfunction.ts b/packages/types/src/wrappedfunction.ts index 410e5da1cfc1..91960b0d59fb 100644 --- a/packages/types/src/wrappedfunction.ts +++ b/packages/types/src/wrappedfunction.ts @@ -1,6 +1,10 @@ -/** JSDoc */ -export interface WrappedFunction extends Function { +/** + * A function that is possibly wrapped by Sentry. + */ +// eslint-disable-next-line @typescript-eslint/ban-types +export type WrappedFunction = T & { + // TODO(v9): Remove this [key: string]: any; - __sentry_wrapped__?: WrappedFunction; - __sentry_original__?: WrappedFunction; -} + __sentry_wrapped__?: WrappedFunction; + __sentry_original__?: T; +};