Deprecated lifecycles #6341

frehner · 2018-09-18T22:54:56Z

Removed all instances of componentWillMount and componentWillReceiveProps

For the most part, cWM was changed to cDM, except in cases where a parent's function needed to be run before a child's, so those were moved into the constructor. (see Router.js as an example).

cWRP was replaced with componentDidUpdate. In one case where infinite loops could happen, I added an equality check to stop that. (see Route.js).

Updated all documentation and examples to remove the old lifecycles and use their supported counterparts.

It appears that I'm the first person to change many of these files after the addition of prettier as a git hook. Many changes, such as adding ; and changing single to double quotes ", appear to be a result of that.

Tests all pass. I linked this updated code to one of my projects and it appears to all work the same as before, but I only use pretty basic RR-dom components and it's not exhaustive.

Hopefully resolves #6060 and is a more robust version of #6256

Moving invariant calls and other things that used to be in cWM into the constructor so that the behavior is the same as before, which also gets all the tests to pass

timdorr · 2018-09-19T13:46:52Z

packages/react-router/modules/Router.js

+    // Do this here so we can setState when a <Redirect> changes the
+    // location in componentDidMount. This happens e.g. when doing
+    // server rendering using a <StaticRouter>.
+    this.unlisten = history.listen(() => {


The listener should be done in a componentDidMount. This will fire during server rendering and even if the component never gets mounted.

The challenge with having it in cDM (which is where I had it originally) is that -- from what I found and the tests that failed -- React doesn't guarantee the order of whether a parent or child's cDM is fired first. Which means that the Redirect component doesn't work, since the logic for redirection happens in cDM as well.

Maybe I could look at putting it in both places and only running if it's in a static router or something? Not sure. Any ideas?

The React blog has some info on this: https://reactjs.org/blog/2018/03/27/update-on-async-rendering.html#adding-event-listeners-or-subscriptions

@timdorr from what I'm reading, they recommend using cDM. Yet that doesn't solve the issue I mentioned above.

I pushed up an alternative solution, what do you think of it?

timdorr · 2018-09-19T13:47:57Z

packages/react-router/modules/StaticRouter.js

@@ -90,7 +90,7 @@ class StaticRouter extends React.Component {

  handleBlock = () => noop;

-  componentWillMount() {
+  componentDidMount() {


StaticRouter is mainly for server rendering, so this lifecycle method won't fire on the server. It should be in a constructor instead.

timdorr · 2018-09-19T13:49:31Z

packages/react-router-redux/package-lock.json

@@ -12,21 +12,21 @@
 			"resolved": "https://registry.npmjs.org/encoding/-/encoding-0.1.12.tgz",
 			"integrity": "sha1-U4tm8+5izRq1HsMjgp0flIDHS+s=",
 			"requires": {
-				"iconv-lite": "~0.4.13"
+				"iconv-lite": "0.4.19"


Based on these lines, it appears you're using an older version of npm. Update to npm@6.4.0 and re-run these installs. That should cut down on the number of changes here.

Ah, I was wondering what was going on here. I tried both 8 and 9, but didn't go to 10. 👍

timdorr · 2018-09-19T13:54:04Z

packages/react-router/package.json

@@ -41,6 +41,7 @@
    "history": "^4.7.2",
    "hoist-non-react-statics": "^2.5.0",
    "invariant": "^2.2.4",
+    "lodash.isequal": "^4.5.0",


Can we use something else to achieve this? This adds a fair number of bytes to the bundle, so I'd like to avoid outside dependencies where we can.

Sure -- I only used it to compare match here (if that link doesn't work, it's in Route.js line 109)

Do you have any other suggestions of how to compare match?

:

Changed location of staticrouter's warning

…static context or not.

frehner · 2018-09-19T15:26:28Z

@timdorr updated, though I'm curious to see if you think this is a good solution to the problem or not.

pshrmn · 2018-09-19T19:43:53Z

packages/react-router/modules/Route.js

-    this.setState({
-      match: this.computeMatch(nextProps, nextContext.router)
-    });
+    const newMatch = this.computeMatch(this.props, this.context.router);


I don't really like re-computing the match in the commit phase; this would spend a render phase rendering based on the outdated state.match just to throw away that work and re-render using the real match.

If it wasn't for the getChildContext() to support legacy context, I would probably just compute the match in render() (maybe looking into memoizing computing matches). This is probably a decent use case for getDerivedStateFromProps(), but would break apps apps that use pre-16.3 version of React (or is there a way to backport gDSFP?).

There's react-lifecycles-compat, but it's rather large.

We've found most folks are on 16.3+ over on React Redux, FWIW.

Yeah, to my understanding, this is probably the best way to do things without a breaking change (and without including lifecycles-compat).

I'm open to ideas though!

frehner · 2018-09-20T14:32:12Z

@timdorr just checking in to see if there's anything else preventing this from merging?

merge from upstream

frehner · 2018-09-20T22:23:42Z

@timdorr just merged in from your latest work in master (awesome!) and resolved conflicts. Personally, I think it would be great if this could also make the beta mentioned by @mjackson in #5908 (assuming it's ready)

timdorr · 2018-09-21T01:17:28Z

Michael let me know he's planning on giving Router his full attention over the coming week, so I expect it will make its way in somehow!

mjackson · 2018-09-25T19:41:25Z

Thanks for the PR, @frehner :)

I did some major refactoring over the past week and eliminated all the deprecated componentWill methods in the process, so we won't need this anymore. You can see some of my commits here and here.

My apologies for not guiding you through this work so we could get this merged, but it was a little tricky and I wanted to make sure I understood all of the implications of making this change, so I decided to do it myself. You can expect this to be published in our next beta release sometime this week. 😅

frehner · 2018-09-25T22:35:58Z

No worries, just glad to see it done! Thanks!

Anthony Frehner added 4 commits September 18, 2018 09:56

First attempt, though tons of tests are failing.

3dd8cd7

Preventing infinite rendering by doing a comparison in cDU

4be74b6

Moving invariant calls and other things that used to be in cWM into the constructor so that the behavior is the same as before, which also gets all the tests to pass

self review

af39f87

Feedback

7c36242

This was referenced Sep 19, 2018

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

Closed

Removal of React 16.3 unsafe lifecycles #6256

Closed

Unsafe Lifecycle warning in React >= 16.3 #6060

Closed

timdorr reviewed Sep 19, 2018

View reviewed changes

Anthony Frehner added 3 commits September 19, 2018 08:53

Reverted lockfiles and used node v10 / npm v6.4.1 to install

4be8fa8

:

Removed lodash.isequal and did the object comparison manually.

f85523c

Changed location of staticrouter's warning

Setup history.listen in different places depending on if you're in a …

b05fc5f

…static context or not.

pshrmn reviewed Sep 19, 2018

View reviewed changes

frehner and others added 2 commits September 20, 2018 15:50

Merge pull request #2 from ReactTraining/master

6af5ada

merge from upstream

Merged in from upstream, and resolved merge conflicts

8b9374c

mjackson closed this Sep 25, 2018

lock bot locked as resolved and limited conversation to collaborators Nov 24, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Deprecated lifecycles #6341

Deprecated lifecycles #6341

frehner commented Sep 18, 2018

timdorr Sep 19, 2018

frehner Sep 19, 2018 •

edited

Loading

timdorr Sep 19, 2018

frehner Sep 19, 2018

timdorr Sep 19, 2018

timdorr Sep 19, 2018

frehner Sep 19, 2018

timdorr Sep 19, 2018

frehner Sep 19, 2018 •

edited

Loading

frehner commented Sep 19, 2018

pshrmn Sep 19, 2018

timdorr Sep 19, 2018

frehner Sep 19, 2018

frehner commented Sep 20, 2018

frehner commented Sep 20, 2018 •

edited

Loading

timdorr commented Sep 21, 2018

mjackson commented Sep 25, 2018

frehner commented Sep 25, 2018

Deprecated lifecycles #6341

Deprecated lifecycles #6341

Conversation

frehner commented Sep 18, 2018

Choose a reason for hiding this comment

frehner Sep 19, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

frehner Sep 19, 2018 • edited Loading

Choose a reason for hiding this comment

frehner commented Sep 19, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

frehner commented Sep 20, 2018

frehner commented Sep 20, 2018 • edited Loading

timdorr commented Sep 21, 2018

mjackson commented Sep 25, 2018

frehner commented Sep 25, 2018

frehner Sep 19, 2018 •

edited

Loading

frehner Sep 19, 2018 •

edited

Loading

frehner commented Sep 20, 2018 •

edited

Loading