1312236 - Update React to 15.3.2

Status: RESOLVED FIXED

(DevTools :: Shared Components, defect, P2)

Description

[Lin Clark [:linclark]]

For performance reasons, we should upgrade to React 15. I haven't benchmarked myself, but here's an article which covers the difference: https://medium.com/modus-create-front-end-development/component-rendering-performance-in-react-df859b474adc#.dwsjxrnev

React 15.3 also gives us PureComponent, which we could use for Reps to easily prevent unnecessary rerenders.

Comment 1

[Lin Clark [:linclark]]

I did a Talos run for this to make sure that it wasn't doing anything wild. All looks good except for debugger.

https://treeherder.mozilla.org/perf.html#/comparesubtest?originalProject=mozilla-inbound&originalRevision=795b9e63823c3750fd9c1a573cdbb0961c994bad&newProject=try&newRevision=09cbcfbfcba1e71513a3a555ba61426f3aa1c5bd&originalSignature=5cb8eb98fa756439ed12f7180a59e08c675b8b67&newSignature=5cb8eb98fa756439ed12f7180a59e08c675b8b67&framework=1

The increase for open in the complicated case is 245%. This makes me wonder... is there any chance that the debugger is using react-dev in prod? The article I included above says that react-dev is about 2x slower in 15 than it was in .14, so that would match up with those numbers. If that's not it, any other ideas?

Comment 2

[Lin Clark [:linclark]]

Here's a try push:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=09cbcfbfcba1&selectedJob=29718489

It looks like there are failing tests in memory tool and aboutdebugging.

Comment 3

[Lin Clark [:linclark]]

Attachment: Bug1312236.patch

Comment 4

[Greg Tatum [:gregtatum]]

Attachment: Fix memory tool for react update

Not sure how you want to land all of this, but here's the patch for fixing
those memory tool issues. It is dependent on the new React version being
there, and will break with the old version.

MozReview-Commit-ID: 97VoPl6tXUy

Comment 5

[James Long (:jlongster)]

The latest debugger should fix this. Let me know if it doesn't!

Comment 6

[Julian Descottes [:jdescottes]]

Logged bug 1315794 to update about:debugging accordingly. Patch attached and ready for review.

Comment 7

[Lin Clark [:linclark]]

Sorry for the delay in reviewing. Would you mind posting a try push? 

Also, does this fix the tree component unit tests? e.g. https://treeherder.mozilla.org/#/jobs?repo=try&revision=9418d35224805beb8ee2d574687754965d1d0f95&selectedJob=30587287

Comment 8

[Brian Grinstead [:bgrins]]

(In reply to Lin Clark [:linclark] from comment #7)
> Sorry for the delay in reviewing. Would you mind posting a try push? 
> 
> Also, does this fix the tree component unit tests? e.g.
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=9418d35224805beb8ee2d574687754965d1d0f95&selectedJob=3
> 0587287

Try push with both patches from this bug applied: https://treeherder.mozilla.org/#/jobs?repo=try&revision=8047f9c6ca6bad03851578bfe92ec7d0a6c0a1a0

Comment 9

[Lin Clark [:linclark]]

It looks like the tests for the tree are still failing. Greg, I believe this tree falls under performance/memory tool's purview, but feel free to ping someone else about it if not.

Since I've switched off the team, I won't have time to coordinate this so I'm going to unassign myself.

Comment 10

[Jarda Snajdr [:jsnajdr]]

I'll take care of the tree tests. I worked on the tree component lately, and React 15 will make a very visible performance improvement for netmonitor.html.

Comment 11

[Brian Grinstead [:bgrins]]

(In reply to Jarda Snajdr [:jsnajdr] from comment #10)
> I'll take care of the tree tests. I worked on the tree component lately, and
> React 15 will make a very visible performance improvement for
> netmonitor.html.

Thanks!

Comment 12

[Jarda Snajdr [:jsnajdr]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=bf07081e66a5

Comment 13

[Jarda Snajdr [:jsnajdr]]

Attachment: Fix Tree component tests after upgrade to React 15

MozReview-Commit-ID: 1hHF3JLg4W

Comment 14

[Jarda Snajdr [:jsnajdr]]

Attachment: Fix debugger.html tests after upgrade to React 15

MozReview-Commit-ID: FCPvqVNytO3

Comment 15

[Jarda Snajdr [:jsnajdr]]

Attached two patches that fix the failing tests.

First there are the shared components - tree, stack trace, splitter. Deprecated component methods "setProps" and "getDOMNode" were removed in React 15, so I replaced them with the supported way how to accomplish these tasks.

In case of "setProps", the right way is to call ReactDOM.render again with the new props.

The React component tests contain a lot of code that was probably intended for some older React version and don't make much sense today. Like waiting for next animation frame after updating props/state and before checking the DOM. Or calling setState() twice to "force render". All these things are completely sync today and no waiting is needed.

The only exception is waiting until a "focus" event is fired on a DOM node, which triggers a re-render. This is not sync, and currently the tests call forceRender() to "wait". This works most of the time, but doesn't really guarantee any event ordering.

All these tests should be rewritten to Enzyme sooner or later.

I also fixed one debugger.html test: browser_dbg-sources.js. It's a trivial markup change - old React wrapped text nodes in a "span", the new one doesn't.

Here are the next steps:

1. Who owns this bug? Anyone objects if I take it and finish it?
2. Anything else needs to be changed as part of the React 15 update, except the patches that are already attached?
3. Who wants to review all this? Lin, Nick and Jordan are no longer on the team.
4. The debugger.html test should be fixed in the Github repo. James, what's the right way how to land this?
5. React 15.4 might get released before we land this. It's in RC3 now.

Comment 16

[Brian Grinstead [:bgrins]]

(In reply to Jarda Snajdr [:jsnajdr] from comment #15)
> 1. Who owns this bug? Anyone objects if I take it and finish it?

Please do!

> 5. React 15.4 might get released before we land this. It's in RC3 now.

That's fine, I'm assuming the migration from 15.3 will be easy so we can do that update in another bug if it's not released by the time this is ready to go

Comment 17

[Jarda Snajdr [:jsnajdr]]

Comment on attachment 8804019 [details] [diff] [review]
Bug1312236.patch

Setting r+ on the main React 15 patch. Tests are fixed, try is green, talos perf results are good.

Comment 18

[James Long (:jlongster)]

(In reply to Jarda Snajdr [:jsnajdr] from comment #15)
> 4. The debugger.html test should be fixed in the Github repo. James, what's
> the right way how to land this?

You can go ahead and land it with this patch so you can land the upgrade. However, the right way is to land it here: https://github.com/jlongster/debugger.html/tree/master/public/js/test/mochitest When we land the next debugger update the tests will be copied over, so that's where the real tests live.

Do you mind making a PR?

Thanks so much for seeing this through!

Comment 19

[James Long (:jlongster)]

Comment on attachment 8811204 [details] [diff] [review]
Fix debugger.html tests after upgrade to React 15

Review of attachment 8811204 [details] [diff] [review]:
-----------------------------------------------------------------

r+ so you can land the upgrade, but if you can, please open a PR to make the change here: https://github.com/jlongster/debugger.html/tree/master/public/js/test/mochitest

If you don't have time, I can do it instead.

Comment 20

[James Long (:jlongster)]

Doh, the debugger lives in devtools-html, not under jlongster: https://github.com/devtools-html/debugger.html

Comment 21

[Jarda Snajdr [:jsnajdr]]

Attachment: Update React to 15.3.2 r=jsnajdr

MozReview-Commit-ID: LLIjkUt5AUU

Comment 22

[Jarda Snajdr [:jsnajdr]]

Attachment: Fix Memory tool for React 15 update r=jsnajdr

MozReview-Commit-ID: 97VoPl6tXUy

Comment 23

[Jarda Snajdr [:jsnajdr]]

Attachment: Fix Tree component tests after upgrade to React 15 r=fitzgen

MozReview-Commit-ID: 1hHF3JLg4W

Comment 24

[Jarda Snajdr [:jsnajdr]]

Attachment: Fix debugger.html tests after upgrade to React 15 r=jlongster

MozReview-Commit-ID: FCPvqVNytO3

Comment 25

[Jarda Snajdr [:jsnajdr]]

(In reply to James Long (:jlongster) from comment #19)
> r+ so you can land the upgrade, but if you can, please open a PR to make the
> change here:
> https://github.com/jlongster/debugger.html/tree/master/public/js/test/
> mochitest

I'll do the PR (to the right repo).

Comment 26

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4a3b9385a2e2
Update React to 15.3.2 r=jsnajdr
https://hg.mozilla.org/integration/mozilla-inbound/rev/98b965a62c11
Fix Memory tool for React 15 update r=jsnajdr
https://hg.mozilla.org/integration/mozilla-inbound/rev/e50389904bee
Fix Tree component tests after upgrade to React 15 r=fitzgen
https://hg.mozilla.org/integration/mozilla-inbound/rev/ea48aaafb70a
Fix debugger.html tests after upgrade to React 15 r=jlongster

Comment 27

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/4a3b9385a2e2
https://hg.mozilla.org/mozilla-central/rev/98b965a62c11
https://hg.mozilla.org/mozilla-central/rev/e50389904bee
https://hg.mozilla.org/mozilla-central/rev/ea48aaafb70a