We need to follow these instructions to XUL fix: http://searchfox.org/mozilla-central/source/devtools/client/shared/vendor/REACT_UPGRADING

Comment on attachment 8908653 [details] Bug 1399493 - Upgrade to React 15.6.1 and include dev & prod version https://reviewboard.mozilla.org/r/180304/#review186506 Thanks

Thanks for doing this Mike. We should consider waiting to land this until after the 57 merge (in two days) since there won't be time for this to be tested on the Nightly cycle and there's a possibility for regression with the upgrade.

(In reply to Brian Grinstead [:bgrins] from comment #3) > Thanks for doing this Mike. We should consider waiting to land this until > after the 57 merge (in two days) since there won't be time for this to be > tested on the Nightly cycle and there's a possibility for regression with > the upgrade. Yeah... I have a few things to land but they are only for performance measurements so I have been holding off landing them for a while now.

I think this should be landable now that the merge has passed

The latest try run is failing so I need to find a way to make our tests run in React dev mode first.

(In reply to Michael Ratcliffe [:miker] [:mratcliffe] from comment #6) > The latest try run is failing so I need to find a way to make our tests run > in React dev mode first. What are the test failures, and are they fixable? I'd prefer to have the test runs match production as closely as possible

From https://treeherder.mozilla.org/#/jobs?repo=try&revision=31a225476957&selectedJob=133299122, it looks like at least that React.addons.TestUtils is no longer defined. Do we need to change the way that object is referenced in the new version, or shim the built version of react to continue exporting this in production builds?

It might worth to evaluate latest react 16.0 as well, since we need to modify our source to make it work anyway

(In reply to Fred Lin [:gasolin] from comment #10) > It might worth to evaluate latest react 16.0 as well, since we need to > modify our source to make it work anyway I'm also interested in trying out 16, but I think doing the 15.6 update will cover some of the work we need to do for 16 without as many potential compatibility issues. So I'd vote we land 15.6.1 and then file a new bug to try upgrading to 16.

I do need to add a plugin that allows us to continue using mixins with ES6 classes. We need to continue to use mixins because whydidyouupdate and objectdiff are both mixins. I know you need it quickly and it is my current task so I suppose I will make sure we can land 15.6 and log 2 new bugs: 1 for adding the mixin plugin and one to upgrade to 16.

@bgrins: Can you have a quick look over this so that we can get it landed. The try failure was just a build failure and other try runs have been fine. Changes: - In the previous React version we included React.TestUtils in the production version... this is now fixed so we only include it if one of the following is true: i. ac_add_options --enable-debug-js-modules is in .mozconfig ii. ac_add_options --disable-debug is in .mozconfig iii. devtools.react.dev is set to true NOTE: i and ii were already present but can't be enabled for our tests, which use React.TestUtils. We needed to add the pref to make React.TestUtils available for tests. - Due to the code being spread out more evenly over the React files we now have three production files and three dev files: - react.js - react-dom.js - react-dom-server.js - react-dev.js - react-dom-dev.js - react-dom-server-dev.js - All of these files are always present because we now have the pref switch (devtools.react.dev). - I renamed REACT_UPGRADING to REACT_UPGRADING.md and changed it to markdown because it is much easier to follow that way... of course, the instructions have been corrected and updated for this version of React.

@bgrins: jlast has already reviewed the original patch but I fixed a bunch of bugs so you just need to review https://reviewboard.mozilla.org/r/180304/diff/1-3/

Comment on attachment 8908653 [details] Bug 1399493 - Upgrade to React 15.6.1 and include dev & prod version https://reviewboard.mozilla.org/r/180304/#review194964 ::: devtools/client/shared/vendor/moz.build (Diff revisions 1 - 3) > 'react-addons-shallow-compare.js', > ] > > -# react-dev is used if either debug mode is enabled, > -# so include it for both > -if CONFIG['DEBUG_JS_MODULES'] or CONFIG['MOZ_DEBUG']: For the record, shipping this with Firefox adds 300kB to the final package. ::: devtools/shared/base-loader.js:609 (Diff revision 3) > + if (AppConstants.DEBUG || > + AppConstants.DEBUG_JS_MODULES || > + prefs.getBoolPref("devtools.react.dev")) { > + options.paths["devtools/client/shared/vendor/react"] = > + "resource://devtools/client/shared/vendor/react-dev"; > + options.paths["devtools/client/shared/vendor/react-dom"] = > + "resource://devtools/client/shared/vendor/react-dom-dev"; > + options.paths["devtools/client/shared/vendor/react-dom-server"] = > + "devtools/client/shared/vendor/react-dom-server-dev"; > + } Do we need to define the mappings both here and in browser-loader? These paths are merged with the ones from browser-loader when we instanciate a BrowserLoader. Is there a file requiring React using the loader and not the browser-loader?

I'd like to make sure I understand exactly why we shouldn't ship TestUtils in a release build before we commit to shipping these dev files to all of our users. AIUI TestUtils is lazily loaded - if that's correct then perf issues should surface only in tests. Is it possible to patch TestUtils into the release bundles (as we currently do) for 15.6 / 16?

(In reply to Brian Grinstead [:bgrins] from comment #17) > I'd like to make sure I understand exactly why we shouldn't ship TestUtils > in a release build before we commit to shipping these dev files to all of > our users. AIUI TestUtils is lazily loaded - if that's correct then perf > issues should surface only in tests. Is it possible to patch TestUtils into > the release bundles (as we currently do) for 15.6 / 16? So after chatting with the React devs about this I have a much clearer picture. TestUtils used to wrap the DOM Renderer and came with quite a performance hit. Looking at the source it seems that it is no longer a wrapper so if we don't need to worry about performance so I should be able to enable TestUtils as long as I don't also enable React.Perf, which would cause performance issues. Whether it is possible to patch it into 15.6 is another issue. Nobody knows so I can make the change and run our test suite to find out. Luckily, our test suite is fairly comprehensive so if there are issues we will know quickly. We are not upgrading to v16 yet as we will need to remove all of our deprecation warnings first as most of the things that cause the warnings are removed in that version. React.Perf and a whole host of other add-ons do not work with v16 so we are better waiting until the React catches up with it's older versions when it comes to add-on compatibility. (In reply to Julian Descottes [:jdescottes] from comment #16) > Comment on attachment 8908653 [details] > Bug 1399493 - Upgrade to React 15.6.1 and include dev & prod version > > https://reviewboard.mozilla.org/r/180304/#review194964 > > ::: devtools/client/shared/vendor/moz.build > (Diff revisions 1 - 3) > > 'react-addons-shallow-compare.js', > > ] > > > > -# react-dev is used if either debug mode is enabled, > > -# so include it for both > > -if CONFIG['DEBUG_JS_MODULES'] or CONFIG['MOZ_DEBUG']: > > For the record, shipping this with Firefox adds 300kB to the final package. > It is about just over 329k, which is a lot of code. I will look at ways to avoid moving it into production... hopefully we can just enable TestUtils in production builds. > ::: devtools/shared/base-loader.js:609 > (Diff revision 3) > > + if (AppConstants.DEBUG || > > + AppConstants.DEBUG_JS_MODULES || > > + prefs.getBoolPref("devtools.react.dev")) { > > + options.paths["devtools/client/shared/vendor/react"] = > > + "resource://devtools/client/shared/vendor/react-dev"; > > + options.paths["devtools/client/shared/vendor/react-dom"] = > > + "resource://devtools/client/shared/vendor/react-dom-dev"; > > + options.paths["devtools/client/shared/vendor/react-dom-server"] = > > + "devtools/client/shared/vendor/react-dom-server-dev"; > > + } > > Do we need to define the mappings both here and in browser-loader? These > paths are merged with the ones from browser-loader when we instanciate a > BrowserLoader. > > Is there a file requiring React using the loader and not the browser-loader? I am fairly sure the JSON Viewer had test failures without this but the only way I can find out is by commenting out the lines and re-running our test suite.

Comment on attachment 8908653 [details] Bug 1399493 - Upgrade to React 15.6.1 and include dev & prod version https://reviewboard.mozilla.org/r/180304/#review194964 > For the record, shipping this with Firefox adds 300kB to the final package. 329k But we no longer ship with the dev version > Do we need to define the mappings both here and in browser-loader? These paths are merged with the ones from browser-loader when we instanciate a BrowserLoader. > > Is there a file requiring React using the loader and not the browser-loader? Nope, I have removed these.

Comment on attachment 8908653 [details] Bug 1399493 - Upgrade to React 15.6.1 and include dev & prod version https://reviewboard.mozilla.org/r/180304/#review195456 Thanks for addressing the issues Mike! Looks good to me now. ::: devtools/client/shared/browser-loader.js:9 (Diff revision 5) > "use strict"; > > var Cu = Components.utils; > const loaders = Cu.import("resource://devtools/shared/base-loader.js", {}); > const { devtools } = Cu.import("resource://devtools/shared/Loader.jsm", {}); > +const { XPCOMUtils } = Cu.import("resource://gre/modules/XPCOMUtils.jsm", {}); not needed anymore ::: devtools/client/shared/browser-loader.js:14 (Diff revision 5) > +const { XPCOMUtils } = Cu.import("resource://gre/modules/XPCOMUtils.jsm", {}); > const { joinURI } = devtools.require("devtools/shared/path"); > const { assert } = devtools.require("devtools/shared/DevToolsUtils"); > const { AppConstants } = devtools.require("resource://gre/modules/AppConstants.jsm"); > > +XPCOMUtils.defineLazyServiceGetter(this, "prefs", not needed anymore ::: devtools/client/shared/vendor/REACT_UPGRADING.md:71 (Diff revision 5) > +``` > +npm install > +grunt build > +``` > + > +## Patching (XUL Workarounds) We are doing more than XUL workarounds, so just "Patching" ? ::: devtools/client/shared/vendor/REACT_UPGRADING.md:198 (Diff revision 5) > > ``` > -})(function(React) { > - //-------------------------------------------------------------------------------------- > +//-------------------------------------------------------------------------------------- > - // START MONKEY PATCH > +// START MONKEY PATCH > +/** Maybe we should leave the code sample in the react file to avoid having to maintain and synchronize the two? ::: devtools/client/shared/vendor/REACT_UPGRADING.md:380 (Diff revision 5) > + <gecko-dev>/devtools/client/shared/vendor/react-dom-dev.js > + cp build/react-dom-server.js > + <gecko-dev>/devtools/client/shared/vendor/react-dom-server-dev.js > + ``` > + > +### Generate a Production Build It's interesting to see that the only difference between prod and dev versions of react files are "production" strings which become "development" (at least that's what it seems to me from doing a quick diff). Maybe in a followup we could think about keeping just one file and patching the files to use a dynamic environment variable? ::: devtools/shared/base-loader.js:26 (Diff revision 5) > > XPCOMUtils.defineLazyServiceGetter(this, "resProto", > "@mozilla.org/network/protocol;1?name=resource", > "nsIResProtocolHandler"); > + > +XPCOMUtils.defineLazyServiceGetter(this, "prefs", not needed anymore.

Comment on attachment 8908653 [details] Bug 1399493 - Upgrade to React 15.6.1 and include dev & prod version https://reviewboard.mozilla.org/r/180304/#review195456 > Maybe we should leave the code sample in the react file to avoid having to maintain and synchronize the two? If only that was compatible with the workflow. At this point the contents of the file have already been replaced and changes have been made so there is no monkeypatch to copy. This is why I have included the monkeypatch in the document. > It's interesting to see that the only difference between prod and dev versions of react files are "production" strings which become "development" (at least that's what it seems to me from doing a quick diff). Maybe in a followup we could think about keeping just one file and patching the files to use a dynamic environment variable? I discussed this with a guy on #reactjs. It would work for this particular version but we can't always assume that the dev and production versions being identical, particularly because they are third party libraries. In the future the production version may well be a much more streamlined version of the dev release. > not needed anymore. The last minute changes always seem to be the ones that cause problems.

One more try run before landing just to be on the safe side.

Pushed by mratcliffe@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/e14d982a968e Upgrade to React 15.6.1 and include dev & prod version r=jdescottes,jlast