diff --git a/packages/react-reconciler/src/ReactFiberHooks.js b/packages/react-reconciler/src/ReactFiberHooks.js index c5b4f6d1821b..edb5eacc8cc2 100644 --- a/packages/react-reconciler/src/ReactFiberHooks.js +++ b/packages/react-reconciler/src/ReactFiberHooks.js @@ -92,6 +92,7 @@ import { FormReset, } from './ReactFiberFlags'; import { + NoFlags as HookNoFlags, HasEffect as HookHasEffect, Layout as HookLayout, Passive as HookPassive, @@ -1774,21 +1775,31 @@ function updateSyncExternalStore( // commit phase if there was an interleaved mutation. In concurrent mode // this can happen all the time, but even in synchronous mode, an earlier // effect may have mutated the store. - if ( + const storeChanged = inst.getSnapshot !== getSnapshot || snapshotChanged || // Check if the subscribe function changed. We can save some memory by // checking whether we scheduled a subscription effect above. (workInProgressHook !== null && - workInProgressHook.memoizedState.tag & HookHasEffect) - ) { + (workInProgressHook.memoizedState.tag & HookHasEffect) !== HookNoFlags); + + // Even if nothing changed during this render, we push the effect so it is + // always in the effect list. That way it re-runs whenever the passive + // effects are reconnected, like when a hidden Activity tree is shown again. + // While the tree was hidden we were not subscribed to the store, so + // mutations during that window notified nobody, and if the reveal didn't + // re-render this component (or rendered before the mutation), nothing + // would ever detect them. When nothing changed, the effect is pushed + // without the HasEffect tag so a regular commit skips it. + pushSimpleEffect( + storeChanged ? HookHasEffect | HookPassive : HookPassive, + createEffectInstance(), + updateStoreInstance.bind(null, fiber, inst, nextSnapshot, getSnapshot), + null, + ); + + if (storeChanged) { fiber.flags |= PassiveEffect; - pushSimpleEffect( - HookHasEffect | HookPassive, - createEffectInstance(), - updateStoreInstance.bind(null, fiber, inst, nextSnapshot, getSnapshot), - null, - ); // Unless we're rendering a blocking lane, schedule a consistency check. // Right before committing, we will walk the tree and check if any of the @@ -1848,7 +1859,8 @@ function updateStoreInstance( // Something may have been mutated in between render and commit. This could // have been in an event that fired before the passive effects, or it could // have been in a layout effect. In that case, we would have used the old - // snapsho and getSnapshot values to bail out. We need to check one more time. + // snapshot and getSnapshot values to bail out. We need to check one more + // time. This effect also re-runs when a hidden Activity tree is revealed. if (checkIfSnapshotChanged(inst)) { // Force a re-render. // We intentionally don't log update times and stacks here because this diff --git a/packages/react-reconciler/src/__tests__/useSyncExternalStore-test.js b/packages/react-reconciler/src/__tests__/useSyncExternalStore-test.js index eb5bc6ed98cd..0a67abe9fe21 100644 --- a/packages/react-reconciler/src/__tests__/useSyncExternalStore-test.js +++ b/packages/react-reconciler/src/__tests__/useSyncExternalStore-test.js @@ -352,6 +352,132 @@ describe('useSyncExternalStore', () => { }, ); + // Regression test for https://github.com/facebook/react/issues/27670 + it('detects store mutations from a layout effect while an Activity subtree is being revealed', async () => { + const store = createExternalStore('revision:1'); + + function App({mode, revision}) { + return ( + + + + + + ); + } + + function Wrapper({children, revision}) { + useLayoutEffect(() => { + store.set('revision:' + revision); + }, [revision]); + + return ( + <> + wrapper:{revision} + {', '} + {children} + + ); + } + + function Subscriber() { + const revision = useSyncExternalStore(store.subscribe, store.getState); + return ; + } + + const root = ReactNoop.createRoot(); + + // Mount the app + await act(() => { + root.render(); + }); + assertLog(['revision:1']); + expect(root).toMatchRenderedOutput('wrapper:1, revision:1'); + expect(store.getSubscriberCount()).toBe(1); + + // Hide the subtree. React unsubscribes from the store. + await act(() => { + root.render(); + }); + assertLog(['revision:1']); + expect(store.getSubscriberCount()).toBe(0); + + // Show the subtree again. A layout effect mutates the store during the + // reveal, after the Subscriber rendered but before it resubscribed. When + // it resubscribes, it must detect the mutation it missed. + await act(() => { + root.render(); + }); + assertLog(['revision:1', 'revision:2']); + expect(store.getSubscriberCount()).toBe(1); + expect(root).toMatchRenderedOutput('wrapper:2, revision:2'); + }); + + // Regression test for https://github.com/facebook/react/issues/27670 + it( + 'detects store mutations that happened while an Activity subtree was ' + + 'hidden, even if the subtree bails out of rendering when revealed', + async () => { + const store = createExternalStore('initial'); + + // Memoized so that revealing the Activity boundary doesn't re-render + // the subscriber. This matches components memoized by React.memo or + // React Compiler. + const Subscriber = React.memo(({label}) => { + const value = useSyncExternalStore(store.subscribe, store.getState); + return ; + }); + + function App({mode, label}) { + return ( + + + + ); + } + + const root = ReactNoop.createRoot(); + await act(() => { + root.render(); + }); + assertLog(['a:initial']); + expect(root).toMatchRenderedOutput('a:initial'); + expect(store.getSubscriberCount()).toBe(1); + + // Re-render the subscriber once with different props, with no store + // change. This replaces its effect list with one that contains only + // the subscription effect, no interleaved mutation check. + await act(() => { + root.render(); + }); + assertLog(['b:initial']); + expect(root).toMatchRenderedOutput('b:initial'); + + // Hide the subtree. React unsubscribes from the store. + await act(() => { + root.render(); + }); + expect(store.getSubscriberCount()).toBe(0); + + // Mutate the store while the subtree is hidden. Nothing is subscribed, + // so no update is scheduled. + await act(() => { + store.set('updated'); + }); + assertLog([]); + + // Show the subtree again. The memoized component bails out of + // rendering, so resubscribing to the store is the only chance to + // detect the mutation that happened while it was hidden. + await act(() => { + root.render(); + }); + assertLog(['b:updated']); + expect(store.getSubscriberCount()).toBe(1); + expect(root).toMatchRenderedOutput('b:updated'); + }, + ); + it('regression: does not infinite loop for only changing store reference in render', async () => { let store = {value: {}}; let listeners = [];