Skip to content

Commit 6671e36

Browse files
acdliteAndyPengc12
authored andcommitted
Warn if optimistic state is updated outside of a transition (facebook#27454)
### Based on facebook#27453 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.
1 parent f72a5f1 commit 6671e36

File tree

2 files changed

+91
-7
lines changed

2 files changed

+91
-7
lines changed

packages/react-reconciler/src/ReactFiberHooks.js

Lines changed: 41 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -143,6 +143,7 @@ import type {BatchConfigTransition} from './ReactFiberTracingMarkerComponent';
143143
import {
144144
requestAsyncActionContext,
145145
requestSyncActionContext,
146+
peekEntangledActionLane,
146147
} from './ReactFiberAsyncAction';
147148
import {HostTransitionContext} from './ReactFiberHostContext';
148149
import {requestTransitionLane} from './ReactFiberRootScheduler';
@@ -2722,6 +2723,7 @@ function startTransition<S>(
27222723
);
27232724

27242725
const prevTransition = ReactCurrentBatchConfig.transition;
2726+
const currentTransition: BatchConfigTransition = {};
27252727

27262728
if (enableAsyncActions) {
27272729
// We don't really need to use an optimistic update here, because we
@@ -2730,15 +2732,14 @@ function startTransition<S>(
27302732
// optimistic update anyway to make it less likely the behavior accidentally
27312733
// diverges; for example, both an optimistic update and this one should
27322734
// share the same lane.
2735+
ReactCurrentBatchConfig.transition = currentTransition;
27332736
dispatchOptimisticSetState(fiber, false, queue, pendingState);
27342737
} else {
27352738
ReactCurrentBatchConfig.transition = null;
27362739
dispatchSetState(fiber, queue, pendingState);
2740+
ReactCurrentBatchConfig.transition = currentTransition;
27372741
}
27382742

2739-
const currentTransition = (ReactCurrentBatchConfig.transition =
2740-
({}: BatchConfigTransition));
2741-
27422743
if (enableTransitionTracing) {
27432744
if (options !== undefined && options.name !== undefined) {
27442745
ReactCurrentBatchConfig.transition.name = options.name;
@@ -3201,14 +3202,48 @@ function dispatchOptimisticSetState<S, A>(
32013202
queue: UpdateQueue<S, A>,
32023203
action: A,
32033204
): void {
3205+
if (__DEV__) {
3206+
if (ReactCurrentBatchConfig.transition === null) {
3207+
// An optimistic update occurred, but startTransition is not on the stack.
3208+
// There are two likely scenarios.
3209+
3210+
// One possibility is that the optimistic update is triggered by a regular
3211+
// event handler (e.g. `onSubmit`) instead of an action. This is a mistake
3212+
// and we will warn.
3213+
3214+
// The other possibility is the optimistic update is inside an async
3215+
// action, but after an `await`. In this case, we can make it "just work"
3216+
// by associating the optimistic update with the pending async action.
3217+
3218+
// Technically it's possible that the optimistic update is unrelated to
3219+
// the pending action, but we don't have a way of knowing this for sure
3220+
// because browsers currently do not provide a way to track async scope.
3221+
// (The AsyncContext proposal, if it lands, will solve this in the
3222+
// future.) However, this is no different than the problem of unrelated
3223+
// transitions being grouped together — it's not wrong per se, but it's
3224+
// not ideal.
3225+
3226+
// Once AsyncContext starts landing in browsers, we will provide better
3227+
// warnings in development for these cases.
3228+
if (peekEntangledActionLane() !== NoLane) {
3229+
// There is a pending async action. Don't warn.
3230+
} else {
3231+
// There's no pending async action. The most likely cause is that we're
3232+
// inside a regular event handler (e.g. onSubmit) instead of an action.
3233+
console.error(
3234+
'An optimistic state update occurred outside a transition or ' +
3235+
'action. To fix, move the update to an action, or wrap ' +
3236+
'with startTransition.',
3237+
);
3238+
}
3239+
}
3240+
}
3241+
32043242
const update: Update<S, A> = {
32053243
// An optimistic update commits synchronously.
32063244
lane: SyncLane,
32073245
// After committing, the optimistic update is "reverted" using the same
32083246
// lane as the transition it's associated with.
3209-
//
3210-
// TODO: Warn if there's no transition/action associated with this
3211-
// optimistic update.
32123247
revertLane: requestTransitionLane(),
32133248
action,
32143249
hasEagerState: false,

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

Lines changed: 50 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1142,7 +1142,7 @@ describe('ReactAsyncActions', () => {
11421142

11431143
// Initial render
11441144
const root = ReactNoop.createRoot();
1145-
await act(() => root.render(<App text="A" />));
1145+
await act(() => root.render(<App />));
11461146
assertLog(['A']);
11471147
expect(root).toMatchRenderedOutput(<div>A</div>);
11481148

@@ -1174,5 +1174,54 @@ describe('ReactAsyncActions', () => {
11741174

11751175
await act(() => resolveText('Wait 2'));
11761176
assertLog(['B']);
1177+
expect(root).toMatchRenderedOutput(<div>B</div>);
1178+
});
1179+
1180+
// @gate enableAsyncActions
1181+
test('useOptimistic warns if outside of a transition', async () => {
1182+
let startTransition;
1183+
let setLoadingProgress;
1184+
let setText;
1185+
function App() {
1186+
const [, _startTransition] = useTransition();
1187+
const [text, _setText] = useState('A');
1188+
const [loadingProgress, _setLoadingProgress] = useOptimistic(0);
1189+
startTransition = _startTransition;
1190+
setText = _setText;
1191+
setLoadingProgress = _setLoadingProgress;
1192+
1193+
return (
1194+
<>
1195+
{loadingProgress !== 0 ? (
1196+
<div key="progress">
1197+
<Text text={`Loading... (${loadingProgress})`} />
1198+
</div>
1199+
) : null}
1200+
<div key="real">
1201+
<Text text={text} />
1202+
</div>
1203+
</>
1204+
);
1205+
}
1206+
1207+
// Initial render
1208+
const root = ReactNoop.createRoot();
1209+
await act(() => root.render(<App />));
1210+
assertLog(['A']);
1211+
expect(root).toMatchRenderedOutput(<div>A</div>);
1212+
1213+
await expect(async () => {
1214+
await act(() => {
1215+
setLoadingProgress('25%');
1216+
startTransition(() => setText('B'));
1217+
});
1218+
}).toErrorDev(
1219+
'An optimistic state update occurred outside a transition or ' +
1220+
'action. To fix, move the update to an action, or wrap ' +
1221+
'with startTransition.',
1222+
{withoutStack: true},
1223+
);
1224+
assertLog(['Loading... (25%)', 'A', 'B']);
1225+
expect(root).toMatchRenderedOutput(<div>B</div>);
11771226
});
11781227
});

0 commit comments

Comments
 (0)