feat: Add DOM collectors. #672
Conversation
|
@launchdarkly/js-sdk-common size report |
|
@launchdarkly/js-client-sdk-common size report |
|
@launchdarkly/js-client-sdk size report |
kinyoklion
force-pushed
the
rlamb/add-dom-collectors
branch
from
163548a
to
1530c61
Compare
packages/telemetry/browser-telemetry/src/collectors/dom/KeypressCollector.ts
Show resolved
Hide resolved
packages/telemetry/browser-telemetry/src/collectors/dom/KeypressCollector.ts
Outdated
Show resolved
Hide resolved
packages/telemetry/browser-telemetry/src/collectors/dom/KeypressCollector.ts
Show resolved
Hide resolved
| type: 'input', | ||
| level: 'info', | ||
| timestamp: Date.now(), | ||
| message: toSelector(target), |
There was a problem hiding this comment.
Is the target the focused element? In the cases where the target is undefined, is a breadcrumb without the message not useful? Perhaps it being undefined is rare.
There was a problem hiding this comment.
All the checks are just for safety as crazy things happen in a browser. If the target is undefined we cannot really do much about that. It would probably be a bug in the browser.
packages/telemetry/browser-telemetry/src/collectors/dom/KeypressCollector.ts
Outdated
Show resolved
Hide resolved
packages/telemetry/browser-telemetry/src/collectors/dom/KeypressCollector.ts
Outdated
Show resolved
Hide resolved
| } = { maxDepth: 10 }, | ||
| ): string { | ||
| // For production we may want to consider if we additionally limit the maximum selector length. | ||
| // Limiting the components should generate reasonable selectors in most cases. |
There was a problem hiding this comment.
Could limiting length lead to a situation where someone changes an element name and that results in one less level of element depth? Is it possible customers will attach critical infrastructure to the telemetry? I'm not experienced enough with it.
There was a problem hiding this comment.
So, these are for breadcrumbs. Breadcrumbs are associated data to help you understand things involved in the path getting to some event.
Generally speaking these aren't really for programmatic use, but for visual inspection and search.
That said someone always can use them that way.
Because they are primarily for human consumption I have focused on readability.
There was a problem hiding this comment.
I think I am going to defer a length decision for now. Basically you would need some pretty wild class names and things to make is a concrete problem.
| // https://developer.mozilla.org/en-US/docs/Web/CSS/Child_combinator | ||
| const CHILD_COMBINATOR = '>'; | ||
| // Spacing around the combinator is optional, but it increases readability. | ||
| const CHILD_SEPARATOR = ` ${CHILD_COMBINATOR} `; |
There was a problem hiding this comment.
Is removing the spaces worth it for additional length? This is sorta a followup to the code comment regarding length limiting.
There was a problem hiding this comment.
Response in other length related comment.
No description provided.