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

Add optional error boundary support #473

Merged
merged 5 commits into from Jan 11, 2018

Conversation

Projects
None yet
2 participants
@dustinsoftware
Collaborator

dustinsoftware commented Nov 12, 2017

I updated a sample so you can see how this works. When specifying an error handler, then errors will be caught during server-render (including those not handled by error boundaries).

Some tests still need to be added, but I wanted to get the discussion started about this approach.

@dustinsoftware dustinsoftware force-pushed the dustinsoftware:error-boundaries branch 2 times, most recently from b656cc0 to bd1badb Nov 12, 2017

@dustinsoftware

This comment has been minimized.

Collaborator

dustinsoftware commented Nov 16, 2017

Ok @Daniel15 this is ready for review

@dustinsoftware

This comment has been minimized.

Collaborator

dustinsoftware commented Nov 30, 2017

@Daniel15 would you have time to take a look at this PR soon? Are you OK with the overall direction?

@Daniel15

Sorry for the delay! In general, this looks pretty good to me.

{
attributes += string.Format(" class=\"{0}\"", ContainerClass);
if (_configuration.ExceptionHandler == null) {

This comment has been minimized.

@Daniel15

Daniel15 Dec 11, 2017

Member

I wonder if we should also allow setting an exception handler per component (eg. as an argument to this method). Hmm.

{
attributes += string.Format(" class=\"{0}\"", ContainerClass);
if (_configuration.ExceptionHandler == null) {

This comment has been minimized.

@Daniel15

Daniel15 Dec 11, 2017

Member

Instead of checking if this is null, what about making it the default handler? That way, _configuration.ExceptionHandler would never be null.

ContainerId
));
}
_configuration.ExceptionHandler(ex);

This comment has been minimized.

@Daniel15

Daniel15 Dec 11, 2017

Member

Might be worth passing ComponentName and ContainerId too.

@dustinsoftware

This comment has been minimized.

Collaborator

dustinsoftware commented Dec 18, 2017

Thanks for the review, I'll push an update soon :)

@dustinsoftware dustinsoftware force-pushed the dustinsoftware:error-boundaries branch 2 times, most recently from b2c4997 to 4ef3074 Dec 19, 2017

@dustinsoftware dustinsoftware force-pushed the dustinsoftware:error-boundaries branch from 4ef3074 to 46b45b3 Dec 19, 2017

@dustinsoftware dustinsoftware force-pushed the dustinsoftware:error-boundaries branch from a6ceeab to 13c9926 Dec 19, 2017

@dustinsoftware

This comment has been minimized.

Collaborator

dustinsoftware commented Jan 6, 2018

@Daniel15 this should be good to go, want to give it another look?

@Daniel15 Daniel15 merged commit f408fa4 into reactjs:master Jan 11, 2018

1 check passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment