Revert yieldy behavior for non-use Suspense (#25537)

To derisk the rollout of `use`, and simplify the implementation, this reverts the yield-to-microtasks behavior for promises that are thrown directly (as opposed to being unwrapped by `use`). We may add this back later. However, the plan is to deprecate throwing a promise directly and migrate all existing Suspense code to `use`, so the extra code probably isn't worth it.
facebook · Oct 22, 2022 · dd5c208 · dd5c208
1 parent 9341775
commit dd5c208
Show file tree

Hide file tree

Showing 16 changed files with 75 additions and 175 deletions.
diff --git a/packages/react-reconciler/src/ReactFiberHooks.new.js b/packages/react-reconciler/src/ReactFiberHooks.new.js
@@ -136,7 +136,7 @@ import {now} from './Scheduler';
 import {
   trackUsedThenable,
   getPreviouslyUsedThenableAtIndex,
-} from './ReactFiberWakeable.new';
+} from './ReactFiberThenable.new';
 
 const {ReactCurrentDispatcher, ReactCurrentBatchConfig} = ReactSharedInternals;
 
@@ -783,6 +783,7 @@ function use<T>(usable: Usable<T>): T {
       const index = thenableIndexCounter;
       thenableIndexCounter += 1;
 
+      // TODO: Unify this switch statement with the one in trackUsedThenable.
       switch (thenable.status) {
         case 'fulfilled': {
           const fulfilledValue: T = thenable.value;

diff --git a/packages/react-reconciler/src/ReactFiberHooks.old.js b/packages/react-reconciler/src/ReactFiberHooks.old.js
@@ -136,7 +136,7 @@ import {now} from './Scheduler';
 import {
   trackUsedThenable,
   getPreviouslyUsedThenableAtIndex,
-} from './ReactFiberWakeable.old';
+} from './ReactFiberThenable.old';
 
 const {ReactCurrentDispatcher, ReactCurrentBatchConfig} = ReactSharedInternals;
 
@@ -783,6 +783,7 @@ function use<T>(usable: Usable<T>): T {
       const index = thenableIndexCounter;
       thenableIndexCounter += 1;
 
+      // TODO: Unify this switch statement with the one in trackUsedThenable.
       switch (thenable.status) {
         case 'fulfilled': {
           const fulfilledValue: T = thenable.value;

diff --git a/...-reconciler/src/ReactFiberWakeable.new.js → ...-reconciler/src/ReactFiberThenable.new.js b/...-reconciler/src/ReactFiberWakeable.new.js → ...-reconciler/src/ReactFiberThenable.new.js
@@ -8,7 +8,6 @@
  */
 
 import type {
-  Wakeable,
   Thenable,
   PendingThenable,
   FulfilledThenable,
@@ -18,14 +17,8 @@ import type {
 import ReactSharedInternals from 'shared/ReactSharedInternals';
 const {ReactCurrentActQueue} = ReactSharedInternals;
 
-let suspendedThenable: Thenable<mixed> | null = null;
-let adHocSuspendCount: number = 0;
-
-// TODO: Sparse arrays are bad for performance.
+let suspendedThenable: Thenable<any> | null = null;
 let usedThenables: Array<Thenable<any> | void> | null = null;
-let lastUsedThenable: Thenable<any> | null = null;
-
-const MAX_AD_HOC_SUSPEND_COUNT = 50;
 
 export function isTrackingSuspendedThenable(): boolean {
   return suspendedThenable !== null;
@@ -39,22 +32,17 @@ export function suspendedThenableDidResolve(): boolean {
   return false;
 }
 
-export function trackSuspendedWakeable(wakeable: Wakeable) {
-  // If this wakeable isn't already a thenable, turn it into one now. Then,
-  // when we resume the work loop, we can check if its status is
-  // still pending.
-  // TODO: Get rid of the Wakeable type? It's superseded by UntrackedThenable.
-  const thenable: Thenable<mixed> = (wakeable: any);
+export function trackUsedThenable<T>(thenable: Thenable<T>, index: number) {
+  if (__DEV__ && ReactCurrentActQueue.current !== null) {
+    ReactCurrentActQueue.didUsePromise = true;
+  }
 
-  if (thenable !== lastUsedThenable) {
-    // If this wakeable was not just `use`-d, it must be an ad hoc wakeable
-    // that was thrown by an older Suspense implementation. Keep a count of
-    // these so that we can detect an infinite ping loop.
-    // TODO: Once `use` throws an opaque signal instead of the actual thenable,
-    // a better way to count ad hoc suspends is whether an actual thenable
-    // is caught by the work loop.
-    adHocSuspendCount++;
+  if (usedThenables === null) {
+    usedThenables = [thenable];
+  } else {
+    usedThenables[index] = thenable;
   }
+
   suspendedThenable = thenable;
 
   // We use an expando to track the status and result of a thenable so that we
@@ -105,34 +93,12 @@ export function trackSuspendedWakeable(wakeable: Wakeable) {
 
 export function resetWakeableStateAfterEachAttempt() {
   suspendedThenable = null;
-  adHocSuspendCount = 0;
-  lastUsedThenable = null;
 }
 
 export function resetThenableStateOnCompletion() {
   usedThenables = null;
 }
 
-export function throwIfInfinitePingLoopDetected() {
-  if (adHocSuspendCount > MAX_AD_HOC_SUSPEND_COUNT) {
-    // TODO: Guard against an infinite loop by throwing an error if the same
-    // component suspends too many times in a row. This should be thrown from
-    // the render phase so that it gets the component stack.
-  }
-}
-
-export function trackUsedThenable<T>(thenable: Thenable<T>, index: number) {
-  if (usedThenables === null) {
-    usedThenables = [];
-  }
-  usedThenables[index] = thenable;
-  lastUsedThenable = thenable;
-
-  if (__DEV__ && ReactCurrentActQueue.current !== null) {
-    ReactCurrentActQueue.didUsePromise = true;
-  }
-}
-
 export function getPreviouslyUsedThenableAtIndex<T>(
   index: number,
 ): Thenable<T> | null {

diff --git a/...-reconciler/src/ReactFiberWakeable.old.js → ...-reconciler/src/ReactFiberThenable.old.js b/...-reconciler/src/ReactFiberWakeable.old.js → ...-reconciler/src/ReactFiberThenable.old.js
@@ -8,7 +8,6 @@
  */
 
 import type {
-  Wakeable,
   Thenable,
   PendingThenable,
   FulfilledThenable,
@@ -18,14 +17,8 @@ import type {
 import ReactSharedInternals from 'shared/ReactSharedInternals';
 const {ReactCurrentActQueue} = ReactSharedInternals;
 
-let suspendedThenable: Thenable<mixed> | null = null;
-let adHocSuspendCount: number = 0;
-
-// TODO: Sparse arrays are bad for performance.
+let suspendedThenable: Thenable<any> | null = null;
 let usedThenables: Array<Thenable<any> | void> | null = null;
-let lastUsedThenable: Thenable<any> | null = null;
-
-const MAX_AD_HOC_SUSPEND_COUNT = 50;
 
 export function isTrackingSuspendedThenable(): boolean {
   return suspendedThenable !== null;
@@ -39,22 +32,17 @@ export function suspendedThenableDidResolve(): boolean {
   return false;
 }
 
-export function trackSuspendedWakeable(wakeable: Wakeable) {
-  // If this wakeable isn't already a thenable, turn it into one now. Then,
-  // when we resume the work loop, we can check if its status is
-  // still pending.
-  // TODO: Get rid of the Wakeable type? It's superseded by UntrackedThenable.
-  const thenable: Thenable<mixed> = (wakeable: any);
+export function trackUsedThenable<T>(thenable: Thenable<T>, index: number) {
+  if (__DEV__ && ReactCurrentActQueue.current !== null) {
+    ReactCurrentActQueue.didUsePromise = true;
+  }
 
-  if (thenable !== lastUsedThenable) {
-    // If this wakeable was not just `use`-d, it must be an ad hoc wakeable
-    // that was thrown by an older Suspense implementation. Keep a count of
-    // these so that we can detect an infinite ping loop.
-    // TODO: Once `use` throws an opaque signal instead of the actual thenable,
-    // a better way to count ad hoc suspends is whether an actual thenable
-    // is caught by the work loop.
-    adHocSuspendCount++;
+  if (usedThenables === null) {
+    usedThenables = [thenable];
+  } else {
+    usedThenables[index] = thenable;
   }
+
   suspendedThenable = thenable;
 
   // We use an expando to track the status and result of a thenable so that we
@@ -105,34 +93,12 @@ export function trackSuspendedWakeable(wakeable: Wakeable) {
 
 export function resetWakeableStateAfterEachAttempt() {
   suspendedThenable = null;
-  adHocSuspendCount = 0;
-  lastUsedThenable = null;
 }
 
 export function resetThenableStateOnCompletion() {
   usedThenables = null;
 }
 
-export function throwIfInfinitePingLoopDetected() {
-  if (adHocSuspendCount > MAX_AD_HOC_SUSPEND_COUNT) {
-    // TODO: Guard against an infinite loop by throwing an error if the same
-    // component suspends too many times in a row. This should be thrown from
-    // the render phase so that it gets the component stack.
-  }
-}
-
-export function trackUsedThenable<T>(thenable: Thenable<T>, index: number) {
-  if (usedThenables === null) {
-    usedThenables = [];
-  }
-  usedThenables[index] = thenable;
-  lastUsedThenable = thenable;
-
-  if (__DEV__ && ReactCurrentActQueue.current !== null) {
-    ReactCurrentActQueue.didUsePromise = true;
-  }
-}
-
 export function getPreviouslyUsedThenableAtIndex<T>(
   index: number,
 ): Thenable<T> | null {

diff --git a/packages/react-reconciler/src/ReactFiberWorkLoop.new.js b/packages/react-reconciler/src/ReactFiberWorkLoop.new.js
@@ -267,10 +267,9 @@ import {processTransitionCallbacks} from './ReactFiberTracingMarkerComponent.new
 import {
   resetWakeableStateAfterEachAttempt,
   resetThenableStateOnCompletion,
-  trackSuspendedWakeable,
   suspendedThenableDidResolve,
   isTrackingSuspendedThenable,
-} from './ReactFiberWakeable.new';
+} from './ReactFiberThenable.new';
 import {schedulePostPaintCallback} from './ReactPostPaintCallback';
 
 const ceil = Math.ceil;
@@ -1739,11 +1738,6 @@ function handleThrow(root, thrownValue): void {
     return;
   }
 
-  const isWakeable =
-    thrownValue !== null &&
-    typeof thrownValue === 'object' &&
-    typeof thrownValue.then === 'function';
-
   if (enableProfilerTimer && erroredWork.mode & ProfileMode) {
     // Record the time spent rendering before an error was thrown. This
     // avoids inaccurate Profiler durations in the case of a
@@ -1753,7 +1747,11 @@ function handleThrow(root, thrownValue): void {
 
   if (enableSchedulingProfiler) {
     markComponentRenderStopped();
-    if (isWakeable) {
+    if (
+      thrownValue !== null &&
+      typeof thrownValue === 'object' &&
+      typeof thrownValue.then === 'function'
+    ) {
       const wakeable: Wakeable = (thrownValue: any);
       markComponentSuspended(
         erroredWork,
@@ -1768,12 +1766,6 @@ function handleThrow(root, thrownValue): void {
       );
     }
   }
-
-  if (isWakeable) {
-    const wakeable: Wakeable = (thrownValue: any);
-
-    trackSuspendedWakeable(wakeable);
-  }
 }
 
 function pushDispatcher(container) {

diff --git a/packages/react-reconciler/src/ReactFiberWorkLoop.old.js b/packages/react-reconciler/src/ReactFiberWorkLoop.old.js
@@ -267,10 +267,9 @@ import {processTransitionCallbacks} from './ReactFiberTracingMarkerComponent.old
 import {
   resetWakeableStateAfterEachAttempt,
   resetThenableStateOnCompletion,
-  trackSuspendedWakeable,
   suspendedThenableDidResolve,
   isTrackingSuspendedThenable,
-} from './ReactFiberWakeable.old';
+} from './ReactFiberThenable.old';
 import {schedulePostPaintCallback} from './ReactPostPaintCallback';
 
 const ceil = Math.ceil;
@@ -1739,11 +1738,6 @@ function handleThrow(root, thrownValue): void {
     return;
   }
 
-  const isWakeable =
-    thrownValue !== null &&
-    typeof thrownValue === 'object' &&
-    typeof thrownValue.then === 'function';
-
   if (enableProfilerTimer && erroredWork.mode & ProfileMode) {
     // Record the time spent rendering before an error was thrown. This
     // avoids inaccurate Profiler durations in the case of a
@@ -1753,7 +1747,11 @@ function handleThrow(root, thrownValue): void {
 
   if (enableSchedulingProfiler) {
     markComponentRenderStopped();
-    if (isWakeable) {
+    if (
+      thrownValue !== null &&
+      typeof thrownValue === 'object' &&
+      typeof thrownValue.then === 'function'
+    ) {
       const wakeable: Wakeable = (thrownValue: any);
       markComponentSuspended(
         erroredWork,
@@ -1768,12 +1766,6 @@ function handleThrow(root, thrownValue): void {
       );
     }
   }
-
-  if (isWakeable) {
-    const wakeable: Wakeable = (thrownValue: any);
-
-    trackSuspendedWakeable(wakeable);
-  }
 }
 
 function pushDispatcher(container) {

diff --git a/packages/react-reconciler/src/__tests__/ReactOffscreenSuspense-test.js b/packages/react-reconciler/src/__tests__/ReactOffscreenSuspense-test.js
@@ -485,32 +485,22 @@ describe('ReactOffscreen', () => {
       // In the same render, also hide the offscreen tree.
       root.render(<App show={false} />);
 
-      if (gate(flags => flags.enableSyncDefaultUpdates)) {
-        expect(Scheduler).toFlushUntilNextPaint([
-          // The outer update will commit, but the inner update is deferred until
-          // a later render.
-          'Outer: 1',
-
-          // Something suspended. This means we won't commit immediately; there
-          // will be an async gap between render and commit. In this test, we will
-          // use this property to schedule a concurrent update. The fact that
-          // we're using Suspense to schedule a concurrent update is not directly
-          // relevant to the test — we could also use time slicing, but I've
-          // chosen to use Suspense the because implementation details of time
-          // slicing are more volatile.
-          'Suspend! [Async: 1]',
-
-          'Loading...',
-        ]);
-      } else {
-        // When default updates are time sliced, React yields before preparing
-        // the fallback.
-        expect(Scheduler).toFlushUntilNextPaint([
-          'Outer: 1',
-          'Suspend! [Async: 1]',
-        ]);
-        expect(Scheduler).toFlushUntilNextPaint(['Loading...']);
-      }
+      expect(Scheduler).toFlushUntilNextPaint([
+        // The outer update will commit, but the inner update is deferred until
+        // a later render.
+        'Outer: 1',
+
+        // Something suspended. This means we won't commit immediately; there
+        // will be an async gap between render and commit. In this test, we will
+        // use this property to schedule a concurrent update. The fact that
+        // we're using Suspense to schedule a concurrent update is not directly
+        // relevant to the test — we could also use time slicing, but I've
+        // chosen to use Suspense the because implementation details of time
+        // slicing are more volatile.
+        'Suspend! [Async: 1]',
+
+        'Loading...',
+      ]);
 
       // Assert that we haven't committed quite yet
       expect(root).toMatchRenderedOutput(

diff --git a/packages/react-reconciler/src/__tests__/ReactSuspenseWithNoopRenderer-test.js b/packages/react-reconciler/src/__tests__/ReactSuspenseWithNoopRenderer-test.js
@@ -3874,6 +3874,7 @@ describe('ReactSuspenseWithNoopRenderer', () => {
         'Suspend! [A2]',
         'Loading...',
         'Suspend! [B2]',
+        'Loading...',
       ]);
       expect(root).toMatchRenderedOutput(
         <>

diff --git a/...ciler/src/__tests__/ReactWakeable-test.js → ...ciler/src/__tests__/ReactThenable-test.js b/...ciler/src/__tests__/ReactWakeable-test.js → ...ciler/src/__tests__/ReactThenable-test.js
@@ -26,6 +26,11 @@ describe('ReactWakeable', () => {
     return props.text;
   }
 
+  // This behavior was intentionally disabled to derisk the rollout of `use`.
+  // It changes the behavior of old, pre-`use` Suspense implementations. We may
+  // add this back; however, the plan is to migrate all existing Suspense code
+  // to `use`, so the extra code probably isn't worth it.
+  // @gate TODO
   test('if suspended fiber is pinged in a microtask, retry immediately without unwinding the stack', async () => {
     let resolved = false;
     function Async() {