@samitouri / QOS-React-1 / commits / adbec0c25a

Fix: `useTransition` after `use` gets stuck in pending state (#29670)

When a component suspends with `use`, we switch to the "re-render" dispatcher during the subsequent render attempt, so that we can reuse the work from the initial attempt. However, once we run out of hooks from the previous attempt, we should switch back to the regular "update" dispatcher. This is conceptually the same fix as the one introduced in https://github.com/facebook/react/pull/26232. That fix only accounted for initial mount, but the useTransition regression test added in f82973302b3f490ec120c3b102e8c3792452dfc9 illustrates that we need to handle updates, too. The issue affects more than just useTransition but because most of the behavior between the "re-render" and "update" dispatchers is the same it's hard to contrive other scenarios in a test, which is probably why it took so long for someone to notice. Closes #28923 and #29209 --------- Co-authored-by: eps1lon <sebastian.silbermann@vercel.com>

Andrew Clark committed May 31, 2024 at 17:52 UTC adbec0c25aff07f04b0678679554505ba2813168

2 files changed +119 -12

packages/react-reconciler/src/ReactFiberHooks.js

+41 -12

@@ -1083,20 +1083,49 @@ function useThenable<T>(thenable: Thenable<T>): T {

1083 thenableState = createThenableState();

1084 }

1085 const result = trackUsedThenable(thenableState, thenable, index);

1086 - if (

1087 - currentlyRenderingFiber.alternate === null &&

1088 - (workInProgressHook === null

1089 - ? currentlyRenderingFiber.memoizedState === null

1090 - : workInProgressHook.next === null)

1091 - ) {

1092 - // Initial render, and either this is the first time the component is

1093 - // called, or there were no Hooks called after this use() the previous

1094 - // time (perhaps because it threw). Subsequent Hook calls should use the

1095 - // mount dispatcher.

1086 +

1087 + // When something suspends with `use`, we replay the component with the

1088 + // "re-render" dispatcher instead of the "mount" or "update" dispatcher.

1089 + //

1090 + // But if there are additional hooks that occur after the `use` invocation

1091 + // that suspended, they wouldn't have been processed during the previous

1092 + // attempt. So after we invoke `use` again, we may need to switch from the

1093 + // "re-render" dispatcher back to the "mount" or "update" dispatcher. That's

1094 + // what the following logic accounts for.

1095 + //

1096 + // TODO: Theoretically this logic only needs to go into the rerender

1097 + // dispatcher. Could optimize, but probably not be worth it.

1098 +

1099 + // This is the same logic as in updateWorkInProgressHook.

1100 + const workInProgressFiber = currentlyRenderingFiber;

1101 + const nextWorkInProgressHook =

1102 + workInProgressHook === null

1103 + ? // We're at the beginning of the list, so read from the first hook from

1104 + // the fiber.

1105 + workInProgressFiber.memoizedState

1106 + : workInProgressHook.next;

1107 +

1108 + if (nextWorkInProgressHook !== null) {

1109 + // There are still hooks remaining from the previous attempt.

1110 + } else {

1111 + // There are no remaining hooks from the previous attempt. We're no longer

1112 + // in "re-render" mode. Switch to the normal mount or update dispatcher.

1113 + //

1114 + // This is the same as the logic in renderWithHooks, except we don't bother

1115 + // to track the hook types debug information in this case (sufficient to

1116 + // only do that when nothing suspends).

1117 + const currentFiber = workInProgressFiber.alternate;

1118 if (__DEV__) {

1097 - ReactSharedInternals.H = HooksDispatcherOnMountInDEV;

1119 + if (currentFiber !== null && currentFiber.memoizedState !== null) {

1120 + ReactSharedInternals.H = HooksDispatcherOnUpdateInDEV;

1121 + } else {

1122 + ReactSharedInternals.H = HooksDispatcherOnMountInDEV;

1123 + }

1124 } else {

1099 - ReactSharedInternals.H = HooksDispatcherOnMount;

1125 + ReactSharedInternals.H =

1126 + currentFiber === null || currentFiber.memoizedState === null

1127 + ? HooksDispatcherOnMount

1128 + : HooksDispatcherOnUpdate;

1129 }

1130 }

1131 return result;

packages/react-reconciler/src/__tests__/ReactUse-test.js

+78

@@ -16,6 +16,7 @@ let act;

16 let use;

17 let useDebugValue;

18 let useState;

19 +let useTransition;

20 let useMemo;

21 let useEffect;

22 let Suspense;

@@ -38,6 +39,7 @@ describe('ReactUse', () => {

39 use = React.use;

40 useDebugValue = React.useDebugValue;

41 useState = React.useState;

42 + useTransition = React.useTransition;

43 useMemo = React.useMemo;

44 useEffect = React.useEffect;

45 Suspense = React.Suspense;

@@ -1915,4 +1917,80 @@ describe('ReactUse', () => {

1917 assertLog(['Hi', 'World']);

1918 expect(root).toMatchRenderedOutput(<div>Hi World</div>);

1919 });

1920 +

1921 + it(

1922 + 'regression: does not get stuck in pending state after `use` suspends ' +

1923 + '(when `use` comes before all hooks)',

1924 + async () => {

1925 + // This is a regression test. The root cause was an issue where we failed to

1926 + // switch from the "re-render" dispatcher back to the "update" dispatcher

1927 + // after a `use` suspends and triggers a replay.

1928 + let update;

1929 + function App({promise}) {

1930 + const value = use(promise);

1931 +

1932 + const [isPending, startLocalTransition] = useTransition();

1933 + update = () => {

1934 + startLocalTransition(() => {

1935 + root.render(<App promise={getAsyncText('Updated')} />);

1936 + });

1937 + };

1938 +

1939 + return <Text text={value + (isPending ? ' (pending...)' : '')} />;

1940 + }

1941 +

1942 + const root = ReactNoop.createRoot();

1943 + await act(() => {

1944 + root.render(<App promise={Promise.resolve('Initial')} />);

1945 + });

1946 + assertLog(['Initial']);

1947 + expect(root).toMatchRenderedOutput('Initial');

1948 +

1949 + await act(() => update());

1950 + assertLog(['Async text requested [Updated]', 'Initial (pending...)']);

1951 +

1952 + await act(() => resolveTextRequests('Updated'));

1953 + assertLog(['Updated']);

1954 + expect(root).toMatchRenderedOutput('Updated');

1955 + },

1956 + );

1957 +

1958 + it(

1959 + 'regression: does not get stuck in pending state after `use` suspends ' +

1960 + '(when `use` in in the middle of hook list)',

1961 + async () => {

1962 + // Same as previous test but `use` comes in between two hooks.

1963 + let update;

1964 + function App({promise}) {

1965 + // This hook is only here to test that `use` resumes correctly after

1966 + // suspended even if it comes in between other hooks.

1967 + useState(false);

1968 +

1969 + const value = use(promise);

1970 +

1971 + const [isPending, startLocalTransition] = useTransition();

1972 + update = () => {

1973 + startLocalTransition(() => {

1974 + root.render(<App promise={getAsyncText('Updated')} />);

1975 + });

1976 + };

1977 +

1978 + return <Text text={value + (isPending ? ' (pending...)' : '')} />;

1979 + }

1980 +

1981 + const root = ReactNoop.createRoot();

1982 + await act(() => {

1983 + root.render(<App promise={Promise.resolve('Initial')} />);

1984 + });

1985 + assertLog(['Initial']);

1986 + expect(root).toMatchRenderedOutput('Initial');

1987 +

1988 + await act(() => update());

1989 + assertLog(['Async text requested [Updated]', 'Initial (pending...)']);

1990 +

1991 + await act(() => resolveTextRequests('Updated'));

1992 + assertLog(['Updated']);

1993 + expect(root).toMatchRenderedOutput('Updated');

1994 + },

1995 + );

1996 });