1234905 - Upgrade React to 0.14.5

Status: RESOLVED FIXED

(Hello (Loop) :: Client, defect, P1)

Comment 1

[Chris Rafuse :crafuse]

Biggest change for upgrade is to implement npm react-dom package ReactDOM.findDOMNode() to squash warnings.

instance.getDOMNode() will be deprecated in version 0.15.x and must be replaced by ReactDOM.findDOMNode(instance).

Where to place the require("react-dom") to be accessible to all tests and js/x components?

Comment 2

[Mark Banner (:standard8)]

(In reply to Chris Rafuse :crafuse from comment #1)
> Where to place the require("react-dom") to be accessible to all tests and
> js/x components?

Have a look in Makefile for the react items which copy the library. It looks like we'll need to do similar copies for react-dom (from the dist directory for dev & prod).

We'll then need to include the react-dom script in the same places we include the react script. Also be aware in webappEntryPoint.js we'll need to require the react-dom in the __PROD__ section as well as appropriately in the non-dev section.

Comment 3

[GitHub Autolander Bot]

Attachment: [loop] chrafuse:bug-1234905-upgrade-react > mozilla:master

Comment 4

[Chris Rafuse :crafuse]

Attachment: Link to Github pull-request: https://github.com/mozilla/loop/pull/35

I have added the react-dom.js and react-dom-server.js and related into the makeFile, webappEntryPoint.js and jar.mn.  Still no scope on panel_test.js or other locations:

Firefox 43.0.0 (Ubuntu 0.0.0) loop.panel loop.panel.PanelView SettingsDropdown Toggle Notifications should toggle mozLoop.doNotDisturb to false FAILED
	ReactDOM is not defined
	@base/add-on/panels/test/panel_test.js?503514cc15b000d0bc29a0033bb131b8d03d2150:411:15

===================

With react-dom-server.js features:

@/home/chrafuse/Documents/mozilla/loop/add-on/panels/test/panel_test.js:277:40
	,Warning: React.renderToStaticMarkup is deprecated. Please use ReactDOMServer.renderToStaticMarkup from require('react-dom/server') instead.,trapErrors/console.error@/home/chrafuse/Documents/mozilla/loop/shared/test/loop_mocha_utils.js:284:15
	[173]</warning@/home/chrafuse/Documents/mozilla/loop/built/add-on/chrome/content/shared/vendor/react.js:20739:9
	deprecated/newFn@/home/chrafuse/Documents/mozilla/loop/built/add-on/chrome/content/shared/vendor/react.js:17651:40
	render@e/chrafuse/Documents/mozilla/loop/built/add-on/chrome/content/panels/js/panel.js:9:5325


I am also attaching the make karma output.  How does react get scope (ex. panel_test.js) and how to do the same for react-dom/-server?

Comment 5

[Chris Rafuse :crafuse]

Attachment: reactUpgradeTestOutput2.txt

Output from make karma.

Comment 6

[Mark Banner (:standard8)]

Comment on attachment 8702725 [details] [review]
Link to Github pull-request: https://github.com/mozilla/loop/pull/35

You need to add in react-dom.js wherever we include react.js already.

We shouldn't be needing the react-dom-server stuff - that's to do with react on the server side.

Comment 7

[Chris Rafuse :crafuse]

Attachment: Karma Output

Comment 8

[Chris Rafuse :crafuse]

Upgrading to React 0.14.5. Got down to 2 failures from 49.  Just need some direction how to resolve these 2 issues.  See Karma output file.

Comment 9

[Mark Banner (:standard8)]

Comment on attachment 8704182 [details]
Karma Output

'Warning: Failed propType: Required prop `userProfile` was not correctly specified in `AccountLink`.'

- I would take a look at where this is in the code, and see what's generating it. It could be that react is now throwing out extra warnings it wasn't before. So maybe our checks aren't useful or something.

ERROR: 'Warning: React.renderToStaticMarkup is deprecated. Please use ReactDOMServer.renderToStaticMarkup from require('react-dom/server') instead.'

- We might need to look at the docs here, and either see if there's a non-server replacement, or include the server stuff. I can take a look later.

ERROR: 'Warning: setProps(...) and replaceProps(...) are deprecated. Instead, call render again at the top level.'

- Probably check the docs about how these have been deprecated and if render can pass different props. I seem to remember we're only doing this from test code, but the fact it isn't available might mess us up a bit.

Comment 10

[Chris Rafuse :crafuse]

Attachment: Karma Output

Some insight into testing rigs as I have this issue now.

https://github.com/facebook/react/issues/4692

Also updated babel-cli into this patch as most of the issues were about domNode access and have fixed in this patch.

May need to pair with you to hack some of the code pattern changes.

Comment 11

[Mark Banner (:standard8)]

Can you update your branch on github please? Its easier for me to have a poke around with the code then.

Comment 12

[Chris Rafuse :crafuse]

Attachment: Karma Output

Rebased to latest master with babel-cli updates.  Had some issues on make build as it did not check that the node modules were installed from the package.json.

Why does make build not install the latest versions from the package.json?
Does add-on not check this?

from make install:
...
make: ./node_modules/.bin/babel: Command not found
make: *** [add-on] Error 127
chrafuse@ubuntu:~/Documents/mozilla/loop$ make node_modules 
make: `node_modules' is up to date.
chrafuse@ubuntu:~/Documents/mozilla/loop$

May need some attention to makeFile if we want this functionality.

Comment 13

[Mark Banner (:standard8)]

Comment on attachment 8704723 [details]
Karma Output

I sent you some hints in email.

Comment 14

[Chris Rafuse :crafuse]

Attachment: Karma Output

Comment 15

[Chris Rafuse :crafuse]

Comment on attachment 8704940 [details]
Karma Output

Updated React.render to ReactDOM.render in standalone and chat views.
New errors appear.

Comment 16

[Mark Banner (:standard8)]

Comment on attachment 8704940 [details]
Karma Output

I've put various comments on the PR directly.

I think if you fix the comments, then you'll be close to passing, save for the warnings about setProps. The setProps items are only used in tests afaik, we'll have to look at on a case-by-case basis. I think some of them are actually effectively just testing react, but some others are checking transitions, and we'll have to look at how we can change those.

Comment 17

[Chris Rafuse :crafuse]

Found this for TestUtils replacement for shallow rendering tests.
NPM module react-shallow-testutils:
https://www.npmjs.com/package/react-shallow-testutils

Comment 18

[Mark Banner (:standard8)]

We should just get this landed.

Comment 19

[Mark Banner (:standard8)]

Comment on attachment 8702718 [details] [review]
[loop] chrafuse:bug-1234905-upgrade-react > mozilla:master

r=Standard8. This was initially written by Chris, and then we spent time pair programming to complete it.

Comment 20

[Mark Banner (:standard8)]

https://github.com/mozilla/loop/commit/a9f968961fb24cd6da459336a0c93da79a88c21f