re-frame icon indicating copy to clipboard operation
re-frame copied to clipboard

Add event error handler

Open nberger opened this issue 8 years ago • 8 comments
trafficstars

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

nberger avatar Oct 17 '17 23:10 nberger

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

nberger avatar Oct 17 '17 23:10 nberger

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!

mike-thompson-day8 avatar Oct 23 '17 20:10 mike-thompson-day8

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.

screenshot of google chrome 24-10-17 4-30-06 pm

danielcompton avatar Oct 24 '17 03:10 danielcompton

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)

nberger avatar Oct 24 '17 15:10 nberger

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.

nberger avatar Jun 21 '18 16:06 nberger

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!

danielcompton avatar Jun 22 '18 01:06 danielcompton

We might need this feature, is this going to be merged eventually? Or should we give up on it?

qleguennec avatar Dec 20 '21 10:12 qleguennec

This PR needs someone who has time to resolve

  1. merge conflicts with master due to the age of the PR
  2. 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

superstructor avatar Dec 20 '21 19:12 superstructor

Merged e91e5b57

kimo-k avatar Nov 02 '23 15:11 kimo-k