Skip to content

Commit

Permalink
Bugfix: useDeferredValue loop during popstate transition (#27559)
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
acdlite authored Oct 21, 2023
1 parent 90172d1 commit 6db7f42
Show file tree
Hide file tree
Showing 2 changed files with 85 additions and 1 deletion.
23 changes: 22 additions & 1 deletion packages/react-dom-bindings/src/client/ReactFiberConfigDOM.js
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
63 changes: 63 additions & 0 deletions packages/react-dom/src/__tests__/ReactDOMFiberAsync-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -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 <Text text={deferredPathname} />;
}

const root = ReactDOMClient.createRoot(container);
await act(() => {
root.render(<App />);
});
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();
});
});
});

0 comments on commit 6db7f42

Please sign in to comment.