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

feat: Add new rule to guarantee events are tracked as passive #1689

Merged
merged 8 commits into from
Jan 29, 2025
2 changes: 2 additions & 0 deletions .eslintrc.js
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
45 changes: 44 additions & 1 deletion eslint-rules/custom-eslint-rules.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -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)` }],
Expand Down Expand Up @@ -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 })",
},
],
})
59 changes: 59 additions & 0 deletions eslint-rules/no-add-event-listener.js
Original file line number Diff line number Diff line change
@@ -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"
)
Comment on lines +40 to +44
Copy link
Member Author

Choose a reason for hiding this comment

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

Wondering if this makes any sense. Most often than not it'll be correct, but it will be a broken import sometimes

Maybe we shouldn't be autofixing this at all?

Copy link
Member

Choose a reason for hiding this comment

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

yeah, I'd leave the fix (if we can't detect the correct path). most folks' editors should be able to create the import on demand anyway

Copy link
Member Author

Choose a reason for hiding this comment

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

I'll see if eslint allows me to detect what file I'm in, if so, I'll keep it


// Replace the call
const callFix = fixer.replaceText(
node,
`addEventListener(${elementText}, ${eventText}, ${callbackText}${optionsText})`
)

return [importFix, callFix]
},
})
}
},
}
},
}
25 changes: 25 additions & 0 deletions playground/ie11/index.html
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
<html>

<head>
<!-- COPY AND PASTE FROM `dist/array.full.e5.js` TO TEST THIS -->
<script>
</script>

<script>
posthog.init('BrpS4SctoaCCsyjlnlun3OzyNJAafdlv__jUWaaJWXg', { api_host: 'https://us.posthog.com', debug: true })
posthog.capture("e5 event")
</script>
</head>

<body>
<h1>This page triggers a simple event using E5 bundle</h1>
<div>
Did it work? Check console
</div>
<div>
You can also look at `posthog.something` to check if listeners were properly added to the window/document
object, such as `posthog.scrollManager.scrollX()`
</div>
</body>

</html>
34 changes: 23 additions & 11 deletions playground/nextjs/pages/replay-examples/canvas.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
23 changes: 12 additions & 11 deletions src/autocapture.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { each, extend, registerEvent } from './utils'
import { addEventListener, each, extend } from './utils'
import {
autocaptureCompatibleElements,
getClassNames,
Expand Down Expand Up @@ -258,6 +258,7 @@ export class Autocapture {
if (!window || !document) {
return
}

const handler = (e: Event) => {
e = e || window?.event
try {
Expand All @@ -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 })
}
}

Expand Down
11 changes: 6 additions & 5 deletions src/entrypoints/dead-clicks-autocapture.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down Expand Up @@ -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)
}
Expand All @@ -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 => {
Expand All @@ -141,7 +142,7 @@ class LazyLoadedDeadClicksAutocapture implements LazyLoadedDeadClicksAutocapture
}

private _startSelectionChangedObserver() {
assignableWindow.addEventListener('selectionchange', this._onSelectionChange)
addEventListener(assignableWindow, 'selectionchange', this._onSelectionChange)
}

private _onSelectionChange = (): void => {
Expand Down
3 changes: 3 additions & 0 deletions src/entrypoints/external-scripts-loader.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}
}
Expand Down
4 changes: 4 additions & 0 deletions src/entrypoints/recorder.ts
Original file line number Diff line number Diff line change
Expand Up @@ -278,6 +278,9 @@ function initXhrObserver(cb: networkCallback, win: IWindow, options: Required<Ne
return originalSend(body)
}

// This is very tricky code, and making it passive won't bring many performance benefits,
// so let's ignore the rule here.
// eslint-disable-next-line posthog-js/no-add-event-listener
xhr.addEventListener('readystatechange', () => {
if (xhr.readyState !== xhr.DONE) {
return
Expand Down Expand Up @@ -325,6 +328,7 @@ function initXhrObserver(cb: networkCallback, win: IWindow, options: Required<Ne
//
})
})

originalOpen.call(xhr, method, url, async, username, password)
}
}
Expand Down
9 changes: 5 additions & 4 deletions src/extensions/replay/sessionrecording.ts
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,7 @@ import { gzipSync, strFromU8, strToU8 } from 'fflate'
import { clampToRange } from '../../utils/number-utils'
import Config from '../../config'
import { includes } from '../../utils/string-utils'
import { addEventListener } from '../../utils'

const LOGGER_PREFIX = '[SessionRecording]'
const logger = createLogger(LOGGER_PREFIX)
Expand Down Expand Up @@ -492,10 +493,10 @@ export class SessionRecording {
this._startCapture(startReason)

// calling addEventListener multiple times is safe and will not add duplicates
window?.addEventListener('beforeunload', this._onBeforeUnload)
window?.addEventListener('offline', this._onOffline)
window?.addEventListener('online', this._onOnline)
window?.addEventListener('visibilitychange', this._onVisibilityChange)
addEventListener(window, 'beforeunload', this._onBeforeUnload)
addEventListener(window, 'offline', this._onOffline)
addEventListener(window, 'online', this._onOnline)
addEventListener(window, 'visibilitychange', this._onVisibilityChange)

// on reload there might be an already sampled session that should be continued before decide response,
// so we call this here _and_ in the decide response
Expand Down
Loading
Loading