[Fiber] Warn for Conditional Use of use() Based on Cache

packages/react-reconciler/src/ReactChildFiber.js

@@ -261,7 +261,7 @@ function unwrapThenable<T>(thenable: Thenable<T>): T {
   if (thenableState === null) {
     thenableState = createThenableState();
   }
-  return trackUsedThenable(thenableState, thenable, index);
+  return trackUsedThenable(thenableState, thenable, index, null);
 }
 
 function coerceRef(workInProgress: Fiber, element: ReactElement): void {

packages/react-reconciler/src/ReactFiberHooks.js

@@ -139,6 +139,7 @@ import {now} from './Scheduler';
 import {
   trackUsedThenable,
   checkIfUseWrappedInTryCatch,
+  checkIfUseWasUsedBefore,
   createThenableState,
   SuspenseException,
   SuspenseActionException,
@@ -646,6 +647,7 @@ function finishRenderingHooks<Props, SecondArg>(
     } else {
       workInProgress.dependencies._debugThenableState = thenableState;
     }
+    checkIfUseWasUsedBefore(workInProgress, thenableState);
   }
 
   // We can assume the previous dispatcher is always this one, since we set it
@@ -1095,7 +1097,12 @@ function useThenable<T>(thenable: Thenable<T>): T {
   if (thenableState === null) {
     thenableState = createThenableState();
   }
-  const result = trackUsedThenable(thenableState, thenable, index);
+  const result = trackUsedThenable(
+    thenableState,
+    thenable,
+    index,
+    __DEV__ ? currentlyRenderingFiber : null,
+  );
 
   // When something suspends with `use`, we replay the component with the
   // "re-render" dispatcher instead of the "mount" or "update" dispatcher.

packages/react-reconciler/src/ReactFiberThenable.js

@@ -14,12 +14,16 @@ import type {
   RejectedThenable,
 } from 'shared/ReactTypes';
 
+import type {Fiber} from './ReactInternalTypes';
+
 import {getWorkInProgressRoot} from './ReactFiberWorkLoop';
 
 import ReactSharedInternals from 'shared/ReactSharedInternals';
 
 import noop from 'shared/noop';
 
+import {HostRoot} from './ReactWorkTags';
+
 opaque type ThenableStateDev = {
   didWarnAboutUncachedPromise: boolean,
   thenables: Array<Thenable<any>>,
@@ -97,10 +101,23 @@ export function isThenableResolved(thenable: Thenable<mixed>): boolean {
   return status === 'fulfilled' || status === 'rejected';
 }
 
+// DEV-only
+let lastSuspendedFiber: null | Fiber = null;
+let lastSuspendedStack: null | Error = null;
+let didIssueUseWarning = false;
+
+export function hasPotentialUseWarnings(): boolean {
+  return lastSuspendedFiber !== null;
+}
+export function clearUseWarnings() {
+  lastSuspendedFiber = null;
+}
+
 export function trackUsedThenable<T>(
   thenableState: ThenableState,
   thenable: Thenable<T>,
   index: number,
+  fiber: null | Fiber, // DEV-only
 ): T {
   if (__DEV__ && ReactSharedInternals.actQueue !== null) {
     ReactSharedInternals.didUsePromise = true;
@@ -246,6 +263,25 @@ export function trackUsedThenable<T>(
       suspendedThenable = thenable;
       if (__DEV__) {
         needsToResetSuspendedThenableDEV = true;
+        if (
+          !didIssueUseWarning &&
+          fiber !== null &&
+          // Only track initial mount for now to avoid warning too much for updates.
+          fiber.alternate === null
+        ) {
+          lastSuspendedFiber = fiber;
+          // Stash an error in case we end up triggering the use() warning.
+          // This ensures that we have a stack trace at the location of the first use()
+          // call since there won't be a second one we have to do that eagerly.
+          lastSuspendedStack = new Error(
+            'This library called use() to suspend in a previous render but ' +
+              'did not call use() when it finished. This indicates an incorrect use of use(). ' +
+              'A common mistake is to call use() only when something is not cached.\n\n' +
+              '  if (cache.value !== undefined) use(cache.promise) else return cache.value\n\n' +
+              'The correct way is to always call use() with a Promise and resolve it with the value.\n\n' +
+              '  return use(cache.promise)',
+          );
+        }
       }
       throw SuspenseException;
     }
@@ -316,3 +352,54 @@ export function checkIfUseWrappedInAsyncCatch(rejectedReason: any) {
     );
   }
 }
+
+function areSameKeyPath(a: Fiber, b: Fiber): boolean {
+  if (a === b) {
+    return true;
+  }
+  if (
+    a.tag !== b.tag ||
+    a.type !== b.type ||
+    a.key !== b.key ||
+    a.index !== b.index
+  ) {
+    return false;
+  }
+  if (a.tag === HostRoot && a.stateNode !== b.stateNode) {
+    // These are both roots but they're different roots so they're not in the same tree.
+    return false;
+  }
+  if (a.return === null || b.return === null) {
+    return false;
+  }
+  return areSameKeyPath(a.return, b.return);
+}
+
+export function checkIfUseWasUsedBefore(
+  unsuspendedFiber: Fiber,
+  thenableState: null | ThenableState,
+): void {
+  if (__DEV__) {
+    if (
+      lastSuspendedFiber !== null &&
+      areSameKeyPath(lastSuspendedFiber, unsuspendedFiber)
+    ) {
+      if (thenableState !== null) {
+        // It's still using use() ever after resolving. We could warn for different number of them but for
+        // now we treat this as ok and clear the state.
+        lastSuspendedFiber = null;
+        lastSuspendedStack = null;
+      } else {
+        // The last suspended Fiber using use() is no longer using use() in the same position.
+        // That's suspicious. Likely it was unblocked by conditionally using use() which is incorrect.
+        if (lastSuspendedStack !== null && !didIssueUseWarning) {
+          didIssueUseWarning = true;
+          // We pass the error object instead of custom message so that the browser displays the error natively.
+          console['error'](lastSuspendedStack);
+        }
+        lastSuspendedFiber = null;
+        lastSuspendedStack = null;
+      }
+    }
+  }
+}

packages/react-reconciler/src/ReactFiberWorkLoop.js

@@ -342,6 +342,8 @@ import {
   SuspenseyCommitException,
   getSuspendedThenable,
   isThenableResolved,
+  hasPotentialUseWarnings,
+  clearUseWarnings,
 } from './ReactFiberThenable';
 import {schedulePostPaintCallback} from './ReactPostPaintCallback';
 import {
@@ -776,12 +778,24 @@ export function requestUpdateLane(fiber: Fiber): Lane {
         transition._updatedFibers = new Set();
       }
       transition._updatedFibers.add(fiber);
+      if (
+        hasPotentialUseWarnings() &&
+        resolveUpdatePriority() === DiscreteEventPriority
+      ) {
+        // If we're updating inside a discrete event, then this might be a new user interaction
+        // and not just an automatically resolved loading sequence. Don't warn unless it happens again.
+        clearUseWarnings();
+      }
     }
 
     return requestTransitionLane(transition);
   }
 
-  return eventPriorityToLane(resolveUpdatePriority());
+  const priority = resolveUpdatePriority();
+  if (__DEV__ && priority === DiscreteEventPriority) {
+    clearUseWarnings();
+  }
+  return eventPriorityToLane(priority);
 }
 
 function requestRetryLane(fiber: Fiber) {

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

@@ -41,47 +41,39 @@ describe('Activity Suspense', () => {
   function resolveText(text) {
     const record = textCache.get(text);
     if (record === undefined) {
+      const promise = Promise.resolve(text);
+      promise.status = 'fulfilled';
+      promise.value = text;
       const newRecord = {
-        status: 'resolved',
-        value: text,
+        promise,
       };
       textCache.set(text, newRecord);
-    } else if (record.status === 'pending') {
+    } else if (record.promise.status === 'pending') {
       const resolve = record.resolve;
-      record.status = 'resolved';
-      record.value = text;
-      resolve();
+      record.promise.status = 'fulfilled';
+      record.promise.value = text;
+      resolve(text);
     }
   }
 
   function readText(text) {
-    const record = textCache.get(text);
-    if (record !== undefined) {
-      switch (record.status) {
-        case 'pending':
-          Scheduler.log(`Suspend! [${text}]`);
-          return use(record.value);
-        case 'rejected':
-          throw record.value;
-        case 'resolved':
-          return record.value;
-      }
-    } else {
-      Scheduler.log(`Suspend! [${text}]`);
+    let record = textCache.get(text);
+    if (record === undefined) {
       let resolve;
       const promise = new Promise(_resolve => {
         resolve = _resolve;
       });
-
-      const newRecord = {
-        status: 'pending',
-        value: promise,
+      promise.status = 'pending';
+      record = {
+        promise,
         resolve,
       };
-      textCache.set(text, newRecord);
-
-      return use(promise);
+      textCache.set(text, record);
+    }
+    if (record.promise.status === 'pending') {
+      Scheduler.log(`Suspend! [${text}]`);
     }
+    return use(record.promise);
   }
 
   function Text({text}) {

scripts/error-codes/codes.json

@@ -550,5 +550,6 @@
   "562": "The render was aborted due to a fatal error.",
   "563": "This render completed successfully. All cacheSignals are now aborted to allow clean up of any unused resources.",
   "564": "Unknown command. The debugChannel was not wired up properly.",
-  "565": "resolveDebugMessage/closeDebugChannel should not be called for a Request that wasn't kept alive. This is a bug in React."
+  "565": "resolveDebugMessage/closeDebugChannel should not be called for a Request that wasn't kept alive. This is a bug in React.",
+  "566": "This library called use() to suspend in a previous render but did not call use() when it finished. This indicates an incorrect use of use(). A common mistake is to call use() only when something is not cached.\n\n  if (cache.value !== undefined) use(cache.promise) else return cache.value\n\nThe correct way is to always call use() with a Promise and resolve it with the value.\n\n  return use(cache.promise)"
 }