From be73e38a6880b7f66ac5ae9352ad8d8aa4647094 Mon Sep 17 00:00:00 2001 From: Francesco Novy Date: Thu, 28 Nov 2024 13:59:40 +0100 Subject: [PATCH] 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. --- .../eventListener/event-target/subject.js | 11 ++ .../eventListener/event-target/test.ts | 29 ++++ packages/browser/src/helpers.ts | 67 ++++++---- .../src/integrations/browserapierrors.ts | 125 +++++++----------- .../test/{integrations => }/helpers.test.ts | 43 +++--- packages/core/src/utils-hoist/object.ts | 3 +- packages/types/src/wrappedfunction.ts | 14 +- 7 files changed, 164 insertions(+), 128 deletions(-) create mode 100644 dev-packages/browser-integration-tests/suites/public-api/instrumentation/eventListener/event-target/subject.js create mode 100644 dev-packages/browser-integration-tests/suites/public-api/instrumentation/eventListener/event-target/test.ts rename packages/browser/test/{integrations => }/helpers.test.ts (84%) 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; +};