Document that setState() callback is not guaranteed to be called if component unmounts

[fatfisz]

Currently the only info I could find about the callback argument of the setState method is this:

The second (optional) parameter is a callback function that will be executed once setState is completed and the component is re-rendered.

I've been working on a small tool that relies on the callbacks being called, and I found out that in some cases that never happened: the state was set, the callback was pushed into the queue, then the component was unmounted and the callback was lost and never called.

Now, I don't say that I can't infer the possibility of such scenario from the description above, but an info about when exactly can such callback be considered lost would be helpful. My findings suggest that componentWillUnmount is the right place, but I'd be more confident with some assurance from the creators of React :)

Also this description suggests that when shouldComponentUpdate returns false (so the component is not re-rendered), the callback will not be called - which is not the case, as the callback still gets called.

I'm using React 0.14.7.

[gaearon]

I've been working on a small tool that relies on the callbacks being called, and I found out that in some cases that never happened: the state was set, the callback was pushed into the queue, then the component was unmounted and the callback was lost and never called.

Could it be that you were calling setState from componentWillMount? I believe there’s a known (albeit undocumented and definitely not obvious) issue there:

react/src/renderers/shared/reconciler/ReactUpdateQueue.js

Lines 140 to 144 in 1e81561

	// TODO: The callback here is ignored when setState is called from
	// componentWillMount. Either fix it or disallow doing so completely in
	// favor of getInitialState. Alternatively, we can disallow
	// componentWillMount during server-side rendering.
	enqueueUpdate(internalInstance);

[gaearon]

The issue I was referring to is #1740 and apparently it was fixed. Would be great to verify. If you can do that, and/or provide a test case reproducing the issue, that would be hugely helpful.

[fatfisz]

I'm sure that the state is not changed from componentWillMount, as the component is already mounted.

The setting in my project is a bit complicated: there are two components passed to the react-router Routes, and one of them has some children. For each of these children a function is saved that, when called, calls setState with a callback.
The trigger for this situation is a route change - I have a function given as the Router's onUpdate prop (which seems to be used as a callback in Router's own setState call), from which the saved functions of the children are called (and so setState is called for each one of them). However, because after that the route component is changed, the children are unmounted along with their parent and so their callbacks disappear.

I traced this down to the runBatchedUpdates function - there is an array of components, and at start the callbacks are all there. However, after ReactReconciler.performUpdateIfNecessary is called for the parent component, its children components get destroyed and their callback queues are set to null. That's why in the next iterations of the loop component._pendingCallbacks is null for the children, and so the callbacks are not called.

I will try to simplify the example down and post a link to jsFiddle.

[fatfisz]

Ok, so this turned out to be a lot more complicated than I thought... here's the demo: https://jsfiddle.net/fatfisz/vLv6k3xL. I will try to describe this here too:

In my example there are 3 components: Container, A, and B. B is contained in A, and A is contained in Container. Depending on its state, Container will render A with or without B.

First we start with all components rendered. Then something cases the state of Container to change so that it will render A without B. But then the update of A is prevented because of the shouldComponentUpdate method returning false (this is the important bit which I have overlooked before, sorry...).

Then the callback for this update (in Container) fires and ruins everything. The update of A is forced (or its state is changed and shouldComponentUpdate returns true this time), but it doesn't cause B to be removed immediately (batched changes?). Then the update of B is forced (the changes are invoked synchronously from Container setState callback). The update for B has a callback attached.

After those changes are batched, first the A component re-renders and finaly B is unmounted. This causes the callbacks registered in B to be cleared out, and so the callback is lost forever.

The conclusion seems to be that if the callback passed to setState is not called before componentWillUnmount, then it probably won't happen after that too. This depends on what happens between inst.componentWillUnmount() and this._pendingCallbacks = null; (from ReactCompositeComponent.js), and it doesn't seem to be much - apart from the ReactReconciler.unmountComponent call, which I don't have a clue about.

Sorry for the complicated example :)

[gaearon]

Thanks for working it out. I realize now that I read your description without paying enough attention.
I missed this part:

I've been working on a small tool that relies on the callbacks being called, and I found out that in some cases that never happened: the state was set, the callback was pushed into the queue, then the component was unmounted and the callback was lost and never called.

IMO this is expected behavior: setState callback fires after changes have been flushed to the DOM. Since they were never flushed, it was never called. However we should probably communicate this better in the documentation.

@spicyj Is my interpretation correct?

[fatfisz]

I also agree that this should be expected, sorry if I was being unclear.

This still leaves the will be executed once setState is completed and the component is re-rendered, which is not true for shouldComponentUpdate returning false. So maybe something like this would do:

The second (optional) parameter is a callback function that will be executed once setState is completed, even if the component is not re-rendered (e.g. when shouldComponentUpdate() returns false). However, if the component is unmounted before the callback is called, it won't be called later anymore. You can handle such situations in the componentWillUnmount() method.

[fatfisz]

Oh, and let's not forget about callbacks for replaceState (haven't checked it, but the callback is pushed to the same queue, so...) and forceUpdate. I'm didn't mention setProps and replaceProps, as they are deprecated and will be removed anyway.

[sophiebits]

Yes, I think that the setState callback fires after the rerender (just after componentDidUpdate) and shouldn't fire if the component unmounts first.

[gaearon]

This doesn't seem like a common confusion so I'm not sure it's worth making the language more complex there.

The docs now say:

The second parameter is an optional callback function that will be executed once setState is completed and the component is re-rendered. Generally we recommend using componentDidUpdate() for such logic instead.

If you follow the tip of using cDU instead, this wouldn't be a problem.

Still, if you see an elegant way to phrase it without making the sentence very complicated, please feel free to send a PR! Thanks.

[kujon]

I run into the same behaviour and I think the docs could elaborate on it a little bit more. In my specific case, I was setting up a subscription in componentDidMount and calling setState to keep track of what I'm subscribed to. The other part of my app would immediately figure out that a redirect is needed and the component would unmount before change of the state was fully applied.

I guess the docs could mention that references/ids of any side-effects should not be kept in the state.

[whymarrh]

I'm only realizing this now as well. I don't think that the recommendation to use cDU is explicit enough to cover this behaviour. I think that the description @fatfisz put forward, while wordier, is more explicit.