-
Notifications
You must be signed in to change notification settings - Fork 46.8k
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
Catch errors from component's render() #5528
Conversation
… incosistent state
Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up at https://code.facebook.com/cla - and if you have received this in error or have any questions, please drop us a line at cla@fb.com. Thanks! |
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks! |
cc @sebmarkbage |
The concern here is performance. try/catch at every level down the tree can negatively impact performance and will deopt this function (at least in otherwise good conditions). The idea was to use try to place these at "anchor points" like entry points into the middle of the tree or error boundary roots. The problem is that there are mutations along the way. |
Can this be enabled behind a flag, so those that choose stability above performance can choose to enable this? |
@globexdesigns We avoid global configurations like the plague. Often bugs only show up due to the complex interactions of having certain flags enabled, and it becomes a nightmare to debug/maintain. Not to mention the added overhead of explaining to users which flags they should enable when, etc. |
We do have different renderers. We could create a stand-alone build for this. As a work around you can just use/publish your own fork. |
according to https://github.com/petkaantonov/bluebird/wiki/Optimization-killers#2-unsupported-syntax you can decrease the impact of the deoptimization of the
ie creating a function that simply calls a function inside of a something like: function tryCatchRender(render) {
try {
return render();
} catch (e) {
e.isError = true;
return e;
}
} 👍 for adding a standalone "safe" build |
This is an attempt at fixing #2461
If a Component's
render()
function throws an error we catch it, display a warning in the console and then we throw the error usingsetTimeout
so that React does not get into inconsistent state but the error is not swallowed and bubbles to the top.Please let us know if this is the right direction. We are really not sure about the usage of
setTimeout
, it feels like a dirty hack. The reason for its usage is to not silently fail but to let the error propagate to the top. But we cannot do it synchronously because otherwise React will end up in state that is "beyond repair".We worked on this PR together with @LeZuse
See the test case here: http://jsfiddle.net/XeeD/pLqphjm4/