Cannot use addons.Perf or addons.ReactTransitionGroup in AMD environment with 15.4.1

[jochenberger]

I use react-with-addons in a RequireJS enviroment. If I try to use React.addons.Perf, I get a ReferenceError: ReactDOM is not defined from https://github.com/facebook/react/blob/15-dev/src/umd/shims/ReactAddonsDOMDependenciesUMDShim.js#L26.
This is a regression from 15.3.x.

[jochenberger]

See https://gist.github.com/jochenberger/9296b6edf23eada7a83ca22ca5685aa8

[gaearon]

This is a duplicate of #8301 and is being fixed in #8374. The fix will be out soon.

[gaearon]

I am leaving this open because need to confirm the fix actually fixes this issue as well.

[jochenberger]

No, this is about 15.4.1.

[gaearon]

Oh, I didn't realize 15.4.1 was already out, my bad.
Did this work with 15.4.0?

[jochenberger]

No, but it worked with 15.3.2, so it's also related to the change in packaging.

[gre]

Why is Perf depending on react-dom, it used to work in react-native context, is that no longer the case?

[gaearon]

@gre

The whole concept of "addons" that reach into internals is a mess IMO.

I think React Native has its own ReactPerf copy now but I'm not sure if it's exposed. You can probably get it with require('react-native/lib/ReactPerf'); or maybe just require('ReactPerf') (not sure how Haste works in RN projects).

We should probably expose it on renderers since it's copied now. Like ReactDOM.Perf and ReactNative.Perf or something.

[jochenberger]

ReactTransitionGroup is broken too, its cWRP calls ReactAddonsDOMDependencies.getReactInstanceMap() and that ends up in https://github.com/facebook/react/blob/15-dev/src/umd/shims/ReactAddonsDOMDependenciesUMDShim.js#L21 where ReactDOM is undefined.

[hsubox76]

I'm also using RequireJS and while 15.4.1 fixed the ability to use ReactDOM, I am unable to use React.addons.TestUtils.renderIntoDocument() for probably the same reasons as above.

renderIntoDocument makes use of ReactAddonsDOMDependencies.getReactTestUtils() which looks for the same undefined ReactDOM variable that ReactPerf and other addons are looking for I believe.

Thanks for getting the 15.4.1 fix out so quickly, by the way.

[gaearon]

It would really help if somebody could experiment with the bundle like in #8301 (comment) and try to figure out a fix before we get a chance to look into this.

[hsubox76]

Adding this require line in ReactAddonsDOMDependenciesUMDShim.js seems to fix it for me:
var ReactDOM = require('../../react-dom/lib/ReactDOMUMDEntry');

But I don't know if that's the correct way to do it. Gist here:

https://gist.github.com/hsubox76/6a81a00d2809c91c3b67d64ba7e1a518#file-reactaddonsdomdependenciesumdshim-js-L16

[gaearon]

This would break for single-file bundles as far as I can tell.

[jochenberger]

@hsubox76: what do you mean by "seems to fix it for me"? Have you tried to build the UMD distribution including this change?

[jochenberger]

That change would include all of ReactDOM in react-with-addons.js. That's probably not what we want.
We cannot add 'react-dom' as a dependency to the AMD define in react-with-addons.js because that would create a circular dependency.
But I guess, we could use RequireJS's require to return the loaded instance of react-dom from within the shim. The following patch makes it work for me inside RequireJS, but I don't know if it breaks other environments in turn. We could still

--- react-with-addons	2016-11-24 10:16:41.355197371 +0100
+++ react.js	2016-11-24 10:25:56.111594068 +0100
@@ -1,7 +1,7 @@
  /**
   * React (with addons) v15.4.1
   */
-(function(f){if(typeof exports==="object"&&typeof module!=="undefined"){module.exports=f()}else if(typeof define==="function"&&define.amd){define([],f)}else{var g;if(typeof window!=="undefined"){g=window}else if(typeof global!=="undefined"){g=global}else if(typeof self!=="undefined"){g=self}else{g=this}g.React = f()}})(function(){var define,module,exports;return (function e(t,n,r){function s(o,u){if(!n[o]){if(!t[o]){var a=typeof require=="function"&&require;if(!u&&a)return a(o,!0);if(i)return i(o,!0);var f=new Error("Cannot find module '"+o+"'");throw f.code="MODULE_NOT_FOUND",f}var l=n[o]={exports:{}};t[o][0].call(l.exports,function(e){var n=t[o][1][e];return s(n?n:e)},l,l.exports,e,t,n,r)}return n[o].exports}var i=typeof require=="function"&&require;for(var o=0;o<r.length;o++)s(r[o]);return s})({1:[function(_dereq_,module,exports){
+(function(f){if(typeof exports==="object"&&typeof module!=="undefined"){module.exports=f()}else if(typeof define==="function"&&define.amd){define(["require"],f)}else{var g;if(typeof window!=="undefined"){g=window}else if(typeof global!=="undefined"){g=global}else if(typeof self!=="undefined"){g=self}else{g=this}g.React = f()}})(function(require){var define,module,exports;return (function e(t,n,r){function s(o,u){if(!n[o]){if(!t[o]){var a=typeof require=="function"&&require;if(!u&&a)return a(o,!0);if(i)return i(o,!0);var f=new Error("Cannot find module '"+o+"'");throw f.code="MODULE_NOT_FOUND",f}var l=n[o]={exports:{}};t[o][0].call(l.exports,function(e){var n=t[o][1][e];return s(n?n:e)},l,l.exports,e,t,n,r)}return n[o].exports}var i=typeof require=="function"&&require;for(var o=0;o<r.length;o++)s(r[o]);return s})({1:[function(_dereq_,module,exports){
 /**
  * Copyright 2013-present, Facebook, Inc.
  * All rights reserved.
@@ -424,21 +424,21 @@
 
 'use strict';
 
-exports.getReactDOM = function () {
-  return ReactDOM;
+var getReactDOM = exports.getReactDOM = function () {
+  return require('react-dom');
 };
 
 exports.getReactInstanceMap = function () {
-  return ReactDOM.__SECRET_INTERNALS_DO_NOT_USE_OR_YOU_WILL_BE_FIRED.ReactInstanceMap;
+  return getReactDOM().__SECRET_INTERNALS_DO_NOT_USE_OR_YOU_WILL_BE_FIRED.ReactInstanceMap;
 };
 
 if ("development" !== 'production') {
   exports.getReactPerf = function () {
-    return ReactDOM.__SECRET_INTERNALS_DO_NOT_USE_OR_YOU_WILL_BE_FIRED.ReactPerf;
+    return getReactDOM().__SECRET_INTERNALS_DO_NOT_USE_OR_YOU_WILL_BE_FIRED.ReactPerf;
   };
 
   exports.getReactTestUtils = function () {
-    return ReactDOM.__SECRET_INTERNALS_DO_NOT_USE_OR_YOU_WILL_BE_FIRED.ReactTestUtils;
+    return getReactDOM().__SECRET_INTERNALS_DO_NOT_USE_OR_YOU_WILL_BE_FIRED.ReactTestUtils;
   };
 }
 },{}],7:[function(_dereq_,module,exports){

Maybe we need to pass an accessor function into the UMD callback or something like that.

[jochenberger]

Yes, the accessor pattern might work. It's pretty ugly though and I haven't adapted the CommonJS part. How would you require ReactDOM in CommonJS?

--- react-with-addons	2016-11-24 10:16:41.355197371 +0100
+++ react.js	2016-11-24 10:42:01.372836180 +0100
@@ -1,7 +1,7 @@
  /**
   * React (with addons) v15.4.1
   */
-(function(f){if(typeof exports==="object"&&typeof module!=="undefined"){module.exports=f()}else if(typeof define==="function"&&define.amd){define([],f)}else{var g;if(typeof window!=="undefined"){g=window}else if(typeof global!=="undefined"){g=global}else if(typeof self!=="undefined"){g=self}else{g=this}g.React = f()}})(function(){var define,module,exports;return (function e(t,n,r){function s(o,u){if(!n[o]){if(!t[o]){var a=typeof require=="function"&&require;if(!u&&a)return a(o,!0);if(i)return i(o,!0);var f=new Error("Cannot find module '"+o+"'");throw f.code="MODULE_NOT_FOUND",f}var l=n[o]={exports:{}};t[o][0].call(l.exports,function(e){var n=t[o][1][e];return s(n?n:e)},l,l.exports,e,t,n,r)}return n[o].exports}var i=typeof require=="function"&&require;for(var o=0;o<r.length;o++)s(r[o]);return s})({1:[function(_dereq_,module,exports){
+(function(f){if(typeof exports==="object"&&typeof module!=="undefined"){module.exports=f()}else if(typeof define==="function"&&define.amd){define(["require"],function(require){return f(function(){return require("react-dom")})})}else{var g;if(typeof window!=="undefined"){g=window}else if(typeof global!=="undefined"){g=global}else if(typeof self!=="undefined"){g=self}else{g=this}g.React = f(function(){return ReactDOM})}})(function(ReactDOMAccessor){var define,module,exports;return (function e(t,n,r){function s(o,u){if(!n[o]){if(!t[o]){var a=typeof require=="function"&&require;if(!u&&a)return a(o,!0);if(i)return i(o,!0);var f=new Error("Cannot find module '"+o+"'");throw f.code="MODULE_NOT_FOUND",f}var l=n[o]={exports:{}};t[o][0].call(l.exports,function(e){var n=t[o][1][e];return s(n?n:e)},l,l.exports,e,t,n,r)}return n[o].exports}var i=typeof require=="function"&&require;for(var o=0;o<r.length;o++)s(r[o]);return s})({1:[function(_dereq_,module,exports){
 /**
  * Copyright 2013-present, Facebook, Inc.
  * All rights reserved.
@@ -424,21 +424,21 @@
 
 'use strict';
 
-exports.getReactDOM = function () {
-  return ReactDOM;
+var getReactDOM = exports.getReactDOM = function () {
+  return ReactDOMAccessor();
 };
 
 exports.getReactInstanceMap = function () {
-  return ReactDOM.__SECRET_INTERNALS_DO_NOT_USE_OR_YOU_WILL_BE_FIRED.ReactInstanceMap;
+  return ReactDOMAccessor().__SECRET_INTERNALS_DO_NOT_USE_OR_YOU_WILL_BE_FIRED.ReactInstanceMap;
 };
 
 if ("development" !== 'production') {
   exports.getReactPerf = function () {
-    return ReactDOM.__SECRET_INTERNALS_DO_NOT_USE_OR_YOU_WILL_BE_FIRED.ReactPerf;
+    return ReactDOMAccessor().__SECRET_INTERNALS_DO_NOT_USE_OR_YOU_WILL_BE_FIRED.ReactPerf;
   };
 
   exports.getReactTestUtils = function () {
-    return ReactDOM.__SECRET_INTERNALS_DO_NOT_USE_OR_YOU_WILL_BE_FIRED.ReactTestUtils;
+    return ReactDOMAccessor().__SECRET_INTERNALS_DO_NOT_USE_OR_YOU_WILL_BE_FIRED.ReactTestUtils;
   };
 }
 },{}],7:[function(_dereq_,module,exports){

[jochenberger]

I think that the cleanest solution would be to build the addons as a separate file. They can depend on react and react-dom then. (probably related to #680)
Of course, that's probably too big a change to make for a minor version.

[gaearon]

Yea, that’s what I would do too but we can’t change this before 16.

[jochenberger]

Reverting the restructuring that led to these issues until then is not an option I guess?

[gaearon]

Unfortunately no, it’s crucial for our direction forward.

[jochenberger]

But not necessarily in 15.x, right?
I know, you have a lot of open issues and it's-open-source-so-fix-it-yourself and all, but IMHO, if React claims to work with AMD, then the changes should be reverted until a solution is found that works in all environments.

[gaearon]

At this point it's going to be too expensive for the ecosystem to revert. Many packages that relied on internal APIs (which is bad by itself) already broke once, we can't break them again. It is also technically very hard to revert now.

We publicly asked for feedback on the release for a month, and this issue was not reported. I understand it is frustrating but this ship has sailed.

If you'd like to help us fix that, please do. It is not obvious to me how to fix it (I tried a few times). Your stance on not submitting a PR because you refuse to sign the CLA isn't very helpful either.

[jochenberger]

Neither is blaming your users for not finding or fixing issues in your project. But this isn't getting us anywhere.
Btw., I'm not frustrated that React stopped working. I just think that this issue should get some more attention. I'm fine if the resolution is that you say that too few people use AMD and you officially drop support.
And I did try to help. In my comments I tried to come up with a solution. I just don't know very much about CommonJS and the bundling madness (no offense). I need some help there, then I can probably come up with a solution that will help someone create a PR.

[gaearon]

I apologize if any of my comments made it look like I was blaming the users. That was not my intention. Yes it’s bad that we broke this, and it’s our fault. That said we are not in a position to revert this now, so we need to figure out a way to fix this forward. We have been trying to fix it but have not found a solution that wouldn’t break something else yet. We welcome yours and anyone else’s help in this. Thanks!

[jochenberger]

Can you share some details? Did you try something similar to my approach or was it something else entirely?

[gaearon]

It was something similar but I had issues with webpack if I recall correctly. I’ll give it another try later today and push my branch somewhere.

[jochenberger]

I tried the first patch that I mentioned:
https://gist.github.com/jochenberger/f376f3e347c2efe52cba774f5a7651cc
It works with browser globals as well as with AMD. Can somebody try it in a CommonJS environment? Be sure to access React.addons.Perf, like in https://gist.github.com/jochenberger/9296b6edf23eada7a83ca22ca5685aa8

[jochenberger]

Btw, I also tried to build something upon the information in http://requirejs.org/docs/api.html#circular, but didn't get far. The problem is that you cannot assign exports but only add properties to it. So the modules could only export their stuff wrapped in additional objects.

[jochenberger]

It might even be enough to apply this patch:

diff --git a/react.js b/react.js
index 545bec4..a4ad999 100644
--- a/react.js
+++ b/react.js
@@ -424,21 +424,21 @@ module.exports = React;
 
 'use strict';
 
-exports.getReactDOM = function () {
-  return ReactDOM;
+var getReactDOM = exports.getReactDOM = function () {
+  return typeof ReactDOM !== 'undefined' ? ReactDOM : require('react-dom');
 };
 
 exports.getReactInstanceMap = function () {
-  return ReactDOM.__SECRET_INTERNALS_DO_NOT_USE_OR_YOU_WILL_BE_FIRED.ReactInstanceMap;
+  return getReactDOM().__SECRET_INTERNALS_DO_NOT_USE_OR_YOU_WILL_BE_FIRED.ReactInstanceMap;
 };
 
 if ("development" !== 'production') {
   exports.getReactPerf = function () {
-    return ReactDOM.__SECRET_INTERNALS_DO_NOT_USE_OR_YOU_WILL_BE_FIRED.ReactPerf;
+    return getReactDOM().__SECRET_INTERNALS_DO_NOT_USE_OR_YOU_WILL_BE_FIRED.ReactPerf;
   };
 
   exports.getReactTestUtils = function () {
-    return ReactDOM.__SECRET_INTERNALS_DO_NOT_USE_OR_YOU_WILL_BE_FIRED.ReactTestUtils;
+    return getReactDOM().__SECRET_INTERNALS_DO_NOT_USE_OR_YOU_WILL_BE_FIRED.ReactTestUtils;
   };
 }
 },{}],7:[function(_dereq_,module,exports){

The change would probably have to be made in https://github.com/facebook/react/blob/master/src/umd/shims/ReactAddonsDOMDependenciesUMDShim.js

[gaearon]

Sorry I haven’t had time to jump on it yet.
Let me list a few cases that need to be checked for now:

• Browser
• AMD with SystemJS
• AMD with RequireJS
• CommonJS with Webpack when both react and react-dom are aliased to UMD builds

The best test case is probably ReactDOM.render() call with <TransitionGroup> from -with-addons build so that we can verify internal imports work both ways.

[jochenberger]

I checked Browser and AMD with RequireJS manually.
Which of these cases are covered by tests? I ran npm test and it seemed to succeed.

[gaearon]

We don't really have any integration tests for the builds in different environments (yea, it’s not good—again, contributions are welcome). npm test only runs internal tests but not packaging.

[jochenberger]

Here's a CodePen with my modified react-with-addons bundle (including the patch from #8392 (comment)):
http://codepen.io/jochenberger/pen/zoMjBz

[jochenberger]

RequireJS : http://codepen.io/jochenberger/pen/dOQeqw

[jochenberger]

I fixed the codepens to use CSSTransitionGroup instead of TransitionGroup.

[gaearon]

The fix in #8392 (comment) doesn't seem to work during build time:

Running "browserify:addons" (browserify) task
>> Error: Cannot find module './react-dom' from '/Users/gaearon/p/react/build/node_modules/react/lib'

Running "browserify:addonsMin" (browserify) task
>> Error: Cannot find module './react-dom' from '/Users/gaearon/p/react/build/node_modules/react/lib'

[gaearon]

I started some work in #8686 but I can't get figure out how to make circular dependencies work in statically analyzable AMD environment (SystemJS Builder in particular, possibly r.js is affected too). Any help welcome.

[gaearon]

Can you please check if the fix in #8686 works for your case?

[gaearon]

Should be fixed via #8686 and released in 15.4.2 (out soon).

[jochenberger]

Sorry, I'm late. FWIW, I just tried with current master and it works fine. Thanks!

[treyp]

Can also confirm that react-with-addons works again on master in my RequireJS setup. Thanks @gaearon!

[gaearon]

Should be fixed after updating all React packages to 15.4.2.
Please verify.

[treyp]

Verified: 15.4.2 from NPM is working on our RequireJS build. Thanks again!