Skip to content

Commit

Permalink
Warn if optimistic state is updated outside of a transition
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
acdlite committed Oct 3, 2023
1 parent e6a8f50 commit 3e5f86e
Show file tree
Hide file tree
Showing 2 changed files with 88 additions and 6 deletions.
46 changes: 40 additions & 6 deletions packages/react-reconciler/src/ReactFiberHooks.js
Original file line number Diff line number Diff line change
Expand Up @@ -143,6 +143,7 @@ import type {BatchConfigTransition} from './ReactFiberTracingMarkerComponent';
import {
requestAsyncActionContext,
requestSyncActionContext,
peekEntangledActionLane,
} from './ReactFiberAsyncAction';
import {HostTransitionContext} from './ReactFiberHostContext';
import {requestTransitionLane} from './ReactFiberRootScheduler';
Expand Down Expand Up @@ -2722,6 +2723,8 @@ function startTransition<S>(
);

const prevTransition = ReactCurrentBatchConfig.transition;
const currentTransition = (ReactCurrentBatchConfig.transition =
({}: BatchConfigTransition));

if (enableAsyncActions) {
// We don't really need to use an optimistic update here, because we
Expand All @@ -2736,9 +2739,6 @@ function startTransition<S>(
dispatchSetState(fiber, queue, pendingState);
}

const currentTransition = (ReactCurrentBatchConfig.transition =
({}: BatchConfigTransition));

if (enableTransitionTracing) {
if (options !== undefined && options.name !== undefined) {
ReactCurrentBatchConfig.transition.name = options.name;
Expand Down Expand Up @@ -3201,14 +3201,48 @@ function dispatchOptimisticSetState<S, A>(
queue: UpdateQueue<S, A>,
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<S, A> = {
// 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,
Expand Down
48 changes: 48 additions & 0 deletions packages/react-reconciler/src/__tests__/ReactAsyncActions-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -1175,4 +1175,52 @@ describe('ReactAsyncActions', () => {
await act(() => resolveText('Wait 2'));
assertLog(['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 ? (
<div key="progress">
<Text text={`Loading... (${loadingProgress})`} />
</div>
) : null}
<div key="real">
<Text text={text} />
</div>
</>
);
}

// Initial render
const root = ReactNoop.createRoot();
await act(() => root.render(<App text="A" />));
assertLog(['A']);
expect(root).toMatchRenderedOutput(<div>A</div>);

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(<div>B</div>);
});
});

0 comments on commit 3e5f86e

Please sign in to comment.