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

Merge Component Lifecycle guide into React.Component reference #1

Conversation

hramos
Copy link

@hramos hramos commented Oct 10, 2016

The overview section at the top of a reference guide is a good place to cover best practices for use of the class whenever the scope is greater than any single method.

  • Merge Component Lifecycle guide into React.Component reference
  • Create React without ES6 Guide

Opening this PR against @lacker's fork so we can then merge these commits into the existing react/docs PR #7919.

screencapture-localhost-4000-react-docs-react-component-html-1476117588924


`React.Component` is an abstract base class, so it rarely makes sense to refer to `React.Component` directly. Instead, you will typically subclass it, and define at least a [`.render()`](#.render) method.

Normally you would define a React component as a plain JavaScript class:
Copy link

Choose a reason for hiding this comment

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

Let's make "JavaScript class" a link to MDN: https://developer.mozilla.org/en/docs/Web/JavaScript/Reference/Classes


#### Updating

State changes can lead to the component being re-rendered. The following methods will be called and are used to determine if the DOM should be updated.
Copy link

Choose a reason for hiding this comment

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

Maybe "Changes to props or state can lead..."? Updating is not just for state.

Copy link

Choose a reason for hiding this comment

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

The following methods will be called and are used to determine if the DOM should be updated.

The only method that determines if DOM should be updated is shouldComponentUpdate, the rest don't influence this. Can we omit "and are used to determine..."?

State changes can lead to the component being re-rendered. The following methods will be called and are used to determine if the DOM should be updated.

- [`.componentWillReceiveProps()`](#.componentwillreceivepropsnextprops) is invoked before a mounted component receives new props and can be used to update state in response to prop changes.
- Components will always re-render when the state is updated. Take a look at [`.shouldComponentUpdate(nextProps, nextState)`](#.shouldcomponentupdatenextprops-nextstate) if you need to change the default behavior.
Copy link

Choose a reason for hiding this comment

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

Components will always re-render when the state is updated

People often misinterpret "re-render" as "expensive DOM change". Can we make it clear re-rendering is usually absolutely fine?

- [`.componentWillReceiveProps()`](#.componentwillreceivepropsnextprops) is invoked before a mounted component receives new props and can be used to update state in response to prop changes.
- Components will always re-render when the state is updated. Take a look at [`.shouldComponentUpdate(nextProps, nextState)`](#.shouldcomponentupdatenextprops-nextstate) if you need to change the default behavior.
- [`.componentWillUpdate(nextProps, nextState)`](#.componentwillupdatenextprops-nextstate) can be used for preparation before an update occurs.
- [`.componentDidUpdate(prevProps, prevState)`](#.componentdidupdateprevprops-prevstate) is invoked after an update and can be used to operate on the DOM after the component has been updated.
Copy link

@gaearon gaearon Oct 10, 2016

Choose a reason for hiding this comment

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

Maybe add "... or to fetch new data in response to prop changes?"


[`.componentWillUnmount()`](#.componentwillunmount) can be used to perform any cleanup, as it is invoked immediately before a component is unmounted and destroyed.

## Reference

- [`constructor(props)`](#constructor)
Copy link

Choose a reason for hiding this comment

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

I understand these are sorted by alphabet, but maybe we can sort them by their order instead? This seems more useful to me, and it's one of the most commonly asked questions about lifecycle.

Copy link
Owner

Choose a reason for hiding this comment

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

Yeah I agree - let me do that in a subsequent PR though, since this one is merging Hector's content into the layout that I set up.

@@ -44,27 +99,47 @@ constructor(props) {

Copy link

@gaearon gaearon Oct 10, 2016

Choose a reason for hiding this comment

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

It's okay to initialize state based on props.

Only if you know what you're doing. Most beginners do this by mistake. This effectively "forks" the props and can lead to bugs.

Instead of syncing props to state, you often want to lift the state up: facebook#7920 (comment).

If you "fork" props by using them for state, you might also want to implement componentWillReceiveProps() to keep the state up-to-date with them. But lifting state up is often easier and less bug-prone.

TODO
`componentDidUpdate()` is invoked immediately after updating occurs. This method is not called for the initial render.

Use this as an opportunity to operate on the DOM when the component has been updated.
Copy link

Choose a reason for hiding this comment

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

This is also a good place to do network requests as long as you compare the props to previous props.

TODO
`componentWillMount()` is invoked immediately before mounting occurs. It is called before `render()`, therefore setting state in this method will not trigger a re-rendering. Avoid introducing any side-effects or subscriptions in this method.

This is the only lifecycle hook called on server rendering.
Copy link

Choose a reason for hiding this comment

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

Maybe add a note that generally we recommend using constructor instead?

@@ -88,7 +163,13 @@ The `displayName` string is used in debugging messages. JSX sets this value auto

Copy link

Choose a reason for hiding this comment

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

If props.color is not provided, it will be set by default to 'blue'.

Maybe mention that defaultProps are only used for undefined props, but not for null props.

@@ -155,11 +240,19 @@ The second parameter is an optional callback function that will be executed once

Copy link

Choose a reason for hiding this comment

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

The second parameter is an optional callback function that will be executed once setState is completed and the component is re-rendered.

Maybe add "Generally we recommend using componentDidUpdate() for such logic instead."

@hramos
Copy link
Author

hramos commented Oct 10, 2016

All feedback addressed. I also removed the dots in the method names. @lacker I think we can merge this into your fork now, and do any further reordering/organization in your main PR.

@lacker lacker merged commit 82171f8 into lacker:reference-react-component Oct 10, 2016
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

Successfully merging this pull request may close these issues.

3 participants