Handle ErrorEvents in TraceKit

[shcallaway]

This PR resolves #1008 . traceKitWindowOnError now handles the case where the ex parameter is an ErrorEvent object.

[michal-rumanek]

It doesn't cover the case where ErrorEvent is passed as message parameter, so it doesn't resolve #1008.

[shcallaway]

@michal-rumanek Woops. I misread the original comment. Fixed now.

[kamilogorek]

Other than this small change, it's ready to be shipped

[kamilogorek]

This is isError test, not isErrorEvent and should be moved to line ~122 w. additional supportsErrorEvent() clause, eg. if (supportsErrorEvent()) assert.isFalse(isError(new ErrorEvent('')));

[shcallaway]

Done!

[michal-rumanek]

@shcallaway Now the error is logged correctly in Sentry panel. Thank you :-)
But, not sure if _oldOnerrorHandler shouldn't be invoked with original message parameter (original ErrorEvent object, not string created by message = message.message).

[shcallaway]

@michal-rumanek The core developers would know best, probably. But I can see multiple ways of handling to the case where message is an ErrorEvent. For example, we could route it to the middle else if block by pulling the Error object out, like this:

if (ex && utils.isErrorEvent(ex)) {
  ex = ex.error;
} else if (message && utils.isErrorEvent(message)) {
  ex = message.error;
}

[michal-rumanek]

I think it's necessary, e.g. for the case when more than one Sentry global error handlers are installed (Raven.install()).
@kamilogorek what do you think?

[kamilogorek]

Good catch @michal-rumanek, we indeed shouldn't modify that's passed down to other handlers.

We also don't need to double check here:

if (ex && utils.isErrorEvent(ex)) {
  ex = ex.error;
} 

if (message && utils.isErrorEvent(message)) {
  message = message.message;
}

and we can write it like this instead

if (utils.isErrorEvent(ex)) ex = ex.error;
if (utils.isErrorEvent(message)) message = message.message;

as passing undefined to utils.isErrorEvent won't do any harm.

[shcallaway]

@michal-rumanek @kamilogorek I disagree about passing the original message to _oldOnerrorHandler. Clearly, the original developer assumed that message would be a string. Otherwise, this [object ErrorEvent] bug would not exist. So it makes sense to pass the string to _oldOnerrorHandler rather than the ErrorEvent.

[shcallaway]

@kamilogorek @michal-rumanek Can I get some feedback on this? My team can't use Sentry until we start seeing real errors instead of ErrorEvents.

[kamilogorek]

@shcallaway thanks for your patience, I was working on getsentry/sentry#6709 for last few days and had to move SDK aside for a while.

Let's get it merged asap.

Clearly, the original developer assumed that message would be a string.

But what if the original developer already handled (or uses a solution that does that) case like this? We'd change expected (well, not so expected :P) behaviour.

[kamilogorek]

@shcallaway I asked other devs and we'd prefer to stick with original arguments.
Can you please change it so that we can merge it Today? :)

[kamilogorek]

Fixed it myself, rebased and merged manually. Preserving correct author info of course :) 315dab5

Thanks for contributing!

[kamilogorek]

Released in 3.21.0

[shcallaway]

You’re the best. Thanks!

[michal-rumanek]

Thank you!

[enapupe]

Hey guys, after seeing the linked issue (#1008) I updated my raven version but I'm still receiving error reports as [object ErrorEvent]. The relevant info:

name	Chrome
version	67.0.3396

Name	raven-js
Version	3.26.3

And my code looks something like:

const handleWindowError = (event) => {
  window.Raven.captureException(event, {
    extra: { state: toJSONable(store.getState()) },
  })
}
window.addEventListener('error', handleWindowError)