Skip to content

Conversation

@duncanbeevers
Copy link
Contributor

This pull request started out trying to handle the case where users throw strings in JavaScript and quickly led to a pretty major change in the operation of Airbrake.push

Airbrake.push now throws the error passed to it after reporting is completed. This parallels the behavior of TraceKit's report except the error may not actually be thrown during the execution of the push, and instead may be rethrown sometime later.

@duncanbeevers
Copy link
Contributor Author

Fixes #47

@vmihailenco
Copy link
Contributor

Not LGTM

This change is ugly and driven by TraceKit. I saw that TraceKit tries to throw exception and then catches it in window.onerror handler. I don't understand what it tries to achieve, but it should not break notifier design. Can we find another way to fix TraceKit?

instead may be rethrown sometime later.

How I am supposed to use exceptions then?

@duncanbeevers
Copy link
Contributor Author

It's driven by TraceKit because TraceKit uses the rethrow information to populate additional information on errors captured by window.onerror in browsers that wouldn't ordinarily support this.

There are two pending changes in TraceKit that should obviate the need for this behavior, occ/TraceKit#49 and occ/TraceKit#54

Please feel free to weigh in on those TraceKit issues.

It is a strange API that allows you to throw and handle exceptions, but not to actually use them as a form of flow control.

Alternatively, we can use the fallback processor that bypasses window.onerror entirely and relies on some simple regular expressions to parse backtrace lines, but has only been tested against backtraces generated by Chrome.

@duncanbeevers
Copy link
Contributor Author

To be clear, I think pulling TraceKit in order to support this very normal usage of exceptions is entirely reasonable.

@vmihailenco
Copy link
Contributor

but has only been tested against backtraces generated by Chrome.

Are you sure that it is tested only for Chrome? I thought this is the only method to get stacktrace from exception.

It is a strange API that allows you to throw and handle exceptions, but not to actually use them as a form of flow control.

There are opinions that exceptions should not be used for flow control. But our case is much simpler. There is no reasons to make push to rethrow exceptions except making TraceKit happy. And I still have impression that we can avoid doing this.

@vmihailenco
Copy link
Contributor

So as I understand the problem is that throw "foobar"; works poorly in JavaScript because such "exceptions" do not have stacktraces. Really resulting exception is just a string. So TraceKit correctly tries to use window.onerror handler to get some information.

So I think we should:

  • always use TraceKit and window.onerror handler.
  • remove fallback processor.
  • document that push can raise exceptions and that exceptions will be correctly handled by window.onerror.

@duncanbeevers
Copy link
Contributor Author

Tracekit.report throws normally. Airbrake.push relies on Tracekit.report, so it behaves a little strangely.

First, since we use a shim implementation, push on an array never throws. We could change the shim to throw when things are pushed onto it,

Second, when the canonical implementation comes in, it processes through the errors pushed onto the shim, and processes any new errors immediately. To maintain a consistent interface between these, neither one ever throws immediately, meaning exceptions can't be used as flow control. However, any errors thrown will be re-raised to window for viewing in the console or whatever.

Perhaps something simpler would be to pull all this plumbing out and simply let wrap swallow errors, the same way that Angular does and then recommend that users add a custom reporter to log or re-raise errors, as they see fit.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants