setState warning and too many getMeteorState calls

[froatsnook]

Hi,

See repro here: https://github.com/froatsnook/meteor-react-set-state-warning

I'm seeing a ton of these:
Warning: setState(...): Can only update a mounted or mounting component. This usually means you called setState() on an unmounted component. This is a no-op.

Basically I have:

TestList = ReactMeteor.createClass({

    getMeteorState: function() {
        return {
            tests: Tests.find(this.props.query).fetch()
        };
    },

    render: function() {
        var items = this.state.tests.map(function(test) { return <Test _id={test._id} key={test._id} />; };
        return <div>{items}</div>;
    }
});

where <Test /> is:

Test = ReactMeteor.createClass({
    getMeteorState: function() {
        return Tests.findOne(this.props._id_;
    },
});

Every time query changes in TestList, getMeteorState gets called in each Test, even though the Test itself doesn't change..

EDIT: OK, so implementing shouldComponentUpdate removes the warning. But is it necessary that getMeteorState be called? Intuitively it doesn't seem necessary, but maybe I don't understand this well enough.

[mikowals]

This is driven by TestList rendering twice: once for change in props and again when getMeteorState() completes. The first TestList render has all children ( at that point the state change is queued in afterFlush()) and triggers componentWillUpdate() on each child Test which queues an afterFlush() state update in each child. The second TestList render runs removing some children and then the children's state changes throwing your warning message.

I don't think afterFlush() is necessary for React v0.13+. From the changelog:

• Calls to setState in life-cycle methods are now always batched and therefore asynchronous. Previously the first call on the first mount was synchronous.
• setState and forceUpdate on an unmounted component now warns instead of throwing. That avoids a possible race condition with Promises.

Getting rid of the afterFlush() around setState() fixes your warning message but still leaves an extra render of TestList (and an extra render of each child) while its state has not changed. To allow this to be correctly prevented with shouldComponentUpdate() I think this package needs to also trigger getMeteorState() in componentWillReceiveProps().

I will make a pull request with the above.

Also, the find in TestList should have a {fields:{_id:1}} option or you should get the whole doc in TestList and pass it to each Test in props. For any change in a single child's properties TestList would try to render all the children, the children would getMeteorState(), and the children would either render or you would have to block the render with shouldComponentUpdate().

[froatsnook]

@mikowals WOW, thank you very much for your helpful input.

[kenlimmj]

I'm encountering exactly the same problem. Would you be able to share exactly how you fixed it?

[froatsnook]

@kenlimmj

What I did was add shouldComponentUpdate to my child component (the thing that is sometimes removed from the DOM as the parent changes). In my case the child only needs to be re-rendered when its name changes: https://github.com/froatsnook/meteor-react-set-state-warning/blob/master/client/Test.jsx#L8-L10