Bug: useSyncExternalStore does not update internal value if a setState is also called, outside useEffect, causing store sets not re-render

[pandaiolo]

In a custom hook, I forgot to wrap my setState call in a useEffect, which did not pose any problems prior to React 18 but now this does not work anymore. The component does not re-render if the store is changed back to its initial value. I guess this might be expected, but there was no warning whatsoever, so it was hard to find out.

So this is more like a question:

• Is that behavior a bug?
• If it is not, would it be useful to have a warning for another developer making the same mistake?

React version: 18

Steps To Reproduce

1. Have a component use useSyncExternalStore (for example redux 8)
2. Call setState outside of useEffect or event handlers (in render)
3. Trigger a change to the store back and forth

Link to code example: https://codesandbox.io/s/react-18-redux-8-reproduce-bug-ojdx43

The current behavior

• The component does re-render if the store value is updated to something different than its initial value
• The component does not re-render if the store is updated to its initial value again

The expected behavior

• The component does re-render if the store value is updated to something different than its initial value
• The component also does re-render if the store is updated to its initial value again

Further digging

When I tried to trace down the issue, I found that the setState called outside useEffect would mess up with the fiber update queue, in particular the prior pushEffect supposedly calling updateStoreInstance with the latest value to have the internal cached instance up to date. It does not get called at all (cancelled? overridden?) because of the unwrapped setState.

As a consequence, that cached instance never gets updated. Any other value but the initial value hence correctly triggers a re-render, as the checkIfSnapshotChanged ends up always comparing against that initial value.

[pandaiolo]

In order to familiarize myself with react codebase, I wrote a small test that fails on this pattern. (even if it is expected so)

Here is it fwiw (in packages/react-reconciler/src/__tests__/useSyncExternalStore-test.js):

test('store value is correctly stored in current hook instance even with interleaved effects occurring', async () => {
    const store = createExternalStore('value:initial');

    function App() {
      const value = useSyncExternalStore(store.subscribe, store.getState);
      const [sameValue, setSameValue] = useState(value);
      if (value !== sameValue) setSameValue(value);
      return <Text text={value} />;
    }

    const root = ReactNoop.createRoot();
    act(() => {
      // Start a render that reads from the store and yields value
      root.render(<App />);
    });
    expect(Scheduler).toHaveYielded(['value:initial']);

    await act(() => {
      store.set('value:changed');
    });
    expect(Scheduler).toHaveYielded(['value:changed']);

    await act(() => {
      store.set('value:initial');
    });
    expect(Scheduler).toHaveYielded(['value:initial']); 
  });
});

The last assertion fails with the setSameValue line, and passes without.

[pandaiolo]

What I don't understand is that in renderWithHooks, there is the following block:

// Check if there was a render phase update
  if (didScheduleRenderPhaseUpdateDuringThisPass) {

Which runs if setState was called in render. Then, it calls again component function - but to do so, it resets the workInProgress state, including updateQueue. IIUC this discards the effects pushed by previous hooks, without flushing them?

That's why useSyncExternalStore effect to update store value is not run, in that case.

The fact that there is code written to manage setState calls in render, seem to acknowledge it is a legit use case?

I must be missing something 😅 how to make sure those effects are run even if component function is called again before end of work?

[markerikson]

Yes, it is legal to call setState() in the body of a function component, as long as you do it conditionally:

• https://beta.reactjs.org/learn/you-might-not-need-an-effect#adjusting-some-state-when-a-prop-changes
• https://reactjs.org/docs/hooks-faq.html#how-do-i-implement-getderivedstatefromprops
• https://blog.isquaredsoftware.com/2020/05/blogged-answers-a-mostly-complete-guide-to-react-rendering-behavior/#setting-state-while-rendering

[pandaiolo]

Ok, rel my previous comment, IIUC the discarding of work in progress hook is expected when state is dispatched in render phase. The work in progress queue should be recreated (to reflect new state).

useSyncExternalStore was using work in progress memoizedState to compare previous value, but in the case of a second call, this already contains the new value, defeating the purpose.

It works when using currentHook in priority. See attached PR with fix, let me know what you think!

[hawx1993]

a good solution to splitting UI and business logic, see: https://github.com/hawx1993/hooks-view-model