-
-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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(utils): Add function to extract relevant component name #9921
Conversation
size-limit report 📦
|
packages/utils/src/browser.ts
Outdated
@@ -86,6 +86,11 @@ function _htmlElementAsString(el: unknown, keyAttrs?: string[]): string { | |||
return ''; | |||
} | |||
|
|||
// If using the component name annotation plugin, this value may be available on the DOM node | |||
if (elem instanceof HTMLElement && elem.dataset && elem.dataset['sentryComponent']) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
l: We might have to check if HTMLElement
is defined globally and if this will break typescript types when folks are running in Node only environments (utils is technically meant to be isomorphic).
I guess we can't extract this util out because replay relies on it cc @mydea
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As per feedback from Luca, can we check if dom types are included when we add this change?
We can do that by looking at the type definitions of the built assets and checking for dom.d.ts
being included.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added some checks to prevent breaking types when running in Node 👍
|
||
let currentElem = elem as SimpleNode; | ||
const MAX_TRAVERSE_HEIGHT = 5; | ||
for (let i = 0; i < MAX_TRAVERSE_HEIGHT; i++) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The issue with this traversing is that it might pick the wrong element, maybe we should do 2-3 to be even more conservative.
But sticky with 5 sounds fine, we can test more in different codebases and see what it looks like.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I left a comment on the first PR talking about this as well. Looking to test it with 5 at the moment, and we can decrease the value later on if we find that it is too aggressive
Adds a utility function to get the frontend component name from a given DOM element. It also adjusts
htmlTreeAsString
to use the component name when applicable, when building out the string.This is one part of this PR, splitting it up into multiple smaller PRs scoped to specific features:
#9855