From 2c3eb709b3a4b5c158339f31871a60ddc2e952c9 Mon Sep 17 00:00:00 2001 From: Rafael Audibert <32079912+rafaeelaudibert@users.noreply.github.com> Date: Wed, 29 Jan 2025 09:43:31 -0300 Subject: [PATCH] feat: Add new rule to guarantee events are tracked as passive (#1689) --- .eslintrc.js | 2 + eslint-rules/custom-eslint-rules.test.js | 45 +++++++- eslint-rules/no-add-event-listener.js | 59 ++++++++++ playground/ie11/index.html | 25 ++++ .../nextjs/pages/replay-examples/canvas.tsx | 34 ++++-- src/autocapture.ts | 23 ++-- src/entrypoints/dead-clicks-autocapture.ts | 11 +- src/entrypoints/external-scripts-loader.ts | 3 + src/entrypoints/recorder.ts | 4 + src/extensions/replay/sessionrecording.ts | 9 +- src/extensions/surveys.tsx | 21 ++-- src/extensions/toolbar.ts | 4 +- src/heatmaps.ts | 12 +- src/posthog-core.ts | 18 ++- src/retry-queue.ts | 7 +- src/scroll-manager.ts | 11 +- src/sessionid.ts | 17 ++- src/utils/index.ts | 108 +++--------------- 18 files changed, 256 insertions(+), 157 deletions(-) create mode 100644 eslint-rules/no-add-event-listener.js create mode 100644 playground/ie11/index.html diff --git a/.eslintrc.js b/.eslintrc.js index 294db20a3..0d78895c1 100644 --- a/.eslintrc.js +++ b/.eslintrc.js @@ -89,7 +89,9 @@ module.exports = { rules: { 'prettier/prettier': 'error', '@typescript-eslint/no-var-requires': 'off', + '@typescript-eslint/no-require-imports': 'off', 'posthog-js/no-direct-null-check': 'off', + 'posthog-js/no-direct-boolean-check': 'off', }, env: { node: true, diff --git a/eslint-rules/custom-eslint-rules.test.js b/eslint-rules/custom-eslint-rules.test.js index 1717e5f49..654192103 100644 --- a/eslint-rules/custom-eslint-rules.test.js +++ b/eslint-rules/custom-eslint-rules.test.js @@ -7,10 +7,19 @@ const noDirectStringCheck = require('./no-direct-string-check') const noDirectDateCheck = require('./no-direct-date-check') const noDirectNumberCheck = require('./no-direct-number-check') const noDirectBooleanCheck = require('./no-direct-boolean-check') +const noAddEventListener = require('./no-add-event-listener') const { RuleTester } = require('eslint') -const ruleTester = new RuleTester() +const ruleTester = new RuleTester({ + parserOptions: { + ecmaVersion: 2015, + sourceType: 'module', + }, + env: { + browser: true, + }, +}) ruleTester.run('no-direct-null-check', noDirectNullCheck, { valid: [{ code: `isNull(x)` }], @@ -122,3 +131,37 @@ ruleTester.run('no-direct-boolean-check', noDirectBooleanCheck, { }, ], }) + +ruleTester.run('no-add-event-listener', noAddEventListener, { + valid: [ + { code: "addEventListener(document, 'click', () => {}, { passive: true })" }, + { code: "addEventListener(window, 'scroll', () => {}, { capture: true, passive: true })" }, + ], + invalid: [ + { + code: "document.addEventListener('mouseleave', () => {})", + errors: [{ message: 'Use addEventListener from @utils instead of calling it directly on elements' }], + output: "import { addEventListener } from './utils'\naddEventListener(document, 'mouseleave', () => {})", + }, + { + code: "element.addEventListener('click', () => {}, true)", + errors: [{ message: 'Use addEventListener from @utils instead of calling it directly on elements' }], + output: "import { addEventListener } from './utils'\naddEventListener(element, 'click', () => {}, { capture: true })", + }, + { + code: "window.addEventListener('click', () => {}, {})", + errors: [{ message: 'Use addEventListener from @utils instead of calling it directly on elements' }], + output: "import { addEventListener } from './utils'\naddEventListener(window, 'click', () => {}, {})", + }, + { + code: "document.addEventListener('pageleave', () => {}, { capture: true })", + errors: [{ message: 'Use addEventListener from @utils instead of calling it directly on elements' }], + output: "import { addEventListener } from './utils'\naddEventListener(document, 'pageleave', () => {}, { capture: true })", + }, + { + code: "document.addEventListener('pageleave', () => {}, { capture: false })", + errors: [{ message: 'Use addEventListener from @utils instead of calling it directly on elements' }], + output: "import { addEventListener } from './utils'\naddEventListener(document, 'pageleave', () => {}, { capture: false })", + }, + ], +}) diff --git a/eslint-rules/no-add-event-listener.js b/eslint-rules/no-add-event-listener.js new file mode 100644 index 000000000..a6c539ae2 --- /dev/null +++ b/eslint-rules/no-add-event-listener.js @@ -0,0 +1,59 @@ +module.exports = { + meta: { + type: 'suggestion', + docs: { + description: 'Enforce usage of addEventListener from @utils instead of native addEventListener', + category: 'Best Practices', + recommended: true, + }, + fixable: 'code', + schema: [], // no options + }, + + create(context) { + return { + CallExpression(node) { + // Check if it's an addEventListener call + const callee = node.callee + if (callee.type === 'MemberExpression' && callee.property.name === 'addEventListener') { + context.report({ + node, + message: 'Use addEventListener from @utils instead of calling it directly on elements', + fix(fixer) { + // Get the element expression + const elementText = context.getSourceCode().getText(callee.object) + + // Get the event type + const eventText = context.getSourceCode().getText(node.arguments[0]) + + // Get the callback + const callbackText = context.getSourceCode().getText(node.arguments[1]) + + // Get options if they exist + const optionsText = + node.arguments[2] != null + ? context.getSourceCode().getText(node.arguments[2]) === 'true' + ? ', { capture: true }' + : `, ${context.getSourceCode().getText(node.arguments[2])}` + : '' + + // Add import if needed (note: this is a basic implementation, it won't always work) + const importFix = fixer.insertTextBefore( + context.getSourceCode().ast, + "import { addEventListener } from './utils'\n" + ) + + // Replace the call + const callFix = fixer.replaceText( + node, + `addEventListener(${elementText}, ${eventText}, ${callbackText}${optionsText})` + ) + + return [importFix, callFix] + }, + }) + } + }, + } + }, +} diff --git a/playground/ie11/index.html b/playground/ie11/index.html new file mode 100644 index 000000000..ae43259f2 --- /dev/null +++ b/playground/ie11/index.html @@ -0,0 +1,25 @@ + + + + + + + + + + +

This page triggers a simple event using E5 bundle

+
+ Did it work? Check console +
+
+ You can also look at `posthog.something` to check if listeners were properly added to the window/document + object, such as `posthog.scrollManager.scrollX()` +
+ + + \ No newline at end of file diff --git a/playground/nextjs/pages/replay-examples/canvas.tsx b/playground/nextjs/pages/replay-examples/canvas.tsx index f24e4a98a..86dd19e3d 100644 --- a/playground/nextjs/pages/replay-examples/canvas.tsx +++ b/playground/nextjs/pages/replay-examples/canvas.tsx @@ -84,21 +84,33 @@ export default function Canvas() { } } - canvas.addEventListener('mousemove', function (e) { - focalLength = e.x - }) + canvas.addEventListener( + 'mousemove', + function (e) { + focalLength = e.x + }, + { passive: true } + ) // Kick off the animation when the mouse enters the canvas - canvas.addEventListener('mouseover', function () { - animate = true - executeFrame() - }) + canvas.addEventListener( + 'mouseover', + function () { + animate = true + executeFrame() + }, + { passive: true } + ) // Pause animation when the mouse exits the canvas - canvas.addEventListener('mouseout', function () { - // mouseDown = false - animate = false - }) + canvas.addEventListener( + 'mouseout', + function () { + // mouseDown = false + animate = false + }, + { passive: true } + ) initializeStars() // Draw the first frame to start animation diff --git a/src/autocapture.ts b/src/autocapture.ts index 917a4a788..41bf407dd 100644 --- a/src/autocapture.ts +++ b/src/autocapture.ts @@ -1,4 +1,4 @@ -import { each, extend, registerEvent } from './utils' +import { addEventListener, each, extend } from './utils' import { autocaptureCompatibleElements, getClassNames, @@ -258,6 +258,7 @@ export class Autocapture { if (!window || !document) { return } + const handler = (e: Event) => { e = e || window?.event try { @@ -267,18 +268,18 @@ export class Autocapture { } } - const copiedTextHandler = (e: Event) => { - e = e || window?.event - this._captureEvent(e, COPY_AUTOCAPTURE_EVENT) - } - - registerEvent(document, 'submit', handler, false, true) - registerEvent(document, 'change', handler, false, true) - registerEvent(document, 'click', handler, false, true) + addEventListener(document, 'submit', handler, { capture: true }) + addEventListener(document, 'change', handler, { capture: true }) + addEventListener(document, 'click', handler, { capture: true }) if (this.config.capture_copied_text) { - registerEvent(document, 'copy', copiedTextHandler, false, true) - registerEvent(document, 'cut', copiedTextHandler, false, true) + const copiedTextHandler = (e: Event) => { + e = e || window?.event + this._captureEvent(e, COPY_AUTOCAPTURE_EVENT) + } + + addEventListener(document, 'copy', copiedTextHandler, { capture: true }) + addEventListener(document, 'cut', copiedTextHandler, { capture: true }) } } diff --git a/src/entrypoints/dead-clicks-autocapture.ts b/src/entrypoints/dead-clicks-autocapture.ts index 7c31754e6..b62033a38 100644 --- a/src/entrypoints/dead-clicks-autocapture.ts +++ b/src/entrypoints/dead-clicks-autocapture.ts @@ -6,6 +6,7 @@ import { DeadClickCandidate, DeadClicksAutoCaptureConfig, Properties } from '../ import { autocapturePropertiesForElement } from '../autocapture' import { isElementInToolbar, isElementNode, isTag } from '../utils/element-utils' import { getNativeMutationObserverImplementation } from '../utils/prototype-utils' +import { addEventListener } from '../utils' function asClick(event: MouseEvent): DeadClickCandidate | null { const eventTarget = getEventTarget(event) @@ -98,11 +99,11 @@ class LazyLoadedDeadClicksAutocapture implements LazyLoadedDeadClicksAutocapture } private _startClickObserver() { - assignableWindow.addEventListener('click', this._onClick) + addEventListener(assignableWindow, 'click', this._onClick) } - private _onClick = (event: MouseEvent): void => { - const click = asClick(event) + private _onClick = (event: Event): void => { + const click = asClick(event as MouseEvent) if (!isNull(click) && !this._ignoreClick(click)) { this._clicks.push(click) } @@ -122,7 +123,7 @@ class LazyLoadedDeadClicksAutocapture implements LazyLoadedDeadClicksAutocapture // This allows the browser to optimize scrolling performance by not waiting for our handling of the scroll event // see https://developer.mozilla.org/en-US/docs/Web/API/EventTarget/addEventListener#passive private _startScrollObserver() { - assignableWindow.addEventListener('scroll', this._onScroll, { capture: true, passive: true }) + addEventListener(assignableWindow, 'scroll', this._onScroll, { capture: true }) } private _onScroll = (): void => { @@ -141,7 +142,7 @@ class LazyLoadedDeadClicksAutocapture implements LazyLoadedDeadClicksAutocapture } private _startSelectionChangedObserver() { - assignableWindow.addEventListener('selectionchange', this._onSelectionChange) + addEventListener(assignableWindow, 'selectionchange', this._onSelectionChange) } private _onSelectionChange = (): void => { diff --git a/src/entrypoints/external-scripts-loader.ts b/src/entrypoints/external-scripts-loader.ts index f1d6b8322..0b0917ecd 100644 --- a/src/entrypoints/external-scripts-loader.ts +++ b/src/entrypoints/external-scripts-loader.ts @@ -41,6 +41,9 @@ const loadScript = (posthog: PostHog, url: string, callback: (error?: string | E if (document?.body) { addScript() } else { + // Inlining this because we don't care about `passive: true` here + // and this saves us ~3% of the bundle size + // eslint-disable-next-line posthog-js/no-add-event-listener document?.addEventListener('DOMContentLoaded', addScript) } } diff --git a/src/entrypoints/recorder.ts b/src/entrypoints/recorder.ts index cade379f9..7d2761960 100644 --- a/src/entrypoints/recorder.ts +++ b/src/entrypoints/recorder.ts @@ -278,6 +278,9 @@ function initXhrObserver(cb: networkCallback, win: IWindow, options: Required { if (xhr.readyState !== xhr.DONE) { return @@ -325,6 +328,7 @@ function initXhrObserver(cb: networkCallback, win: IWindow, options: Required { + + addEventListener(selectorOnPage, 'click', () => { if (surveyPopup) { surveyPopup.style.display = surveyPopup.style.display === 'none' ? 'block' : 'none' - surveyPopup.addEventListener('PHSurveyClosed', () => { + addEventListener(surveyPopup, 'PHSurveyClosed', () => { this.removeSurveyFromFocus(survey.id) surveyPopup.style.display = 'none' }) } }) + selectorOnPage.setAttribute('PHWidgetSurveyClickListener', 'true') } } @@ -435,8 +438,8 @@ export function usePopupVisibility( } } - window.addEventListener('PHSurveyClosed', handleSurveyClosed) - window.addEventListener('PHSurveySent', handleSurveySent) + addEventListener(window, 'PHSurveyClosed', handleSurveyClosed) + addEventListener(window, 'PHSurveySent', handleSurveySent) if (millisecondDelay > 0) { return handleShowSurveyWithDelay() @@ -562,9 +565,7 @@ export function SurveyPopup({ /> )} - ) : ( - <> - ) + ) : null } export function Questions({ @@ -725,10 +726,12 @@ export function FeedbackWidget({ } } if (survey.appearance?.widgetType === 'selector') { - const widget = document.querySelector(survey.appearance.widgetSelector || '') - widget?.addEventListener('click', () => { + const widget = document.querySelector(survey.appearance.widgetSelector || '') ?? undefined + + addEventListener(widget, 'click', () => { setShowSurvey(!showSurvey) }) + widget?.setAttribute('PHWidgetSurveyClickListener', 'true') } }, []) diff --git a/src/extensions/toolbar.ts b/src/extensions/toolbar.ts index 644b3fd24..0e3c7090c 100644 --- a/src/extensions/toolbar.ts +++ b/src/extensions/toolbar.ts @@ -1,4 +1,4 @@ -import { registerEvent, trySafe } from '../utils' +import { addEventListener, trySafe } from '../utils' import { PostHog } from '../posthog-core' import { ToolbarParams } from '../types' import { _getHashParam } from '../utils/request-utils' @@ -178,7 +178,7 @@ export class Toolbar { // Turbolinks doesn't fire an onload event but does replace the entire body, including the toolbar. // Thus, we ensure the toolbar is only loaded inside the body, and then reloaded on turbolinks:load. - registerEvent(window, 'turbolinks:load', () => { + addEventListener(window, 'turbolinks:load', () => { this.setToolbarState(ToolbarState.UNINITIALIZED) this.loadToolbar(toolbarParams) }) diff --git a/src/heatmaps.ts b/src/heatmaps.ts index db976ed4e..b708a369c 100644 --- a/src/heatmaps.ts +++ b/src/heatmaps.ts @@ -1,4 +1,3 @@ -import { registerEvent } from './utils' import RageClick from './extensions/rageclick' import { DeadClickCandidate, Properties, RemoteConfig } from './types' import { PostHog } from './posthog-core' @@ -11,6 +10,7 @@ import { createLogger } from './utils/logger' import { isElementInToolbar, isElementNode, isTag } from './utils/element-utils' import { DeadClicksAutocapture, isDeadClicksEnabledForHeatmaps } from './extensions/dead-clicks-autocapture' import { includes } from './utils/string-utils' +import { addEventListener } from './utils' const DEFAULT_FLUSH_INTERVAL = 5000 @@ -60,9 +60,7 @@ export class Heatmaps { this.instance = instance this._enabledServerSide = !!this.instance.persistence?.props[HEATMAPS_ENABLED_SERVER_SIDE] - window?.addEventListener('beforeunload', () => { - this.flush() - }) + addEventListener(window, 'beforeunload', this.flush) } public get flushIntervalMilliseconds(): number { @@ -132,8 +130,10 @@ export class Heatmaps { return } - registerEvent(document, 'click', (e) => this._onClick((e || window?.event) as MouseEvent), false, true) - registerEvent(document, 'mousemove', (e) => this._onMouseMove((e || window?.event) as MouseEvent), false, true) + addEventListener(document, 'click', (e) => this._onClick((e || window?.event) as MouseEvent), { capture: true }) + addEventListener(document, 'mousemove', (e) => this._onMouseMove((e || window?.event) as MouseEvent), { + capture: true, + }) this.deadClicksCapture = new DeadClicksAutocapture( this.instance, diff --git a/src/posthog-core.ts b/src/posthog-core.ts index 1affca54b..f3b160601 100644 --- a/src/posthog-core.ts +++ b/src/posthog-core.ts @@ -4,9 +4,9 @@ import { each, eachArray, extend, - registerEvent, safewrapClass, isCrossDomainCookie, + addEventListener, } from './utils' import { assignableWindow, document, location, navigator, userAgent, window } from './utils/globals' import { PostHogFeatureFlags } from './posthog-featureflags' @@ -552,7 +552,11 @@ export class PostHog { } // Set up event handler for pageleave // Use `onpagehide` if available, see https://calendar.perfplanet.com/2020/beaconing-in-practice/#beaconing-reliability-avoiding-abandons - window?.addEventListener?.('onpagehide' in self ? 'pagehide' : 'unload', this._handle_unload.bind(this)) + // + // Not making it passive to try and force the browser to handle this before the page is unloaded + addEventListener(window, 'onpagehide' in self ? 'pagehide' : 'unload', this._handle_unload.bind(this), { + passive: false, + }) this.toolbar.maybeLoadToolbar() @@ -2150,13 +2154,17 @@ const add_dom_loaded_handler = function () { // 'loaded' is an IE thing dom_loaded_handler() } else { - document.addEventListener('DOMContentLoaded', dom_loaded_handler, false) + addEventListener(document, 'DOMContentLoaded', dom_loaded_handler, { capture: false }) } + + return } - // fallback handler, always will work + // Only IE6-8 don't support `document.addEventListener` and we don't support them + // so let's simply log an error stating PostHog couldn't be initialized + // We're checking for `window` to avoid erroring out on a SSR context if (window) { - registerEvent(window, 'load', dom_loaded_handler, true) + logger.error("Browser doesn't support `document.addEventListener` so PostHog couldn't be initialized") } } diff --git a/src/retry-queue.ts b/src/retry-queue.ts index 129090356..b21ab2263 100644 --- a/src/retry-queue.ts +++ b/src/retry-queue.ts @@ -5,6 +5,7 @@ import { logger } from './utils/logger' import { window } from './utils/globals' import { PostHog } from './posthog-core' import { extendURLParams } from './request' +import { addEventListener } from './utils' const thirtyMinutes = 30 * 60 * 1000 @@ -45,11 +46,13 @@ export class RetryQueue { if (!isUndefined(window) && 'onLine' in window.navigator) { this.areWeOnline = window.navigator.onLine - window.addEventListener('online', () => { + + addEventListener(window, 'online', () => { this.areWeOnline = true this.flush() }) - window.addEventListener('offline', () => { + + addEventListener(window, 'offline', () => { this.areWeOnline = false }) } diff --git a/src/scroll-manager.ts b/src/scroll-manager.ts index c2a1849db..90677c216 100644 --- a/src/scroll-manager.ts +++ b/src/scroll-manager.ts @@ -1,6 +1,7 @@ import { window } from './utils/globals' import { PostHog } from './posthog-core' import { isArray } from './utils/type-utils' +import { addEventListener } from './utils' export interface ScrollContext { // scroll is how far down the page the user has scrolled, @@ -59,14 +60,10 @@ export class ScrollManager { // `capture: true` is required to get scroll events for other scrollable elements // on the page, not just the window // see https://developer.mozilla.org/en-US/docs/Web/API/EventTarget/addEventListener#usecapture - // - // `passive: true` is used to tell the browser that the scroll event handler will not call `preventDefault()` - // This allows the browser to optimize scrolling performance by not waiting for our handling of the scroll event - // see https://developer.mozilla.org/en-US/docs/Web/API/EventTarget/addEventListener#passive startMeasuringScrollPosition() { - window?.addEventListener('scroll', this._updateScrollData, { capture: true, passive: true }) - window?.addEventListener('scrollend', this._updateScrollData, { capture: true, passive: true }) - window?.addEventListener('resize', this._updateScrollData, { passive: true }) + addEventListener(window, 'scroll', this._updateScrollData, { capture: true }) + addEventListener(window, 'scrollend', this._updateScrollData, { capture: true }) + addEventListener(window, 'resize', this._updateScrollData) } public scrollElement(): Element | undefined { diff --git a/src/sessionid.ts b/src/sessionid.ts index 07f4d82a0..2ae7fc180 100644 --- a/src/sessionid.ts +++ b/src/sessionid.ts @@ -10,6 +10,7 @@ import { createLogger } from './utils/logger' import { clampToRange } from './utils/number-utils' import { PostHog } from './posthog-core' +import { addEventListener } from './utils' const logger = createLogger('[SessionId]') @@ -198,11 +199,17 @@ export class SessionIdManager { * We conditionally check the primaryWindowExists value in the constructor to decide if the window id in the last session storage should be carried over. */ private _listenToReloadWindow(): void { - window?.addEventListener('beforeunload', () => { - if (this._canUseSessionStorage()) { - sessionStore.remove(this._primary_window_exists_storage_key) - } - }) + addEventListener( + window, + 'beforeunload', + () => { + if (this._canUseSessionStorage()) { + sessionStore.remove(this._primary_window_exists_storage_key) + } + }, + // Not making it passive to try and force the browser to handle this before the page is unloaded + { capture: false } + ) } /* diff --git a/src/utils/index.ts b/src/utils/index.ts index 2a6b8b90f..10ba3bf3c 100644 --- a/src/utils/index.ts +++ b/src/utils/index.ts @@ -1,7 +1,7 @@ -import { Breaker, EventHandler, Properties } from '../types' -import { hasOwnProperty, isArray, isFormData, isFunction, isNull, isNullish, isString } from './type-utils' +import { Breaker, Properties } from '../types' +import { hasOwnProperty, isArray, isFormData, isNull, isNullish, isString } from './type-utils' import { logger } from './logger' -import { nativeForEach, nativeIndexOf, window } from './globals' +import { nativeForEach, nativeIndexOf } from './globals' const breaker: Breaker = {} @@ -208,92 +208,6 @@ export function _copyAndTruncateStrings = Record(value: T[], predicate: (value: T) => boolean): T | undef } return undefined } + +// Use this instead of element.addEventListener to avoid eslint errors +// this properly implements the default options for passive event listeners +export function addEventListener( + element: Window | Document | Element | undefined, + event: string, + callback: EventListener, + options?: AddEventListenerOptions +): void { + const { capture = false, passive = true } = options ?? {} + + // This is the only place where we are allowed to call this function + // because the whole idea is that we should be calling this instead of the built-in one + // eslint-disable-next-line posthog-js/no-add-event-listener + element?.addEventListener(event, callback, { capture, passive }) +}