fix(instrumentation-user-interaction): support clicks in React apps #537
Conversation
Codecov Report
@@ Coverage Diff @@
## main #537 +/- ##
==========================================
+ Coverage 94.82% 94.87% +0.04%
==========================================
Files 157 157
Lines 9862 9867 +5
Branches 957 963 +6
==========================================
+ Hits 9352 9361 +9
+ Misses 510 506 -4
|
| if (event) { | ||
| parentSpan = plugin._eventsSpanMap.get(event); | ||
| } | ||
| if (once) { | ||
| plugin.removePatchedListener(this, type, listener); | ||
| } | ||
| const span = plugin._createSpan(target, type, parentSpan); | ||
| const span = | ||
| target instanceof HTMLElement |
There was a problem hiding this comment.
_createSpan already has a number of "does this have enough information to produce a good span? -> if not, undefined" checks, and I think this one belongs inside _createSpan rather than outside it, to keep said logic centralized.
| let span: api.Span | undefined; | ||
| const activeZone = this; | ||
| if (target) { | ||
| if (target instanceof HTMLElement) { |
There was a problem hiding this comment.
See previous comment - if _createSpan does this check, it doesn't need to be repeated elsewhere.
|
|
||
| try { | ||
| btn1.click(); | ||
| btn2.click(); |
There was a problem hiding this comment.
I appreciate this test (having two targets and ensuring that the span names properly line up)!
| @@ -40,6 +40,12 @@ const FILE_URL = | |||
| 'https://raw.githubusercontent.com/open-telemetry/opentelemetry-js/main/package.json'; | |||
|
|
|||
| describe('UserInteractionInstrumentation', () => { | |||
| afterEach(() => { | |||
| while (document.body.lastChild) { | |||
| document.body.removeChild(document.body.lastChild); | |||
There was a problem hiding this comment.
I'm not sure why this is necessary? Perhaps a comment?
Which problem is this PR solving?
This fixes #168.
React <17 listens on
document. It didn't work previously, because we were only listening onHTMLElementinstances.React >=17 listens on a page root (usually a div or
document.body), so all spans had the same xpath.Short description of the changes
This PR changes current behaviour.
The good news is that the current behaviour is undocumented - I didn't have to change existing unit tests.
Previously the element on which the listener has been added was taken into a span (in
div.addEventListener- thedivwas always in span's xpath, not the clicked child).After this change, the
event.targetwill be taken (the actually clicked element).