Skip to content
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

Closed
wants to merge 1 commit into from

Conversation

XeeD
Copy link

@XeeD XeeD commented Nov 22, 2015

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 using setTimeout 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/

@facebook-github-bot
Copy link

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!

@facebook-github-bot
Copy link

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

@zpao
Copy link
Member

zpao commented Nov 24, 2015

cc @sebmarkbage

@sebmarkbage
Copy link
Collaborator

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.

@EvHaus
Copy link

EvHaus commented Nov 24, 2015

Can this be enabled behind a flag, so those that choose stability above performance can choose to enable this?

@jimfb
Copy link
Contributor

jimfb commented Nov 24, 2015

@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.

@sebmarkbage
Copy link
Collaborator

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.

@koulmomo
Copy link

according to https://github.com/petkaantonov/bluebird/wiki/Optimization-killers#2-unsupported-syntax you can decrease the impact of the deoptimization of the try {} catch (in V8 at least) by

isolating to a minimal function

ie creating a function that simply calls a function inside of a try {} catch

something like:

function tryCatchRender(render) {
    try {
        return render();
    } catch (e) {
        e.isError = true;
        return e;
    }
}

👍 for adding a standalone "safe" build

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

Successfully merging this pull request may close these issues.

7 participants