Skip to content

Add docs for getDerivedStateFromError [16.6] #1223

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

Merged
merged 3 commits into from
Oct 23, 2018

Conversation

bvaughn
Copy link
Contributor

@bvaughn bvaughn commented Oct 3, 2018

Do not merge until React 16.6 has been released

Update React.Component reference and "Error Boundary" docs pages to include the newly added static getDerivedStateFromError() lifecycle.

@reactjs-bot
Copy link

reactjs-bot commented Oct 3, 2018

Deploy preview for reactjs ready!

Built with commit 09aeb02

https://deploy-preview-1223--reactjs.netlify.com

For more details, see [*Error Handling in React 16*](/blog/2017/07/26/error-handling-in-react-16.html).

`componentDidCatch()` is called during the "commit" phase, so side-effects are permitted.
It can be used for things like logging errors:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Edit: It should be used

@@ -22,18 +22,21 @@ Error boundaries are React components that **catch JavaScript errors anywhere in
> * Server side rendering
> * Errors thrown in the error boundary itself (rather than its children)

A class component becomes an error boundary if it defines a new lifecycle method called `componentDidCatch(error, info)`:
A class component becomes an error boundary if it defines either (or both) of the lifecycle methods [`static getDerivedStateFromError()`](/docs/react-component.html#static-getderivedstatefromerror) or [`componentDidCatch()`](/docs/react-component.html#componentdidcatch).
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I'm new to React, I read this and think "Why are there two methods that do the same thing?"

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's good input. I'll add a little more context there.


> Note
>
> Error boundaries only catch errors in the components **below** them in the tree. An error boundary can’t catch an error within itself.
> In the event of an error, you can render a fallback UI with `componentDidCatch()` by calling `setState`, but this will be deprecated in a future release.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we move this explanation earlier, and explain that defining componentDidCatch without also defining getDerivedStateFromError is also deprecated, and in the future you must define getDerivedStateFromError in order for the component to act as an error boundary?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I was on the fence about this. It felt a little odd to say this in the docs before we've actually officially deprecated the method.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree, but if we're going to have the two methods we should probably explain why

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we do explain why, by saying one is used for rendering fallback UI and the other is used for e.g. logging. I was just a bit reluctant to say "and the other will also be deprecated" until we've actually done that.

It also feels a bit awkward to move the warning any earlier because then it would disconnect the code example. I don't feel strongly about this.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've made the other two edits. I'm not sure what to do with this one. If you have concrete wording or placement suggestions though I'm happy to make them.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok maybe if you change the other sentence above then it will fix my concern

@@ -22,18 +22,21 @@ Error boundaries are React components that **catch JavaScript errors anywhere in
> * Server side rendering
> * Errors thrown in the error boundary itself (rather than its children)

A class component becomes an error boundary if it defines a new lifecycle method called `componentDidCatch(error, info)`:
A class component becomes an error boundary if it defines either (or both) of the lifecycle methods [`static getDerivedStateFromError()`](/docs/react-component.html#static-getderivedstatefromerror) or [`componentDidCatch()`](/docs/react-component.html#componentdidcatch). Use `static getDerivedStateFromError()` to render a fallback UI after an error has been thrown. Use `componentDidCatch()` to log error information.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok yeah this looks better

@bvaughn bvaughn added the v16.6 label Oct 3, 2018
@bvaughn
Copy link
Contributor Author

bvaughn commented Oct 3, 2018

I've tagged this as next release: don't merge to make it easy to merge when the release goes out (since I won't be around to remember to do it).

@bvaughn bvaughn changed the title Updated error boundary docs to reference new getDerivedStateFromError Add docs for getDerivedStateFromError [16.6] Oct 3, 2018

> Note
>
> Error boundaries only catch errors in the components **below** them in the tree. An error boundary can’t catch an error within itself.
> In the event of an error, you can render a fallback UI with `componentDidCatch()` by calling `setState`, but this will be deprecated in a future release.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what are advantages of separating those 2 things? i understand the part about side effects being allowed in one and not in the other one, but componentDidCatch is working for both cases at the moment, so what's the reason behind separating them? non of them should really be called often (they are last resort kind of thing) so it raises a question if they are both needed as they will for sure be confusing for newcomers

Copy link
Contributor Author

@bvaughn bvaughn Oct 6, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a great question! 😄Hopefully documentation will be sufficient to avoid confusion for newcomers, but it's a valid concern.

My understanding of this is that there are a couple of motivating factors:

It works with server-side rendering. componentDidCatch is a commit phase lifecycle, but there is no commit phase on the server. getDerivedStateFromError is a render phase lifecycle, and so it can be used to enable error handling on the server.

Render phase recovery is safer. The story for error recovery via componentDidCatch is a little janky, since it relies on an intermediate commit of "null" for everything below the component that errored. This might result in subsequent errors inside of any components higher up in the tree that implement componentDidMount or componentDidUpdate and just assume that their refs will be non-null (because they always are in the non-error case).

It's more optimal for async rendering. Because state-updates from commit phase lifecycles are always synchronous, and because componentDidCatch is called during the commit phase– using componentDidCatch for error recovery is not optimal because it forces the fallback UI to always render synchronously. (This is admittedly not a huge concern, since error recovery should be an edge case.)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I ofc dont judge the chosen APIs too harshly as Im not competent enough to do that, not knowing all intricacies of the implementation. Just commenting from the user's perspective. And thanks for the explanation!

Just want to comment a little bit on the given arguments

  • It works with server-side rendering - this is appealing
  • Render phase recovery is safer. - seems like implementation detail that could be handled differently, but i understand it would probably make the implementation more clunky as it would have to be special cased commit hook
  • It's more optimal for async rendering. - not the strongest argument (as you have mentioned - those should be edge cases and thus being optimal for rendering shouldnt matter as much)

@gaearon gaearon merged commit 43339e2 into reactjs:master Oct 23, 2018
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.

6 participants