fix: type error accessing null object and added test case #1620
Conversation
|
@supermar1010 is attempting to deploy a commit to the PostHog Team on Vercel. A member of the Team first needs to authorize it. |
src/utils/element-utils.ts
Outdated
| @@ -1,8 +1,8 @@ | |||
| import { TOOLBAR_CONTAINER_CLASS, TOOLBAR_ID } from '../constants' | |||
|
|
|||
| export function isElementInToolbar(el: Element): boolean { | |||
| export function isElementInToolbar(el: Element | null): boolean { | |||
| // NOTE: .closest is not supported in IE11 hence the operator check | |||
There was a problem hiding this comment.
totally fly-by note for myself that this comment is probably out of date now that we're making ES6 and ES5 bundles separately
| $heatmap_data: { | ||
| 'http://replaced/': [ | ||
| { | ||
| target_fixed: false, |
There was a problem hiding this comment.
so, does this mean that the mousemove event has no clientX or clientY?
if so we're actually better ignoring them completely since the data is invalid without a coordinate.
There was a problem hiding this comment.
Yes, it's a mouse move event without any clientX or clientY. According to mozilla dev everything is optional, so it is valid to send an event with any data.
Yeah ignoring is probably a good call!
There was a problem hiding this comment.
you're welcome to add that to this PR - if you're interested?
i can do as follow-up otherwise
There was a problem hiding this comment.
we're already dropping events with no x and y during ingestion, so it's only a nice-to-have not to omit them (although good to not do work)
so only open question for me is if you'd like to handle that in this PR?
There was a problem hiding this comment.
I think I'd leave that to you, I'm not sure where you'd implement that filtering.
pauldambra
changed the title
|
Unit tests should run successful now! |
Changes
We stumbled onto this bug, with rxdb dispatching mouse events without data and thus producing a posthog crash.
This typecast is why the error wasn't spotted by ts. The type of
e.targetis originallyEventTarget | null, but looses the| nullin this cast.Checklist