New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Why are Error Boundaries not triggered for event handlers? #11409

Closed
idhard opened this Issue Oct 31, 2017 · 37 comments

Comments

Projects
None yet
@idhard

idhard commented Oct 31, 2017

Do you want to request a feature or report a bug?
feature , question ?

What is the current behavior?
componentDidCatch is not triggered when the error occurred on event handlers in react components

What is the expected behavior?
to be honest , without reading the full documentation about error boundaries , my first attempt to test error boundaries was to trigger an error in an event handler (ouch!) , then i discovered that componentDidCatch is triggered only on react lifecycle methods and render . I'm wondering why this design decision has been done like that ? it would be convenient to have only one component that handle all unexpected exceptions inside our components , instead now we should have two ways to handle errors inside the component.
I have also created an stackoverflow question : https://stackoverflow.com/questions/47020422/why-reactjs-error-boundaries-are-not-triggered-on-event-handlers with the same concern.
thanks!

Which versions of React, and which browser / OS are affected by this issue? Did this work in previous versions of React?
"react": "^16.0.0"

@blling

This comment has been minimized.

blling commented Oct 31, 2017

Error boundaries aren't supposed to catch errors in event handlers. Only in render and lifecycles.

@gaearon

This comment has been minimized.

Member

gaearon commented Oct 31, 2017

@gaearon gaearon closed this Oct 31, 2017

@idhard

This comment has been minimized.

idhard commented Oct 31, 2017

yeah i have followed the documentation , however i'm not convinced of the reasons to not catch the errors on event handlers , i also know you can catch the error by try/catch blocks but this would mean develop a new way to handle and display errors on the component instead of using only error boundaries , i also read that React doesn't need to know about errors on event handlers but for the developers would be very convenient , i wanted to dig a bit more about the reasons of this design decision.
thanks

@gaearon gaearon changed the title from Why ReactJs Error Boundaries are not triggered on event handlers? to Why are Error Boundaries not triggered for event handlers? Oct 31, 2017

@gaearon

This comment has been minimized.

Member

gaearon commented Oct 31, 2017

OK, let's keep it open for the discussion.

I don't think there was any specific reason other than that we couldn't agree on the behavior, and whether this is beneficial or not. @acdlite might remember it better.

@gaearon gaearon reopened this Oct 31, 2017

@idhard

This comment has been minimized.

idhard commented Oct 31, 2017

great , i think the main benefit is to have a uniform way to handle errors across all components using error boundaries, otherwise it encourage you to develop two ways to handle exceptions or extend the current behavior of EB to support it. Besides adopters of this new feature may be tempted to trigger the errors on event handlers (my case :D) waiting to have the same behavior or perhaps i'm the only one :D.

for example in the codepen :
https://codepen.io/anon/pen/VrLbqj?editors=0011
if you move the exception to the handler the UI get blocked and users doesn't have a glue what is going on , according the documentation to save this error we would have to implement a try/catch block and set the state to display errors instead of relying only on EB.

@acdlite

This comment has been minimized.

Member

acdlite commented Oct 31, 2017

A few reasons we've decided against this for now:

  • Event handlers can be passed down multiple levels. If they throw, we don't know which component the error originated from: the DOM node that triggered the event, or the component that owns the handler itself. For the latter case, it's not even clear how we'd know which component that is.
  • Even if we could reliably figure out which component originated the error, it would only work if the error surfaces synchronously, before the end of the event dispatch. However, many errors occur asynchronously, like in a promise handler or timeout. We have no way of catching those with React.
  • Similar to the previous point, if we gave event errors the same semantics as render errors, it raises the question of whether we should do this for other types of events, like network events, or user-defined subscriptions. This would require us exposing an API like invokeCallback to wrap an arbitrary function with our error handling.
@idhard

This comment has been minimized.

idhard commented Oct 31, 2017

thanks for the explanation @acdlite , here my humble thoughts

Event handlers can be passed down multiple levels. If they throw, we don't know component the error originated from: the DOM node that triggered the event, or the component that owns the handler itself. For the latter case, it's not even clear how we'd know which component that is.

what is the difference when we pass props down the sub-tree of our components ? i guess it may also happen an exception inside the sub-tree of the component, in that case you can keep a reference to the DOM which trigger the exception ?

Even if we could reliably figure out which component originated the error, it would only work if the error surfaces synchronously, before the end of the event dispatch. However, many errors occur asynchronously, like in a promise handler or timeout. We have no way of catching those with React.

wouldn't be the same case for async side effects on render lifecycle methods , like in componentDidMount() to call an external API ?
there is a ticket closed #11334 and a useful fiddle : https://jsfiddle.net/t37cutyk/
if is not supported for lifecycles i think it can also not be supported for event handlers ;)

Similar to the previous point, if we gave event errors the same semantics as render errors, it raises the question of whether we should do this for other types of events, like network events, or user-defined subscriptions. This would require us exposing an API like invokeCallback to wrap an arbitrary function with our error handling.

this is out of my understanding about React error handling.

@gaearon

This comment has been minimized.

Member

gaearon commented Oct 31, 2017

what is the difference when we pass props down the sub-tree of our components ? i guess it may also happen an exception inside the sub-tree of the component, in that case you can keep a reference to the DOM which trigger the exception ?

When a React component throws during rendering we know which component it is because React called this component. Components don't call each other's render methods so we can be sure it's the last component we called.

For event handlers, a LikeButton can pass an onClick handler down to Button and down to a <button>. Which one should we consider the source one, and how could we possibly track it?

@idhard

This comment has been minimized.

idhard commented Nov 1, 2017

For event handlers, a LikeButton can pass an onClick handler down to Button and down to a . Which one should we consider the source one, and how could we possibly track it?

i guess you may be able to do some kind of tracking on the synthetic events implementation of React, I'm talking without much of the knowledge , then for me the source one would be the that call the handle function passed through props and would bubble up the error. The componentDidCatch would be triggered at the level where the handle function has been created , probably LikeButton. To clarify a bit , without error boundaries i would attempt to save the error with a try/catch inside the LikeButton and not in the childrens so would make sense to me to display the errors at that level. Knowing that the source of the exception happen in the would be merely for debugging purposes.

@gaearon

This comment has been minimized.

Member

gaearon commented Nov 1, 2017

I don't see how you could do such tracking.

@idhard

This comment has been minimized.

idhard commented Nov 1, 2017

certainly me neither , i have a weak concept of React internals , but if you agree that having an unified way to handle exceptions for event handlers is worth the effort i could look deeper into it with your guidance.

I would start looking somewhere here : https://github.com/facebook/react/blob/master/packages/react-dom/src/events/ReactDOMEventListener.js#L180
however i understand that Error Boundaries has been though for lifecycles methods and may require some refactoring to support event handlers , which i don't know if it enter in its scope , personally i think it would improve overall development experience for error handling , but certainly we can address this issues with other techniques like try/catch and printing errors "manually".

@gaearon

This comment has been minimized.

Member

gaearon commented Nov 1, 2017

I know the React codebase well. The issue isn't that I don't know where to start, it's that I don't see how implementing this could be possible in the first place without significantly changing React API. As long as event handlers are just functions, there is no way for us to tell which component they are defined in.

@idhard

This comment has been minimized.

idhard commented Nov 1, 2017

ok then , really appreciate your time, this ticket #10474 and looking at the source code helped me to understand a bit more the underlying of it .

@JamesTheHacker

This comment has been minimized.

JamesTheHacker commented Nov 13, 2017

There is a way to do it, but it's not pretty: https://jaketrent.com/post/react-error-boundaries-event-handlers/

You catch the error in your event handler. In the catch block you set some state like this.setState({ error: err }). This causes a rerender. In the render block if this.state.error is set you rethrow if (this.state.err) throw this.state.err.

@mikkelwf

This comment has been minimized.

mikkelwf commented Nov 29, 2017

I was really looking forward to componentDidCatch, but after my first test of this behaviour here i must admit i'm rather disappointed.
Since event handling is such a big part of the app's interface, it's really irritating that there is no way of handling this globally.
I primarily see componentDidCatch as a way to present the user with a (workable explanation) to what happened. At least providing some kind of catch-all of an unhandled event error so we can choose to render an error message to the user would be a great leap forward.
Right now all event error handling must be done locally which is tedious and opens up for missing a lot of errors.
The worst thing is the app, instead of displaying some sensible state, is rendered "working" in a non-working state, which is really frustrating UX.
Please reconsider changing the behaviour of componentDidCatch

@gaearon

This comment has been minimized.

Member

gaearon commented Nov 29, 2017

This discussion is not helpful if people keep adding comments like "please add this" and don't reply to the actual questions about how it should work. Here's a set of problems: #11409 (comment). If you want to participate in discussion please don't ignore them, and suggest solutions for them.

@mikkelwf

This comment has been minimized.

mikkelwf commented Nov 30, 2017

@gaearon if you're referring to my comment, i'm simply an end user of react, that does not have the technical insight into the inner workings of React to participate, nor do i have any solutions to the issue.
I'm simply stating that the current solution in not 100% usable. For me personally it would be great to have some kind of UI catch-all that could be triggered whenever these event errors happen, so i can display a failed state to the user. The origin of the event if (for now at least) not important. I know this is probably not something that will make it into core.
And my disappointment with the current implementation also stems from the fact that you (react team) kinda sold the react 16 componentDidCatch at the best new thing since sliced bread when it was released, and having this major caveat not to handle event errors (you kinda have to dive into the documentation to find out) was a bit of a letdown. Sorry.

@gaearon

This comment has been minimized.

Member

gaearon commented Nov 30, 2017

i'm simply an end user of react, that does not have the technical insight into the inner workings of React to participate

I am not asking you to have knowledge of internals. In #11409 (comment) we're saying that we don't see a plausible way to do it in JavaScript at all without fundamentally changing the API or introducing more confusion. This statement has nothing to do with how React works but with technical constraints of what we can do with JavaScript, and this API. Even if each of us was to rewrite React from scratch for the sake of this feature, I don't quite see how we could do it.

You don't need to know how React works internally to participate in a discussion. Try to do a mind experiment and think: "if I had to approach React from a clean slate, how could this possibly work".

All I'm trying to say that further comments that don't attempt this aren't helpful. Just saying "please support" or "this is disappointing" doesn't move this issue forward so I ask future posters to refrain from it. We've already explained why it seems very tricky or impossible to us with the current API.

Now, we might be wrong and we would be very happy to learn about a solution! But you're not engaging in that conversation, and instead repeat the point that we already replied to. So the conversation goes in circles.

I understand it might be difficult to think about. It's often difficult for us too. But in that case let's keep the discussion clean and tidy in case somebody else can help us figure it out, and not repeat the same points many times.

@hampusohlsson

This comment has been minimized.

hampusohlsson commented Dec 6, 2017

I think I found a decent workaround - posting here if anyone is interested. If you are using async/await, It is pretty easy to implement a boundary component that captures failures in both callbacks and rejected promises.

The idea is to recursively loop through all children, check their props and wrap any functions in a try/catch closure. It only works because the nature of async await and it is probably not the most efficient solution (if you have a large component tree inside the <ErrorBoundary /> component).

Here is a quick proof of concept: https://codesandbox.io/s/61p3rnnj2w

@gaearon

This comment has been minimized.

Member

gaearon commented Dec 6, 2017

Not recommended. In larger apps or perf-sensitive code, this is indeed going to be slow.

@idhard

This comment has been minimized.

idhard commented Dec 9, 2017

@Gareon , do you think this issue could go to https://github.com/reactjs/rfcs ? i could elaborate it more taking in account the comments below and different proposals , i think @hampusohlsson solution , beside the drawbacks , is what we would like to see supported by React. thanks

@gaearon

This comment has been minimized.

Member

gaearon commented Dec 9, 2017

Yep, that’s a good way to do it.

@hampusohlsson

This comment has been minimized.

hampusohlsson commented Dec 19, 2017

@idhard I'd be happy to assist writing up an RFC

@idhard

This comment has been minimized.

idhard commented Dec 21, 2017

@hampusohlsson yeah sure ! , i have done the first steps of RFC creation , here is the fork : https://github.com/idhard/rfcs , i haven't had much time to fill it up . I think that if users are passing async functions to the event handlers , it shouldn't be very complicated to wrap them in try/catch blocks inside React and bubble up the error , if they are passing normal functions and doing async then react will not handle it .

@sw-yx

This comment has been minimized.

Contributor

sw-yx commented Feb 5, 2018

is this still active? I would be interested in using this functionality.

@dalhorinek

This comment has been minimized.

dalhorinek commented Feb 6, 2018

As there is a need usually to bind handler to this, we have this function that handles the problem with ErrorBoundaries in handlers

export const bindFunction = (handler, obj, ...initArgs) => {
  return ((...args) => {
    try {
      return handler.apply(obj, initArgs.concat(args))
    } catch(error) {
      if (isFunction(obj.setState)) {
        obj.setState(() => { throw error })
      }
    }
  })
}

and inside a component we have this

constructor(...args) {
  super(...args)

  this.handler = bindFunction(this.handler, this)
  // instead of this.handler = this.handler.bind(this)
}

I'm not the author of this solution, I found it here https://jaketrent.com/post/react-error-boundaries-event-handlers/ and also the throw in setState is mentioned in a comment there.

@cameron-martin

This comment has been minimized.

cameron-martin commented Feb 8, 2018

A few reasons we've decided against this for now:

  • Event handlers can be passed down multiple levels. If they throw, we don't know which component the error originated from: the DOM node that triggered the event, or the component that owns the handler itself. For the latter case, it's not even clear how we'd know which component that is.

I'd expect the error to originate from the component that rendered the DOM node that the event handler is attached to. Are you saying that this isn't possible?

  • Even if we could reliably figure out which component originated the error, it would only work if the error surfaces synchronously, before the end of the event dispatch. However, many errors occur asynchronously, like in a promise handler or timeout. We have no way of catching those with React.

I'd expect this to be covered by returning a rejected promise from an event handler, and this to be treated the same as if the event handlers threw an error. Then it'd be up to the user to make sure any asynchronous errors result in the promise returned from the event handler being rejected.

On a related note, I'd also expect that returning a rejected promise from (most) lifecycle methods would result in the same behaviour as throwing an error.

  • Similar to the previous point, if we gave event errors the same semantics as render errors, it raises the question of whether we should do this for other types of events, like network events, or user-defined subscriptions. This would require us exposing an API like invokeCallback to wrap an arbitrary function with our error handling.

What do you mean by "network events, or user-defined subscriptions"?

@idhard

This comment has been minimized.

idhard commented Mar 2, 2018

hey @gaearon great presentation for the future of React ! , im wondering if the new API are addressing this issue in a nicer way ? although there wasn't a demo for using the new api with errorBoundaries i understood they will play nicely together.

@mzedeler

This comment has been minimized.

mzedeler commented Apr 13, 2018

@idhard - I've looked through this discussion because I needed clarification with regard to exactly what is being caught by the error boundary, since the documentation doesn't seem to explicitly say that errors in event handlers aren't caught.

I think the explanation makes sense - it is not feasible to track down the component that owns the code that blew up, so the only option is to isolate it to the component that triggered the error. I am not sure that would be very helpful, but I guess it would be feasible to implement.

My key takeaway from this discussion is that it is worthwhile trying to move as much code out of event handlers and to the lifecycle methods of the components, if possible. By doing that, it is possible to reduce the risk of triggering an error that won't be caught.

But another thing is that it would be nice to explicitly write in the documentation that errors in event handlers are not dealt with by error boundaries. I don't mind adding it to the documentation and creating at PR for it, @gaearon.

@gnapse

This comment has been minimized.

gnapse commented Apr 13, 2018

since the documentation doesn't seem to explicitly say that errors in event handlers aren't caught

@mzedeler The documentation does mention that explicitly, as was pointed out early in this discussion: https://reactjs.org/docs/error-boundaries.html#how-about-event-handlers

@mzedeler

This comment has been minimized.

mzedeler commented Apr 13, 2018

Wonderful. Thanks, @gnapse :-)

@JochemG

This comment has been minimized.

JochemG commented Apr 22, 2018

If you want events to be captured in ErrorBoundaries you can always just force them in the lifecycle and handle them there :)

class ReactEventToLifecycle {
    queuedEvents = [];``

    register(setState, event) {
        return (e) => {
            const value = e.target.value;
            this.queuedEvents.push(() => event(value));
            setState({});
        };
    } 

    handleEvents() {
        const queue = this.queuedEvents;
        this.queuedEvents = [];
        for (const event of queue) {
            event();
        }
        return queue.length;
    }
}
class MyInput extends Component {
    pendingEvents = new ReactEventToLifecycle();
    handleOnChange(event) {
        const {onChange} = this.props;
        onChange(event);
    }
    shouldComponentUpdate() {
        return !this.pendingEvents.handleEvents();
    }
    render() {
        const {value, onChange, ...otherProps} = this.props;
        return <input {...{
            onChange: this.pendingEvents.register(this, onChange),
            value,
            ...otherProps
        }}/>;
    }
}

(NOTE: this solution needs a bit more research for edge cases. E.g. for text input/textareas, if the current value === 'my value' and I write a character after 'my', then the cursor goes to the end of the text all the time... setSelectionRange can solve this case of course, but there might be other issues that I missed)

@jmathew

This comment has been minimized.

jmathew commented May 1, 2018

@dalhorinek 's solution was what I ended up going with.

In my case I had a async ComponentDidMount which I modified like so:

async componentDidMount(){ 
     try {
         await someAsync();
     catch (e) {
         if(e instanceof SomeErrorICanHandle) { ... }
         else {
               this.setState(() => { throw e }); // "Rethrow"
         }
     }
}
@degr

This comment has been minimized.

degr commented Nov 16, 2018

Due some reasons, I was need to log all errors on client side. Unfortunately it possible to log only component life cycle with componentDidCatch, and not possible to log event handlers. After couple hours of work I found solution. To do it, you need find first try-catch block inside of react-dom.production.js file, and change it to this shitcode:

 try{
       b.apply(c,l)
    }catch(m){
        if(typeof React.handlersLog === 'function') {
            React.handlersLog(b, m);
        }
        this.onError(m)
    }}

Then, you should add handlersLog method to React object. I did it by this way:

React.handlersLog = (handler, message) => {
    RequestService.doPost("/errors", "handler\n\n" + handler.toString() + "\n\n" + (message.stack || message));
};

Why I do handler.toString? Because production build is uglified and minified, and all additional information about issue can help.

It work. I did not see anything, that can break react architecture. If guys from react team will add this feature into next release, it will be great. Of course, React.handlersLog not good decision, because you can't add more than one handler, but for my current situation it's ok.

If you need to do same hook for development build, search for func.apply(context, funcArgs); inside of react-dom.development.js

@Eddie-CooRo

This comment has been minimized.

Eddie-CooRo commented Nov 17, 2018

hi @gaearon
Thanks for your detailed explanations
I'm not so much into technical aspects, but I wonder whether we can use "useCallback" hook now, in order to determine where the error is originated from, the same way we find out the origin of each state in "useState" hook

@degr

This comment has been minimized.

degr commented Nov 28, 2018

My solution work in production environment during 2 weeks, and we found around 10 bugs. Couple of them was reproducible only in IE, couple of them was new for us, and happen in some special cases. So, if it is possible, please add such kind of hook to react. I hope it can be done as minor change, but as great feature, because no one of existing frameworks allow to handle exceptions inside of event handlers inside of one try-catch block.

@rvillanueva

This comment has been minimized.

rvillanueva commented Dec 2, 2018

@gaearon Sorry if this is a somewhat different line of questioning, but what are your thoughts about directly invoking componentDidCatch()? I'm manually passing it exceptions from my fetch requests and it seems to be working fine, but I wanted to make sure I wasn't causing any unexpected side effects.

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