Warn if optimistic state is updated outside of a transition (#27454)
### Based on #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.
Andrew Clark committed
Oct 4, 2023 at 10:57 UTC
88d56b8e818d0c48eb6642303169c1fadeb99d59
2 files changed
+91
-7
packages/react-reconciler/src/ReactFiberHooks.js
+41
-6
@@ -143,6 +143,7 @@ import type {BatchConfigTransition} from './ReactFiberTracingMarkerComponent';
143
import {
144
requestAsyncActionContext,
145
requestSyncActionContext,
146
+ peekEntangledActionLane,
147
} from './ReactFiberAsyncAction';
148
import {HostTransitionContext} from './ReactFiberHostContext';
149
import {requestTransitionLane} from './ReactFiberRootScheduler';
@@ -2722,6 +2723,7 @@ function startTransition<S>(
2723
);
2724
2725
const prevTransition = ReactCurrentBatchConfig.transition;
2726
+ const currentTransition: BatchConfigTransition = {};
2727
2728
if (enableAsyncActions) {
2729
// We don't really need to use an optimistic update here, because we
@@ -2730,15 +2732,14 @@ function startTransition<S>(
2732
// optimistic update anyway to make it less likely the behavior accidentally
2733
// diverges; for example, both an optimistic update and this one should
2734
// share the same lane.
2735
+ ReactCurrentBatchConfig.transition = currentTransition;
2736
dispatchOptimisticSetState(fiber, false, queue, pendingState);
2737
} else {
2738
ReactCurrentBatchConfig.transition = null;
2739
dispatchSetState(fiber, queue, pendingState);
2740
+ ReactCurrentBatchConfig.transition = currentTransition;
2741
}
2742
2739
- const currentTransition = (ReactCurrentBatchConfig.transition =
2740
- ({}: BatchConfigTransition));
2741
-
2743
if (enableTransitionTracing) {
2744
if (options !== undefined && options.name !== undefined) {
2745
ReactCurrentBatchConfig.transition.name = options.name;
@@ -3201,14 +3202,48 @@ function dispatchOptimisticSetState<S, A>(
3202
queue: UpdateQueue<S, A>,
3203
action: A,
3204
): 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
+
3242
const update: Update<S, A> = {
3243
// An optimistic update commits synchronously.
3244
lane: SyncLane,
3245
// After committing, the optimistic update is "reverted" using the same
3246
// lane as the transition it's associated with.
3209
- //
3210
- // TODO: Warn if there's no transition/action associated with this
3211
- // optimistic update.
3247
revertLane: requestTransitionLane(),
3248
action,
3249
hasEagerState: false,
packages/react-reconciler/src/__tests__/ReactAsyncActions-test.js
+50
-1
@@ -1142,7 +1142,7 @@ describe('ReactAsyncActions', () => {
1142
1143
// Initial render
1144
const root = ReactNoop.createRoot();
1145
- await act(() => root.render(<App text="A" />));
1145
+ await act(() => root.render(<App />));
1146
assertLog(['A']);
1147
expect(root).toMatchRenderedOutput(<div>A</div>);
1148
@@ -1174,5 +1174,54 @@ describe('ReactAsyncActions', () => {
1174
1175
await act(() => resolveText('Wait 2'));
1176
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>);
1226
});
1227
});