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

Add an errorReporter option to the hot() HOC factory #811

Closed
insin opened this issue Jan 21, 2018 · 8 comments
Closed

Add an errorReporter option to the hot() HOC factory #811

insin opened this issue Jan 21, 2018 · 8 comments

Comments

@insin
Copy link
Contributor

insin commented Jan 21, 2018

If you add an errorReporter option to hot(), that can be passed on to the <AppContainer> rendered by the HOC it creates.

I've already done this locally to test it out (along with a local change to react-hot-loader-loader to support configuring errorReporter via Webpack rule options): 0e384bb

@insin
Copy link
Contributor Author

insin commented Jan 21, 2018

One weird thing I've noticed is that if I create a runtime error in a component's render() method the error boundary doesn't handle it during HMR patching, but it does if I refresh the page to render from scratch.

Immediately after adding {nonexistent.object} to the render() method of a simple App component in src/App.js, which is exporting hot(module)(App):
firefox_2018-01-22_02-09-00

After refreshing the page, the error boundary works and my configured errorReporter is rendered:
firefox_2018-01-22_02-09-25

@gregberge
Copy link
Collaborator

We will deprecate errorReporter this is why it is not in hot. Instead the user would have to implement its own ErrorBoundary.

But we need to check deeply the behaviour you experienced. Maybe componentDidCatch is not called with HMR. @theKashey an idea?

@theKashey
Copy link
Collaborator

Should. But it is easy to reproduce the problem and found out the problem.

@insin
Copy link
Contributor Author

insin commented Jan 22, 2018

If errorReporter is going away and the user has to provide their own error boundary, how will they know when to clear the error after it's been fixed when HMR is performed? It looks like AppContainer is currently doing some work with react-hot-loader internals to manage that.

@theKashey
Copy link
Collaborator

theKashey commented Jan 22, 2018

You are right. If error boundary will be "above" the AppContainer - it will be notified about an error, but not about HRM event, as long this is just different things.
Implementing some notification mechanism is not a good idea.

There are two ways to solve

  • export a top function, wrap with it your application, and we will deep-force-update it on HRM event, bypassing all the boundaries.
  • support custom error reporters. Far easier way. Maybe not via hot function, but via setConfig.

@theKashey
Copy link
Collaborator

Ok, I've found a problem with error and open an PR to fix it.
Why do I thounght that try...finally will swallow the error?

@theKashey
Copy link
Collaborator

About errorBoundaries - @neoziro just proposed in another issue, to place it just after AppContainer. Should work for any case (after PR to swallow error got merged)

gregberge added a commit that referenced this issue Jan 22, 2018
@gregberge
Copy link
Collaborator

Documented in v4.0.0-beta.17.

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

3 participants