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 inside render #164

Closed
gaearon opened this issue Jul 20, 2015 · 19 comments
Closed

Catch errors inside render #164

gaearon opened this issue Jul 20, 2015 · 19 comments

Comments

@gaearon
Copy link
Owner

gaearon commented Jul 20, 2015

This is something I demoed in my React Europe talk but have not released yet.

Basically when you make a mistake inside render, React Hot Loader can catch the error and display a “red screen of death” inline instead of crashing your app:

screen shot 2015-07-20 at 19 21 34

There is a PR to React Hot API which is exactly what I showed at the conf, but before turning this into an officially supported feature, I want to make it more extensible. I want to make the error renderer customizable because we want to support React Native too. Some projects may want to make custom renderers that display the error on top of body instead on inline, etc.

Ideally it would be opt in, and the API would be something like react-hot-loader?reporter=npm-module-name where npm-module-name is some package that exports a React (or React Native) <Reporter> component as a default export. This component will be used as <Reporter error={e} /> by React Hot Loader, when specified.

Here is an example of such component by @KeywordBrain: https://github.com/KeywordBrain/redbox-react.

If somebody wants to take a stab at it, please do! (But let us know in this thread.)

If not, I'll get around to implementing this in August.

@gaearon
Copy link
Owner Author

gaearon commented Jul 20, 2015

Edit: fixed the screenshot. :-) El Capitan screenshotting is broken..

@davidpfahler
Copy link

@gaearon Just to be clear: The next step here is to implement the opt-in error report API, not any specific reporter, right?

@gaearon
Copy link
Owner Author

gaearon commented Jul 20, 2015

Yes, the next step is to modify React Hot Loader to read (and resolve via Webpack's require.resolve I think) the reporter from the query config and pass it to React Hot API, then modify React Hot API based on my current PR so it uses that reporter, or, if reporter is not specified, behaves like it used to behave (just throw).

@syranide
Copy link
Contributor

Intuitively this seems separate from react-hot-loader, I could imagine wanting just this. Or a special version of this in prod where the errors are also submitted elsewhere. EDIT: But obviously there is a benefit if react-hot-loader ships with one to avoid breaking React everytime you make a mistake, but I'd prefer it if it wasn't opinionated (perhaps it could just return the last render result?)... I prefer my errors in the console, in full.

@gaearon
Copy link
Owner Author

gaearon commented Jul 21, 2015

@syranide Interesting. Yeah this could be separate. What API do you imagine?

@syranide
Copy link
Contributor

@gaearon All it really boils down to is just overloading the render-function right? So decorators should work just fine. It seems beneficial to be able to piggyback on react-hot-loader with a global option for all renders to be wrapped with try/catch and an fallback render-callback (which gets the exception as an argument) for when that happens.

That's the extent to which I see react-hot-loader involved in this as I could imagine wanting to have the same feature on a per-component basis applied by a decorator. Being able to reuse the same "fallback render-callback" for all components when using react-hot-loader but just a few special components in non-hot or production.

tl;dr I see this more as react-hot-loader providing easy access for decorating all components, not the error-box as a feature of react-hot-loader. Which means this could even boil down to react-hot-loader just accepting a bunch of decorators that should be applied to all components, nothing more.

@davidpfahler
Copy link

@gaearon @syranide You want to take a look at babel-plugin-react-error-catcher which just "adds a decorator as a higher-order component to React components". I have two questions:

  1. Is it better to use react-hot-loader to apply a decorator to all components than using something like a babel plugin or other method? Do we gain anything in terms of being able to hot reload?
  2. How could an API look like to provide decorators to react-hot-loader?

@gaearon
Copy link
Owner Author

gaearon commented Jul 22, 2015

@davidpfahler

We want to eventually deprecate React Hot Loader in favor of a Babel plugin: https://github.com/gaearon/react-hotify-boilerplate

So this is consistent with how we want to do things in the future. As for HOCs, I'm not sold: https://github.com/loggur/babel-plugin-react-error-catcher/issues/1

@davidpfahler
Copy link

@syranide

After a short discussion with @gaearon we think this is a reasonable course of action:

react-hot-loader is moving towards decorators and a babel plugin, which you can see at gaearon/react-hotify (WIP). Once this is completed, it will be easy to implement this as just another decorator (which can then also be used outside of react-hot-loader) as described by @syranide.

In the meantime, we don't want to pass on exception catching and rendering. So we are going to go ahead with @gaearon 's initial idea of implementing a new API like this: react-hot-loader?reporter=npm-module-name. Adding this API just to deprecate it in a few weeks in not a huge concern, because once we move to decorator as described above, we are going to make a big breaking change anyway.

@syranide
Copy link
Contributor

@davidpfahler 👍

But I'm not sure what prevents react-hot-loader from using a list of decorators right now, react-hot-loader doesn't need to use decorators itself, it just needs to apply them to the classes it processes.

Anyway, I don't necessarily mind the reporter-feature either, it may be more accessible and could make sense in that it could have additional behaviors that makes sense only for react-hot-loader... say try/catch all life-cycle methods too, and in-case of error it will cleanly remount the component on next update or w/e.

@philholden
Copy link

It would be quite cool to have a transition effect so the error modal expands out of the component where the error originated.

@gaearon
Copy link
Owner Author

gaearon commented Jul 24, 2015

@philholden To clarify, error is not in the modal. (It's just I was demoing that RHL can change stuff inside dynamic stateful components like modals.) The error is placed exactly where the error'd component was rendering (with width: 100%; height: 100% to fill the space).

@philholden
Copy link

I was partly thinking of components where there is not enough space to show an error message or that are offscreen (still probably not a great idea though as you lose inspectability).

@gaearon
Copy link
Owner Author

gaearon commented Jul 24, 2015

@philholden That's why I want custom reporters. You could write a reporter that checks if the bounding rect is smaller than some value and shows an overlay in the screen corner in this case.

@ghost
Copy link

ghost commented Jul 26, 2015

Anyone watching this thread may want to check out babel-plugin-react-error-catcher. It is compatible with hot reloaders and is designed to use custom reporter functions and/or components.

@davidpfahler
Copy link

For everyone's interest: You can find a working example of catching errors with react-hot-loader and redbox-react in the redbox-react repo. It relies on a PR to react-hot-loader and a PR to react-hot-api, so you probably don't want to use it, yet.

There is also an example using @timbur 's babel-plugin-react-error-catcher.

Feedback, issues, pull request and contributions of any kind are always welcome.

@gaearon gaearon mentioned this issue Aug 28, 2015
13 tasks
@gaearon
Copy link
Owner Author

gaearon commented Apr 18, 2016

With React Hot Loader 3, we now have error reporting on initial mount.
It doesn’t work for updates yet but I think it should once facebook/react#6020 lands.
Keeping this issue open to track this.

@gaearon gaearon added this to the v3.x milestone Apr 18, 2016
@allyraza
Copy link

allyraza commented Jun 2, 2016

Hi Dan, thank you for putting this together I have been using this package for some time now, I am implementing an overlay much like hot-middleware overlay in spare time is it okay if I send a PR?

@gregberge
Copy link
Collaborator

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

No branches or pull requests

6 participants