Added react-dom/test-utils bundle #9414
Conversation
scripts/rollup/bundles.js
Outdated
| 'src/renderers/shared/**/*.js', | ||
| 'src/test/**/*.js', // ReactTestUtils is currently very coupled to DOM. | ||
|
|
||
| 'src/isomorphic/classic/types/checkPropTypes.js', |
There was a problem hiding this comment.
It's weird we include this into bundles. Why aren't we calling PropTypes.checkPropTypes?
There was a problem hiding this comment.
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
bvaughn
force-pushed
the
react-dom-test-utils
branch
from
e8ea754 to
aa88a15
Compare
|
Is this only for Node? What’s the plan for www? |
scripts/rollup/bundles.js
Outdated
| isRenderer: false, | ||
| label: 'react-test-utils', | ||
| manglePropertiesOnProd: false, | ||
| name: 'react-test-utils', |
There was a problem hiding this comment.
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?
bvaughn
force-pushed
the
react-dom-test-utils
branch
from
ab65d2b to
64b63f4
Compare
* 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
force-pushed
the
react-dom-test-utils
branch
from
64b63f4 to
681a32e
Compare
|
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 |
| 'use strict'; | ||
|
|
||
| if (process.env.NODE_ENV !== 'production') { | ||
| module.exports = require('./cjs/test-utils.development.js'); |
There was a problem hiding this comment.
Does it make sense to always export it? Or at least throw in production.
There was a problem hiding this comment.
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!
| } = require('ReactDOM-fb'); | ||
|
|
||
| module.exports = __SECRET_INTERNALS_DO_NOT_USE_OR_YOU_WILL_BE_FIRED.ReactTestUtils; | ||
| module.exports = require('react-dom/test-utils'); |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Locally I've pointed entry and fbEntry at the same file. (Haven't pushed yet.)
|
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. |
3 participants
react-addons-test-utilswas moved toreact-dom/test-utilswith 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 fromprop-types/checkPropTypesand thesrc/isomorphic/classic/types/checkPropTypes.jsforwarding file has been deleted.I'm new to the bundles, but I tested my build changes using
fixtures/domandfixtures/packagingand I ran some simple Jest tests against the result of the newtest-utiltarget. Seemed okay. ¯_(ツ)_/¯