From 6db7f4209e6f32ebde298a0b7451710dd6aa3e19 Mon Sep 17 00:00:00 2001 From: Andrew Clark Date: Sat, 21 Oct 2023 09:11:25 -0700 Subject: [PATCH] Bugfix: useDeferredValue loop during popstate transition (#27559) During a popstate event, we attempt to render updates synchronously even if they are transitions, to preserve scroll position if possible. We do this by entangling the transition lane with the Sync lane. However, if rendering the transition spawns additional transition updates (e.g. a setState inside useEffect), there's no reason to render those synchronously, too. We should use the normal transition behavior. This fixes an issue where useDeferredValue during a popstate event would spawn a transition update that was itself also synchronous. --- .../src/client/ReactFiberConfigDOM.js | 23 ++++++- .../src/__tests__/ReactDOMFiberAsync-test.js | 63 +++++++++++++++++++ 2 files changed, 85 insertions(+), 1 deletion(-) diff --git a/packages/react-dom-bindings/src/client/ReactFiberConfigDOM.js b/packages/react-dom-bindings/src/client/ReactFiberConfigDOM.js index 0e42d00847995..519484f27222e 100644 --- a/packages/react-dom-bindings/src/client/ReactFiberConfigDOM.js +++ b/packages/react-dom-bindings/src/client/ReactFiberConfigDOM.js @@ -583,8 +583,29 @@ export function getCurrentEventPriority(): EventPriority { return getEventPriority(currentEvent.type); } +let currentPopstateTransitionEvent: Event | null = null; export function shouldAttemptEagerTransition(): boolean { - return window.event && window.event.type === 'popstate'; + const event = window.event; + if (event && event.type === 'popstate') { + // This is a popstate event. Attempt to render any transition during this + // event synchronously. Unless we already attempted during this event. + if (event === currentPopstateTransitionEvent) { + // We already attempted to render this popstate transition synchronously. + // Any subsequent attempts must have happened as the result of a derived + // update, like startTransition inside useEffect, or useDV. Switch back to + // the default behavior for all remaining transitions during the current + // popstate event. + return false; + } else { + // Cache the current event in case a derived transition is scheduled. + // (Refer to previous branch.) + currentPopstateTransitionEvent = event; + return true; + } + } + // We're not inside a popstate event. + currentPopstateTransitionEvent = null; + return false; } export const isPrimaryRenderer = true; diff --git a/packages/react-dom/src/__tests__/ReactDOMFiberAsync-test.js b/packages/react-dom/src/__tests__/ReactDOMFiberAsync-test.js index b8b8847a9dff5..87c9cfe627dc6 100644 --- a/packages/react-dom/src/__tests__/ReactDOMFiberAsync-test.js +++ b/packages/react-dom/src/__tests__/ReactDOMFiberAsync-test.js @@ -746,5 +746,68 @@ describe('ReactDOMFiberAsync', () => { }); assertLog([]); expect(div.textContent).toBe('/path/b'); + await act(() => { + root.unmount(); + }); + }); + + it('regression: infinite deferral loop caused by unstable useDeferredValue input', async () => { + function Text({text}) { + Scheduler.log(text); + return text; + } + + let i = 0; + function App() { + const [pathname, setPathname] = React.useState('/path/a'); + // This is an unstable input, so it will always cause a deferred render. + const {value: deferredPathname} = React.useDeferredValue({ + value: pathname, + }); + if (i++ > 100) { + throw new Error('Infinite loop detected'); + } + React.useEffect(() => { + function onPopstate() { + React.startTransition(() => { + setPathname('/path/b'); + }); + } + window.addEventListener('popstate', onPopstate); + return () => window.removeEventListener('popstate', onPopstate); + }, []); + + return ; + } + + const root = ReactDOMClient.createRoot(container); + await act(() => { + root.render(); + }); + assertLog(['/path/a']); + expect(container.textContent).toBe('/path/a'); + + // Simulate a popstate event + await act(async () => { + const popStateEvent = new Event('popstate'); + + // Simulate a popstate event + window.event = popStateEvent; + window.dispatchEvent(popStateEvent); + await waitForMicrotasks(); + window.event = undefined; + + // The transition lane is attempted synchronously (in a microtask). + // Because the input to useDeferredValue is referentially unstable, it + // will spawn a deferred task at transition priority. However, even + // though it was spawned during a transition event, the spawned task + // not also be upgraded to sync. + assertLog(['/path/a']); + }); + assertLog(['/path/b']); + expect(container.textContent).toBe('/path/b'); + await act(() => { + root.unmount(); + }); }); });