re-frame
re-frame copied to clipboard
Add event error handler
A simplified alternative to #330. As discussed with @danielcompton in that other PR, this is a less invasive and easier to explain/document way to handle exceptions.
Adds a global error handler via reg-event-error-handler that gets called when an exception pops up from the interceptor chain call. The handler is called with an ExceptionInfo that contains keys #{:direction :exception :interceptor}. Handler is expected to do side-effectfs: send the error information to an errors service, dispatch an event to do that or to add the error information to the db, etc.
Original issue: #231
I'd be happy to add more documentation and examples if there's interest in this change. I'll leave some pointers here:
docs/FAQs/CatchingEventExceptions.md- probably here
docs/Debugging-Event-Handlers.md - maybe mention this feature in the README
This is looking good. I've got a request for a name change, which will follow soon, but nothing at all significant.
Docs wise, the following will need attention:
- docs/API.md
- Changes.md
- docs/Debugging-Event-Handlers.md
- remove /docs/FAQs/CatchingEventExceptions.md (also see /docs/FAQs/README.md)
Many Thanks!
Hey I've taken a look at this and pushed a commit to add more context to the error. I'm happy with the concept and implementation, the only thing I'm really thinking about is whether we can capture the original stack trace of the error pointing to the registered handler. Currently it is still pointing to re-frame's internals.
whether we can capture the original stack trace of the error pointing to the registered handler. Currently it is still pointing to re-frame's internals.
What do you think about (.-stack (ex-cause e))? It should give the original stacktrace. Wrapping the exception adds an indirection layer but there's a way to get the original stack trace. I think it's reasonable, it's a tradeoff. And it provides a way to report the original exception with the full stack trace to an error service for example. If this way looks good to you, perhaps we could mention it in the docs (I still have to add some doc changes)
I haven't been needing this change as of late, and at the same time it seems it didn't catch much interest, so I'll close the PR, while keeping the branch in my fork in case anyone wants to use it.
@danielcompton thanks again for taking the time to review and discuss it.
Hey sorry we haven't been able to merge this, I still think it's a useful change, we just haven't had any time budget to review it. I'd like to keep it open, as I think this could be useful for others too, we've just got other internal stuff we need to take care of first. Sorry for the wait!
We might need this feature, is this going to be merged eventually? Or should we give up on it?
This PR needs someone who has time to resolve
- merge conflicts with
masterdue to the age of the PR - any outstanding issues from existing comments (not new issues, in the interests of closure); e.g.
(.-stack (ex-cause e))to get the original stacktrace.
I don't have that time at the moment.
If someone else could do the above and update this PR or open a new PR based on these changes I will merge it.
@qleguennec
Merged e91e5b57