Added react-dom/test-utils bundle #9414

bvaughn · 2017-04-13T00:26:59Z

react-addons-test-utils was moved to react-dom/test-utils with the release of 15.5. This is my attempt to add a new bundle for 16 alpha in the same place. I did a quick Jest test of the generated bundle and it seemed ok but I'm still learning my way around the new build tools.

I also removed some indirection in the build/bundling process for checkPropTypes. That's now imported directly from prop-types/checkPropTypes and the src/isomorphic/classic/types/checkPropTypes.js forwarding file has been deleted.

I'm new to the bundles, but I tested my build changes using fixtures/dom and fixtures/packaging and I ran some simple Jest tests against the result of the new test-util target. Seemed okay. ¯_(ツ)_/¯

gaearon · 2017-04-13T00:58:49Z

scripts/rollup/bundles.js

+      'src/renderers/shared/**/*.js',
+      'src/test/**/*.js', // ReactTestUtils is currently very coupled to DOM.
+
+      'src/isomorphic/classic/types/checkPropTypes.js',


It's weird we include this into bundles. Why aren't we calling PropTypes.checkPropTypes?

That is a bit silly, now that you point it out. It was unintentional. src/isomorphic/classic/types/checkPropTypes.js just forwards to prop-types/checkPropTypes. But I can remove the unnecessary indirection. 😄

Files now import directly from prop-types/checkPropTypes

gaearon · 2017-04-13T14:50:29Z

Is this only for Node? What’s the plan for www?

gaearon · 2017-04-13T15:09:02Z

scripts/rollup/bundles.js

+    isRenderer: false,
+    label: 'react-test-utils',
+    manglePropertiesOnProd: false,
+    name: 'react-test-utils',


If you run build, you will see that there’s a single react-test-utils.development.js left in the /build/ folder while everything else is neatly put into folders. This is probably because name corresponds to npm name, but there is no top-level /packages/react-test-utils/ so it doesn’t understand where to take package.json from, and skips it. (I admit this behavior is confusing.)

The rule of thumb is that nothing should end up in top-level /build/ folder, and that /build/packages/ should be all publish-able “as is” after build. I don’t think this is currently publish-able as is because test utils don’t actually make their way into the react-dom package.

You probably want to put 'react-dom/test-utils' in this field instead. This will ensure it gets copied into /build/packages/react-dom/ instead. You can see we are using the same trick for react-dom/server.

However, I’m not sure how it will end up on npm. Don’t we need to add a test-utils.js file to /packages/react-dom/ which points to the entry point, similarly to how we have server.js in /packages/react-dom/? And we’d probably want to add more entries to "files" in /packages/react-dom/package.json so that the new files actually end up in the package. Otherwise this PR does not really do anything to make it appear in the react-dom package.

I am slightly concerned about www. I would like to avoid deviating too much between how ReactTestUtils works in open source and in www. Currently, ReactTestUtils is a shim (/scripts/rollup/shims/facebook-www/) that re-exports a hidden value from ReactDOM. If we’re separating them in open source, can we also separate them in www?

The way I see it, we could do it by removing ReactTestUtils from the secret exports of ReactDOMFBEntry.js and ReactDOMFiberFBEntry.js, removing the ReactTestUtils.js shim from /scripts/rollup/shims/facebook-www/, and instead adding a FB_DEV target to this bundle. We would need to make sure it imports require('react-dom') as an external so that it doesn’t duplicate ReactDOM inside.

I have a similar concern about duplication in the open source build too (which this PR implements). If you build it and look inside the bundle, it completely duplicates ReactDOM inside. However this means that components using findDOMNode will import them from react-dom that is different from the one react-dom/test-utils is using. If you look at 15.5, react-dom/test-utils intentionally shared the implementation with react-dom itself because otherwise it can’t function correctly. So I think the need to declare react-dom as an external affects not only FB bundle, but open source bundle as well.

Does this make sense?

* Moved test-utils into react-dom package * Added test-utils to package.json 'files' list * Added ReactTestUtilsFBEntry module (though it doesn't really do anything at the moment) * Replaced ReactDOM references with react-dom to avoid duplicating code

bvaughn · 2017-04-13T18:04:37Z

Thanks for the detailed feedback, Dan.

I think I've implemented most of your suggestions- but I need to figure out the shallow renderer situation before I can proceed with this PR because of TestUtils.createRenderer. I'm going to try and tackle that now (in this PR also).

gaearon · 2017-04-13T18:15:11Z

packages/react-dom/test-utils.js

+'use strict';
+
+if (process.env.NODE_ENV !== 'production') {
+  module.exports = require('./cjs/test-utils.development.js');


Does it make sense to always export it? Or at least throw in production.

I considered throwing, but I thought it would be immediately obvious when you tried to use it. I guess I can throw a more meaningful error message at least, sure!

gaearon · 2017-04-13T18:18:37Z

src/fb/ReactTestUtilsFBEntry.js

-} = require('ReactDOM-fb');
-
-module.exports = __SECRET_INTERNALS_DO_NOT_USE_OR_YOU_WILL_BE_FIRED.ReactTestUtils;
+module.exports = require('react-dom/test-utils');


I don't understand this yet. Why does a FB entry point simply re-export? Can't we point the FB entry point to whatever react-dom/test-utils truly is?

Locally I've pointed entry and fbEntry at the same file. (Haven't pushed yet.)

bvaughn · 2017-04-14T22:20:40Z

Closing this in favor of #9426. Originally I was going to keep them separate- that branch built off of this one- but they ended up bleeding together too much. The suggestions from this PR have already been made on that branch.

Added react-dom/test-utils target

70d5c46

facebook-github-bot added the CLA Signed label Apr 13, 2017

bvaughn requested review from trueadm and acdlite April 13, 2017 00:27

gaearon reviewed Apr 13, 2017

View reviewed changes

Removed level of indirection for checkPropTypes

aa88a15

Files now import directly from prop-types/checkPropTypes

bvaughn force-pushed the react-dom-test-utils branch from e8ea754 to aa88a15 Compare April 13, 2017 02:22

gaearon requested changes Apr 13, 2017

View reviewed changes

bvaughn force-pushed the react-dom-test-utils branch from ab65d2b to 64b63f4 Compare April 13, 2017 17:55

bvaughn force-pushed the react-dom-test-utils branch from 64b63f4 to 681a32e Compare April 13, 2017 17:57

gaearon reviewed Apr 13, 2017

View reviewed changes

bvaughn mentioned this pull request Apr 14, 2017

Shallow renderer and test utils bundles #9426

Merged

bvaughn closed this Apr 14, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added react-dom/test-utils bundle #9414

Added react-dom/test-utils bundle #9414

bvaughn commented Apr 13, 2017 •

edited

Loading

gaearon Apr 13, 2017

bvaughn Apr 13, 2017

gaearon commented Apr 13, 2017

gaearon Apr 13, 2017

bvaughn commented Apr 13, 2017

gaearon Apr 13, 2017

bvaughn Apr 13, 2017

gaearon Apr 13, 2017

bvaughn Apr 13, 2017

bvaughn commented Apr 14, 2017

Added react-dom/test-utils bundle #9414

Added react-dom/test-utils bundle #9414

Conversation

bvaughn commented Apr 13, 2017 • edited Loading

gaearon Apr 13, 2017

Choose a reason for hiding this comment

bvaughn Apr 13, 2017

Choose a reason for hiding this comment

gaearon commented Apr 13, 2017

gaearon Apr 13, 2017

Choose a reason for hiding this comment

bvaughn commented Apr 13, 2017

gaearon Apr 13, 2017

Choose a reason for hiding this comment

bvaughn Apr 13, 2017

Choose a reason for hiding this comment

gaearon Apr 13, 2017

Choose a reason for hiding this comment

bvaughn Apr 13, 2017

Choose a reason for hiding this comment

bvaughn commented Apr 14, 2017

bvaughn commented Apr 13, 2017 •

edited

Loading