fix(user-interaction): EventTarget is undefined in IE #627
Conversation
Codecov Report
@@ Coverage Diff @@
## main #627 +/- ##
==========================================
+ Coverage 96.82% 97.75% +0.93%
==========================================
Files 9 7 -2
Lines 630 357 -273
Branches 124 56 -68
==========================================
- Hits 610 349 -261
+ Misses 20 8 -12
|
plugins/web/opentelemetry-instrumentation-user-interaction/src/instrumentation.ts
Outdated
Show resolved
Hide resolved
|
Don't think this can be tested with just adding an unit test but rather this requires a full cross-browser testing setup (as it's dependent on the browser / running environment). In fact the current tests, only ran in IE, would catch this issue as they fail due to non-existant EventTarget |
|
@obecny was working on multi-browser testing but i'm not sure what the current state is on that |
|
I'd like to wait for @obecny approval since he wrote this plugin |
There was a problem hiding this comment.
Don't think this can be tested with just adding an unit test but rather this requires a full cross-browser testing setup (as it's dependent on the browser / running environment). In fact the current tests, only ran in IE, would catch this issue as they fail due to non-existant EventTarget
you can simply do a unit tests with mocking window object so that you can simulate this and then make sure it was called.
plugins/web/opentelemetry-instrumentation-user-interaction/src/instrumentation.ts
Outdated
Show resolved
Hide resolved
| this._unwrap(EventTarget.prototype, 'removeEventListener'); | ||
| api.diag.debug( | ||
| 'removing previous patch from method removeEventListener' | ||
| EVENT_TARGETS.forEach(target => { |
There was a problem hiding this comment.
IE doesn't have addEventListener() rather we should be using attachEvent(both of which hang off window (for IE)).
I think it would be better to check for the existence of "attachEvent" and if so wrap as before otherwise wrap addEventListener.
There was a problem hiding this comment.
Above is assuming that we want to support IE <= 8
There was a problem hiding this comment.
I don't think we need to worry about anything below ie11 tbh.
| * Document -> Node * -> EventTarget **! -> Object | ||
| * Window * -> WindowProperties ! -> EventTarget **! -> Object | ||
| */ | ||
| const EVENT_TARGETS = window.EventTarget |
There was a problem hiding this comment.
As isWrapped handles detection of whether a function has already been wrapped, why not just always list the entry points that we want to wrap, rather than the split (which really would require tests to be run against IE to ensure this doesn't get broken).
There was a problem hiding this comment.
This also assumes (which is generally true), that the root object names "Node", "Window" "EventTarget" always exist in all runtimes (which they may not).
Would suggest that we have an additional "wrap" function that can take the target and funcName so that it can do the following and wrap the correct level.
- Check if
targetis not null/undefined (if true -- nothing to wrap => abort wrap) - Check if
target[funcName]is defined (effectively check for instance level override) (if true wrap instance function (for wrapping simple polyfill's); if false check next) - Get
prototypeoftarget(this can be tricky for all environments) - Check if
targetPrototype[funcName]exists (if true wrap; otherwise abort wrap)
Examples of getting the correct target are in the ApplicationInsights code I wrote, this is more than what OT needs as it supports a larger set of runtimes (ie. all browsers (IE7+), Node, React, Angular, ReactNative and variants etc.) https://github.com/microsoft/ApplicationInsights-JS/blob/master/shared/AppInsightsCore/src/JavaScriptSDK/InstrumentHooks.ts#L152-L181
There was a problem hiding this comment.
Just tracked through the this._wrap which is really the shimmer project dependency, it does almost all of the above except for the prototype lookup if it's not defined on the target. It also has a dependency on Object.defineProperty (which I assume is not the only place in the IT code 😄 ) so my previous comment about IE <= 8 is mute as this just won't work anyway.
There was a problem hiding this comment.
Considering opentelemetry's support for browsers ("should work in currently supported versions of major browsers.") and microsoft's lifecycle policy for IE I think it's fair to let older versions of IE rest and sometime next century (optimistic, I know) maybe this and IE 11 can be thrown away.
This also assumes (which is generally true), that the root object names "Node", "Window" "EventTarget" always exist in all runtimes (which they may not).
Think a browser runtime that doesn't support DOM spec essentials like those has bigger issues than opentelemetry support, separate question if/how much browser instrumentations should protect the user from trying to run it in server side environments or expect users to know better than that
There was a problem hiding this comment.
ping @MSNev was your concerns resolved ?
There was a problem hiding this comment.
Think a browser runtime that doesn't support DOM spec essentials like those has bigger issues than opentelemetry support
It's not the browser runtimes you need to be aware of as they generally have them all, it's other native runtimes that "support" JavaScript...
maybe this and IE 11 can be thrown away.
I'm still waiting for IE7/8 to be thrown away and as part of the ApplicationInsights work we have to make sure it keeps working (mostly for the health industry) as they tend to lag and still have older versions of Windows and IE. Additionally, when you run IE 11 as an "embedded" browser (create a C# app and drop a browser component on it), by default it runs in IE7 mode.
I haven't checked if you have created separate tests (I suspect not, based on comments and the runtime), so this would mean that this comment is still valid
why not just always list the entry points that we want to wrap, rather than the split (which really would require tests to be run against IE to ensure this doesn't get broken).
So this really means that we are adding a code path this is never tested (the primary concern)
There was a problem hiding this comment.
@MSNev what do you think should be changed and how so that we can proceed with this PR further. In current state do you think this PR provides any regression or still adds some value comparing to what we have now - even if some tests are missing ?
There was a problem hiding this comment.
Not seeing any regression -- signed off
plugins/web/opentelemetry-instrumentation-user-interaction/test/userInteraction.nozone.test.ts
Show resolved
Hide resolved
Which problem is this PR solving?
#537 changed the patched interface from HTMLElement to EventTarget. Problem is EventTarget doesn't exist in Internet Explorer (11)
Instead addEventListener & removeEventListener are provided on interface closest to where EventTarget would be on prototype chain:
(for comparison same code in chrome confirming EventTarget on modern browsers)
Short description of the changes
Added check if browser has EventTarget, using a list of alternative targets if not.
There is a small difference between IE and modern browsers as EventTarget is used by more interfaces, this difference could be removed by using a list of targets as zone.js uses but I'm not seeing any of those being a target of user interactions so don't think it's worth it for now.