Obscure DOMChildrenOperations error when doing multiple updates

[sophiebits]

This throws TypeError: Cannot call method 'removeChild' of null.

http://jsbin.com/caxipewo/1/edit

/** @jsx React.DOM */

var X = React.createClass({
  getInitialState: function() {
    return {s: 0};
  },
  render: function() {
    if (this.state.s === 0) {
      return <div>
        <span></span>
      </div>;
    } else if (this.state.s === 1) {
      return <div></div>;
    } else {
      return <span></span>;
    }
  },
  go: function() {
    this.setState({s: 1});
    this.setState({s: 2});
    this.setState({s: 0});
    this.setState({s: 1});
  }
});

var Y = React.createClass({
  render: function() {
    return <div>
      <Z />
    </div>;
  }
});

var Z = React.createClass({
  render: function() { return <div />; },
  componentWillUpdate: function() {
    x.go()
  }
});

var xNode = document.getElementById("x");
var yNode = document.getElementById("y");

var x = React.renderComponent(<X />, xNode);

React.renderComponent(<Y />, yNode);
React.renderComponent(<Y />, yNode);

Maybe this can be simplified some more but this was the simplest repro I could make.

Thanks @fforw for sending a repro case over.

[syranide]

Wow, it really was a bug. Narrowed it down a little, but it's still rediculously "complex". Big props to @fforw for repro!

http://jsbin.com/caxipewo/2/

Anyway, just glancing at the code and from experience, I can safely assume that the issue is that DOMChildrenOperations receives multiple updates to the same node from ReactMultiChild. Which intuitively didn't make any sense at all to me, since updateChildren always calls processQueue when it's done and updateChildren is basically just recursive.

However, when taking another peek at the code, I'm pretty sure that the issue is that ReactMountChild (in this case) makes the invalid assumption that all updates belongs to the same React root. When that is not the case, an updateChildren to a component which itself updates another root will only processUpdates once everything is done. The updates to the other root will NOT processQueue after each updateChildren on that root because updateDepth will always greater than 0.

In practice this means that whenever updateChildren updates another root, all DOM updates on that root will be batched one after the other (as opposed to batching component updates) which is a violation of assumptions in DOMChildrenOperations. It's also very inefficient. Intuitively it seems that what should happen is that all component updates within a renderComponent call should be batchedUpdates (but that is not for sure), exactly how this should be solved in practice I'm not 100% sure right now.

But I'm 95% sure that this is what's happening, and the repro does have all the ingredients.

[syranide]

Speaking a bit to @spicyj in chat, I feel like the issue is that renderComponent and unmountComponentNodeAt when called from within updateChildren become asynchronous as opposed to synchronous as they usually are. Which leads to this issue, but also possibly a lot of other subtle issues.

[syranide]

http://jsbin.com/caxipewo/4/edit

Apparently renderComponent is not at all to blame...

[syranide]

http://jsbin.com/caxipewo/6/edit

Narrowed it down even further, apparently it breaks immediately on the first setState, the error just didn't show up until later unless prodded.

I still don't feel any more enlightened on the issue though...

[plievone]

@syranide Your latest two jsbins (4 & 6) throw different error for me (from this.refs.B.getDOMNode() in X componentDidUpdate):

Error: Invariant Violation: findComponentRoot(..., .0.0.0): Unable to find element. 
This probably means the DOM was unexpectedly mutated (e.g., by the browser). 
Try inspecting the child nodes of the element with React ID ``.

And, if one removes Z from A render(), the same X componentDidUpdate throws Uncaught TypeError: Cannot read property 'B' of undefined, because then A render won't have any refs on that render so this.refs will be undefined... React docs warn that "Never access refs inside of any component's render method - or while any component's render method is even running anywhere in the call stack.", don't know if it's relevant here.

[syranide]

@plievone Yeah different error, but I'm sure the cause is the same. The issue is the span not actually ever being rendered, just that in the old repro it doesn't actually turn into an error until later.

[plievone]

@syranide Ok, so perhaps here's a reduced test case, see console:
http://jsbin.com/caxipewo/11/edit
componentDidUpdate is being called twice in a row, and the first time the span is not there in DOM even though it has been rendered already. One culprit for this behavior may be that you are effectively calling setState on sibling component where setState would not be allowed on component itself (triggered via componentWillUpdate).

[syranide]

@plievone Huh? It does end up being rendered in your test case, and there's no error?

[plievone]

@syranide, no I didn't want to throw the error so that you can see that the componentDidUpdate will be called double. The point is that first the span is not there and if one would try to access it (as in your ref.getDOMNode()) it would throw. See console for flow.

[syranide]

@plievone this.refs.B is defined so it's an error regardless, and more than that, React expects the node to be there, but it isn't and throws an error. Even after s = 1 it still isn't physically there in the DOM. So something is wrong. Unless I'm missing something.

[plievone]

@syranide Yes, there is definitely a problem there as you said, sorry for not being clear. I just wanted to reduce it so that there are no refs or forceUpdates which may bring other problems. One could throw the error in componentDidUpdate for clarity, as the innerHTML is definitely empty where it should not be. But it is interesting that it will be called right away again, and then the span will be there.