From e24b0e29d01907dd6ac21122cd84b5140bf74e92 Mon Sep 17 00:00:00 2001 From: Sebastian Markbage Date: Mon, 28 Jul 2025 21:44:12 -0400 Subject: [PATCH 1/3] Error if use() is used conditionally based on a cache --- .../react-reconciler/src/ReactChildFiber.js | 2 +- .../react-reconciler/src/ReactFiberHooks.js | 9 ++- .../src/ReactFiberThenable.js | 80 +++++++++++++++++++ scripts/error-codes/codes.json | 3 +- 4 files changed, 91 insertions(+), 3 deletions(-) diff --git a/packages/react-reconciler/src/ReactChildFiber.js b/packages/react-reconciler/src/ReactChildFiber.js index f574162b41b..e71a4feae30 100644 --- a/packages/react-reconciler/src/ReactChildFiber.js +++ b/packages/react-reconciler/src/ReactChildFiber.js @@ -261,7 +261,7 @@ function unwrapThenable(thenable: Thenable): T { if (thenableState === null) { thenableState = createThenableState(); } - return trackUsedThenable(thenableState, thenable, index); + return trackUsedThenable(thenableState, thenable, index, null); } function coerceRef(workInProgress: Fiber, element: ReactElement): void { diff --git a/packages/react-reconciler/src/ReactFiberHooks.js b/packages/react-reconciler/src/ReactFiberHooks.js index 63332124e2f..760a79b7e9e 100644 --- a/packages/react-reconciler/src/ReactFiberHooks.js +++ b/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( } 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(thenable: Thenable): 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. diff --git a/packages/react-reconciler/src/ReactFiberThenable.js b/packages/react-reconciler/src/ReactFiberThenable.js index 4f52131a649..dbe459054fd 100644 --- a/packages/react-reconciler/src/ReactFiberThenable.js +++ b/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>, @@ -97,10 +101,16 @@ export function isThenableResolved(thenable: Thenable): boolean { return status === 'fulfilled' || status === 'rejected'; } +// DEV-only +let lastSuspendedFiber: null | Fiber = null; +let lastSuspendedStack: null | Error = null; +let didIssueUseWarning = false; + export function trackUsedThenable( thenableState: ThenableState, thenable: Thenable, index: number, + fiber: null | Fiber, // DEV-only ): T { if (__DEV__ && ReactSharedInternals.actQueue !== null) { ReactSharedInternals.didUsePromise = true; @@ -246,6 +256,25 @@ export function trackUsedThenable( 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 +345,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; + } + } + } +} diff --git a/scripts/error-codes/codes.json b/scripts/error-codes/codes.json index a9867c154c6..c732993fde8 100644 --- a/scripts/error-codes/codes.json +++ b/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)" } From aecfc9e869124fbf76aa84b0a164212b8515564f Mon Sep 17 00:00:00 2001 From: Sebastian Markbage Date: Mon, 28 Jul 2025 21:55:45 -0400 Subject: [PATCH 2/3] Don't warn if there's a discrete user event update in between renders --- .../react-reconciler/src/ReactFiberThenable.js | 7 +++++++ .../react-reconciler/src/ReactFiberWorkLoop.js | 16 +++++++++++++++- 2 files changed, 22 insertions(+), 1 deletion(-) diff --git a/packages/react-reconciler/src/ReactFiberThenable.js b/packages/react-reconciler/src/ReactFiberThenable.js index dbe459054fd..972def362a1 100644 --- a/packages/react-reconciler/src/ReactFiberThenable.js +++ b/packages/react-reconciler/src/ReactFiberThenable.js @@ -106,6 +106,13 @@ 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( thenableState: ThenableState, thenable: Thenable, diff --git a/packages/react-reconciler/src/ReactFiberWorkLoop.js b/packages/react-reconciler/src/ReactFiberWorkLoop.js index 1107c5bd462..9426555f7f0 100644 --- a/packages/react-reconciler/src/ReactFiberWorkLoop.js +++ b/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) { From ca86d744ade4c0798699c0544512e5499c666462 Mon Sep 17 00:00:00 2001 From: Sebastian Markbage Date: Mon, 28 Jul 2025 22:08:04 -0400 Subject: [PATCH 3/3] Update internal test that was using the wrong pattern --- .../src/__tests__/ActivitySuspense-test.js | 44 ++++++++----------- 1 file changed, 18 insertions(+), 26 deletions(-) diff --git a/packages/react-reconciler/src/__tests__/ActivitySuspense-test.js b/packages/react-reconciler/src/__tests__/ActivitySuspense-test.js index b18573efa5b..b74e3c0d2b1 100644 --- a/packages/react-reconciler/src/__tests__/ActivitySuspense-test.js +++ b/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}) {