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

Docs - Replace use of deprecated lifecycle methods with the methods r… #6313

Closed
wants to merge 1 commit into from
Closed

Docs - Replace use of deprecated lifecycle methods with the methods r… #6313

wants to merge 1 commit into from

Conversation

jmuzsik
Copy link

@jmuzsik jmuzsik commented Aug 30, 2018

Replace use of deprecated lifecycle methods with the methods recommended by the React team in the documentation.

I am not sure if the automatic editing of the files because of this precommit hook: "precommit": "pretty-quick --staged" was supposed to happen but I assume that the style changes are desired. All I changed was replacing componentWillReceiveProps with componentDidUpdate and add one sentence that it is not recommended to use the lifecycle methods that are soon to be deprecated with a link that explains.

I used componentDidUpdate in each example as that is what is recommended: Check out Abramov comment.

Also, if desired, I can go through the docs and make sure that each page in the docs gets the precommit hook and send another quick PR.

Thanks!


## history is mutable

The history object is mutable. Therefore it is recommended to access the [`location`](./location.md) from the render props of [`<Route>`](./Route.md), not from `history.location`. This ensures your assumptions about React are correct in lifecycle hooks. For example:

```jsx
class Comp extends React.Component {
componentWillReceiveProps(nextProps) {
componentDidMount(nextProps) {
Copy link
Member

Choose a reason for hiding this comment

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

I think you mean cDU here.

```

### `on*` properties

React Router v3 provides `onEnter`, `onUpdate`, and `onLeave` methods. These were essentially recreating React's lifecycle methods.

With v4, you should use the lifecycle methods of the component rendered by a `<Route>`. Instead of `onEnter`, you would use `componentDidMount` or `componentWillMount`. Where you would use `onUpdate`, you can use `componentDidUpdate` or `componentWillUpdate` (or possibly `componentWillReceiveProps`). `onLeave` can be replaced with `componentWillUnmount`.
With v4, you should use the lifecycle methods of the component rendered by a `<Route>`. Instead of `onEnter`, you would use `componentDidMount` or `componentWillMount`. Where you would use `onUpdate`, you can use `componentDidUpdate` or `componentWillUpdate` (or possibly `componentWillReceiveProps`). `onLeave` can be replaced with `componentWillUnmount`. Though, `componentWillUpdate`, `componentWillReceiveProps`, and `componentWillUnmount` will [soon be deprecated](https://reactjs.org/blog/2018/03/27/update-on-async-rendering.html).
Copy link
Member

Choose a reason for hiding this comment

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

componentWillUnmount is not deprecated. componentWillMount is, though.

@@ -134,7 +139,7 @@ class PendingNavDataLoader extends Component {
previousLocation: null
}

componentWillReceiveProps(nextProps) {
componentDidMount(nextProps) {
Copy link
Member

Choose a reason for hiding this comment

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

Did you mean cDU?

@@ -18,7 +18,7 @@ class Sidebar extends Component {
)
}

componentWillReceiveProps(nextProps) {
(nextProps) {
Copy link
Member

Choose a reason for hiding this comment

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

There is no method name here.


componentWillReceiveProps(nextProps) {
componentDidMount(nextProps) {
Copy link
Member

Choose a reason for hiding this comment

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

Same here. Probably meant componentDidUpdate.

}
}
```

Hope that helps get you thinking. Again, the animations themselves are the same with the router or not, the difference is knowing when to trigger them. Here's a list for things to check in `componentWillReceiveProps` to decide what to do with an animation based on the router's location:
Hope that helps get you thinking. Again, the animations themselves are the same with the router or not, the difference is knowing when to trigger them. Here's a list for things to check in `componentDidMount` to decide what to do with an animation based on the router's location:
Copy link
Member

Choose a reason for hiding this comment

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

cDU


It is also found on `history.location` but you shouldn't use that because its mutable. You can read more about that in the [history](./history.md) doc.

A location object is never mutated so you can use it in the lifecycle hooks to determine when navigation happens, this is really useful for data fetching and animation.

```js
componentWillReceiveProps(nextProps) {
componentDidMount(nextProps) {
Copy link
Member

Choose a reason for hiding this comment

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

cDU

@@ -18,7 +18,7 @@ class Sidebar extends Component {
)
}

componentWillReceiveProps(nextProps) {
componentDidUpdate(nextProps) {
Copy link
Member

Choose a reason for hiding this comment

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

Same prevProps change here.


componentWillReceiveProps(nextProps) {
componentDidUpdate(nextProps) {
Copy link
Member

Choose a reason for hiding this comment

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

And here


## history is mutable

The history object is mutable. Therefore it is recommended to access the [`location`](./location.md) from the render props of [`<Route>`](./Route.md), not from `history.location`. This ensures your assumptions about React are correct in lifecycle hooks. For example:

```jsx
class Comp extends React.Component {
componentWillReceiveProps(nextProps) {
componentDidUpdate(nextProps) {
Copy link
Member

Choose a reason for hiding this comment

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

And here.


It is also found on `history.location` but you shouldn't use that because its mutable. You can read more about that in the [history](./history.md) doc.

A location object is never mutated so you can use it in the lifecycle hooks to determine when navigation happens, this is really useful for data fetching and animation.

```js
componentWillReceiveProps(nextProps) {
componentDidUpdate(nextProps) {
Copy link
Member

Choose a reason for hiding this comment

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

Aaaaand here.

@timdorr
Copy link
Member

timdorr commented Aug 30, 2018

Sorry for the pedantry!

@jmuzsik
Copy link
Author

jmuzsik commented Aug 30, 2018

No problem!

const navigatingToParent = nextProps.atParent && !this.props.atParent
const animationEnded = this.props.animating && !nextProps.animating
const navigatingToParent = prevProps.atParent && !this.props.atParent;
const animationEnded = this.props.animating && !prevProps.animating;
Copy link
Member

Choose a reason for hiding this comment

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

This one actually does need to be swapped around because it's checking if we were animating before and are no longer animating now.

})

// load data while the old screen remains
loadNextData(routes, nextProps.location).then((data) => {
loadNextData(routes, prevProps.location).then((data) => {
Copy link
Member

Choose a reason for hiding this comment

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

This should be this.props.location.

```

Going from child to parent:

```js
nextProps.match.isExact && !this.props.match.isExact
prevProps.match.isExact && !this.props.match.isExact;
Copy link
Member

Choose a reason for hiding this comment

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

This and the one a few lines down also need to be swapped for similar reasons.

@frehner
Copy link
Contributor

frehner commented Sep 19, 2018

I totally missed this when I created #6341 -- hopefully there aren't too many (if any) conflicts in the changes to the docs. If there are, I can take a look at reconciling them when this is merged (if this is merged first).

@jmuzsik
Copy link
Author

jmuzsik commented Sep 19, 2018

No problem, I haven't looked at this for a while as Tim vanished suddenly. If Tim wants to close this and pull yours in to keep things simple, do so by all means! No worries.

@timdorr
Copy link
Member

timdorr commented Sep 19, 2018

I'm here! I just have a lot of issues/PRs to go through daily, so I missed the update here :/

@timdorr timdorr closed this Sep 19, 2018
@lock lock bot locked as resolved and limited conversation to collaborators Nov 18, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants