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

React Dom Server: Broken Build for version 15.4.1 #8788

Closed
hitmands opened this issue Jan 14, 2017 · 40 comments
Closed

React Dom Server: Broken Build for version 15.4.1 #8788

hitmands opened this issue Jan 14, 2017 · 40 comments

Comments

@hitmands
Copy link

Do you want to request a feature or report a bug?
BUG

What is the current behavior?
It breaks the code execution

file: react-dom/dist/react-dom-server.min.js

117:[function(t,n,r){"use strict";var o=e.__SECRET_INTERNALS_DO_NOT_USE_OR_YOU_WILL_BE_FIRED;n.exports=o.ReactCurrentOwner},{}]

Beautified:

function(t, n, r) {
  "use strict";
  var o = e.__SECRET_INTERNALS_DO_NOT_USE_OR_YOU_WILL_BE_FIRED;
  
  n.exports = o.ReactCurrentOwner
}

Output:

TypeError: Cannot read property 'ReactCurrentOwner' of undefined
    at Object.r.117

What is the expected behavior?
var ReactDomServer = require('react-dom/dist/react-dom-server.min.js'); with the desired goal of having a server-side rendering for production which skips unnecessary comments, warnings, etc. React Documentation Page

  • Which versions of React: 15.4.1;
  • platform: NodeJS (server side rendering)
  • Did this work in previous versions of React? I don't know
@gaearon
Copy link
Collaborator

gaearon commented Jan 14, 2017

You are likely using different versions of React and ReactDOMServer. Please make sure you use the exact same version. I also recommend updating both to 15.4.2.

@gaearon
Copy link
Collaborator

gaearon commented Jan 14, 2017

Also, I don't think importing the UMD bundle is supported in Node environment unless you are also doing the same for React. You can't use ReactDOMServer single-file distribution together with React CommonJS entry point.

@hitmands
Copy link
Author

Hi Dan,
thanks for the reply,
I'm using the version 15.4.1 for both React and ReactDom (Reply 1), I'm going to update them to the 15.4.2 version but I hope that there are not incompatibilities between two patch releases (unless documentation).

For the 2 Reply I will have a check and let you know as soon as possible.
Thanks again

@gaearon
Copy link
Collaborator

gaearon commented Jan 14, 2017

This is a patch release so no, it shouldn't be a problem. If anything, 15.4.2 fixes some issues.

But I am pretty certain that if you can't mix UMD with CommonJS packages so that's the likely source of the problem.

@hitmands
Copy link
Author

Requiring the UMD Bundle for react is working fine var React = require('react/dist/react.min'); but it is still throwing for ReactDomServer, so the problem doesn't depend on the necessity of using the same approach between React and ReactDomServer...
Plus, I updated react and react-dom to the 15.4.2 version.

In react-dom/dist/react-dom-server.min i can see a line module.exports = e(require("react")); which requires React by itself, so my node_modules don't affect this.

Furthermore, logging through the bundle I can see that:

function(t, n, r) {
  "use strict";
  var o = e.__SECRET_INTERNALS_DO_NOT_USE_OR_YOU_WILL_BE_FIRED;
  // e is an instanceof React and has no __SECRET_INTERNALS_DO_NOT_USE_OR_YOU_WILL_BE_FIRED field
  n.exports = o.ReactCurrentOwner
}

@gaearon
Copy link
Collaborator

gaearon commented Jan 14, 2017

Hmm I think you're right. I don't we ever supported using UMD builds in the Node environment.
Is there a reason you're not using react-dom/server instead?

@hitmands
Copy link
Author

ReactDomServer.renderToString renders with debug info such as warnings and something else, I don't want this behaviour for my production environments. As the doc says, we should use the minified build for production...

I'm trying to optimize performances in general.

@gaearon
Copy link
Collaborator

gaearon commented Jan 14, 2017

Yea, we should make it clearer the advice to use minified builds is mostly for client-side since we don't have a good way to do this on the server side yet.

On the server side, it should be enough to run your app with NODE_ENV set to 'production', e.g. NODE_ENV=production node myserver.js. Does this help?

@gaearon
Copy link
Collaborator

gaearon commented Jan 14, 2017

I'll close since we don't support it.

For more heavy server side optimization tips check out this PR: raxjs/server-side-rendering-comparison#5

I hope it helps!

@gaearon gaearon closed this as completed Jan 14, 2017
@aweary
Copy link
Contributor

aweary commented Jan 14, 2017

@gaearon it's worth noting that this did work with 15.3.0 and earlier (at least, it didn't throw). I've seen this recommended a few times in SSR performance articles/talks, so it might not be an uncommon pattern.

@aweary
Copy link
Contributor

aweary commented Jan 14, 2017

(see this popular talk on SSR performance in React that recommends it)

@gaearon
Copy link
Collaborator

gaearon commented Jan 14, 2017

Hmm, I wasn't aware that this used to work.

@gaearon gaearon reopened this Jan 14, 2017
@jddxf
Copy link
Contributor

jddxf commented Jan 15, 2017

Seems this would happen since 15.4.0.Not sure if it's the desired behavior.
See #7164

@gaearon
Copy link
Collaborator

gaearon commented Jan 15, 2017

Yes, that's the main reason, but we can't revert it (there are good reasons for this change).
So if we fix it, we need to fix forward.

@magv
Copy link

magv commented Feb 3, 2017

Note that not only the server build of react-dom is affected; this fails in the same way with React 15.4.2:

var React = require("react/dist/react.min.js");
var ReactDOM = require("react-dom/dist/react-dom.min.js");

(The use case for such an operation is loading the minified version of React inside Atom).

@gaearon
Copy link
Collaborator

gaearon commented Feb 4, 2017

@magv

This accidentally happened to work because package boundaries were fake, but now they are real. However, now I’m not sure we could support this even if we wanted to.

These are UMD bundles. However you are loading them in a CommonJS environment. How would you expect them to “find” one another? They don’t know each other’s locations.

Since you’re in CommonJS, they can’t set globals, like they do in the browser. So react-dom/dist/react-dom.min.js can’t guess that React is located at react/dist/react.min.js.

I understand it’s frustrating this doesn’t work anymore, but it was never officially supported (you won’t find this usage suggested anywhere in the docs), and as far as I understand, it’s not even feasible to support after the true package split.

The real solution here is to bundle your code if you rely on this. If you bundle the code (e.g. with webpack, browserify or rollup), you can always specify alias (or equivalent) to tell the bundler to use react/dist/react.min.js instead of react, and react-dom/dist/react-dom.min.js instead of react-dom. Since the bundler resolves paths everywhere, it would also override the path in react-dom.min.js to refer to the correct React. Without the bundler, we can’t do this.

Let me know if I missed something, but so far it seems like we can’t fix it. Sorry! For server rendering performance, we intend to provide flat precompiled bundles in the future which should have the same effect.

@hitmands
Copy link
Author

hitmands commented Feb 4, 2017

I was using webpack and that's the reason because require('react/dist/react.min.js') was working even using the umd bundle in a commonjs environment.
I think that closing the bug could be acceptable, but at the same time we should focus on making server-side usage more clear, both in docs and in components.

@gaearon
Copy link
Collaborator

gaearon commented Feb 4, 2017

With Webpack, using alias in configuration for this should work well.

@magv
Copy link

magv commented Feb 6, 2017

@gaearon, in the case of Atom you can, in fact, set globals by using window.

Alternatively you could, if you wanted to, provide an additional set of minified files for react and react-dom that require their correct minified counterparts, and thus work in server environments (or something to this effect -- my understanding of the glue react-dom uses to find react is minimal at the moment).

As to using bundling tools, this is certainly an option, but a rather inconvenient one as far as Atom extension development goes (since it introduces a build stage).

Of course, if you've decided not to support importing from minified files in react and react-dom packages, I won't argue further.

@szimek
Copy link

szimek commented Jun 10, 2017

@gaearon Is it really enough to set NODE_ENV=production to ensure that react and react-dom/server don't load and execute unnecessary code in production? Is it mentioned anywhere in the docs?

I'm using Webpack 2.6.1 and I'm trying to use minified versions of react and react-dom/server (both 15.5.4) using the following config:

resolve: {
  alias: {
    react: require.resolve('react/dist/react.min.js'),
    'react-dom/server': require.resolve('react-dom/dist/react-dom-server.min.js'),
  },
},

but I'm getting exactly the same error, i.e. Cannot read property 'ReactCurrentOwner' of undefined.

@luchillo17
Copy link

@szimek I'm having the exact same error, except in React-Native and no react-dom, any idea where to look?

image

@gaearon
Copy link
Collaborator

gaearon commented Jun 13, 2017

@luchillo17 You seem to be mixing incompatible versions of react and react-native.

@luchillo17
Copy link

Oh i didn't think that'd be a thing, is there an official compatibility chart?

@gaearon
Copy link
Collaborator

gaearon commented Jun 13, 2017

React Native always supports only one version of react package. You can find out which one it is by checking node_modules/react-native/package.json and looking for react peer dependency there. You should already get a warning if you use the wrong one.

@luchillo17
Copy link

@gaearon I've looked at that package.json file and the version is 0.44.3, the peer dependency is 16.0.0-alpha.6 and the react version i have installed is 16.0.0-alpha.6.

image

At this point i'm considering starting a new project and moving my source folder.

@luchillo17
Copy link

I've been testing more, this is my output in a component with just React-Native's Text component:
image
And this is with Native-Base's Text component:
image
Please don't tell me Native-Base also needs to ensure some compatibility.

@gaearon
Copy link
Collaborator

gaearon commented Jun 14, 2017

Please provide a project reproducing the problem. People generally use RN without such issues so you need to give more details to help you.

@luchillo17
Copy link

Can i tell react-native-cli to use npm instead of yarn?, i think most people don't use yarn right?

@gaearon
Copy link
Collaborator

gaearon commented Jun 14, 2017

Sorry, you'd need to ask this in react-native repository. I don't know.

@luchillo17
Copy link

So i guess providing you with a repo would mean nothing since it's a React-Native app?

@gaearon
Copy link
Collaborator

gaearon commented Jun 14, 2017

I can take a look at it to see if anything is wrong on the react side of things but I can't answer questions like

Can i tell react-native-cli to use npm instead of yarn?

because react-native-cli is not part of this repo.

@luchillo17
Copy link

Also i'm using typescript, maybe that's part of the issue?

@gaearon
Copy link
Collaborator

gaearon commented Jun 14, 2017

I don't know. As for your report,

I've looked at that package.json file and the version is 0.44.3

This doesn't sound right. You can see ReactGlobalSharedState.js in your stack trace. This file doesn't exist in react-native@0.44.3 (try searching). So if you see it in the redbox, you are not running 0.44.

That file was added in react-native@0.45. But 0.45 also bumped minimal required react version which wouldn't crash with it.

So something is wrong with your report.

@luchillo17
Copy link

Ok i've updated deps, now dependencies are like this:
image

And error has changed to:
image

@gaearon
Copy link
Collaborator

gaearon commented Jun 14, 2017

Please provide minimal code reproducing the problem. It's likely the problem is exactly what the error message says. In future versions we'll provide more details there.

@luchillo17
Copy link

I'll try making it from zero because the current error message says Maximum call stack size exceeded (which makes me think i have an infinite loop somewhere).

The error above shows after activating Debug JS Remotely.

@luchillo17
Copy link

@gaearon I've started a new one, still no business logic, only adding typescript, now it shows Maximum call stack size exceeded from start until i activate Debug JS Remotely from menu, this one i can publish to Github, do you want to take a look?

@luchillo17
Copy link

@gaearon I've put my base project here:
https://github.com/luchillo17/react-native-error-repo
Can you see if it works for you?

@gaearon
Copy link
Collaborator

gaearon commented Jun 19, 2017

If it's not related to the issue you originally described, please create a new issue with your project in React Native repo. Thanks!

@luchillo17
Copy link

@gaearon I've created one in React Native repo since it might be related to them instead of just React:
facebook/react-native#14614

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

No branches or pull requests

7 participants