Skip to content

Commit 85c2b51

Browse files
authored
Fix: Optimistic update does not get reset (facebook#27453)
I found a bug where if an optimistic update causes a component to rerender, and there are no other state updates during that render, React bails out without applying the update. Whenever a hook detects a change, we must mark the component as dirty to prevent a bailout. We check for changes by comparing the new state to `hook.memoizedState`. However, when implementing optimistic state rebasing, I incorrectly reset `hook.memoizedState` to the incoming base state, even though I only needed to reset `hook.baseState`. This was just a mistake on my part. This wasn't caught by the existing tests because usually when the optimistic state changes, there's also some other state that marks the component as dirty in the same render. I fixed the bug and added a regression test.
1 parent db69f95 commit 85c2b51

File tree

2 files changed

+69
-6
lines changed

2 files changed

+69
-6
lines changed

packages/react-reconciler/src/ReactFiberHooks.js

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -1817,9 +1817,9 @@ function updateOptimisticImpl<S, A>(
18171817
// as an argument. It's called a passthrough because if there are no pending
18181818
// updates, it will be returned as-is.
18191819
//
1820-
// Reset the base state and memoized state to the passthrough. Future
1821-
// updates will be applied on top of this.
1822-
hook.baseState = hook.memoizedState = passthrough;
1820+
// Reset the base state to the passthrough. Future updates will be applied
1821+
// on top of this.
1822+
hook.baseState = passthrough;
18231823

18241824
// If a reducer is not provided, default to the same one used by useState.
18251825
const resolvedReducer: (S, A) => S =
@@ -1853,9 +1853,9 @@ function rerenderOptimistic<S, A>(
18531853

18541854
// This is a mount. No updates to process.
18551855

1856-
// Reset the base state and memoized state to the passthrough. Future
1857-
// updates will be applied on top of this.
1858-
hook.baseState = hook.memoizedState = passthrough;
1856+
// Reset the base state to the passthrough. Future updates will be applied
1857+
// on top of this.
1858+
hook.baseState = passthrough;
18591859
const dispatch = hook.queue.dispatch;
18601860
return [passthrough, dispatch];
18611861
}

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

Lines changed: 63 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1112,4 +1112,67 @@ describe('ReactAsyncActions', () => {
11121112
</>,
11131113
);
11141114
});
1115+
1116+
// @gate enableAsyncActions
1117+
test('useOptimistic can update repeatedly in the same async action', async () => {
1118+
let startTransition;
1119+
let setLoadingProgress;
1120+
let setText;
1121+
function App() {
1122+
const [, _startTransition] = useTransition();
1123+
const [text, _setText] = useState('A');
1124+
const [loadingProgress, _setLoadingProgress] = useOptimistic(0);
1125+
startTransition = _startTransition;
1126+
setText = _setText;
1127+
setLoadingProgress = _setLoadingProgress;
1128+
1129+
return (
1130+
<>
1131+
{loadingProgress !== 0 ? (
1132+
<div key="progress">
1133+
<Text text={`Loading... (${loadingProgress})`} />
1134+
</div>
1135+
) : null}
1136+
<div key="real">
1137+
<Text text={text} />
1138+
</div>
1139+
</>
1140+
);
1141+
}
1142+
1143+
// Initial render
1144+
const root = ReactNoop.createRoot();
1145+
await act(() => root.render(<App text="A" />));
1146+
assertLog(['A']);
1147+
expect(root).toMatchRenderedOutput(<div>A</div>);
1148+
1149+
await act(async () => {
1150+
startTransition(async () => {
1151+
setLoadingProgress('25%');
1152+
await getText('Wait 1');
1153+
setLoadingProgress('75%');
1154+
await getText('Wait 2');
1155+
startTransition(() => setText('B'));
1156+
});
1157+
});
1158+
assertLog(['Loading... (25%)', 'A']);
1159+
expect(root).toMatchRenderedOutput(
1160+
<>
1161+
<div>Loading... (25%)</div>
1162+
<div>A</div>
1163+
</>,
1164+
);
1165+
1166+
await act(() => resolveText('Wait 1'));
1167+
assertLog(['Loading... (75%)', 'A']);
1168+
expect(root).toMatchRenderedOutput(
1169+
<>
1170+
<div>Loading... (75%)</div>
1171+
<div>A</div>
1172+
</>,
1173+
);
1174+
1175+
await act(() => resolveText('Wait 2'));
1176+
assertLog(['B']);
1177+
});
11151178
});

0 commit comments

Comments
 (0)