Bug: Nested setState and unstable_batchedUpdates (are they ignored?)

[salvoravida]

Nested setState and unstable_batchedUpdates (are them ignored?)

React version: 16.12

Steps To Reproduce

https://codesandbox.io/s/batchedupdates-uselayouteffect-evj8s

open profile after click, you will see 3 commit.

it seems that even if we use unstable_batchedUpdates, nested setStates called on
didUpdate/layouteffect do not get batched.

[markerikson]

The sandbox isn't actually testing the behavior you think it is.

First, React already wraps all event handlers in unstable_batchedUpdates(). That's why multiple queued state updates in a single handler result in one re-render. So, the manual call to unstable_batchedUpdates() does nothing there.

Second, either way, that button click event handler is only queueing one state update, not multiple, so there's no change in behavior.

Third, each of those child components is adding an additional state update in the commit phase, but that's after the batchedUpdates() wrapper has already completed, so it's not like there's any way that could get batched.

On the other hand, what React-Redux does is call unstable_batchedUpdates(), in the commit phase, as multiple nested child components are notified and queue state updates. Very different logic.

[salvoravida]

Sorry I do not think so. React redux call notifynestedsub under uselayouteffecct that is exact the problem of this issue. Open profile of this original nested redux update with connect.

Anyway it would be useful to know from react core team if this is an expected behavior, setStates sync in uselayouteffecct does not get batched, and there are many commit even if user will not see them due to sync.

[bvaughn]

Mark is right that unstable_batchedUpdates only groups state updates that are scheduled within the callback you pass it. (Once the updates are processed, you're no longer in the scope of unstable_batchedUpdates and normal batching behavior is applied.)

[markerikson]

@bvaughn: just to confirm something:

If a component tree has something like this:

function Parent() {
    useLayoutEffect(() => {
        ReactDOM.unstable_batchedUpdates( () => {
            eventEmitter.trigger("update");
        });
        
    });

    // return some nested child components
}

function Child() {
    const [, forceUpdate] = useReducer(s => s + 1, 0);

    useLayoutEffect( () => {
        eventEmitter.on("update", forceUpdate);
        return () => eventEmitter.off("update", forceUpdate);
    }, []);
}

I would expect that when the parent component re-renders, all nested children would queue updates due to the event emitter triggering. But, because the calls to forceUpdate() are all within the parent's batchedUpdates(), all of those will get batched into a single re-render, which then happens synchronously as part of Parent's commit phase.

Is that a correct summary?

[salvoravida]

Mark is right that unstable_batchedUpdates only groups state updates that are scheduled within the callback you pass it. (Once the updates are processed, you're no longer in the scope of unstable_batchedUpdates and normal batching behavior is applied.)

just to confirm the call stack of this issue:

ReactDOM.unstable_batchedUpdates(() => {
      setCount1(c => c + 1);
      // ....
      useLayoutEffect1(()=>{
              setCount2(c => c + 1);
               // ....
              useLayoutEffect2(()=>{
                     setCount3(c => c + 1);
                       // ....
              }) 
      })     
});

so the batches are interrupted @ first useLayoutEffect? (even if next calls are sync) ?

[bvaughn]

@markerikson Yes.

@salvoravida That is not the call stack. The function you pass to useLayoutEffect won't be called under the render has finished, and the render won't even start until the function you pass to unstable_batchedUpdates has finished running. (That's how updates are batched. React waits until that callback is done so that it knows no more updates are pending.)

[salvoravida]

@bvaughn so there is no way to batch all updates from an external source emitted in top down in a single commit.

useMutableSource will handle this case?

[markerikson]

@salvoravida : The issue for React-Redux specifically is that we can't run the mapState functions for nested connected components until their parents have completed rendering, because we need the latest props to pass to mapState, and parent components might stop rendering a connected child (aka, the "zombie child" and "stale props" issues).

If those weren't a point of concern, we could just go ahead and let all components subscribe to the root Subscription, and batch all of them into a single render pass.

So, for our current implementation, we must have multiple commits to handle things correctly working down the tree.

[salvoravida]

@markerikson yes i know the issues, and i agree with you.

What i exspected is that using LayoutEffects handlers that are sync with renders, and batched updates, multiple sync commit where merged to one.

Maybe we see 4 commit into Profiler, but as they are sync, they are de-facto merged as browser will (not paint/user will not see) them until render-sync finished

@bvaughn is correct?

[markerikson]

Yeah. In our case, they're separate render passes, all run synchronously during the commit phase. Because they're run sync, the browser won't repaint until after the commit phase is complete.

[bvaughn]

Maybe we see 4 commit into Profiler, but as they are sync, they are de-facto merged as browser will (not paint/user will not see) them until render-sync finished

This sounds misleading 😄 If the Profiler reports 4 commits, there were 4 commits- (which means e.g. 4 DOM mutations)- even if there was no painting between them.

I'm going to go ahead and close this issue since I think the original question has been answered!