facebook / react

The library for web and native user interfaces.
https://react.dev
MIT License
228.71k stars 46.81k forks source link

React DOM tree root should always have a node reference #6232

Closed mbrio closed 7 years ago

mbrio commented 8 years ago

I've built a task execution pipeline utilizing React v14 and I've begun testing it against React v15 and have noticed that all of a sudden I'm getting an invariant error: React DOM tree root should always have a node reference.

When I comment the line that is causing this error (https://github.com/facebook/react/blob/3b96650e39ddda5ba49245713ef16dbc52d25e9e/src/renderers/dom/client/ReactDOMComponentTree.js#L171) everything works as it did with v14, and all of my unit tests pass. I assume this check is important to the new internal updates that have been made since v14.

I was able to make it work with a hacky bit of code that adds properties to the instance, _flags and _nativeNode, you can see this here: https://github.com/mbrio/react-pipeline/blob/react-15/src/startTasks.js#L8-L9.

I believe the issue is that with v14 I was able to use the server rendering code but in v15 I can no longer call forceUpdate or setState without errors occurring. Is there a way of bypassing the above error using the client renderer, or a way of calling setState/forceUpdate with the server renderer?

singggum3b commented 8 years ago

I'm also experience this issue when rendering server-side

ColemanGariety commented 8 years ago

I'm also getting this issue since updating to React 15.

The only solution I can find is (while using flux) wrapping the Store.addChangeListener(this._onChange) line in an if (process.browser) block. This is unsatisfactory, though.

All it does it prevent the state-changing code to be "reached" by the server. I put all my state-changing code in that function, including setting a cookie. This way, server never touches it.

jimfb commented 8 years ago

@mbrio @JacksonGariety What browser are you guys using? Any chance you were using Chrome 49? We've seen a similar bug (https://github.com/facebook/react/issues/6472) and I'm unable to reproduce in Chrome 50, so I'm wondering if this might just have been a chrome bug.

@mbrio @JacksonGariety Can either of you provide a jsfiddle that demonstrates the issue? You can fork http://jsfiddle.net/9kungxn4/ and use ReactDOMServer to get access to the server-side-rendering logic if you need that for your repro.

ColemanGariety commented 8 years ago

@jimfb if you update the files on your server to react 15.0.1 I think you'll see the error in this fiddle I made: http://jsfiddle.net/v509v7Lm/3/

jimfb commented 8 years ago

Using v15.0.1, I get that event emitter is not defined: https://jsfiddle.net/ft5xeugv/

jimfb commented 8 years ago

Using EventEmitter from CDN, I don't seen any errors: https://jsfiddle.net/dv85q3w2/

damienleroux commented 8 years ago

+1? @jimfb, did you succeed to reproduce? I have the same problem with server-side rendering since I have update from 0.14.7 to 15.0.1:

uncaughtException: React DOM tree root should always have a node reference. Invariant Violation: React DOM tree root should always have a node reference. at invariant (D:\WebFront\GitHubs\frontEnd\node_modules\fbjs\lib\invariant.j s:38:15) at getNodeFromInstance (D:\WebFront\GitHubs\frontEnd\nodemodules\react\lib\ ReactDOMComponentTree.js:164:67) at ReactDOMComponent.Mixin.getNativeNode (D:\WebFront\GitHubs\frontEnd\node modules\react\lib\ReactDOMComponent.js:846:12) at Object.ReactReconciler.getNativeNode (D:\WebFront\GitHubs\frontEnd\node_m odules\react\lib\ReactReconciler.js:54:29) at ReactDOMComponent.ReactMultiChild.Mixin._updateChildren (D:\WebFront\GitH ubs\frontEnd\node_modules\react\lib\ReactMultiChild.js:302:42) at ReactDOMComponent.ReactMultiChild.Mixin.updateChildren (D:\WebFront\GitHu bs\frontEnd\node_modules\react\lib\ReactMultiChild.js:259:12) at ReactDOMComponent.Mixin._updateDOMChildren (D:\WebFront\GitHubs\frontEnd\ node_modules\react\lib\ReactDOMComponent.js:841:12) at ReactDOMComponent.Mixin.updateComponent (D:\WebFront\GitHubs\frontEnd\nod e_modules\react\lib\ReactDOMComponent.js:687:10) at ReactDOMComponent.Mixin.receiveComponent (D:\WebFront\GitHubs\frontEnd\no de_modules\react\lib\ReactDOMComponent.js:643:10) at ReactDOMComponent.wrapper as receiveComponent at Object.ReactReconciler.receiveComponent (D:\WebFront\GitHubs\frontEnd\nod e_modules\react\lib\ReactReconciler.js:103:22) at ReactCompositeComponentMixin._updateRenderedComponent (D:\WebFront\GitHub s\frontEnd\node_modules\react\lib\ReactCompositeComponent.js:653:23) at ReactCompositeComponentMixin._performComponentUpdate (D:\WebFront\GitHubs \frontEnd\node_modules\react\lib\ReactCompositeComponent.js:635:10) at ReactCompositeComponentMixin.updateComponent (D:\WebFront\GitHubs\frontEn d\node_modules\react\lib\ReactCompositeComponent.js:564:12) at wrapper as updateComponent at ReactCompositeComponentMixin.receiveComponent (D:\WebFront\GitHubs\frontE nd\node_modules\react\lib\ReactCompositeComponent.js:487:10) at Object.ReactReconciler.receiveComponent (D:\WebFront\GitHubs\frontEnd\nod e_modules\react\lib\ReactReconciler.js:103:22) at ReactCompositeComponentMixin._updateRenderedComponent (D:\WebFront\GitHub s\frontEnd\node_modules\react\lib\ReactCompositeComponent.js:653:23) at ReactCompositeComponentMixin._performComponentUpdate (D:\WebFront\GitHubs \frontEnd\node_modules\react\lib\ReactCompositeComponent.js:635:10) at ReactCompositeComponentMixin.updateComponent (D:\WebFront\GitHubs\frontEn d\node_modules\react\lib\ReactCompositeComponent.js:564:12) at wrapper as updateComponent at ReactCompositeComponentMixin.receiveComponent (D:\WebFront\GitHubs\frontE nd\node_modules\react\lib\ReactCompositeComponent.js:487:10)

gbhrdt commented 8 years ago

I am also getting the same error for SSR after updating to 15.0.1.

jimfb commented 8 years ago

@damienleroux @gbhrdt I am not sure. We have a couple of fairly complicated repros (100-ish lines, with dependencies like ReactRouter and Redux) of a similar bug which may or may not be related. We don't yet have any repro that is super simple/actionable and demonstrates a bug in React as oppose to ReactRouter/Redux. So to answer your question, if you can provide a simple Repro, that would still be super helpful, otherwise we will look at the other issues (https://github.com/facebook/react/issues/6538 and https://github.com/facebook/react/issues/6371) and loop back to this one later.

jimfb commented 8 years ago

@damienleroux @gbhrdt Ok, the repro that we have for the other issues is NOT related to anything dealing with SSR, so yes, we still need a repro of this!

kniknak commented 8 years ago

I've got same problem in Firefox. But there's no errors in Chrome. It possibly could be related with NODE_ENV = production|development and minification, I'm still trying to find out

TooTallNate commented 8 years ago

I'm getting this on the server-side using react-dom/server renderToString(). Repro case:

import React from 'react';
import { renderToString } from 'react-dom/server';

const El = React.createClass({
  getInitialState() {
    return { foo: 'bar' };
  },

  componentWillMount() {
    setTimeout(() => this.setState({ foo: 'baz' }), 1000);
  },

  render() {
    return (
      <span>{ this.state.foo }</span>
    );
  }
});

const html = renderToString(<El />);
console.log(html);

Compile w/ babel, then run:

$ node build/t.js
<span data-reactroot="" data-reactid="1" data-react-checksum="-798879379">bar</span>
/Users/nrajlich/GitHub/TooTallNate/n8.io/node_modules/react/node_modules/fbjs/lib/invariant.js:45
    throw error;
    ^

Invariant Violation: React DOM tree root should always have a node reference.
    at invariant (/Users/nrajlich/GitHub/TooTallNate/n8.io/node_modules/react/node_modules/fbjs/lib/invariant.js:38:15)
    at Object.getNodeFromInstance (/Users/nrajlich/GitHub/TooTallNate/n8.io/node_modules/react/lib/ReactDOMComponentTree.js:164:67)
    at Object.ReactDOMIDOperations.dangerouslyProcessChildrenUpdates (/Users/nrajlich/GitHub/TooTallNate/n8.io/node_modules/react/lib/ReactDOMIDOperations.js:30:38)
    at Object.wrapper [as processChildrenUpdates] (/Users/nrajlich/GitHub/TooTallNate/n8.io/node_modules/react/lib/ReactPerf.js:66:21)
    at processQueue (/Users/nrajlich/GitHub/TooTallNate/n8.io/node_modules/react/lib/ReactMultiChild.js:134:29)
    at ReactDOMComponent.ReactMultiChild.Mixin.updateTextContent (/Users/nrajlich/GitHub/TooTallNate/n8.io/node_modules/react/lib/ReactMultiChild.js:228:7)
    at ReactDOMComponent.Mixin._updateDOMChildren (/Users/nrajlich/GitHub/TooTallNate/n8.io/node_modules/react/lib/ReactDOMComponent.js:834:14)
    at ReactDOMComponent.Mixin.updateComponent (/Users/nrajlich/GitHub/TooTallNate/n8.io/node_modules/react/lib/ReactDOMComponent.js:687:10)
    at ReactDOMComponent.Mixin.receiveComponent (/Users/nrajlich/GitHub/TooTallNate/n8.io/node_modules/react/lib/ReactDOMComponent.js:643:10)
    at ReactDOMComponent.wrapper [as receiveComponent] (/Users/nrajlich/GitHub/TooTallNate/n8.io/node_modules/react/lib/ReactPerf.js:66:21)

Note that the error is thrown after 1 second, in the setState() call.

jimfb commented 8 years ago

@TooTallNate

  componentWillMount() {
    setTimeout(() => this.setState({ foo: 'baz' }), 1000);
  },

I feel like setState should not be called in a setTimeout registered in componentWillMount, since the setState will likely occur after the render has occurred. You should consider calling setState directly within componentWillMount, or using componentDidMount for this use case if it needs to happen after mounting.

Sounds like a usage error to me. Perhaps we could warn in this case, but I don't think this was ever a supported pattern.

I'm assuming we have a regression not related to setting state after a component has rendered. If everyone above is seeing this behavior as a result of something like @TooTallNate's example, I think we could just close out this error (or warn in this situation).

TooTallNate commented 8 years ago

It sounds like componentDidMount is the correct pattern for my example indeed.

To explain my situation a bit, I would like to find a good pattern to allow the server-side to wait for this component to "resolve", so-to-speak, before calling renderToString(). Are there any well-established and recommended patterns there?

jimfb commented 8 years ago

@TooTallNate Yeah, there are a bunch of people who want features like that. See https://github.com/facebook/react/issues/6481 and https://github.com/facebook/react/issues/1739. Short answer is: "no, we don't have a good solution for that".

03eltond commented 8 years ago

I'm also noticing this problem server side with a pattern similar to @TooTallNate. My specific case is trying to use react-router server side with async routes (we too want to wait for components to resolve due to code splitting etc.). When using async routes in react-router (aka getComponent, getIndexRoute, getChildRoutes, etc), the component will not immediately render since the route has not resolved as shown here. Later, when the route resolves, the state is set via an event listener that was attached in componentWillMount as shown here. On the client, this is no problem as the component renders again, but on the server, the render does not run again and the error is thrown. I realize this possibly should be addressed within react-router but thought I'd chime in since it seems relevant to the conversation.

jimfb commented 8 years ago

@03eltond The code you linked calls setState directly (not in a setTimeout) which is legal and entirely different from the example @TooTallNate showed. Does anyone have a simple repro (jsfiddle) that does not use setTimeout&friends?

fahrradflucht commented 8 years ago

I maybe have one... If you try to render this code on the server (I know rendering it on the server exactly in this setup doesn't make a lot of sense) I get the same error, but I'm not sure how Firebase does it's thing because it's minified.

aweary commented 8 years ago

@Fahrradflucht I think that's basically the same as what @TooTallNate posted since you're calling setState in a callback for an async action (this.firebaseRef.limitToLast(25).on) invoked in componentWillMount

03eltond commented 8 years ago

@jimfb a few lines up you can see the setState is actually wrapped in an event listener and is thus async and similar to calling with setTimeout.

jimfb commented 8 years ago

cc @ryanflorence @taion @mjackson @timdorr any comment?

https://github.com/reactjs/react-router/blob/master/modules/Router.js#L77

timdorr commented 8 years ago

@03eltond For SSR, we provide match , which, in essence, resolves all the asynchronous props and state before passing off execution to the callback. You shouldn't be using <Router> on the server, just <RouterContext>.

koistya commented 8 years ago

To explain my situation a bit, I would like to find a good pattern to allow the server-side to wait for this component to "resolve", so-to-speak, before calling renderToString(). Are there any well-established and recommended patterns there? -- @TooTallNate

As a side note, in React Starter Kit we discourage developers from making React components render asynchronously in favor of having all the async logic at the routing level. Here is an example of a typical app route (compatible with universal-router):

import fetch from '../../core/fetch';
import Layout from '../../components/Layout';
import Post from './Post';

export default {
  path: '/posts/:id',
  async action({ params }) {
    const resp = await fetch(`/api/posts/${params.id}`);
    const data = await resp.json();
    return {
      title: data.title,
      component: <Layout><Post {...data} /></Layout>
    }
  }
}
timdorr commented 8 years ago

@TooTallNate

To explain my situation a bit, I would like to find a good pattern to allow the server-side to wait for this component to "resolve", so-to-speak, before calling renderToString(). Are there any well-established and recommended patterns there?

The best thing to do is to look through your component tree prior to rendering. For data fetching, I have used tricks like static properties (applied via a decorator) on my components (UserPage.fetchData) or special props that I read from the ReactElement form. It depends on who is controlling the data flow (the component or its container).

What would be better is async property or render() support, but that doesn't seem to be on the horizon for the near term. In the meantime, static props seem to be the best way (they're similar to how Relay works too).

taion commented 8 years ago

As @timdorr says, with React Router, <Router> should not be used when rendering on the server – only the lower-level <RouterContext> component, that receives its state externally, in conjunction with the match helper that will (potentially) asynchronously run the match and invoke its callback when the match is completed.

I don't think we can move that setState call into componentDidMount, because it can potentially run synchronously, and we want it to run before the initial render in those cases. Maybe we just need to add an invariant that <Router> is not used when rendering on the server?

denvned commented 8 years ago

For SSR, we provide match, which, in essence, resolves all the asynchronous props and state before passing off execution to the callback. You shouldn't be using <Router> on the server, just <RouterContext>.

Actually, It is currently possible to use <Router> on the server:

const history = createMemoryHistory({ entries: [ req.url ] });
match({ history, routes }, (error, redirectLocation, renderProps) => {
  ReactDOMServer.renderToString(<Router {...renderProps} />);
});

And it could be bit simpler if https://github.com/reactjs/react-router/pull/3394 was merged:

match({ location: req.url, routes }, (error, redirectLocation, renderProps) => {
  ReactDOMServer.renderToString(<Router {...renderProps} />);
});

So, the question is why should I use <RouterContext> instead of <Router> on the server, while it is much more convenient to use <Router> on both, the client, and the server? (e.g. applyRouterMiddleware can be used only with <Router>).

cc @timdorr , @taion, @mjackson

timdorr commented 8 years ago

<Router> runs within the context of ReactDOM.render, resolving many of its effects asynchronously. match runs outside of the React context to resolve any asynchronous activity before passing into Reactland. You may use , but on the server side this is duplicate behavior. It will result in us creating duplicate subscribers and internal objects, which is wasteful in a server app. You are correct that right now it's the only way to do middlewares, but I believe the plan is to clean up all of that.

Keep in mind that back in the pre-1.0 days, we had Router.run as a required API, which did much of the same job as match now does. Maybe we need to bring it back? I'm not sure.

Regardless, this issue on the React repo isn't really the place to be discussing these things...

denvned commented 8 years ago

this issue on the React repo isn't really the place to be discussing these things...

@timdorr Yeah, lets continue in https://github.com/reactjs/react-router/issues/3290.

sophiebits commented 8 years ago

The repro case that @TooTallNate posted does throw this error in v15 but also throws an error in 0.14.7:

Uncaught TypeError: Cannot read property 'firstChild' of undefined

We want to add a warning when calling setState after a server render (#5473) but at least that case isn't a regression. I'll look into some of these others in a bit.

sophiebits commented 8 years ago

@JacksonGariety After cloning your repo (and manually installing babel-plugin-transform-object-rest-spread, chalk, lodash) I can load http://127.0.0.1:1337/ without errors. Am I doing something wrong?

sophiebits commented 8 years ago

Can anyone here give repro instructions for an example that works fine in 0.14 but is broken in 15? Small repro cases (ex: jsfiddle) preferred but I can also work with a full app to clone. Otherwise, I'll close this issue because all the cases I've tried so far do not repro or were also "broken" in 0.14.

ColemanGariety commented 8 years ago

@spicyj remove this conditional and try again: https://github.com/JacksonGariety/topsters2/blob/master/src/Components/Chart.js#L21

The error is specifically caused by ChartStore.addChangeListener(this._onChange).

nossila commented 8 years ago

I was having the same issue then I decided to remove parts of my project until I figured out what could be causing the issue. This are the lines that are causing it for me, removing them everything worked fine:

if (index === posts.length - 1) {
  this.props.relay.setVariables({
    pageSize: posts.length + pageSize
  });
}

Here's a more complete version of the React component with those lines so you can try to reproduce it: https://gist.github.com/nossila/a2f51a54c16ccf51268a49a1c0217074

Edited for clarity

mieszko4 commented 8 years ago

I had this issue while using setTimeout with this.setState. I've fixed this by adding a check global.document !== undefined since I only needed the timeout on browser side (I used it for an image slider).

f0rr0 commented 8 years ago

https://github.com/facebook/react/issues/7103

alanmoo commented 8 years ago

Not sure if this is relevant here, but it might help someone: I solved this on my react-router (non redux) SSR project by switching some componentWillMount methods to componentDidMount. These made async calls, though the project has other similar components. The difference with these is that the components making the async call aren't necessarily being rendered when ReactDOM serves them up. They're in a tab switcher which doesn't mount the nodes unless a state condition is met. (That should probably be handled by react-router as well, but weird code happens.)

papigers commented 8 years ago

@nossila having the same issue.l,also using react list. Have you found any solution?

mnpenner commented 8 years ago

I'm hitting this error when I call close() in this function:

export default function openReactModal(componentFn, modalProps={}) {
    let reactRoot = document.createElement('div');
    document.body.appendChild(reactRoot);
    const close = () => {
        if(reactRoot === null) {
            console.warn('You shouldn\'t call close() more than once');
            return;
        }

        ReactDOM.unmountComponentAtNode(reactRoot);
        reactRoot.parentNode.removeChild(reactRoot);
        reactRoot = null;
    };
    ReactDOM.render(<Modal isOpen={true} {...modalProps}>{componentFn({close})}</Modal>, reactRoot);
}

Why can't I unmount a root component?

mhuggins commented 7 years ago

I'm hitting the same issue as @mnpenner.

mhuggins commented 7 years ago

I ended up working around this by wrapping the unmountComponentAtNode code with _.defer (same as setTimeout(..., 0)).

nghuuphuoc commented 7 years ago

@mhuggins Your work around helped me too.

rafaelbiten commented 7 years ago

I was having the same problem and after reading a few comments I understood the problem. I have a modal that can be closed by clicking on the dark background overlay or by clicking a close button inside the modal content container. Clicking the overlay was fine, but clicking the button would cause the error since it was also triggering the same event on the overlay.

Adding event.stopPropagation(); to the event handler fixed the issue for me.

VictorQueiroz commented 7 years ago

requestAnimationFrame() did the job for me

slorber commented 7 years ago

@rafaelbiten thanks I had the same problem and your solution seems to work. Do you know why your solution works? Because in my case I'm 100% certain I'm not calling twice the closePortal hook and it still fails. Wondering if it's not some kind of event bubbling from unstable_renderSubtreeIntoContainer

tiendq commented 7 years ago

@mhuggins Thanks, setTimeout works for me too. I thinks problem is we call unmountComponentAtNode from inside of an event handler whle root element is actually removed when the event handler completed.

anasjaghoub commented 7 years ago

for those who still facing this problem, I've faced the same issue and non of the above worked with me. @slorber _renderSubtreeIntoContainer that causes the issue. In my case I was rendering a child component outside its parent. Modal component that renders out side the parent. and was using a context provider to pass the context for the Modal. this caused the problem and making the parent component to re-render every time and loses the state.

What fixed it is using https://github.com/cloudflare/react-gateway which provides rendering outside the parent.

Sinewyk commented 7 years ago

I thought "server side rendering" was side effects free. But apparently not.

We use it to inject content into Leaflet popovers, it just happens that during a cycle of Parent.setState, the Parent rerenders itself, which triggers the componentDidMount of a child component (the Map wrapper) ...

The map wrapper create clusters and create the associated popovers, by calling ReactDOMServer.renderToStaticMarkup() on a bunch of stuff. And these all trigger the React DOM tree root should always have a node reference ... by following the stack trace and what I could understand from it (very little), even though I called renderToStaticMarkup and it should not care about where I'm doing it from ... there seems to be shared state because I already was during a browser side transaction.

I should probably be able to make a small repro project. With theses steps.

edit: only on chrome though.

SiqiTian-minted commented 7 years ago

Now I'm seeing this error, only on Chrome 59 beta. All other browsers are fine.....

screen shot 2017-05-02 at 1 42 32 pm

azizhk commented 7 years ago

Getting this error on Chrome 57 as well. Haven't made any updates / changes actually. I think React Devtools got updated yesterday. And just checked disabling Devtools makes the error go away. But enabling devtools does not bring it back on first reload but on second reload after opening React panel in devtools.

jesenko commented 7 years ago

Disabling React Devtools fixed issue for me also...