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

dist/react-dom.js cannot get React.__SECRET_DOM_DO_NOT_USE_OR_YOU_WILL_BE_FIRED #7092

Closed
mocheng opened this issue Jun 21, 2016 · 12 comments
Closed

Comments

@mocheng
Copy link

mocheng commented Jun 21, 2016

In version 15.1.0, npm installed react-dom has dist/react-dom file which imports React DOM related stuff from React. However, require('react') doesn't have React.__SECRET_DOM_DO_NOT_USE_OR_YOU_WILL_BE_FIRED property, since react doesn't export it.

As a result, we cannot use dist/react-dom and it is totally useless.

@keyz
Copy link
Contributor

keyz commented Jun 21, 2016

I think the */dist/* files are intended to be used as global <script src="..."> tags in browser but not as node requires. In a browser environment ReactDOM will pick up the global React object which contains the property.

Do you have a specific use case that you'll have to do require('react-dom/dist/react-dom')?

@keyz
Copy link
Contributor

keyz commented Jun 21, 2016

That being said, I think it's true that this line https://github.com/facebook/react/blob/master/vendor/react-dom.js#L5 might be redundant. What do you think @zpao?

@mocheng
Copy link
Author

mocheng commented Jun 22, 2016

@keyanzhang I just want to use minfied ReactDOMServer for server side rendering. Now, I have to use below hack.

ReactDOMServer = require('react/dist/react').__SECRET_DOM_SERVER_DO_NOT_USE_OR_YOU_WILL_BE_FIRED;

The minified version has better performance in server side rendering. I don't why React doesn't encourage to use it.

@satya164
Copy link

@mocheng You can set NODE_ENV to production to achieve the same thing.

@keyz
Copy link
Contributor

keyz commented Jun 22, 2016

@satya164 No, it's actually not going to be the same. The minified version of React is more performant since the minification eliminates dead code and skips dynamic process.env.NODE_ENV lookups (which are costly).

@syranide
Copy link
Contributor

syranide commented Jun 22, 2016

@keyanzhang IMHO if that's a concern then you should really be minifying your entire codebase to deal with all cases of it. Unless someone can show a benchmark showing a consistent measurable difference (which I doubt there is) then this seems premature, dead code would only marginally affect start-up time and I just can't see process.env.NODE_ENV being costly enough that it would be measurable.

@keyz
Copy link
Contributor

keyz commented Jun 22, 2016

@syranide yeah I think minifying the entire codebase is an option. The problem of directly requiring dist/react*.min is that you'll need to change all your requires/imports, or you'll end up with 2 different builds of React. Specifically, if there's a third party library that imports React then you'll need to change that as well :(. So I think adding a minification pass in webpack/browserify makes sense.

Other than that, (surprisingly) reading process.env isn't cheap: nodejs/node#3104 (comment)

@gaearon
Copy link
Collaborator

gaearon commented Jun 22, 2016

The problem of directly requiring dist/react*.min is that you'll need to change all your requires/imports, or you'll end up with 2 different builds of React. Specifically, if there's a third party library that imports React then you'll need to change that as well :(.

Usually this is solved by using an option like alias in Webpack that just “redirects” requires, e.g. alias: { 'react': 'react/dist/react.min' }.

@syranide
Copy link
Contributor

@gaearon But if you're using webpack then minifying is already easily part of your process, this is really about native node.js I would say?

@gaearon
Copy link
Collaborator

gaearon commented Jun 22, 2016

But if you're using webpack then minifying is already easily part of your process, this is really about native node.js I would say?

I don’t know. With Webpack people often used precompiled bundles for faster build times.

@zpao
Copy link
Member

zpao commented Jun 22, 2016

Usually this is solved by using an option like alias in Webpack that just “redirects” requires, e.g. alias: { 'react': 'react/dist/react.min' }.

In that case, the problem here becomes solved I assume since the require in react-dom/dist/react-dom-server.js would also get rewritten.

I think the recommendation here is going to be that if you are going to try to use the dist files in node, you might have a bad time. We don't recommend it. If you are trying to get performance improvements with minification and DEV checks removed, then you should precompile everything. Otherwise it's going to be a problem. There is the small issue with the UMD setup that @keyanzhang pointed out, but even if we addressed that, there are further issues that @keyanzhang brought up with components and dependencies requiring the node module. You have to rewrite to address that, at which point the problem is solved or you can easily include React itself into your build and minifiy (even if that does add some build time).

@zpao zpao closed this as completed Jun 22, 2016
@zpao
Copy link
Member

zpao commented Jun 22, 2016

FWIW, in the future the npm module may be a single file (or rather 2, with a single dev/prod check to split) and the whole optimization point becomes moot. (#6351 is part of that if you want to follow along)

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

No branches or pull requests

6 participants