Skip to content

fix(snaptshot): Ensure attr.name is defined when collecting element attributes#160

Merged
Lms24 merged 2 commits intosentry-v2from
lms/fix-attr.name-undefined
Jan 25, 2024
Merged

fix(snaptshot): Ensure attr.name is defined when collecting element attributes#160
Lms24 merged 2 commits intosentry-v2from
lms/fix-attr.name-undefined

Conversation

@Lms24
Copy link
Member

@Lms24 Lms24 commented Jan 23, 2024

Probably fixes getsentry/sentry-javascript#10292

Not sure how attr.name could be undefined here but according to the issue, it happens and we should probably guard adding the attribute.

Open question (see my comment)

// Looks like `attr.name` can be undefined although the types say differently
// see: https://github.com/getsentry/sentry-javascript/issues/10292
if (attr.name && !ignoreAttribute(tagName, attr.name, attr.value)) {
attributes[attr.name] = transformAttribute(
Copy link
Member Author

Choose a reason for hiding this comment

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

just noticed this: Should we also lower-case the attribute here?

Copy link
Member

Choose a reason for hiding this comment

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

not sure 🤔 cc @billyvg ?

Copy link
Member

Choose a reason for hiding this comment

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

I would leave as-is, seems like a risky change. Just looked around and see that we do access keys that are now lower cased: https://github.com/getsentry/rrweb/pull/160/files#diff-6434d77e679a3d1e4217dcad03ee6f02551e41dc0b077a06a45020e68e1b4ee3L911

@github-actions
Copy link

size-limit report 📦

Path Size
rrweb - record only (gzipped) 16.69 KB (+0.04% 🔺)
rrweb - record & CanvasManager only (gzipped) 19.34 KB (+0.03% 🔺)
rrweb - record only (min) 56.95 KB (+0.02% 🔺)
rrweb - record with treeshaking flags (gzipped) 15.47 KB (+0.04% 🔺)

@Lms24 Lms24 merged commit 04b3de5 into sentry-v2 Jan 25, 2024
@Lms24 Lms24 deleted the lms/fix-attr.name-undefined branch January 25, 2024 13:00
billyvg pushed a commit that referenced this pull request Jan 30, 2024
… attributes (#160)

Probably fixes
getsentry/sentry-javascript#10292

Not sure how `attr.name` could be undefined here but according to the
issue, it happens and we should probably guard adding the attribute.
billyvg pushed a commit that referenced this pull request Apr 26, 2024
… attributes (#160)

Probably fixes
getsentry/sentry-javascript#10292

Not sure how `attr.name` could be undefined here but according to the
issue, it happens and we should probably guard adding the attribute.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Cannot read properties of undefined (reading 'toLowerCase') on mousemove

3 participants