Batching can update one component twice, causing unpredictable lifecycle

[m4tthumphrey]

This one is hard to explain or reproduce but I'll try...

I'm using the following mixin which has changed slightly from the original I found in a gist a while back:

var React = require('react');

var LayeredComponentMixin = {

  componentDidMount: function() {
    this._layer = document.createElement('div');
    document.body.appendChild(this._layer);
    this._renderLayer();
  },

  componentDidUpdate: function() {
    this._renderLayer();
  },

  componentWillUnmount: function() {
    this._unrenderLayer();
    document.body.removeChild(this._layer);
  },

  _renderLayer: function() {
    var layer = this.renderLayer();

    if (null === layer) {
      layer = <noscript />;
    }

    React.render(layer, this._layer);

    if (this.layerDidMount) {
      this.layerDidMount(this._layer);
    }
  },

  _unrenderLayer: function() {
    if (this.layerWillUnmount) {
      this.layerWillUnmount(this._layer);
    }

    React.unmountComponentAtNode(this._layer);
  }

};

module.exports = LayeredComponentMixin;

This allows me to create layered components, in my case a modal dialog. I use the following component to build my modals:

var React = require('react');

var ToolboxUserActionCreators = require('../../actions/ToolboxUserActionCreators');

var ModalStore = require('../../stores/ModalStore');

function getStateFromStore() {
  return {
    modalCount: ModalStore.getModalCount()
  };
}

var Modal = React.createClass({

  componentWillMount: function() {
    this.setState({
      modalLevel: ModalStore.getModalCount()
    });
  },

  componentDidMount: function() {
    ModalStore.addChangeListener(this.onChange);
    document.addEventListener('keydown', this.handleKeyDown);
  },

  componentWillUnmount: function() {
    ModalStore.removeChangeListener(this.onChange);
    document.removeEventListener('keydown', this.handleKeyDown);
  },

  getInitialState: function() {
    return getStateFromStore();
  },

  getDefaultProps: function() {
    return {
      className: 'feature'
    }
  },

  render: function() {
    var className               = 'toolbox2-modal-content toolbox2-modal-' + this.props.className;
    var modalBackdropClassName  = 'toolbox2-modal-backdrop';
    var handleBackdropClick     = this.handleBackdropClick;
    var killClick               = this.killClick;
    var modalLevel              = this.state.modalLevel;
    var totalModals             = this.state.modalCount;

    if (modalLevel < totalModals) {
      modalBackdropClassName += ' toolbox2-modal-backdrop-secondary';
    }

    return (
      <div className={modalBackdropClassName} onClick={handleBackdropClick}>
        <div className={className} onClick={killClick}>
          {this.props.children}
        </div>
      </div>
    );
  },

  killClick: function(e) {    
    e.stopPropagation();
  },

  handleBackdropClick: function() {
    this.props.onRequestClose();
  },

  handleKeyDown: function(e) {
    if (e.keyCode === 27) {
      this.handleBackdropClick();
    }
  },

  onChange: function() {
    this.setState(getStateFromStore());
  }

});

module.exports = Modal;

This combination works perfectly. That is until I close the highest modal. If an <input /> component is used inside of the <Modal> render method, ie this.props.children, the following error is thrown. All other element types (that I've tried including textarea and select work fine, but input throws the following:

Uncaught Error: Invariant Violation: getDOMNode(): A component must be mounted to have a DOM node

The trace is as follows:

Uncaught Error: Invariant Violation: getDOMNode(): A component must be mounted to have a DOM node. bundle.js:60783
invariant bundle.js:60783
ReactBrowserComponentMixin.getDOMNode bundle.js:62069
ReactCompositeComponent.createClass.componentDidUpdate bundle.js:64334
assign.notifyAll bundle.js:68718
ON_DOM_READY_QUEUEING.close bundle.js:72258
Mixin.closeAll bundle.js:68960
Mixin.perform bundle.js:68901
Mixin.perform bundle.js:68887
assign.perform bundle.js:56107
(anonymous function) bundle.js:56186
wrapper bundle.js:53734
Mixin.closeAll bundle.js:68960
Mixin.perform bundle.js:68901
ReactDefaultBatchingStrategy.batchedUpdates bundle.js:64050
batchedUpdates bundle.js:56122
ReactEventListener.dispatchEvent bundle.js:64980

ReactCompositeComponent.createClass.componentDidUpdate bundle.js:64334 is here.

This only happens when there are several modals on top of each other; one dialog with an <input /> runs fine with no error.

The weird thing is that the app still runs fine, the error thrown has no impact (apart from being thrown) on how my app performs....

This is on 0.12RC1 using webpack to build.

[m4tthumphrey]

I've spent all day on this and still no closer to solving it. I have since found out though that if I use the ESC key to close the modal no error is thrown...

[syranide]

Like the error says, it seems your calling getDOMNode on a component after it has been unmounted. Go through all your getDOMNode calls and make sure they're correct, then do the same again but log this.isMounted() and figure out which one it is that's called "too late".

[m4tthumphrey]

This is the point tho, the getDOMNode() is coming from the ReactDOMInput component (see here) not any of my components....

[m4tthumphrey]

I'm debugging deep into the core now and ALL <input /> components are marked as UNMOUNTED which is causing the problem. I really don't get this. How can a component update if it's unmounted? And it's ONLY <input />... <select /> <textarea /> etc are all fine...

[plievone]

ReactDOMInput uses asap (formerly setImmediate) to queue updates, perhaps there is something about unmounting that should be added to tests?

[m4tthumphrey]

I have temporarily "fixed" this by calling this.forceUpdate() just before the modal is closed. Not sure why this fixes it but it does and it puts an end to 5 days of trying to figure it out!

[sophiebits]

@m4tthumphrey Can you build a simple repro case for this on jsbin or jsfiddle? This does sound like a bug but without a way for me to reproduce it it'll be hard to fix.

[m4tthumphrey]

I'll try to reproduce this on jsf tomorrow... @spicyj

[m4tthumphrey]

Still haven't had a chance to replicate but it appears that this is only happening Firefox and IE and not in Chrome.

[m4tthumphrey]

@spicyj I don't think I'm going to be able to replicate this outside of my app (its too big). If I make the app available to you publically, would that help?

[m4tthumphrey]

K, so I've wrapped componentDidUpdate() in both ReactDOMInput and ReactDOMSelect with

if (this.isMounted()) {
  // ...
}

and all is good in all browsers. Obviously there is a bug somewhere but I'm too deep in this problem to find the root cause. I will leave this issue open and if anyone fancies digging into it let me know and I'll set up a public version of the dev app so you can take a look.

[zpao]

I think this should be fixed in master thanks to #2503

[m4tthumphrey]

Hey guys, I actually updated to 12.1 today and I tested it before adding the hack and it has been working fine so looks like it was fixed in 0.12.1 somewhere :)

[sophiebits]

0.12.1 just had changes to JSX so if you were on 0.12 before you shouldn't see any difference.

[m4tthumphrey]

This has raised it's ugly head again since 0.13... The error message is now:

Uncaught Error: Invariant Violation: Component (with keys: getDOMNode,_handleChange,props,context,state,refs,_reactInternalInstance) contains render method but is not mounted in the DOM

@spicyj @zpao @syranide

[sophiebits]

@m4tthumphrey Do you have a repro case for this? Are you sure you're calling getDOMNode at a time that the node is still in the DOM?

[m4tthumphrey]

Well yes I guess so. It works fine in 0.12.2. And no its so baked in my app that I can't repro it without uploading the test app, which I'm happy to do if it helps.

The call to getDOMNode() is happening inside componentDidMount() and componentDidUpdate()

    var button = this.refs.tooltip.getDOMNode();

[mfunkie]

This is getting me too after upgrading to 0.13. The problem seems to exist with an input that I have nested in a few components within a React Bootstrap Modal and I close the modal. I can't get much more of a trace on this than that though...

[m4tthumphrey]

@mfunkie Yes that case sounds exactly the same except I'm not using bootstrap.

[mfunkie]

More on this, with my modal, I'm using the React Bootstrap Overlay Mixin which creates a new React tree. If I just render the Modal as a child component, opening and closing it does not trigger the error anymore. Could this be related to React elements being on a different Tree, React.unmountComponentAtNode being called, and for some reason that Input node is still being checked against for updates?

[mfunkie]

It turns out in my issue, I had two components listening to the same Flux Store. The Container component that would run the Overlay, and the inner body of the Modal. By adding the following function to the inner Modal Body, I was able to avoid the error

shouldComponentUpdate(nextProps, nextState) {
  if (!nextState.modalOpen && this.state.modalOpen) {
    return false;
  }
  return true;
}

My best guess is that the change listener was still queued up to run and an inner state was rendered after the Modal had already been considered closed.

[wilkerlucio]

@mfunkie I'm having the same issue as you, I was using react-modal before and just changed to the one in react-bootstrap.

[chetstone]

I'm also using react-bootstrap and ran into this issue. I'm using react@0.12.2, react-bootstrap@0.17.0

@mfunkie workaround fixed it for me. Thanks!

[gaearon]

This tripped me, I'd have expected findDOMNode(this) to return null for unmounted components.

[gaearon]

I guess throwing makes sense, but the error message is too cryptic:

Uncaught Error: Invariant Violation: Component (with keys: getDOMNode,_handleChange,props,context,state,refs,_reactInternalInstance) contains render method but is not mounted in the DOM

I honestly thought I had duplicate React or something, so I didn't even bother to check whether the component was mounted. (It wasn't.)

[jimfb]

@gaearon Tracking error mesage verbiage as #4233.

[aaronjensen]

The error does not happen if this line is removed: https://github.com/aaronjensen/react-2410-repro/blob/master/app/create_router.jsx#L10

[m4tthumphrey]

@spicyj I know, I wasn't criticising or anything, just updating the issue. To be honest I haven't looked into this issue for months as still use the temp solution mentioned above. I'll revisit this week.

[sophiebits]

@aaronjensen Thanks for the repro case. I'll try to look into this.

@m4tthumphrey No worries.

[sophiebits]

Okay, I tracked down the root cause.

If updates to a parent and child component are both enqueued during a batch and while the parent is updating another update to the child is enqueued, we end up reconciling the child twice which breaks other assumptions and interleaves lifecycle methods incorrectly. In both 0.13.2 and master (if you fix the batchedUpdates reference), this code

var Parent = React.createClass({
  getChild: function() {
    return this.refs.child;
  },
  render: function() {
    return <Child ref="child" />;
  }
});

var once = false;
var Child = React.createClass({
  getInitialState: function() {
    return {updated: false};
  },
  componentWillUpdate: function() {
    if (!once) {
      once = true;
      this.setState({updated: true});
    }
  },
  componentDidUpdate: function() {
    console.log('child update');
  },
  render: function() {
    console.log('child render');
    return <div />;
  }
});

var parent = React.render(<Parent />, document.getElementById('container'));
var child = parent.getChild();
console.log('--- start of batch');
React.addons.batchedUpdates(function() {
  parent.forceUpdate();
  child.forceUpdate();
});
console.log('-- end of batch');

produces the output

child render
--- start of batch
child render
child render
child update
child update
-- end of batch

where the render and update lines should be interleaved.

[sophiebits]

I thought we had a warning for setState in componentWillUpdate but I guess not.

[m4tthumphrey]

Just an quick update. I thought I'd give 0.13.3 a go and straight away ran into

Uncaught Error: Invariant Violation: Component (with keys: getDOMNode,_handleChange,props,context,state,refs,_reactInternalInstance) contains render method but is not mounted in the DOM

Which doesn't happen on 0.12.2 (the version I'm currently resting on).

[sophiebits]

@m4tthumphrey That message means the same as "Invariant Violation: getDOMNode(): A component must be mounted to have a DOM node." In #4727 I improved the message to make it a bit clearer.

I wouldn't expect this issue to be fixed until we make a specific attempt to fix it at which time you'll see stuff happening on this issue.

[m4tthumphrey]

I've just upgraded my app to 0.14 and so far I haven't been able to reproduce this bug. @spicyj could anything you know of fixed this prior to the 0.14 release?

[sophiebits]

No, it isn't fixed. The test case I posted still has the problem.

[sophiebits]

#3762 has a nice repro from @scarletsky; should verify that it gets fixed too.