Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

🐛 Use EventTarget.prototype.addEventListener instead of the method #3137

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
53 changes: 51 additions & 2 deletions packages/core/src/browser/addEventListener.spec.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import type { Configuration } from '@datadog/browser-core'
import type { MockZoneJs } from '../../test'
import { isIE } from '../tools/utils/browserDetection'
import type { Configuration } from '../domain/configuration'
import { createNewEvent, mockZoneJs } from '../../test'
import type { MockZoneJs } from '../../test'
import { noop } from '../tools/utils/functionUtils'
import { addEventListener, DOM_EVENT } from './addEventListener'

Expand Down Expand Up @@ -35,6 +36,54 @@ describe('addEventListener', () => {
})
})

it('Use the EventTarget.prototype.addEventListener when the eventTarget is an instance of EventTarget', () => {
if (isIE()) {
pending('EventTarget not supported in IE')
}

const addEventListenerSpy = jasmine.createSpy()
const removeEventListenerSpy = jasmine.createSpy()

// The stopLeakDetection function of the global after each hook will reset the EventTarget.prototype.addEventListener
Copy link
Contributor Author

@amortemousque amortemousque Nov 15, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The addEventListener override conflicts with the leakDetection. The best way I found is to let the stopLeakDetection remove the override

EventTarget.prototype.addEventListener = addEventListenerSpy
EventTarget.prototype.removeEventListener = removeEventListenerSpy

const htmlDivElement = document.createElement('div')
htmlDivElement.addEventListener = jasmine.createSpy()
htmlDivElement.removeEventListener = jasmine.createSpy()

const { stop } = addEventListener({ allowUntrustedEvents: false }, htmlDivElement, DOM_EVENT.CLICK, noop)

const event = createNewEvent(DOM_EVENT.CLICK)
htmlDivElement.dispatchEvent(event)
stop()

// eslint-disable-next-line @typescript-eslint/unbound-method
expect(htmlDivElement.addEventListener).not.toHaveBeenCalled()
// eslint-disable-next-line @typescript-eslint/unbound-method
expect(htmlDivElement.removeEventListener).not.toHaveBeenCalled()

expect(addEventListenerSpy).toHaveBeenCalled()
expect(removeEventListenerSpy).toHaveBeenCalled()
})

it('Use the addEventListener method when the eventTarget is not an instance of EventTarget', () => {
const listener = jasmine.createSpy()

const customEventTarget = {
addEventListener: jasmine.createSpy(),
removeEventListener: jasmine.createSpy(),
} as unknown as EventTarget

const { stop } = addEventListener({ allowUntrustedEvents: false }, customEventTarget, 'change', listener)
stop()

// eslint-disable-next-line @typescript-eslint/unbound-method
expect(customEventTarget.addEventListener).toHaveBeenCalled()
// eslint-disable-next-line @typescript-eslint/unbound-method
expect(customEventTarget.removeEventListener).toHaveBeenCalled()
})

describe('Untrusted event', () => {
beforeEach(() => {
configuration = { allowUntrustedEvents: false } as Configuration
Expand Down
8 changes: 6 additions & 2 deletions packages/core/src/browser/addEventListener.ts
Original file line number Diff line number Diff line change
Expand Up @@ -127,11 +127,15 @@ export function addEventListeners<Target extends EventTarget, EventName extends

const options = passive ? { capture, passive } : capture

const add = getZoneJsOriginalValue(eventTarget, 'addEventListener')
// Use the window.EventTarget.prototype when possible to avoid wrong overrides (e.g: https://github.com/salesforce/lwc/issues/1824)
const listenerTarget =
window.EventTarget && eventTarget instanceof EventTarget ? window.EventTarget.prototype : eventTarget

const add = getZoneJsOriginalValue(listenerTarget, 'addEventListener')
eventNames.forEach((eventName) => add.call(eventTarget, eventName, listenerWithMonitor, options))

function stop() {
const remove = getZoneJsOriginalValue(eventTarget, 'removeEventListener')
const remove = getZoneJsOriginalValue(listenerTarget, 'removeEventListener')
eventNames.forEach((eventName) => remove.call(eventTarget, eventName, listenerWithMonitor, options))
}

Expand Down
3 changes: 3 additions & 0 deletions packages/core/src/domain/report/reportObservable.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,9 @@ describe('report observable', () => {
let configuration: Configuration

beforeEach(() => {
if (!window.ReportingObserver) {
pending('ReportingObserver not supported')
}
configuration = {} as Configuration
reportingObserver = mockReportingObserver()
cspEventListener = mockCspEventListener()
Expand Down
28 changes: 28 additions & 0 deletions packages/core/test/emulate/mockEventTarget.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
export class MockEventTarget {
public listeners: { [k: string]: EventListener[] } = {}

addEventListener(type: string, listener: EventListener, _options?: boolean | AddEventListenerOptions): void {
if (!this.listeners[type]) {
this.listeners[type] = []
}

this.listeners[type].push(listener)
}

removeEventListener(type: string, listener: EventListenerOrEventListenerObject) {
if (!this.listeners[type]) {
throw new Error(`Can't remove a listener. Event "${type}" doesn't exits.`)
}

this.listeners[type] = this.listeners[type].filter((lst) => listener !== lst)
}

dispatchEvent(event: Event): boolean {
if (this.listeners[event.type]) {
this.listeners[event.type].forEach((listener) => {
listener.apply(this, [event])
})
}
return true
}
}
9 changes: 6 additions & 3 deletions packages/core/test/emulate/mockReportingObserver.ts
Original file line number Diff line number Diff line change
Expand Up @@ -50,9 +50,12 @@ export function mockReportingObserver() {
export type MockCspEventListener = ReturnType<typeof mockCspEventListener>

export function mockCspEventListener() {
spyOn(document, 'addEventListener').and.callFake((_type: string, listener: EventListener) => {
listeners.push(listener)
})
// The stopLeakDetection function of the global after each hook will reset the EventTarget.prototype.addEventListener
EventTarget.prototype.addEventListener = jasmine
.createSpy()
.and.callFake((_type: string, listener: EventListener) => {
listeners.push(listener)
})

const listeners: EventListener[] = []

Expand Down
31 changes: 2 additions & 29 deletions packages/core/test/emulate/mockXhr.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import { isServerError, noop } from '../../src'
import { registerCleanupTask } from '../registerCleanupTask'
import { createNewEvent } from './createNewEvent'
import { MockEventTarget } from './mockEventTarget'

export function mockXhr() {
const originalXhr = XMLHttpRequest
Expand Down Expand Up @@ -30,35 +31,7 @@ export function withXhr({
setup(xhr as unknown as MockXhr)
}

class MockEventEmitter {
public listeners: { [k: string]: Array<(event: Event) => void> } = {}

addEventListener(name: string, callback: () => void) {
if (!this.listeners[name]) {
this.listeners[name] = []
}

this.listeners[name].push(callback)
}

removeEventListener(name: string, callback: () => void) {
if (!this.listeners[name]) {
throw new Error(`Can't remove a listener. Event "${name}" doesn't exits.`)
}

this.listeners[name] = this.listeners[name].filter((listener) => listener !== callback)
}

dispatchEvent(evt: Event) {
if (!this.listeners[evt.type]) {
return
}

this.listeners[evt.type].forEach((listener) => listener.apply(this, [evt]))
}
}

export class MockXhr extends MockEventEmitter {
export class MockXhr extends MockEventTarget {
public static onSend: (xhr: MockXhr) => void | undefined
public response: string | undefined = undefined
public responseText: string | undefined = undefined
Expand Down
1 change: 1 addition & 0 deletions packages/core/test/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ export * from './emulate/mockFlushController'
export * from './emulate/mockExperimentalFeatures'
export * from './emulate/mockFetch'
export * from './emulate/mockXhr'
export * from './emulate/mockEventTarget'
export * from './emulate/mockRequestIdleCallback'
export * from './typeUtils'
export * from './coreConfiguration'
Expand Down
3 changes: 3 additions & 0 deletions packages/rum-core/src/domain/error/trackReportError.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,9 @@ describe('trackReportError', () => {
let configuration: RumConfiguration

beforeEach(() => {
if (!window.ReportingObserver) {
pending('ReportingObserver not supported')
}
configuration = mockRumConfiguration()
errorObservable = new Observable()
notifyLog = jasmine.createSpy('notifyLog')
Expand Down
76 changes: 25 additions & 51 deletions packages/rum/test/mockWorker.ts
Original file line number Diff line number Diff line change
@@ -1,34 +1,15 @@
import type { DeflateWorker, DeflateWorkerAction, DeflateWorkerResponse } from '@datadog/browser-core'
import type { DeflateWorker, DeflateWorkerAction } from '@datadog/browser-core'
import { string2buf } from '../../worker/src/domain/deflate'
import { createNewEvent } from '../../core/test'
import { createNewEvent, MockEventTarget } from '../../core/test'

type DeflateWorkerListener = (event: { data: DeflateWorkerResponse }) => void

export class MockWorker implements DeflateWorker {
export class MockWorker extends MockEventTarget implements DeflateWorker {
public onmessage = null
public onmessageerror = null
public onerror = null

readonly pendingMessages: DeflateWorkerAction[] = []

private streams = new Map<number, Uint8Array[]>()
private listeners: {
message: Set<DeflateWorkerListener>
error: Set<(error: unknown) => void>
} = { message: new Set(), error: new Set() }

addEventListener(eventName: 'message' | 'error', listener: any): void {
this.listeners[eventName].add(listener)
}

removeEventListener(eventName: 'message' | 'error', listener: any): void {
this.listeners[eventName].delete(listener)
}

dispatchEvent(): boolean {
// Partial implementation, feel free to implement
throw new Error('not yet implemented')
}

postMessage(message: DeflateWorkerAction): void {
this.pendingMessages.push(message)
Expand All @@ -43,7 +24,7 @@ export class MockWorker implements DeflateWorker {
}

get messageListenersCount() {
return this.listeners.message.size
return this.listeners.message.length
}

processAllMessages(): void {
Expand All @@ -61,15 +42,13 @@ export class MockWorker implements DeflateWorker {
if (message) {
switch (message.action) {
case 'init':
this.listeners.message.forEach((listener) =>
listener(
createNewEvent('message', {
data: {
type: 'initialized',
version: 'dev',
},
})
)
this.dispatchEvent(
createNewEvent('message', {
data: {
type: 'initialized',
version: 'dev',
},
})
)
break
case 'write':
Expand All @@ -82,20 +61,17 @@ export class MockWorker implements DeflateWorker {
// In the mock worker, for simplicity, we'll just use the UTF-8 encoded string instead of deflating it.
const binaryData = string2buf(message.data)
stream.push(binaryData)

this.listeners.message.forEach((listener) =>
listener(
createNewEvent('message', {
data: {
type: 'wrote',
id: message.id,
streamId: message.streamId,
result: binaryData,
trailer: new Uint8Array([32]), // emulate a trailer with a single space
additionalBytesCount: binaryData.length,
},
})
)
this.dispatchEvent(
createNewEvent('message', {
data: {
type: 'wrote',
id: message.id,
streamId: message.streamId,
result: binaryData,
trailer: new Uint8Array([32]), // emulate a trailer with a single space
additionalBytesCount: binaryData.length,
},
})
)
}
break
Expand All @@ -107,13 +83,11 @@ export class MockWorker implements DeflateWorker {
}

dispatchErrorEvent() {
const error = createNewEvent('worker')
this.listeners.error.forEach((listener) => listener(error))
const error = createNewEvent('error')
this.dispatchEvent(error)
}

dispatchErrorMessage(error: Error | string, streamId?: number) {
this.listeners.message.forEach((listener) =>
listener(createNewEvent('message', { data: { type: 'errored', error, streamId } }))
)
this.dispatchEvent(createNewEvent('message', { data: { type: 'errored', error, streamId } }))
}
}