_instrumentDOM doing the wrong thing

[pauldraper]

https://github.com/getsentry/sentry-javascript/blob/5.6.1/packages/browser/src/integrations/breadcrumbs.ts#L169

This is calling breadCrumbEventHandler curried function with this which is EventTarget/Node instead of Event. And it's calling it when the event listener is added, instead of when an event is fired.

(IDK why this has been typed as any, but this is what happens when types are ignored.)

It's causing terrible rendering performance because it's debouncing with setTimeout/clearTimeout every single time addEventListener('click', ...) is called.

[kamilogorek]

Hello there,

This is calling breadCrumbEventHandler curried function with this which is EventTarget/Node instead of Event.

Which is correct: https://developer.mozilla.org/en-US/docs/Web/API/EventTarget/addEventListener#The_value_of_this_within_the_handler

And it's calling it when the event listener is added, instead of when an event is fired.

It's a higher-order function. It's not called instantly, see line:

sentry-javascript/packages/browser/src/integrations/breadcrumbs.ts

Lines 144 to 149 in 1936ddd

	return function(
	this: any,
	eventName: string,
	fn: EventListenerOrEventListenerObject,
	options?: boolean | AddEventListenerOptions,
	): (eventName: string, fn: EventListenerOrEventListenerObject, capture?: boolean, secure?: boolean) => void {

(IDK why this has been typed as any, but this is what happens when types are ignored.)

Because you can pass bound functions to event-listeners if you want to override value of this.

It's causing terrible rendering performance because it's debouncing with setTimeout/clearTimeout every single time addEventListener('click', ...) is called.

terrible

I'd appreciate some benchmarks around this :)

Thanks!

[pauldraper]

Let me try to be clearer.

 breadcrumbEventHandler('click', true)(this);

is being run when this is EventTarget/Node. Maybe that's the right value of this, but it's not the right value of the the argument. The single argument of the second argument list should be Event. (Were this to not be typed as any, this would become immediately clear.)

Example: https://stackblitz.com/edit/typescript-hfrsmv

User codes calls addEventListener.

Sentry calls breadcrumbEventHandler.

Sentry calls anonymous function which will call setTimeout.

A stack trace of setTimeout invocation, showing user code -> sentry/browser/dist/helpers.js -> sentry/browser/dist/integrations/breadcumb -> setTimeout

---

1. Third screenshot shows that either your compile-time or your runtime types are wrong, because event: Event is a DOM Element.

2. Last screenshot shows that setTimeout gets called when addEventListener for click. I haven't included a benchmark, but calling setTimeout can significantly hurt performance. E.g. 10 x 100 table cells, each with a click handler. (Yes, in the simple case, it could install harness event bubbling to add fewer handlers.) Anyway...I'm 60% sure this is a non-functional and unintentional side effect.

[pauldraper]

Did this make sense?

Can the "Status: Needs Information" label be removed?

[kamilogorek]

Yes, it's much clearer now, thanks for detailed writeup. I'll try to tackle this soon, but don't have enough time recently to take care of everything. I'll keep everyone posted if I'll make any progress.