Framework: Upgrade to React 0.14 #1498

aduth · 2015-12-11T16:09:06Z

This pull request seeks to upgrade Calypso from React 0.13.3 to React 0.14.3. This requires a significant number of changes, particularly related to the split between react and react-dom packages (see full change log).

The upgrade results in many warnings during testing and development. These include advisements to use react-addons- packages in place of react/addons and against rendering to the document.body element (common in tests). The purpose of this pull request is not to resolve these warnings and assumes that they'll be addressed separately.

However, warnings on use of getDOMNode were included in this pull request, as they are pervasive enough to be considered obnoxious in using the application in development environments. All instances of getDOMNode have been replaced with direct ref usage for DOM elements (since a ref is the DOM node reference in React 0.14) or React.findDOMNode (when the element is a React component or references the this instance directly).

Testing instructions:

Run make test from the root directory and verify that all tests pass (with warnings).

Ensure that regressions should be found in navigating the application in the browser.

Code verification:

Ensure that no ReactDOM methods are used with the base React package.

Verify that all removals of getDOMNode apply only to DOM elements.

gwwar · 2015-12-11T18:53:57Z

Wow this is massive, thanks for taking on this issue! I took a quick spin of the branch and found a minor issue in infinite-list which I pushed to the branch. Feel free to squash that in later.

👍 Everything else I tested worked well, plans, editor, settings etc, but since this whole changeset is so large, it'd be good to have a lot more 👀 on this. The console was pretty chatty with warnings, but we can tackle those in future PRs.

For anyone else taking a look I would recommend taking a closer look at any places where we replaced getDOMNode() with the bare react instance, instead of a DOM node. In most cases this is fine, but we should double check that each react instance has those node properties.

sirbrillig · 2015-12-11T19:42:14Z

client/components/section-nav/tabs.jsx

@@ -139,7 +140,7 @@ var NavTabs = React.createClass( {
 				return;
 			}

-			navGroupWidth = this.refs.navGroup.getDOMNode().offsetWidth;
+			navGroupWidth = this.refs.navGroup.offsetWidth;


Here we use this.refs.navGroup.offesetWidth and above we use ReactDom.findDOMNode( this.refs[ 'tab-' + index ] ).offsetWidth. It seems to me that one of these is incorrect or at least redundant? (Or maybe not since one is a React component and the other is a DOM component?)

(Or maybe not since one is a React component and the other is a DOM component?)

This is correct. When the ref is applied to a DOM component (in this case, a div), the ref returns the DOM node directly, whereas in the case where it is a React element (in the above case, a <SectionTab />), we still must use ReactDom.findDOMNode if we want access to its rendered node.

Cool. That makes sense now.

sirbrillig · 2015-12-11T20:07:07Z

Looks good from a code point of view! 95% of the changes are just replacing React with ReactDom or getDOMNode with the direct reference. For those other eyes who look at this, maybe check those places where there were changes that didn't just change modules (I wonder if there's some way to generate a view like that in GH?)

blowery · 2015-12-12T14:58:46Z

@aduth out of curiosity, did you do this with the jscodeshift script or by hand? https://github.com/reactjs/react-codemod/blob/master/README.md

blowery · 2015-12-12T15:04:29Z

Functional testing looks good

aduth · 2015-12-12T15:19:32Z

@aduth out of curiosity, did you do this with the jscodeshift script or by hand?

I kinda wish I had known of this earlier (though I'm not sure how well this script would work with the additional imports required?). This was entirely by hand, with some help from mass file search.

I really appreciate everyone's help with reviews. This is a big one. I'll hope to merge on Monday after another round of rebase + search. Already ran into two separate merge conflicts on Friday, so really don't want to let this one linger too long.

ockham · 2015-12-14T13:22:05Z

There's another package.json that specifies a react dependency in https://github.com/Automattic/wp-calypso/blob/master/client/my-sites/all-sites-icon/package.json#L7, which is currently at 0.12.1. Do we want to upgrade that, or can we just drop it?

aduth · 2015-12-14T13:26:19Z

@ockham : Good catch. It's my understanding that nested package.json aren't considered for their dependencies, but are only used these days to control the entry point of a directory. In which case, I think we can safely remove this file. I'll do a scan over the entire codebase for similar instances.

gziolo · 2015-12-14T13:29:17Z

client/auth/controller.js

@@ -1,6 +1,7 @@
 /**
 * External dependencies
 */
+import ReactDom from 'react-dom';


In the future we could even shorten it to:

import { render } from 'react-dom'; ... render( ... );

In the future we could even shorten it to:

Worth discussing, sure. Sometimes I worry that the brevity of it loses important context, i.e. in this case, not as clear that it's using React's render and not some local helper function I've declared.

r-dom is not used and should be removed

We should seek to eliminate this component altogether. The only hold-up now that React 0.14 has introduced timeouts is to remove dependence on Dialog `onClosed`, which is only used by the Reader in unmounting the article component after close.

Required in react-redux when using React 0.14

Framework: Upgrade to React 0.14

aduth · 2015-12-14T14:27:06Z

Worth pointing out that the changes here did not capture all instances of using a ref that could have been simplified to not use ReactDom.findDOMNode, but instead leverages findDOMNode overloading which returns the same value if passed a DOM node.

aduth added Framework [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels Dec 11, 2015

aduth mentioned this pull request Dec 11, 2015

Notices: Redux everything! #1496

Merged

sirbrillig reviewed Dec 11, 2015
View reviewed changes

aduth force-pushed the update/react-0-14 branch from 8f8d229 to 03ded7e Compare December 11, 2015 20:43

gziolo reviewed Dec 14, 2015
View reviewed changes

aduth force-pushed the update/react-0-14 branch 2 times, most recently from eae26a4 to 3b4772f Compare December 14, 2015 14:05

aduth added 7 commits December 14, 2015 09:09

Framework: Upgrade React to 0.14 with split ReactDom

21cacae

Framework: Upgrade React peer dependencies to latest

45dd717

r-dom is not used and should be removed

Framework: Fix tests broken as a result of React 0.14 upgrade

8549b2d

Framework: Remove all references to getDOMNode

e511e6f

Framework: Render ReduxProvider as direct child

51998c8

Required in react-redux when using React 0.14

Framework: Remove nested package.json React dependencies

b1f1e74

aduth force-pushed the update/react-0-14 branch from 3b4772f to b1f1e74 Compare December 14, 2015 14:10

aduth added [Status] Ready to Merge and removed [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels Dec 14, 2015

aduth added a commit that referenced this pull request Dec 14, 2015

Merge pull request #1498 from Automattic/update/react-0-14

a265f8b

Framework: Upgrade to React 0.14

aduth merged commit a265f8b into master Dec 14, 2015

aduth deleted the update/react-0-14 branch December 14, 2015 14:19

scruffian removed the [Status] Ready to Merge label Dec 14, 2015

aduth mentioned this pull request Dec 14, 2015

Exporter: Move data into the Redux store #500

Merged

This was referenced Dec 21, 2015

Framework: Replace LinkedStateMixin addon with standalone package #1901

Merged

Framework: Update React addons dependency if addons are unused #1917

Merged

This was referenced Jan 15, 2016

Translations appear as [object Object] when community translator is enabled #2455

Closed

Translator Jumpstart: Add back textOnly support #2457

Merged

aduth mentioned this pull request Jan 18, 2017

Docs: Add routing.md #10719

Merged

This was referenced Nov 2, 2020

[Snyk] Security upgrade node-sass from 3.3.3 to 3.7.0 NonProphets/wp-calypso#28

Open

[Snyk] Security upgrade node-sass from 3.4.2 to 3.7.0 mayuba/wp-calypso#41

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Framework: Upgrade to React 0.14 #1498

Framework: Upgrade to React 0.14 #1498

aduth commented Dec 11, 2015

gwwar commented Dec 11, 2015

sirbrillig Dec 11, 2015

aduth Dec 11, 2015

sirbrillig Dec 11, 2015

sirbrillig commented Dec 11, 2015

blowery commented Dec 12, 2015

blowery commented Dec 12, 2015

aduth commented Dec 12, 2015

ockham commented Dec 14, 2015

aduth commented Dec 14, 2015

gziolo Dec 14, 2015

aduth Dec 14, 2015

aduth commented Dec 14, 2015

Framework: Upgrade to React 0.14 #1498

Framework: Upgrade to React 0.14 #1498

Conversation

aduth commented Dec 11, 2015

gwwar commented Dec 11, 2015

sirbrillig Dec 11, 2015

Choose a reason for hiding this comment

aduth Dec 11, 2015

Choose a reason for hiding this comment

sirbrillig Dec 11, 2015

Choose a reason for hiding this comment

sirbrillig commented Dec 11, 2015

blowery commented Dec 12, 2015

blowery commented Dec 12, 2015

aduth commented Dec 12, 2015

ockham commented Dec 14, 2015

aduth commented Dec 14, 2015

gziolo Dec 14, 2015

Choose a reason for hiding this comment

aduth Dec 14, 2015

Choose a reason for hiding this comment

aduth commented Dec 14, 2015