From d4c4b14c469cc74203fd9e2b70c2331d030d6b30 Mon Sep 17 00:00:00 2001 From: Andrew Clark Date: Tue, 3 Oct 2023 14:29:17 -0400 Subject: [PATCH] Warn if optimistic state is updated outside of a transition MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit If optimistic state is updated, and there's no startTransition on the stack, there are two likely scenarios. One possibility is that the optimistic update is triggered by a regular event handler (e.g. `onSubmit`) instead of an action. This is a mistake and we will warn. The other possibility is the optimistic update is inside an async action, but after an `await`. In this case, we can make it "just work" by associating the optimistic update with the pending async action. Technically it's possible that the optimistic update is unrelated to the pending action, but we don't have a way of knowing this for sure because browsers currently do not provide a way to track async scope. (The AsyncContext proposal, if it lands, will solve this in the future.) However, this is no different than the problem of unrelated transitions being grouped together — it's not wrong per se, but it's not ideal. Once AsyncContext starts landing in browsers, we will provide better warnings in development for these cases. --- .../react-reconciler/src/ReactFiberHooks.js | 47 ++++++++++++++--- .../src/__tests__/ReactAsyncActions-test.js | 51 ++++++++++++++++++- 2 files changed, 91 insertions(+), 7 deletions(-) diff --git a/packages/react-reconciler/src/ReactFiberHooks.js b/packages/react-reconciler/src/ReactFiberHooks.js index 24746bde8d330..ae4b68b592d5c 100644 --- a/packages/react-reconciler/src/ReactFiberHooks.js +++ b/packages/react-reconciler/src/ReactFiberHooks.js @@ -143,6 +143,7 @@ import type {BatchConfigTransition} from './ReactFiberTracingMarkerComponent'; import { requestAsyncActionContext, requestSyncActionContext, + peekEntangledActionLane, } from './ReactFiberAsyncAction'; import {HostTransitionContext} from './ReactFiberHostContext'; import {requestTransitionLane} from './ReactFiberRootScheduler'; @@ -2722,6 +2723,7 @@ function startTransition( ); const prevTransition = ReactCurrentBatchConfig.transition; + const currentTransition: BatchConfigTransition = {}; if (enableAsyncActions) { // We don't really need to use an optimistic update here, because we @@ -2730,15 +2732,14 @@ function startTransition( // optimistic update anyway to make it less likely the behavior accidentally // diverges; for example, both an optimistic update and this one should // share the same lane. + ReactCurrentBatchConfig.transition = currentTransition; dispatchOptimisticSetState(fiber, false, queue, pendingState); } else { ReactCurrentBatchConfig.transition = null; dispatchSetState(fiber, queue, pendingState); + ReactCurrentBatchConfig.transition = currentTransition; } - const currentTransition = (ReactCurrentBatchConfig.transition = - ({}: BatchConfigTransition)); - if (enableTransitionTracing) { if (options !== undefined && options.name !== undefined) { ReactCurrentBatchConfig.transition.name = options.name; @@ -3201,14 +3202,48 @@ function dispatchOptimisticSetState( queue: UpdateQueue, action: A, ): void { + if (__DEV__) { + if (ReactCurrentBatchConfig.transition === null) { + // An optimistic update occurred, but startTransition is not on the stack. + // There are two likely scenarios. + + // One possibility is that the optimistic update is triggered by a regular + // event handler (e.g. `onSubmit`) instead of an action. This is a mistake + // and we will warn. + + // The other possibility is the optimistic update is inside an async + // action, but after an `await`. In this case, we can make it "just work" + // by associating the optimistic update with the pending async action. + + // Technically it's possible that the optimistic update is unrelated to + // the pending action, but we don't have a way of knowing this for sure + // because browsers currently do not provide a way to track async scope. + // (The AsyncContext proposal, if it lands, will solve this in the + // future.) However, this is no different than the problem of unrelated + // transitions being grouped together — it's not wrong per se, but it's + // not ideal. + + // Once AsyncContext starts landing in browsers, we will provide better + // warnings in development for these cases. + if (peekEntangledActionLane() !== NoLane) { + // There is a pending async action. Don't warn. + } else { + // There's no pending async action. The most likely cause is that we're + // inside a regular event handler (e.g. onSubmit) instead of an action. + console.error( + 'An optimistic state update occurred outside a transition or ' + + 'action. To fix, move the update to an action, or wrap ' + + 'with startTransition.', + ); + } + } + } + const update: Update = { // An optimistic update commits synchronously. lane: SyncLane, // After committing, the optimistic update is "reverted" using the same // lane as the transition it's associated with. - // - // TODO: Warn if there's no transition/action associated with this - // optimistic update. revertLane: requestTransitionLane(), action, hasEagerState: false, diff --git a/packages/react-reconciler/src/__tests__/ReactAsyncActions-test.js b/packages/react-reconciler/src/__tests__/ReactAsyncActions-test.js index 13143fa5ecf5b..2a36c4cb8c832 100644 --- a/packages/react-reconciler/src/__tests__/ReactAsyncActions-test.js +++ b/packages/react-reconciler/src/__tests__/ReactAsyncActions-test.js @@ -1142,7 +1142,7 @@ describe('ReactAsyncActions', () => { // Initial render const root = ReactNoop.createRoot(); - await act(() => root.render()); + await act(() => root.render()); assertLog(['A']); expect(root).toMatchRenderedOutput(
A
); @@ -1174,5 +1174,54 @@ describe('ReactAsyncActions', () => { await act(() => resolveText('Wait 2')); assertLog(['B']); + expect(root).toMatchRenderedOutput(
B
); + }); + + // @gate enableAsyncActions + test('useOptimistic warns if outside of a transition', async () => { + let startTransition; + let setLoadingProgress; + let setText; + function App() { + const [, _startTransition] = useTransition(); + const [text, _setText] = useState('A'); + const [loadingProgress, _setLoadingProgress] = useOptimistic(0); + startTransition = _startTransition; + setText = _setText; + setLoadingProgress = _setLoadingProgress; + + return ( + <> + {loadingProgress !== 0 ? ( +
+ +
+ ) : null} +
+ +
+ + ); + } + + // Initial render + const root = ReactNoop.createRoot(); + await act(() => root.render()); + assertLog(['A']); + expect(root).toMatchRenderedOutput(
A
); + + await expect(async () => { + await act(() => { + setLoadingProgress('25%'); + startTransition(() => setText('B')); + }); + }).toErrorDev( + 'An optimistic state update occurred outside a transition or ' + + 'action. To fix, move the update to an action, or wrap ' + + 'with startTransition.', + {withoutStack: true}, + ); + assertLog(['Loading... (25%)', 'A', 'B']); + expect(root).toMatchRenderedOutput(
B
); }); });