Closed Bug 1312236 Opened 3 years ago Closed 3 years ago

Update React to 15.3.2

Categories

(DevTools :: Shared Components, defect, P2)

defect

Tracking

(firefox53 fixed)

RESOLVED FIXED
Firefox 53
Tracking Status
firefox53 --- fixed

People

(Reporter: linclark, Assigned: jsnajdr)

References

Details

Attachments

(4 files, 4 obsolete files)

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.
Assignee: nobody → lclark
Priority: -- → P2
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?
Flags: needinfo?(jlong)
Flags: needinfo?(jlaster)
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.
Flags: needinfo?(jdescottes)
Flags: needinfo?(gtatum)
Attached patch Bug1312236.patch (obsolete) — Splinter Review
Attached patch Fix memory tool for react update (obsolete) — Splinter Review
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
Attachment #8804727 - Flags: review?(lclark)
Flags: needinfo?(gtatum)
The latest debugger should fix this. Let me know if it doesn't!
Flags: needinfo?(jlong)
Depends on: 1315794
Logged bug 1315794 to update about:debugging accordingly. Patch attached and ready for review.
Flags: needinfo?(jdescottes)
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
Flags: needinfo?(gtatum)
(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
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.
Assignee: lclark → nobody
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.
(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!
MozReview-Commit-ID: 1hHF3JLg4W
MozReview-Commit-ID: FCPvqVNytO3
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.
Flags: needinfo?(jlong)
(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
Assignee: nobody → jsnajdr
Attachment #8811204 - Flags: review?(jlong)
Attachment #8811203 - Flags: review?(nfitzgerald)
Blocks: 1309866
Attachment #8804727 - Flags: review?(lclark) → review+
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.
Flags: needinfo?(gtatum)
Attachment #8804019 - Flags: review+
Attachment #8811203 - Flags: review?(nfitzgerald) → review+
(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!
Flags: needinfo?(jlong)
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.
Attachment #8811204 - Flags: review?(jlong) → review+
Doh, the debugger lives in devtools-html, not under jlongster: https://github.com/devtools-html/debugger.html
MozReview-Commit-ID: LLIjkUt5AUU
MozReview-Commit-ID: 97VoPl6tXUy
MozReview-Commit-ID: 1hHF3JLg4W
MozReview-Commit-ID: FCPvqVNytO3
Attachment #8804019 - Attachment is obsolete: true
Attachment #8804727 - Attachment is obsolete: true
Attachment #8811203 - Attachment is obsolete: true
Attachment #8811204 - Attachment is obsolete: true
Attachment #8815087 - Flags: review+
Attachment #8815088 - Flags: review+
Attachment #8815089 - Flags: review+
Attachment #8815090 - Flags: review+
(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).
Flags: needinfo?(jlaster)
Keywords: checkin-needed
Pushed by cbook@mozilla.com:
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
Keywords: checkin-needed
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.