Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix ReactARTStack bundles to not include DOM Stack inside #9394

Merged
merged 2 commits into from
Apr 10, 2017

Conversation

gaearon
Copy link
Collaborator

@gaearon gaearon commented Apr 10, 2017

Only affects our internal FB bundles.

function replaceInternalModules() {
// we inline these modules in the bundles rather than leave them as external
return {
"'react-dom'": `'${resolve('./src/renderers/dom/ReactDOM.js')}'`,
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rather than have this special case in all bundles, I opted for importing ReactDOM explicitly in test utils.

@gaearon
Copy link
Collaborator Author

gaearon commented Apr 10, 2017

This doesn't matter much but makes ART bundle smaller for test cases (where we still use it). Which I guess is a minor benefit. But it also removes the react-dom aliasing hack which is confusing.

@gaearon gaearon merged commit 457b812 into facebook:master Apr 10, 2017
@gaearon gaearon deleted the bundle-fixes branch April 10, 2017 21:02
@acdlite
Copy link
Collaborator

acdlite commented Apr 21, 2017

@gaearon This isn't working for me in master. If I run yarn build -- --type=FB I still get require('react-dom') in ART and test renderer builds.

@trueadm
Copy link
Contributor

trueadm commented Apr 21, 2017

@acdlite @gaearon this may be because 'react-dom is specified in the externals for the Fiber bundle (scripts/rollup/bundles.js)?

@acdlite
Copy link
Collaborator

acdlite commented Apr 21, 2017

Ah I read the title of this PR incorrectly and assumed it was related. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants