From 78ace86045192ab08ed562a67a9c29a16abafeab Mon Sep 17 00:00:00 2001 From: Luca Forstner Date: Tue, 28 Nov 2023 09:16:59 +0000 Subject: [PATCH 1/4] fix: Don't depend on browser types in `types` --- packages/replay/src/coreHandlers/handleDom.ts | 4 ++-- packages/replay/src/coreHandlers/util/domUtils.ts | 2 +- packages/types/src/instrument.ts | 6 ++++-- packages/utils/src/instrument/dom.ts | 2 ++ 4 files changed, 9 insertions(+), 5 deletions(-) diff --git a/packages/replay/src/coreHandlers/handleDom.ts b/packages/replay/src/coreHandlers/handleDom.ts index 53a92575b027..a5c20810481b 100644 --- a/packages/replay/src/coreHandlers/handleDom.ts +++ b/packages/replay/src/coreHandlers/handleDom.ts @@ -41,7 +41,7 @@ export const handleDomListener: (replay: ReplayContainer) => (handlerData: Handl handleClick( replay.clickDetector, result as Breadcrumb & { timestamp: number; data: { nodeId: number } }, - getClickTargetNode(handlerData.event) as HTMLElement, + getClickTargetNode(handlerData.event as Event) as HTMLElement, ); } @@ -97,7 +97,7 @@ function getDomTarget(handlerData: HandlerDataDom): { target: Node | null; messa // Accessing event.target can throw (see getsentry/raven-js#838, #768) try { - target = isClick ? getClickTargetNode(handlerData.event) : getTargetNode(handlerData.event); + target = isClick ? getClickTargetNode(handlerData.event as Event) : getTargetNode(handlerData.event as Event); message = htmlTreeAsString(target, { maxStringLength: 200 }) || ''; } catch (e) { message = ''; diff --git a/packages/replay/src/coreHandlers/util/domUtils.ts b/packages/replay/src/coreHandlers/util/domUtils.ts index 83f34dc19f31..2a39d83749c1 100644 --- a/packages/replay/src/coreHandlers/util/domUtils.ts +++ b/packages/replay/src/coreHandlers/util/domUtils.ts @@ -15,7 +15,7 @@ export function getClosestInteractive(element: Element): Element { * This is useful because if you click on the image in , * The target will be the image, not the button, which we don't want here */ -export function getClickTargetNode(event: HandlerDataDom['event'] | MouseEvent | Node): Node | INode | null { +export function getClickTargetNode(event: Event | MouseEvent | Node): Node | INode | null { const target = getTargetNode(event); if (!target || !(target instanceof Element)) { diff --git a/packages/types/src/instrument.ts b/packages/types/src/instrument.ts index 16ec5a3b75b7..41c5a86583f1 100644 --- a/packages/types/src/instrument.ts +++ b/packages/types/src/instrument.ts @@ -63,7 +63,8 @@ export interface HandlerDataFetch { } export interface HandlerDataDom { - event: Event | { target: EventTarget }; + // TODO: Replace `object` here with an vendored type for browser Events. We can't depend on the `DOM` or `react` TS types package here. + event: object | { target: object }; name: string; global?: boolean; } @@ -82,7 +83,8 @@ export interface HandlerDataError { column?: number; error?: Error; line?: number; - msg: string | Event; + // TODO: Replace `object` here with an vendored type for browser Events. We can't depend on the `DOM` or `react` TS types package here. + msg: string | object; url?: string; } diff --git a/packages/utils/src/instrument/dom.ts b/packages/utils/src/instrument/dom.ts index 71f64e43ebc5..aab64c8c149b 100644 --- a/packages/utils/src/instrument/dom.ts +++ b/packages/utils/src/instrument/dom.ts @@ -1,3 +1,5 @@ +// TODO(v8): Move everything in this file into the browser package. Nothing here is generic and we run risk of leaking browser types into non-browser packages. + /* eslint-disable @typescript-eslint/no-explicit-any */ /* eslint-disable @typescript-eslint/ban-types */ import type { HandlerDataDom } from '@sentry/types'; From a91d21e216f7e5d356d993d1e2aec28a4a942c68 Mon Sep 17 00:00:00 2001 From: Luca Forstner Date: Tue, 28 Nov 2023 09:23:26 +0000 Subject: [PATCH 2/4] Add more todos to dangerous code --- packages/utils/src/instrument/history.ts | 2 ++ packages/utils/src/instrument/index.ts | 2 ++ packages/utils/src/instrument/xhr.ts | 2 ++ 3 files changed, 6 insertions(+) diff --git a/packages/utils/src/instrument/history.ts b/packages/utils/src/instrument/history.ts index b13eabf99e3b..dc144c0e5818 100644 --- a/packages/utils/src/instrument/history.ts +++ b/packages/utils/src/instrument/history.ts @@ -1,3 +1,5 @@ +// TODO(v8): Move everything in this file into the browser package. Nothing here is generic and we run risk of leaking browser types into non-browser packages. + /* eslint-disable @typescript-eslint/no-explicit-any */ /* eslint-disable @typescript-eslint/ban-types */ import type { HandlerDataHistory } from '@sentry/types'; diff --git a/packages/utils/src/instrument/index.ts b/packages/utils/src/instrument/index.ts index e74f6788f6e2..2e2255496dd2 100644 --- a/packages/utils/src/instrument/index.ts +++ b/packages/utils/src/instrument/index.ts @@ -1,3 +1,5 @@ +// TODO(v8): Consider moving this file (or at least parts of it) into the browser package. The registered handlers are mostly non-generic and we risk leaking runtime specific code into generic packages. + import { DEBUG_BUILD } from '../debug-build'; import type { InstrumentHandlerCallback as _InstrumentHandlerCallback, diff --git a/packages/utils/src/instrument/xhr.ts b/packages/utils/src/instrument/xhr.ts index d0245dcdd2ee..eb3ec8e158d5 100644 --- a/packages/utils/src/instrument/xhr.ts +++ b/packages/utils/src/instrument/xhr.ts @@ -1,3 +1,5 @@ +// TODO(v8): Move everything in this file into the browser package. Nothing here is generic and we run risk of leaking browser types into non-browser packages. + /* eslint-disable @typescript-eslint/no-explicit-any */ /* eslint-disable @typescript-eslint/ban-types */ import type { HandlerDataXhr, SentryWrappedXMLHttpRequest, WrappedFunction } from '@sentry/types'; From 44548fcd722a9c0f83b572f313de8c01fd8438e4 Mon Sep 17 00:00:00 2001 From: Luca Forstner Date: Tue, 28 Nov 2023 09:25:58 +0000 Subject: [PATCH 3/4] typo --- packages/types/src/instrument.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/types/src/instrument.ts b/packages/types/src/instrument.ts index 41c5a86583f1..5c42c8cebf27 100644 --- a/packages/types/src/instrument.ts +++ b/packages/types/src/instrument.ts @@ -63,7 +63,7 @@ export interface HandlerDataFetch { } export interface HandlerDataDom { - // TODO: Replace `object` here with an vendored type for browser Events. We can't depend on the `DOM` or `react` TS types package here. + // TODO: Replace `object` here with a vendored type for browser Events. We can't depend on the `DOM` or `react` TS types package here. event: object | { target: object }; name: string; global?: boolean; @@ -83,7 +83,7 @@ export interface HandlerDataError { column?: number; error?: Error; line?: number; - // TODO: Replace `object` here with an vendored type for browser Events. We can't depend on the `DOM` or `react` TS types package here. + // TODO: Replace `object` here with a vendored type for browser Events. We can't depend on the `DOM` or `react` TS types package here. msg: string | object; url?: string; } From d08590be8ab01d134a74eff2dee1556777046988 Mon Sep 17 00:00:00 2001 From: Luca Forstner Date: Tue, 28 Nov 2023 09:40:05 +0000 Subject: [PATCH 4/4] lint --- packages/replay/src/coreHandlers/util/domUtils.ts | 1 - 1 file changed, 1 deletion(-) diff --git a/packages/replay/src/coreHandlers/util/domUtils.ts b/packages/replay/src/coreHandlers/util/domUtils.ts index 2a39d83749c1..889ccd0c6052 100644 --- a/packages/replay/src/coreHandlers/util/domUtils.ts +++ b/packages/replay/src/coreHandlers/util/domUtils.ts @@ -1,5 +1,4 @@ import type { INode } from '@sentry-internal/rrweb-snapshot'; -import type { HandlerDataDom } from '@sentry/types'; const INTERACTIVE_SELECTOR = 'button,a';