From 0b2564e507c4a25aa15d946b1da07a71607615de Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Beno=C3=AEt=20Zugmeyer?= Date: Wed, 7 Dec 2022 16:53:03 +0100 Subject: [PATCH 1/4] =?UTF-8?q?=E2=99=BB=EF=B8=8F=20[RUMF-1449]=20rename?= =?UTF-8?q?=20EventEmitter=20to=20EventTarget?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Use the native name rather than introducing our own concept. --- packages/core/src/tools/utils.ts | 29 +++++---------- .../view/trackFirstHidden.spec.ts | 36 +++++++++---------- .../view/trackFirstHidden.ts | 6 ++-- .../view/trackInitialViewTimings.spec.ts | 8 ++--- .../view/trackInitialViewTimings.ts | 6 ++-- 5 files changed, 36 insertions(+), 49 deletions(-) diff --git a/packages/core/src/tools/utils.ts b/packages/core/src/tools/utils.ts index b8c467c25d..5ff39479c2 100644 --- a/packages/core/src/tools/utils.ts +++ b/packages/core/src/tools/utils.ts @@ -357,19 +357,6 @@ export function safeTruncate(candidate: string, length: number, suffix = '') { return `${candidate.slice(0, correctedLength)}${suffix}` } -export interface EventEmitter { - addEventListener( - event: DOM_EVENT, - listener: (event: Event) => void, - options?: boolean | { capture?: boolean; passive?: boolean } - ): void - removeEventListener( - event: DOM_EVENT, - listener: (event: Event) => void, - options?: boolean | { capture?: boolean; passive?: boolean } - ): void -} - interface AddEventListenerOptions { once?: boolean capture?: boolean @@ -377,7 +364,7 @@ interface AddEventListenerOptions { } /** - * Add an event listener to an event emitter object (Window, Element, mock object...). This provides + * Add an event listener to an event target object (Window, Element, mock object...). This provides * a few conveniences compared to using `element.addEventListener` directly: * * * supports IE11 by: using an option object only if needed and emulating the `once` option @@ -387,16 +374,16 @@ interface AddEventListenerOptions { * * returns a `stop` function to remove the listener */ export function addEventListener( - emitter: EventEmitter, + eventTarget: EventTarget, event: DOM_EVENT, listener: (event: E) => void, options?: AddEventListenerOptions ) { - return addEventListeners(emitter, [event], listener, options) + return addEventListeners(eventTarget, [event], listener, options) } /** - * Add event listeners to an event emitter object (Window, Element, mock object...). This provides + * Add event listeners to an event target object (Window, Element, mock object...). This provides * a few conveniences compared to using `element.addEventListener` directly: * * * supports IE11 by: using an option object only if needed and emulating the `once` option @@ -408,10 +395,10 @@ export function addEventListener( * * with `once: true`, the listener will be called at most once, even if different events are listened */ export function addEventListeners( - emitter: EventEmitter, + eventTarget: EventTarget, events: DOM_EVENT[], listener: (event: E) => void, - { once, capture, passive }: { once?: boolean; capture?: boolean; passive?: boolean } = {} + { once, capture, passive }: AddEventListenerOptions = {} ) { const wrappedListener = monitor( once @@ -423,8 +410,8 @@ export function addEventListeners( ) const options = passive ? { capture, passive } : capture - events.forEach((event) => emitter.addEventListener(event, wrappedListener, options)) - const stop = () => events.forEach((event) => emitter.removeEventListener(event, wrappedListener, options)) + events.forEach((event) => eventTarget.addEventListener(event, wrappedListener, options)) + const stop = () => events.forEach((event) => eventTarget.removeEventListener(event, wrappedListener, options)) return { stop, diff --git a/packages/rum-core/src/domain/rumEventsCollection/view/trackFirstHidden.spec.ts b/packages/rum-core/src/domain/rumEventsCollection/view/trackFirstHidden.spec.ts index 5623794f17..88c0731a63 100644 --- a/packages/rum-core/src/domain/rumEventsCollection/view/trackFirstHidden.spec.ts +++ b/packages/rum-core/src/domain/rumEventsCollection/view/trackFirstHidden.spec.ts @@ -17,11 +17,11 @@ describe('trackFirstHidden', () => { it('should ignore events', () => { setPageVisibility('hidden') - const emitter = document.createElement('div') - const firstHidden = trackFirstHidden(emitter) + const eventTarget = document.createElement('div') + const firstHidden = trackFirstHidden(eventTarget) - emitter.dispatchEvent(createNewEvent(DOM_EVENT.PAGE_HIDE, { timeStamp: 100 })) - emitter.dispatchEvent(createNewEvent(DOM_EVENT.VISIBILITY_CHANGE, { timeStamp: 100 })) + eventTarget.dispatchEvent(createNewEvent(DOM_EVENT.PAGE_HIDE, { timeStamp: 100 })) + eventTarget.dispatchEvent(createNewEvent(DOM_EVENT.VISIBILITY_CHANGE, { timeStamp: 100 })) expect(firstHidden.timeStamp).toBe(0 as RelativeTime) }) @@ -33,43 +33,43 @@ describe('trackFirstHidden', () => { }) it('should return the timestamp of the first pagehide event', () => { - const emitter = document.createElement('div') - const firstHidden = trackFirstHidden(emitter) + const eventTarget = document.createElement('div') + const firstHidden = trackFirstHidden(eventTarget) - emitter.dispatchEvent(createNewEvent(DOM_EVENT.PAGE_HIDE, { timeStamp: 100 })) + eventTarget.dispatchEvent(createNewEvent(DOM_EVENT.PAGE_HIDE, { timeStamp: 100 })) expect(firstHidden.timeStamp).toBe(100 as RelativeTime) }) it('should return the timestamp of the first visibilitychange event if the page is hidden', () => { - const emitter = document.createElement('div') - const firstHidden = trackFirstHidden(emitter) + const eventTarget = document.createElement('div') + const firstHidden = trackFirstHidden(eventTarget) setPageVisibility('hidden') - emitter.dispatchEvent(createNewEvent(DOM_EVENT.VISIBILITY_CHANGE, { timeStamp: 100 })) + eventTarget.dispatchEvent(createNewEvent(DOM_EVENT.VISIBILITY_CHANGE, { timeStamp: 100 })) expect(firstHidden.timeStamp).toBe(100 as RelativeTime) }) it('should ignore visibilitychange event if the page is visible', () => { - const emitter = document.createElement('div') - const firstHidden = trackFirstHidden(emitter) + const eventTarget = document.createElement('div') + const firstHidden = trackFirstHidden(eventTarget) - emitter.dispatchEvent(createNewEvent(DOM_EVENT.VISIBILITY_CHANGE, { timeStamp: 100 })) + eventTarget.dispatchEvent(createNewEvent(DOM_EVENT.VISIBILITY_CHANGE, { timeStamp: 100 })) expect(firstHidden.timeStamp).toBe(Infinity as RelativeTime) }) it('should ignore subsequent events', () => { - const emitter = document.createElement('div') - const firstHidden = trackFirstHidden(emitter) + const eventTarget = document.createElement('div') + const firstHidden = trackFirstHidden(eventTarget) - emitter.dispatchEvent(createNewEvent(DOM_EVENT.PAGE_HIDE, { timeStamp: 100 })) + eventTarget.dispatchEvent(createNewEvent(DOM_EVENT.PAGE_HIDE, { timeStamp: 100 })) // Subsequent events: - emitter.dispatchEvent(createNewEvent(DOM_EVENT.PAGE_HIDE, { timeStamp: 200 })) + eventTarget.dispatchEvent(createNewEvent(DOM_EVENT.PAGE_HIDE, { timeStamp: 200 })) setPageVisibility('hidden') - emitter.dispatchEvent(createNewEvent(DOM_EVENT.VISIBILITY_CHANGE, { timeStamp: 200 })) + eventTarget.dispatchEvent(createNewEvent(DOM_EVENT.VISIBILITY_CHANGE, { timeStamp: 200 })) expect(firstHidden.timeStamp).toBe(100 as RelativeTime) }) diff --git a/packages/rum-core/src/domain/rumEventsCollection/view/trackFirstHidden.ts b/packages/rum-core/src/domain/rumEventsCollection/view/trackFirstHidden.ts index 7c4c87be64..6d8c13f4a2 100644 --- a/packages/rum-core/src/domain/rumEventsCollection/view/trackFirstHidden.ts +++ b/packages/rum-core/src/domain/rumEventsCollection/view/trackFirstHidden.ts @@ -1,10 +1,10 @@ -import type { EventEmitter, RelativeTime } from '@datadog/browser-core' +import type { RelativeTime } from '@datadog/browser-core' import { addEventListeners, DOM_EVENT } from '@datadog/browser-core' let trackFirstHiddenSingleton: { timeStamp: RelativeTime } | undefined let stopListeners: (() => void) | undefined -export function trackFirstHidden(emitter: EventEmitter = window) { +export function trackFirstHidden(eventTarget: EventTarget = window) { if (!trackFirstHiddenSingleton) { if (document.visibilityState === 'hidden') { trackFirstHiddenSingleton = { @@ -15,7 +15,7 @@ export function trackFirstHidden(emitter: EventEmitter = window) { timeStamp: Infinity as RelativeTime, } ;({ stop: stopListeners } = addEventListeners( - emitter, + eventTarget, [DOM_EVENT.PAGE_HIDE, DOM_EVENT.VISIBILITY_CHANGE], (event) => { if (event.type === 'pagehide' || document.visibilityState === 'hidden') { diff --git a/packages/rum-core/src/domain/rumEventsCollection/view/trackInitialViewTimings.spec.ts b/packages/rum-core/src/domain/rumEventsCollection/view/trackInitialViewTimings.spec.ts index 9429e411de..b84000bf75 100644 --- a/packages/rum-core/src/domain/rumEventsCollection/view/trackInitialViewTimings.spec.ts +++ b/packages/rum-core/src/domain/rumEventsCollection/view/trackInitialViewTimings.spec.ts @@ -161,13 +161,13 @@ describe('trackFirstContentfulPaintTiming', () => { describe('largestContentfulPaintTiming', () => { let setupBuilder: TestSetupBuilder let lcpCallback: jasmine.Spy<(value: RelativeTime) => void> - let emitter: Element + let eventTarget: Element beforeEach(() => { lcpCallback = jasmine.createSpy() - emitter = document.createElement('div') + eventTarget = document.createElement('div') setupBuilder = setup().beforeBuild(({ lifeCycle }) => - trackLargestContentfulPaintTiming(lifeCycle, emitter, lcpCallback) + trackLargestContentfulPaintTiming(lifeCycle, eventTarget, lcpCallback) ) resetFirstHidden() }) @@ -189,7 +189,7 @@ describe('largestContentfulPaintTiming', () => { it('should be discarded if it is reported after a user interaction', () => { const { lifeCycle } = setupBuilder.build() - emitter.dispatchEvent(createNewEvent(DOM_EVENT.KEY_DOWN, { timeStamp: 1 })) + eventTarget.dispatchEvent(createNewEvent(DOM_EVENT.KEY_DOWN, { timeStamp: 1 })) lifeCycle.notify(LifeCycleEventType.PERFORMANCE_ENTRIES_COLLECTED, [FAKE_LARGEST_CONTENTFUL_PAINT_ENTRY]) expect(lcpCallback).not.toHaveBeenCalled() diff --git a/packages/rum-core/src/domain/rumEventsCollection/view/trackInitialViewTimings.ts b/packages/rum-core/src/domain/rumEventsCollection/view/trackInitialViewTimings.ts index bb71154367..726b91e12f 100644 --- a/packages/rum-core/src/domain/rumEventsCollection/view/trackInitialViewTimings.ts +++ b/packages/rum-core/src/domain/rumEventsCollection/view/trackInitialViewTimings.ts @@ -1,4 +1,4 @@ -import type { Duration, EventEmitter, RelativeTime } from '@datadog/browser-core' +import type { Duration, RelativeTime } from '@datadog/browser-core' import { assign, addEventListeners, @@ -116,7 +116,7 @@ export function trackFirstContentfulPaintTiming(lifeCycle: LifeCycle, callback: */ export function trackLargestContentfulPaintTiming( lifeCycle: LifeCycle, - emitter: EventEmitter, + eventTarget: EventTarget, callback: (lcpTiming: RelativeTime) => void ) { const firstHidden = trackFirstHidden() @@ -126,7 +126,7 @@ export function trackLargestContentfulPaintTiming( // but the web-vitals reference implementation uses this as a safeguard. let firstInteractionTimestamp = Infinity const { stop: stopEventListener } = addEventListeners( - emitter, + eventTarget, [DOM_EVENT.POINTER_DOWN, DOM_EVENT.KEY_DOWN], (event) => { firstInteractionTimestamp = event.timeStamp From b05917931653c2d175bb5624c439d39a05840f91 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Beno=C3=AEt=20Zugmeyer?= Date: Wed, 7 Dec 2022 17:38:33 +0100 Subject: [PATCH 2/4] =?UTF-8?q?=F0=9F=9A=9A=20[RUMF-1449]=20move=20addEven?= =?UTF-8?q?tListener=20functions=20to=20a=20dedicated=20module?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- packages/core/src/browser/addEventListener.ts | 95 +++++++++++++++++++ .../core/src/browser/pageExitObservable.ts | 2 +- .../src/domain/report/reportObservable.ts | 3 +- .../src/domain/session/sessionManager.spec.ts | 3 +- .../core/src/domain/session/sessionManager.ts | 7 +- packages/core/src/index.ts | 1 + packages/core/src/tools/utils.ts | 95 +------------------ 7 files changed, 106 insertions(+), 100 deletions(-) create mode 100644 packages/core/src/browser/addEventListener.ts diff --git a/packages/core/src/browser/addEventListener.ts b/packages/core/src/browser/addEventListener.ts new file mode 100644 index 0000000000..022841b8b9 --- /dev/null +++ b/packages/core/src/browser/addEventListener.ts @@ -0,0 +1,95 @@ +import { monitor } from '../tools/monitor' + +export const enum DOM_EVENT { + BEFORE_UNLOAD = 'beforeunload', + CLICK = 'click', + DBL_CLICK = 'dblclick', + KEY_DOWN = 'keydown', + LOAD = 'load', + POP_STATE = 'popstate', + SCROLL = 'scroll', + TOUCH_START = 'touchstart', + TOUCH_END = 'touchend', + TOUCH_MOVE = 'touchmove', + VISIBILITY_CHANGE = 'visibilitychange', + DOM_CONTENT_LOADED = 'DOMContentLoaded', + POINTER_DOWN = 'pointerdown', + POINTER_UP = 'pointerup', + POINTER_CANCEL = 'pointercancel', + HASH_CHANGE = 'hashchange', + PAGE_HIDE = 'pagehide', + MOUSE_DOWN = 'mousedown', + MOUSE_UP = 'mouseup', + MOUSE_MOVE = 'mousemove', + FOCUS = 'focus', + BLUR = 'blur', + CONTEXT_MENU = 'contextmenu', + RESIZE = 'resize', + CHANGE = 'change', + INPUT = 'input', + PLAY = 'play', + PAUSE = 'pause', + SECURITY_POLICY_VIOLATION = 'securitypolicyviolation', + SELECTION_CHANGE = 'selectionchange', +} + +interface AddEventListenerOptions { + once?: boolean + capture?: boolean + passive?: boolean +} + +/** + * Add an event listener to an event target object (Window, Element, mock object...). This provides + * a few conveniences compared to using `element.addEventListener` directly: + * + * * supports IE11 by: using an option object only if needed and emulating the `once` option + * + * * wraps the listener with a `monitor` function + * + * * returns a `stop` function to remove the listener + */ +export function addEventListener( + eventTarget: EventTarget, + event: DOM_EVENT, + listener: (event: E) => void, + options?: AddEventListenerOptions +) { + return addEventListeners(eventTarget, [event], listener, options) +} + +/** + * Add event listeners to an event target object (Window, Element, mock object...). This provides + * a few conveniences compared to using `element.addEventListener` directly: + * + * * supports IE11 by: using an option object only if needed and emulating the `once` option + * + * * wraps the listener with a `monitor` function + * + * * returns a `stop` function to remove the listener + * + * * with `once: true`, the listener will be called at most once, even if different events are listened + */ +export function addEventListeners( + eventTarget: EventTarget, + events: DOM_EVENT[], + listener: (event: E) => void, + { once, capture, passive }: AddEventListenerOptions = {} +) { + const wrappedListener = monitor( + once + ? (event: Event) => { + stop() + listener(event as E) + } + : (listener as (event: Event) => void) + ) + + const options = passive ? { capture, passive } : capture + events.forEach((event) => eventTarget.addEventListener(event, wrappedListener, options)) + const stop = () => events.forEach((event) => eventTarget.removeEventListener(event, wrappedListener, options)) + + return { + stop, + } +} diff --git a/packages/core/src/browser/pageExitObservable.ts b/packages/core/src/browser/pageExitObservable.ts index 4f6a5391f8..7692eca8c0 100644 --- a/packages/core/src/browser/pageExitObservable.ts +++ b/packages/core/src/browser/pageExitObservable.ts @@ -1,5 +1,5 @@ import { Observable } from '../tools/observable' -import { addEventListener, DOM_EVENT } from '../tools/utils' +import { addEventListener, DOM_EVENT } from './addEventListener' export const enum PageExitReason { HIDDEN = 'visibility_hidden', diff --git a/packages/core/src/domain/report/reportObservable.ts b/packages/core/src/domain/report/reportObservable.ts index 533e981193..225e5e5d2b 100644 --- a/packages/core/src/domain/report/reportObservable.ts +++ b/packages/core/src/domain/report/reportObservable.ts @@ -1,7 +1,8 @@ import { toStackTraceString } from '../../tools/error' import { monitor } from '../../tools/monitor' import { mergeObservables, Observable } from '../../tools/observable' -import { DOM_EVENT, includes, addEventListener, safeTruncate } from '../../tools/utils' +import { includes, safeTruncate } from '../../tools/utils' +import { addEventListener, DOM_EVENT } from '../../browser/addEventListener' import type { Report, BrowserWindow, ReportType } from './browser.types' export const RawReportType = { diff --git a/packages/core/src/domain/session/sessionManager.spec.ts b/packages/core/src/domain/session/sessionManager.spec.ts index b63b01848d..371ab0330f 100644 --- a/packages/core/src/domain/session/sessionManager.spec.ts +++ b/packages/core/src/domain/session/sessionManager.spec.ts @@ -2,9 +2,10 @@ import type { CookieOptions } from '../../browser/cookie' import { COOKIE_ACCESS_DELAY, getCookie, setCookie } from '../../browser/cookie' import type { Clock } from '../../../test/specHelper' import { mockClock, restorePageVisibility, setPageVisibility, createNewEvent } from '../../../test/specHelper' -import { ONE_HOUR, DOM_EVENT, ONE_SECOND } from '../../tools/utils' +import { ONE_HOUR, ONE_SECOND } from '../../tools/utils' import type { RelativeTime } from '../../tools/timeUtils' import { isIE } from '../../tools/browserDetection' +import { DOM_EVENT } from '../../browser/addEventListener' import type { SessionManager } from './sessionManager' import { startSessionManager, stopSessionManager, VISIBILITY_CHECK_DELAY } from './sessionManager' import { SESSION_COOKIE_NAME } from './sessionCookieStore' diff --git a/packages/core/src/domain/session/sessionManager.ts b/packages/core/src/domain/session/sessionManager.ts index 3b363e5d0c..bc651f427e 100644 --- a/packages/core/src/domain/session/sessionManager.ts +++ b/packages/core/src/domain/session/sessionManager.ts @@ -6,6 +6,7 @@ import { ContextHistory } from '../../tools/contextHistory' import type { RelativeTime } from '../../tools/timeUtils' import { relativeNow, clocksOrigin } from '../../tools/timeUtils' import { monitor } from '../../tools/monitor' +import { DOM_EVENT, addEventListener, addEventListeners } from '../../browser/addEventListener' import { tryOldCookiesMigration } from './oldCookiesMigration' import { startSessionStore } from './sessionStore' import { SESSION_TIME_OUT_DELAY } from './sessionConstants' @@ -70,9 +71,9 @@ export function stopSessionManager() { } function trackActivity(expandOrRenewSession: () => void) { - const { stop } = utils.addEventListeners( + const { stop } = addEventListeners( window, - [utils.DOM_EVENT.CLICK, utils.DOM_EVENT.TOUCH_START, utils.DOM_EVENT.KEY_DOWN, utils.DOM_EVENT.SCROLL], + [DOM_EVENT.CLICK, DOM_EVENT.TOUCH_START, DOM_EVENT.KEY_DOWN, DOM_EVENT.SCROLL], expandOrRenewSession, { capture: true, passive: true } ) @@ -86,7 +87,7 @@ function trackVisibility(expandSession: () => void) { } }) - const { stop } = utils.addEventListener(document, utils.DOM_EVENT.VISIBILITY_CHANGE, expandSessionWhenVisible) + const { stop } = addEventListener(document, DOM_EVENT.VISIBILITY_CHANGE, expandSessionWhenVisible) stopCallbacks.push(stop) const visibilityCheckInterval = setInterval(expandSessionWhenVisible, VISIBILITY_CHECK_DELAY) diff --git a/packages/core/src/index.ts b/packages/core/src/index.ts index 3d17930979..abf52ca94f 100644 --- a/packages/core/src/index.ts +++ b/packages/core/src/index.ts @@ -75,6 +75,7 @@ export { areCookiesAuthorized, getCookie, setCookie, deleteCookie, COOKIE_ACCESS export { initXhrObservable, XhrCompleteContext, XhrStartContext } from './browser/xhrObservable' export { initFetchObservable, FetchResolveContext, FetchStartContext, FetchContext } from './browser/fetchObservable' export { createPageExitObservable, PageExitEvent, PageExitReason } from './browser/pageExitObservable' +export * from './browser/addEventListener' export { initConsoleObservable, ConsoleLog } from './domain/console/consoleObservable' export { BoundedBuffer } from './tools/boundedBuffer' export { catchUserErrors } from './tools/catchUserErrors' diff --git a/packages/core/src/tools/utils.ts b/packages/core/src/tools/utils.ts index 5ff39479c2..4db28d7f33 100644 --- a/packages/core/src/tools/utils.ts +++ b/packages/core/src/tools/utils.ts @@ -1,3 +1,4 @@ +import { DOM_EVENT, addEventListener } from '../browser/addEventListener' import { display } from './display' import { monitor } from './monitor' @@ -9,39 +10,6 @@ export const ONE_YEAR = 365 * ONE_DAY export const ONE_KIBI_BYTE = 1024 export const ONE_MEBI_BYTE = 1024 * ONE_KIBI_BYTE -export const enum DOM_EVENT { - BEFORE_UNLOAD = 'beforeunload', - CLICK = 'click', - DBL_CLICK = 'dblclick', - KEY_DOWN = 'keydown', - LOAD = 'load', - POP_STATE = 'popstate', - SCROLL = 'scroll', - TOUCH_START = 'touchstart', - TOUCH_END = 'touchend', - TOUCH_MOVE = 'touchmove', - VISIBILITY_CHANGE = 'visibilitychange', - DOM_CONTENT_LOADED = 'DOMContentLoaded', - POINTER_DOWN = 'pointerdown', - POINTER_UP = 'pointerup', - POINTER_CANCEL = 'pointercancel', - HASH_CHANGE = 'hashchange', - PAGE_HIDE = 'pagehide', - MOUSE_DOWN = 'mousedown', - MOUSE_UP = 'mouseup', - MOUSE_MOVE = 'mousemove', - FOCUS = 'focus', - BLUR = 'blur', - CONTEXT_MENU = 'contextmenu', - RESIZE = 'resize', - CHANGE = 'change', - INPUT = 'input', - PLAY = 'play', - PAUSE = 'pause', - SECURITY_POLICY_VIOLATION = 'securitypolicyviolation', - SELECTION_CHANGE = 'selectionchange', -} - export const enum ResourceType { DOCUMENT = 'document', XHR = 'xhr', @@ -357,67 +325,6 @@ export function safeTruncate(candidate: string, length: number, suffix = '') { return `${candidate.slice(0, correctedLength)}${suffix}` } -interface AddEventListenerOptions { - once?: boolean - capture?: boolean - passive?: boolean -} - -/** - * Add an event listener to an event target object (Window, Element, mock object...). This provides - * a few conveniences compared to using `element.addEventListener` directly: - * - * * supports IE11 by: using an option object only if needed and emulating the `once` option - * - * * wraps the listener with a `monitor` function - * - * * returns a `stop` function to remove the listener - */ -export function addEventListener( - eventTarget: EventTarget, - event: DOM_EVENT, - listener: (event: E) => void, - options?: AddEventListenerOptions -) { - return addEventListeners(eventTarget, [event], listener, options) -} - -/** - * Add event listeners to an event target object (Window, Element, mock object...). This provides - * a few conveniences compared to using `element.addEventListener` directly: - * - * * supports IE11 by: using an option object only if needed and emulating the `once` option - * - * * wraps the listener with a `monitor` function - * - * * returns a `stop` function to remove the listener - * - * * with `once: true`, the listener will be called at most once, even if different events are listened - */ -export function addEventListeners( - eventTarget: EventTarget, - events: DOM_EVENT[], - listener: (event: E) => void, - { once, capture, passive }: AddEventListenerOptions = {} -) { - const wrappedListener = monitor( - once - ? (event: Event) => { - stop() - listener(event as E) - } - : (listener as (event: Event) => void) - ) - - const options = passive ? { capture, passive } : capture - events.forEach((event) => eventTarget.addEventListener(event, wrappedListener, options)) - const stop = () => events.forEach((event) => eventTarget.removeEventListener(event, wrappedListener, options)) - - return { - stop, - } -} - export function elementMatches(element: Element & { msMatchesSelector?(selector: string): boolean }, selector: string) { if (element.matches) { return element.matches(selector) From 669b1ec5f0aa441a4a95429de21fc9bfd862e45e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Beno=C3=AEt=20Zugmeyer?= Date: Wed, 7 Dec 2022 16:16:49 +0100 Subject: [PATCH 3/4] =?UTF-8?q?=F0=9F=90=9B=20[RUMF-1449]=20implement=20a?= =?UTF-8?q?=20workaround=20for=20Firefox=20memory=20leak?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- .../core/src/browser/addEventListener.spec.ts | 37 +++++++++++++++++++ packages/core/src/browser/addEventListener.ts | 11 +++++- packages/core/src/index.ts | 1 + .../src/tools/getZoneJsOriginalValue.spec.ts | 35 ++++++++++++++++++ .../core/src/tools/getZoneJsOriginalValue.ts | 33 +++++++++++++++++ packages/core/test/specHelper.ts | 29 +++++++++++++++ .../src/browser/domMutationObservable.spec.ts | 27 +++++--------- .../src/browser/domMutationObservable.ts | 23 +++++------- 8 files changed, 163 insertions(+), 33 deletions(-) create mode 100644 packages/core/src/browser/addEventListener.spec.ts create mode 100644 packages/core/src/tools/getZoneJsOriginalValue.spec.ts create mode 100644 packages/core/src/tools/getZoneJsOriginalValue.ts diff --git a/packages/core/src/browser/addEventListener.spec.ts b/packages/core/src/browser/addEventListener.spec.ts new file mode 100644 index 0000000000..1e4aedd0b5 --- /dev/null +++ b/packages/core/src/browser/addEventListener.spec.ts @@ -0,0 +1,37 @@ +import { stubZoneJs } from '../../test/specHelper' +import { noop } from '../tools/utils' + +import { addEventListener, DOM_EVENT } from './addEventListener' + +describe('addEventListener', () => { + describe('Zone.js support', () => { + let zoneJsStub: ReturnType + + beforeEach(() => { + zoneJsStub = stubZoneJs() + }) + + afterEach(() => { + zoneJsStub.restore() + }) + + it('uses the original addEventListener method instead of the method patched by Zone.js', () => { + const zoneJsPatchedAddEventListener = jasmine.createSpy() + const eventTarget = document.createElement('div') + zoneJsStub.replaceProperty(eventTarget, 'addEventListener', zoneJsPatchedAddEventListener) + + addEventListener(eventTarget, DOM_EVENT.CLICK, noop) + expect(zoneJsPatchedAddEventListener).not.toHaveBeenCalled() + }) + + it('uses the original removeEventListener method instead of the method patched by Zone.js', () => { + const zoneJsPatchedRemoveEventListener = jasmine.createSpy() + const eventTarget = document.createElement('div') + zoneJsStub.replaceProperty(eventTarget, 'removeEventListener', zoneJsPatchedRemoveEventListener) + + const { stop } = addEventListener(eventTarget, DOM_EVENT.CLICK, noop) + stop() + expect(zoneJsPatchedRemoveEventListener).not.toHaveBeenCalled() + }) + }) +}) diff --git a/packages/core/src/browser/addEventListener.ts b/packages/core/src/browser/addEventListener.ts index 022841b8b9..5d5233467e 100644 --- a/packages/core/src/browser/addEventListener.ts +++ b/packages/core/src/browser/addEventListener.ts @@ -1,4 +1,5 @@ import { monitor } from '../tools/monitor' +import { getZoneJsOriginalValue } from '../tools/getZoneJsOriginalValue' export const enum DOM_EVENT { BEFORE_UNLOAD = 'beforeunload', @@ -86,8 +87,14 @@ export function addEventListeners( ) const options = passive ? { capture, passive } : capture - events.forEach((event) => eventTarget.addEventListener(event, wrappedListener, options)) - const stop = () => events.forEach((event) => eventTarget.removeEventListener(event, wrappedListener, options)) + + const add = getZoneJsOriginalValue(eventTarget, 'addEventListener') + events.forEach((event) => add.call(eventTarget, event, wrappedListener, options)) + + function stop() { + const remove = getZoneJsOriginalValue(eventTarget, 'removeEventListener') + events.forEach((event) => remove.call(eventTarget, event, wrappedListener, options)) + } return { stop, diff --git a/packages/core/src/index.ts b/packages/core/src/index.ts index abf52ca94f..9fef7b638a 100644 --- a/packages/core/src/index.ts +++ b/packages/core/src/index.ts @@ -58,6 +58,7 @@ export * from './tools/utils' export * from './tools/createEventRateLimiter' export * from './tools/browserDetection' export { sendToExtension } from './tools/sendToExtension' +export { getZoneJsOriginalValue } from './tools/getZoneJsOriginalValue' export { instrumentMethod, instrumentMethodAndCallOriginal, instrumentSetter } from './tools/instrumentMethod' export { ErrorSource, diff --git a/packages/core/src/tools/getZoneJsOriginalValue.spec.ts b/packages/core/src/tools/getZoneJsOriginalValue.spec.ts new file mode 100644 index 0000000000..4c0a8c7be1 --- /dev/null +++ b/packages/core/src/tools/getZoneJsOriginalValue.spec.ts @@ -0,0 +1,35 @@ +import { stubZoneJs } from '../../test/specHelper' + +import { getZoneJsOriginalValue } from './getZoneJsOriginalValue' +import { noop } from './utils' + +describe('getZoneJsOriginalValue', () => { + let zoneJsStub: ReturnType | undefined + + function originalValue() { + // just a function that does nothing but different than 'noop' as we'll want to differentiate + // them. + } + const object = { + name: originalValue, + } + + afterEach(() => { + zoneJsStub?.restore() + }) + + it('returns the original value directly if Zone is not not defined', () => { + expect(getZoneJsOriginalValue(object, 'name')).toBe(originalValue) + }) + + it("returns undefined if Zone is defined but didn't patch that method", () => { + zoneJsStub = stubZoneJs() + expect(getZoneJsOriginalValue(object, 'name')).toBe(originalValue) + }) + + it('returns the original value if Zone did patch the method', () => { + zoneJsStub = stubZoneJs() + zoneJsStub.replaceProperty(object, 'name', noop) + expect(getZoneJsOriginalValue(object, 'name')).toBe(originalValue) + }) +}) diff --git a/packages/core/src/tools/getZoneJsOriginalValue.ts b/packages/core/src/tools/getZoneJsOriginalValue.ts new file mode 100644 index 0000000000..786da8e6fd --- /dev/null +++ b/packages/core/src/tools/getZoneJsOriginalValue.ts @@ -0,0 +1,33 @@ +export interface BrowserWindowWithZoneJs extends Window { + Zone?: { + __symbol__: (name: string) => string + } +} + +/** + * Gets the original value for a DOM API that was potentially patched by Zone.js. + * + * Zone.js[1] is a library that patches a bunch of JS and DOM APIs. It usually stores the original + * value of the patched functions/constructors/methods in a hidden property prefixed by + * __zone_symbol__. + * + * In multiple occasions, we observed that Zone.js is the culprit of important issues leading to + * browser resource exhaustion (memory leak, high CPU usage). This method is used as a workaround to + * use the original DOM API instead of the one patched by Zone.js. + * + * [1]: https://github.com/angular/angular/tree/main/packages/zone.js + */ +export function getZoneJsOriginalValue( + target: Target, + name: Name +): Target[Name] { + const browserWindow = window as BrowserWindowWithZoneJs + let original: Target[Name] | undefined + if (browserWindow.Zone) { + original = (target as any)[browserWindow.Zone.__symbol__(name)] + } + if (!original) { + original = target[name] + } + return original +} diff --git a/packages/core/test/specHelper.ts b/packages/core/test/specHelper.ts index 142a3cd841..49a8f638fc 100644 --- a/packages/core/test/specHelper.ts +++ b/packages/core/test/specHelper.ts @@ -4,6 +4,7 @@ import { resetNavigationStart } from '../src/tools/timeUtils' import { buildUrl } from '../src/tools/urlPolyfill' import { noop, objectEntries, assign } from '../src/tools/utils' import type { BrowserWindowWithEventBridge } from '../src/transport' +import type { BrowserWindowWithZoneJs } from '../src/tools/getZoneJsOriginalValue' // to simulate different build env behavior export interface BuildEnvWindow { @@ -485,3 +486,31 @@ export function interceptRequests() { }, } } + +export function stubZoneJs() { + const browserWindow = window as BrowserWindowWithZoneJs + const restorers: Array<() => void> = [] + + function getSymbol(name: string) { + return `__zone_symbol__${name}` + } + + browserWindow.Zone = { __symbol__: getSymbol } + + return { + restore: () => { + delete browserWindow.Zone + restorers.forEach((restorer) => restorer()) + }, + getSymbol, + replaceProperty(target: Target, name: Name, replacement: Target[Name]) { + const original = target[name] + target[name] = replacement + ;(target as any)[getSymbol(name)] = original + restorers.push(() => { + delete (target as any)[getSymbol(name)] + target[name] = original + }) + }, + } +} diff --git a/packages/rum-core/src/browser/domMutationObservable.spec.ts b/packages/rum-core/src/browser/domMutationObservable.spec.ts index 5c833468ee..5ad5bf61c4 100644 --- a/packages/rum-core/src/browser/domMutationObservable.spec.ts +++ b/packages/rum-core/src/browser/domMutationObservable.spec.ts @@ -1,5 +1,5 @@ import { isIE } from '@datadog/browser-core' -import type { BrowserWindow } from './domMutationObservable' +import { stubZoneJs } from '../../../core/test/specHelper' import { createDOMMutationObservable, getMutationObserverConstructor } from './domMutationObservable' // The MutationObserver invokes its callback in an event loop microtask, making this asynchronous. @@ -108,38 +108,31 @@ describe('domMutationObservable', () => { ) describe('Zone.js support', () => { - const browserWindow = window as BrowserWindow - const OriginalMutationObserverConstructor = browserWindow.MutationObserver! + let zoneJsStub: ReturnType + const OriginalMutationObserverConstructor = window.MutationObserver beforeEach(() => { - browserWindow.Zone = { __symbol__: zoneSymbol } + zoneJsStub = stubZoneJs() }) afterEach(() => { - delete browserWindow.Zone - delete browserWindow[zoneSymbol('MutationObserver') as any] - browserWindow.MutationObserver = OriginalMutationObserverConstructor + zoneJsStub.restore() + window.MutationObserver = OriginalMutationObserverConstructor }) it('gets the original MutationObserver constructor from the "window" object (Zone.js >= 0.8.6)', () => { - browserWindow.MutationObserver = function () { + zoneJsStub.replaceProperty(window, 'MutationObserver', function () { // This won't be instantiated. - } as any - browserWindow[zoneSymbol('MutationObserver') as any] = OriginalMutationObserverConstructor as any - + } as any) expect(getMutationObserverConstructor()).toBe(OriginalMutationObserverConstructor) }) it('gets the original MutationObserver constructor from a patched instance (Zone.js < 0.8.6)', () => { - browserWindow.MutationObserver = function (this: any, callback: () => void) { - this[zoneSymbol('originalInstance')] = new OriginalMutationObserverConstructor(callback) + window.MutationObserver = function (this: any, callback: () => void) { + this[zoneJsStub.getSymbol('originalInstance')] = new OriginalMutationObserverConstructor(callback) } as any expect(getMutationObserverConstructor()).toBe(OriginalMutationObserverConstructor) }) - - function zoneSymbol(name: string) { - return `__zone_symbol__${name}` - } }) }) diff --git a/packages/rum-core/src/browser/domMutationObservable.ts b/packages/rum-core/src/browser/domMutationObservable.ts index 777d40dc7f..3d708b9395 100644 --- a/packages/rum-core/src/browser/domMutationObservable.ts +++ b/packages/rum-core/src/browser/domMutationObservable.ts @@ -1,4 +1,4 @@ -import { monitor, noop, Observable } from '@datadog/browser-core' +import { monitor, noop, Observable, getZoneJsOriginalValue } from '@datadog/browser-core' export function createDOMMutationObservable() { const MutationObserver = getMutationObserverConstructor() @@ -21,11 +21,10 @@ export function createDOMMutationObservable() { } type MutationObserverConstructor = new (callback: MutationCallback) => MutationObserver + export interface BrowserWindow extends Window { MutationObserver?: MutationObserverConstructor - Zone?: { - __symbol__: (name: string) => string - } + Zone?: unknown } export function getMutationObserverConstructor(): MutationObserverConstructor | undefined { @@ -44,16 +43,12 @@ export function getMutationObserverConstructor(): MutationObserverConstructor | // [1] https://github.com/angular/angular/issues/26948 // [2] https://github.com/angular/angular/issues/31712 if (browserWindow.Zone) { - const zoneSymbol = browserWindow.Zone.__symbol__ - // Zone.js 0.8.6+ is storing original class constructors into the browser 'window' object[3]. // // [3] https://github.com/angular/angular/blob/6375fa79875c0fe7b815efc45940a6e6f5c9c9eb/packages/zone.js/lib/common/utils.ts#L288 - constructor = browserWindow[zoneSymbol('MutationObserver') as any] as unknown as - | MutationObserverConstructor - | undefined + constructor = getZoneJsOriginalValue(browserWindow, 'MutationObserver') - if (!constructor && browserWindow.MutationObserver) { + if (browserWindow.MutationObserver && constructor === browserWindow.MutationObserver) { // Anterior Zone.js versions (used in Angular 2) does not expose the original MutationObserver // in the 'window' object. Luckily, the patched MutationObserver class is storing an original // instance in its properties[4]. Let's get the original MutationObserver constructor from @@ -61,11 +56,11 @@ export function getMutationObserverConstructor(): MutationObserverConstructor | // // [4] https://github.com/angular/zone.js/blob/v0.8.5/lib/common/utils.ts#L412 - const patchedInstance = new browserWindow.MutationObserver(noop) as any - const originalInstance = patchedInstance[zoneSymbol('originalInstance')] as - | { constructor: MutationObserverConstructor } - | undefined + const patchedInstance = new browserWindow.MutationObserver(noop) as { + originalInstance?: { constructor: MutationObserverConstructor } + } + const originalInstance = getZoneJsOriginalValue(patchedInstance, 'originalInstance') constructor = originalInstance && originalInstance.constructor } } From 25b3365654d78d76a14b5bde5f302c36ed9a72a0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Beno=C3=AEt=20Zugmeyer?= Date: Fri, 9 Dec 2022 11:41:13 +0100 Subject: [PATCH 4/4] =?UTF-8?q?=F0=9F=91=8C=F0=9F=9A=9A=20move=20stubZoneJ?= =?UTF-8?q?s=20in=20its=20own=20module?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- .../core/src/browser/addEventListener.spec.ts | 2 +- .../src/tools/getZoneJsOriginalValue.spec.ts | 2 +- packages/core/test/specHelper.ts | 29 ------------------- packages/core/test/stubZoneJs.ts | 29 +++++++++++++++++++ .../src/browser/domMutationObservable.spec.ts | 2 +- 5 files changed, 32 insertions(+), 32 deletions(-) create mode 100644 packages/core/test/stubZoneJs.ts diff --git a/packages/core/src/browser/addEventListener.spec.ts b/packages/core/src/browser/addEventListener.spec.ts index 1e4aedd0b5..ce4295ec56 100644 --- a/packages/core/src/browser/addEventListener.spec.ts +++ b/packages/core/src/browser/addEventListener.spec.ts @@ -1,4 +1,4 @@ -import { stubZoneJs } from '../../test/specHelper' +import { stubZoneJs } from '../../test/stubZoneJs' import { noop } from '../tools/utils' import { addEventListener, DOM_EVENT } from './addEventListener' diff --git a/packages/core/src/tools/getZoneJsOriginalValue.spec.ts b/packages/core/src/tools/getZoneJsOriginalValue.spec.ts index 4c0a8c7be1..960720c364 100644 --- a/packages/core/src/tools/getZoneJsOriginalValue.spec.ts +++ b/packages/core/src/tools/getZoneJsOriginalValue.spec.ts @@ -1,4 +1,4 @@ -import { stubZoneJs } from '../../test/specHelper' +import { stubZoneJs } from '../../test/stubZoneJs' import { getZoneJsOriginalValue } from './getZoneJsOriginalValue' import { noop } from './utils' diff --git a/packages/core/test/specHelper.ts b/packages/core/test/specHelper.ts index 49a8f638fc..142a3cd841 100644 --- a/packages/core/test/specHelper.ts +++ b/packages/core/test/specHelper.ts @@ -4,7 +4,6 @@ import { resetNavigationStart } from '../src/tools/timeUtils' import { buildUrl } from '../src/tools/urlPolyfill' import { noop, objectEntries, assign } from '../src/tools/utils' import type { BrowserWindowWithEventBridge } from '../src/transport' -import type { BrowserWindowWithZoneJs } from '../src/tools/getZoneJsOriginalValue' // to simulate different build env behavior export interface BuildEnvWindow { @@ -486,31 +485,3 @@ export function interceptRequests() { }, } } - -export function stubZoneJs() { - const browserWindow = window as BrowserWindowWithZoneJs - const restorers: Array<() => void> = [] - - function getSymbol(name: string) { - return `__zone_symbol__${name}` - } - - browserWindow.Zone = { __symbol__: getSymbol } - - return { - restore: () => { - delete browserWindow.Zone - restorers.forEach((restorer) => restorer()) - }, - getSymbol, - replaceProperty(target: Target, name: Name, replacement: Target[Name]) { - const original = target[name] - target[name] = replacement - ;(target as any)[getSymbol(name)] = original - restorers.push(() => { - delete (target as any)[getSymbol(name)] - target[name] = original - }) - }, - } -} diff --git a/packages/core/test/stubZoneJs.ts b/packages/core/test/stubZoneJs.ts new file mode 100644 index 0000000000..53242c2d23 --- /dev/null +++ b/packages/core/test/stubZoneJs.ts @@ -0,0 +1,29 @@ +import type { BrowserWindowWithZoneJs } from '../src/tools/getZoneJsOriginalValue' + +export function stubZoneJs() { + const browserWindow = window as BrowserWindowWithZoneJs + const restorers: Array<() => void> = [] + + function getSymbol(name: string) { + return `__zone_symbol__${name}` + } + + browserWindow.Zone = { __symbol__: getSymbol } + + return { + restore: () => { + delete browserWindow.Zone + restorers.forEach((restorer) => restorer()) + }, + getSymbol, + replaceProperty(target: Target, name: Name, replacement: Target[Name]) { + const original = target[name] + target[name] = replacement + ;(target as any)[getSymbol(name)] = original + restorers.push(() => { + delete (target as any)[getSymbol(name)] + target[name] = original + }) + }, + } +} diff --git a/packages/rum-core/src/browser/domMutationObservable.spec.ts b/packages/rum-core/src/browser/domMutationObservable.spec.ts index 5ad5bf61c4..a78973dd28 100644 --- a/packages/rum-core/src/browser/domMutationObservable.spec.ts +++ b/packages/rum-core/src/browser/domMutationObservable.spec.ts @@ -1,5 +1,5 @@ import { isIE } from '@datadog/browser-core' -import { stubZoneJs } from '../../../core/test/specHelper' +import { stubZoneJs } from '../../../core/test/stubZoneJs' import { createDOMMutationObservable, getMutationObserverConstructor } from './domMutationObservable' // The MutationObserver invokes its callback in an event loop microtask, making this asynchronous.