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

Fix: useDeferredValue initialValue suspends forever without switching to final (#27888)

Fixes a bug in the experimental `initialValue` option for `useDeferredValue` (added in #27500). If rendering the `initialValue` causes the tree to suspend, React should skip it and switch to rendering the final value instead. It should not wait for `initialValue` to resolve. This is not just an optimization, because in some cases the initial value may _never_ resolve — intentionally. For example, if the application does not provide an instant fallback state. This capability is, in fact, the primary motivation for the `initialValue` API. I mostly implemented this correctly in the original PR, but I missed some cases where it wasn't working: - If there's no Suspense boundary between the `useDeferredValue` hook and the component that suspends, and we're not in the shell of the transition (i.e. there's a parent Suspense boundary wrapping the `useDeferredValue` hook), the deferred task would get incorrectly dropped. - Similarly, if there's no Suspense boundary between the `useDeferredValue` hook and the component that suspends, and we're rendering a synchronous update, the deferred task would get incorrectly dropped. What these cases have in common is that it causes the `useDeferredValue` hook itself to be replaced by a Suspense fallback. The fix was the same for both. (It already worked in cases where there's no Suspense fallback at all, because those are handled differently, at the root.) The way I discovered this was when investigating a particular bug in Next.js that would happen during a 'popstate' transition (back/forward), but not during a regular navigation. That's because we render popstate transitions synchronously to preserve browser's scroll position — which in this case triggered the second scenario above.

Andrew Clark committed Jan 7, 2024 at 23:17 UTC f1039be4a48384e7e4b0a87d4d92c48e900053b9

5 files changed +172 -5

packages/react-dom/src/__tests__/ReactDOMFizzDeferredValue-test.js

+32

@@ -85,6 +85,38 @@ describe('ReactDOMFizzForm', () => {

85 expect(container.textContent).toEqual('Final');

86 });

88 + // @gate enableUseDeferredValueInitialArg

89 + // @gate enablePostpone

90 + it(

91 + 'if initial value postpones during hydration, it will switch to the ' +

92 + 'final value instead',

93 + async () => {

94 + function Content() {

95 + const isInitial = useDeferredValue(false, true);

96 + if (isInitial) {

97 + React.unstable_postpone();

98 + }

99 + return <Text text="Final" />;

100 + }

101 +

102 + function App() {

103 + return (

104 + <Suspense fallback={<Text text="Loading..." />}>

105 + <Content />

106 + </Suspense>

107 + );

108 + }

109 +

110 + const stream = await ReactDOMServer.renderToReadableStream(<App />);

111 + await readIntoContainer(stream);

112 + expect(container.textContent).toEqual('Loading...');

113 +

114 + // After hydration, it's updated to the final value

115 + await act(() => ReactDOMClient.hydrateRoot(container, <App />));

116 + expect(container.textContent).toEqual('Final');

117 + },

118 + );

119 +

120 // @gate enableUseDeferredValueInitialArg

121 it(

122 'useDeferredValue during hydration has higher priority than remaining ' +

packages/react-reconciler/src/ReactFiberBeginWork.js

+41 -3

@@ -90,6 +90,7 @@ import {

90 ShouldCapture,

91 ForceClientRender,

92 Passive,

93 + DidDefer,

94 } from './ReactFiberFlags';

95 import ReactSharedInternals from 'shared/ReactSharedInternals';

96 import {

@@ -257,6 +258,7 @@ import {

258 renderDidSuspendDelayIfPossible,

259 markSkippedUpdateLanes,

260 getWorkInProgressRoot,

261 + peekDeferredLane,

262 } from './ReactFiberWorkLoop';

263 import {enqueueConcurrentRenderForLane} from './ReactFiberConcurrentUpdates';

264 import {pushCacheProvider, CacheContext} from './ReactFiberCacheComponent';

@@ -2228,9 +2230,22 @@ function shouldRemainOnFallback(

2230 );

2231 }

2232

2231 -function getRemainingWorkInPrimaryTree(current: Fiber, renderLanes: Lanes) {

2232 - // TODO: Should not remove render lanes that were pinged during this render

2233 - return removeLanes(current.childLanes, renderLanes);

2233 +function getRemainingWorkInPrimaryTree(

2234 + current: Fiber | null,

2235 + primaryTreeDidDefer: boolean,

2236 + renderLanes: Lanes,

2237 +) {

2238 + let remainingLanes =

2239 + current !== null ? removeLanes(current.childLanes, renderLanes) : NoLanes;

2240 + if (primaryTreeDidDefer) {

2241 + // A useDeferredValue hook spawned a deferred task inside the primary tree.

2242 + // Ensure that we retry this component at the deferred priority.

2243 + // TODO: We could make this a per-subtree value instead of a global one.

2244 + // Would need to track it on the context stack somehow, similar to what

2245 + // we'd have to do for resumable contexts.

2246 + remainingLanes = mergeLanes(remainingLanes, peekDeferredLane());

2247 + }

2248 + return remainingLanes;

2249 }

2250

2251 function updateSuspenseComponent(

@@ -2259,6 +2274,11 @@ function updateSuspenseComponent(

2274 workInProgress.flags &= ~DidCapture;

2275 }

2276

2277 + // Check if the primary children spawned a deferred task (useDeferredValue)

2278 + // during the first pass.

2279 + const didPrimaryChildrenDefer = (workInProgress.flags & DidDefer) !== NoFlags;

2280 + workInProgress.flags &= ~DidDefer;

2281 +

2282 // OK, the next part is confusing. We're about to reconcile the Suspense

2283 // boundary's children. This involves some custom reconciliation logic. Two

2284 // main reasons this is so complicated.

@@ -2329,6 +2349,11 @@ function updateSuspenseComponent(

2349 const primaryChildFragment: Fiber = (workInProgress.child: any);

2350 primaryChildFragment.memoizedState =

2351 mountSuspenseOffscreenState(renderLanes);

2352 + primaryChildFragment.childLanes = getRemainingWorkInPrimaryTree(

2353 + current,

2354 + didPrimaryChildrenDefer,

2355 + renderLanes,

2356 + );

2357 workInProgress.memoizedState = SUSPENDED_MARKER;

2358 if (enableTransitionTracing) {

2359 const currentTransitions = getPendingTransitions();

@@ -2368,6 +2393,11 @@ function updateSuspenseComponent(

2393 const primaryChildFragment: Fiber = (workInProgress.child: any);

2394 primaryChildFragment.memoizedState =

2395 mountSuspenseOffscreenState(renderLanes);

2396 + primaryChildFragment.childLanes = getRemainingWorkInPrimaryTree(

2397 + current,

2398 + didPrimaryChildrenDefer,

2399 + renderLanes,

2400 + );

2401 workInProgress.memoizedState = SUSPENDED_MARKER;

2402

2403 // TODO: Transition Tracing is not yet implemented for CPU Suspense.

@@ -2402,6 +2432,7 @@ function updateSuspenseComponent(

2432 current,

2433 workInProgress,

2434 didSuspend,

2435 + didPrimaryChildrenDefer,

2436 nextProps,

2437 dehydrated,

2438 prevState,

@@ -2464,6 +2495,7 @@ function updateSuspenseComponent(

2495 }

2496 primaryChildFragment.childLanes = getRemainingWorkInPrimaryTree(

2497 current,

2498 + didPrimaryChildrenDefer,

2499 renderLanes,

2500 );

2501 workInProgress.memoizedState = SUSPENDED_MARKER;

@@ -2834,6 +2866,7 @@ function updateDehydratedSuspenseComponent(

2866 current: Fiber,

2867 workInProgress: Fiber,

2868 didSuspend: boolean,

2869 + didPrimaryChildrenDefer: boolean,

2870 nextProps: any,

2871 suspenseInstance: SuspenseInstance,

2872 suspenseState: SuspenseState,

@@ -3063,6 +3096,11 @@ function updateDehydratedSuspenseComponent(

3096 const primaryChildFragment: Fiber = (workInProgress.child: any);

3097 primaryChildFragment.memoizedState =

3098 mountSuspenseOffscreenState(renderLanes);

3099 + primaryChildFragment.childLanes = getRemainingWorkInPrimaryTree(

3100 + current,

3101 + didPrimaryChildrenDefer,

3102 + renderLanes,

3103 + );

3104 workInProgress.memoizedState = SUSPENDED_MARKER;

3105 return fallbackChildFragment;

3106 }

packages/react-reconciler/src/ReactFiberFlags.js

@@ -41,6 +41,7 @@ export const StoreConsistency = /* */ 0b0000000000000100000000000000

41 // possible, because we're about to run out of bits.

42 export const ScheduleRetry = StoreConsistency;

43 export const ShouldSuspendCommit = Visibility;

44 +export const DidDefer = ContentReset;

46 export const LifecycleEffectMask =

packages/react-reconciler/src/ReactFiberWorkLoop.js

+16 -1

@@ -127,6 +127,7 @@ import {

127 Visibility,

128 MountPassiveDev,

129 MountLayoutDev,

130 + DidDefer,

131 } from './ReactFiberFlags';

132 import {

133 NoLanes,

@@ -714,6 +715,20 @@ export function requestDeferredLane(): Lane {

715 workInProgressDeferredLane = requestTransitionLane();

716 }

717 }

718 +

719 + // Mark the parent Suspense boundary so it knows to spawn the deferred lane.

720 + const suspenseHandler = getSuspenseHandler();

721 + if (suspenseHandler !== null) {

722 + // TODO: As an optimization, we shouldn't entangle the lanes at the root; we

723 + // can entangle them using the baseLanes of the Suspense boundary instead.

724 + // We only need to do something special if there's no Suspense boundary.

725 + suspenseHandler.flags |= DidDefer;

726 + }

727 +

728 + return workInProgressDeferredLane;

729 +}

730 +

731 +export function peekDeferredLane(): Lane {

732 return workInProgressDeferredLane;

733 }

734

@@ -1361,7 +1376,7 @@ export function performSyncWorkOnRoot(root: FiberRoot, lanes: Lanes): null {

1376 // The render unwound without completing the tree. This happens in special

1377 // cases where need to exit the current render without producing a

1378 // consistent tree or committing.

1364 - markRootSuspended(root, lanes, NoLane);

1379 + markRootSuspended(root, lanes, workInProgressDeferredLane);

1380 ensureRootIsScheduled(root);

1381 return null;

1382 }

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

+82 -1

@@ -409,7 +409,7 @@ describe('ReactDeferredValue', () => {

409 // @gate enableUseDeferredValueInitialArg

410 it(

411 'if a suspended render spawns a deferred task, we can switch to the ' +

412 - 'deferred task without finishing the original one',

412 + 'deferred task without finishing the original one (no Suspense boundary)',

413 async () => {

414 function App() {

415 const text = useDeferredValue('Final', 'Loading...');

@@ -439,6 +439,87 @@ describe('ReactDeferredValue', () => {

439 },

440 );

441

442 + // @gate enableUseDeferredValueInitialArg

443 + it(

444 + 'if a suspended render spawns a deferred task, we can switch to the ' +

445 + 'deferred task without finishing the original one (no Suspense boundary, ' +

446 + 'synchronous parent update)',

447 + async () => {

448 + function App() {

449 + const text = useDeferredValue('Final', 'Loading...');

450 + return <AsyncText text={text} />;

451 + }

452 +

453 + const root = ReactNoop.createRoot();

454 + // TODO: This made me realize that we don't warn if an update spawns a

455 + // deferred task without being wrapped with `act`. Usually it would work

456 + // anyway because the parent task has to wrapped with `act`... but not

457 + // if it was flushed with `flushSync` instead.

458 + await act(() => {

459 + ReactNoop.flushSync(() => root.render(<App />));

460 + });

461 + assertLog([

462 + 'Suspend! [Loading...]',

463 + // The initial value suspended, so we attempt the final value, which

464 + // also suspends.

465 + 'Suspend! [Final]',

466 + ]);

467 + expect(root).toMatchRenderedOutput(null);

468 +

469 + // The final value loads, so we can skip the initial value entirely.

470 + await act(() => resolveText('Final'));

471 + assertLog(['Final']);

472 + expect(root).toMatchRenderedOutput('Final');

473 +

474 + // When the initial value finally loads, nothing happens because we no

475 + // longer need it.

476 + await act(() => resolveText('Loading...'));

477 + assertLog([]);

478 + expect(root).toMatchRenderedOutput('Final');

479 + },

480 + );

481 +

482 + // @gate enableUseDeferredValueInitialArg

483 + it(

484 + 'if a suspended render spawns a deferred task, we can switch to the ' +

485 + 'deferred task without finishing the original one (Suspense boundary)',

486 + async () => {

487 + function App() {

488 + const text = useDeferredValue('Final', 'Loading...');

489 + return <AsyncText text={text} />;

490 + }

491 +

492 + const root = ReactNoop.createRoot();

493 + await act(() =>

494 + root.render(

495 + <Suspense fallback={<Text text="Fallback" />}>

496 + <App />

497 + </Suspense>,

498 + ),

499 + );

500 + assertLog([

501 + 'Suspend! [Loading...]',

502 + 'Fallback',

503 +

504 + // The initial value suspended, so we attempt the final value, which

505 + // also suspends.

506 + 'Suspend! [Final]',

507 + ]);

508 + expect(root).toMatchRenderedOutput('Fallback');

509 +

510 + // The final value loads, so we can skip the initial value entirely.

511 + await act(() => resolveText('Final'));

512 + assertLog(['Final']);

513 + expect(root).toMatchRenderedOutput('Final');

514 +

515 + // When the initial value finally loads, nothing happens because we no

516 + // longer need it.

517 + await act(() => resolveText('Loading...'));

518 + assertLog([]);

519 + expect(root).toMatchRenderedOutput('Final');

520 + },

521 + );

522 +

523 // @gate enableUseDeferredValueInitialArg

524 it(

525 'if a suspended render spawns a deferred task that also suspends, we can ' +