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

componentDidCatch doesn't work in React 16's renderToString #10442

Closed
aickin opened this issue Aug 11, 2017 · 11 comments
Closed

componentDidCatch doesn't work in React 16's renderToString #10442

aickin opened this issue Aug 11, 2017 · 11 comments

Comments

@aickin
Copy link
Contributor

aickin commented Aug 11, 2017

I tried out a simple test case for the new componentDidCatch error handling code in React 16 SSR with the following code:

import React from "react"
import { renderToString } from "react-dom/server"

class ErrorBoundary extends React.Component {
  constructor(props) {
    super(props);
    this.state = { hasError: false };
  }

  componentDidCatch(error, info) {
    // Display fallback UI
    console.log("componentDidCatch");
    this.setState({ hasError: true });
  }

  render() {
    if (this.state.hasError) {
      // You can render any custom fallback UI
      return <h1>Something went wrong.</h1>;
    }
    return this.props.children;
  }
}

class ThrowingComponent extends React.Component {
  render() {
    throw new Error("Ooooops");
  }
}

console.log(renderToString(<ErrorBoundary><ThrowingComponent/></ErrorBoundary>));

and the result with 16.0.0-beta.5 was:

/Users/.../throwingimpl.js:68
      throw new Error("Ooooops");
      ^

Error: Ooooops
    at ThrowingComponent.render (/Users/.../throwingimpl.js:27:11)
    at resolve (/Users/.../node_modules/react-dom/cjs/react-dom-server.node.development.js:1891:18)
    at ReactDOMServerRenderer.render (/Users/.../node_modules/react-dom/cjs/react-dom-server.node.development.js:1988:22)
    at ReactDOMServerRenderer.read (/Users/.../node_modules/react-dom/cjs/react-dom-server.node.development.js:1964:19)
    at renderToString (/Users/.../node_modules/react-dom/cjs/react-dom-server.node.development.js:2234:25)
    at Object.<anonymous> (/Users/.../throwingimpl.js:31:13)
    at Module._compile (module.js:569:30)
    at loader (/Users/.../node_modules/babel-register/lib/node.js:144:5)
    at Object.require.extensions.(anonymous function) [as .js] (/Users/.../node_modules/babel-register/lib/node.js:154:7)
    at Module.load (module.js:503:32)

As far as I can tell, this code should not throw but should rather output <h1>Something went wrong.</h1>.

I'm afraid that maybe we didn't add componentDidCatch to the new server renderer.

@aickin aickin mentioned this issue Aug 11, 2017
@sophiebits
Copy link
Collaborator

This is intentional / a known limitation. With streaming rendering it's impossible to "call back" markup that has already been sent, and we opted to keep renderToString and renderToNodeStream's output identical.

@sebmarkbage has ideas about how we could potentially support something along these lines one day if we have a small client runtime (ex: outputting script tags that remove previously-sent content).

@sebmarkbage
Copy link
Collaborator

That's intentional. The next next out-of-order server renderer will have error boundaries. But it doesn't really make sense on an in-order streaming server rendering. Since we could've already flushed the boundary. We could buffer inside error boundaries but that would mean that we don't actually do any streaming rendering since the best practice is to have an error boundary at the root.

We could support this in the renderToString API but that's easy-ish to implement yourself around it so not high pri and would likely go into the work of the out-of-order renderer.

@aickin
Copy link
Contributor Author

aickin commented Aug 11, 2017

Ah, got it. My fault! ;) Thanks, and closing this issue.

@asmagin
Copy link

asmagin commented Mar 13, 2018

This looks like a dirty but a much needed hack to overcome the intentional limitation: https://github.com/doochik/babel-plugin-transform-react-ssr-try-catch

Would be nice to have something like that built-in.

@zekchan
Copy link

zekchan commented Apr 20, 2018

In your case you can use https://github.com/zekchan/react-ssr-error-boundary - it is component using renderToStaticMarkup on server for render your component

@asmagin
Copy link

asmagin commented Apr 21, 2018

renderToStaticMarkup() is not the same as renderToString(), as it is not rendering React attributes and markup won't be resolved on client-side.

Anyway, I solved it already by inheriting SafePureComponent from PureComponent and implementing error catching in its render().
The implementation of render() is calling safeRender(), which is wrapped in try/catch and implemented in all components derived from SafePureComponent.

@zekchan
Copy link

zekchan commented Apr 22, 2018

Workaround with try-catch'ing render method will help you only if error throw in that render method. But if error occurs deep in react tree it will not.
renderToString() method in react 16 render only one special attribute - data-reactroot

@asmagin
Copy link

asmagin commented Apr 22, 2018

It is true, that's why I'm creation all components from SafePureComponent.
Also the root cause of the problem is streaming that in React16 used for renderToStaticMarkup() as well (by the moment your error happened, part of the component is sent already)

@japesh
Copy link

japesh commented Jul 24, 2018

try-catch'ing the renderToString method can also be helpful at server side as mentioned in following issue #3730

@abhirajambasta
Copy link

@mwamedacen
Copy link

Thank you @abhirajambasta for react-isomorphic-error-boundary package. However there's a limitation to it (see nayaknotnull/react-isomorphic-error-boundary#6).

Any update about this subject please ?

jnatten added a commit to NDLANO/ndla-frontend that referenced this issue Sep 20, 2024
This introduced a bug where a thrown error in the `masthead` would fail
on server because of SSR limitations of `componentDidCatch`.

See: facebook/react#10442 (comment)

We should explore ways to get around this, but for now lets go back to
the sad way.

This reverts commit 306b641.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

8 participants