Skip to content

Commit

Permalink
Fix: useDeferredValue should reuse previous value (#24413)
Browse files Browse the repository at this point in the history
During an urgent update, useDeferredValue should reuse the previous
value. The regression test I added shows that it was reverting to
the initial value instead.

The cause of the bug was trivial: the update path doesn't update the
hook's `memoizedState` field. Only the mount path.

None of the existing tests happened to catch this because to trigger the
bug, you have to do an urgent update that isn't the first update after
initial render. In all of the existing tests that included an urgent
update, it was the first update, so the "previous" value and the initial
value happened to be the same thing.
  • Loading branch information
acdlite authored Apr 21, 2022
1 parent 9ae80d6 commit bd08137
Show file tree
Hide file tree
Showing 3 changed files with 77 additions and 0 deletions.
1 change: 1 addition & 0 deletions packages/react-reconciler/src/ReactFiberHooks.new.js
Original file line number Diff line number Diff line change
Expand Up @@ -1993,6 +1993,7 @@ function updateDeferredValueImpl<T>(hook: Hook, prevValue: T, value: T): T {
markWorkInProgressReceivedUpdate();
}

hook.memoizedState = value;
return value;
}
}
Expand Down
1 change: 1 addition & 0 deletions packages/react-reconciler/src/ReactFiberHooks.old.js
Original file line number Diff line number Diff line change
Expand Up @@ -1993,6 +1993,7 @@ function updateDeferredValueImpl<T>(hook: Hook, prevValue: T, value: T): T {
markWorkInProgressReceivedUpdate();
}

hook.memoizedState = value;
return value;
}
}
Expand Down
75 changes: 75 additions & 0 deletions packages/react-reconciler/src/__tests__/ReactDeferredValue-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -221,4 +221,79 @@ describe('ReactDeferredValue', () => {
</div>,
);
});

it('regression test: during urgent update, reuse previous value, not initial value', async () => {
function App({value: propValue}) {
const [value, setValue] = useState(null);
if (value !== propValue) {
setValue(propValue);
}

const deferredValue = useDeferredValue(value);

const child = useMemo(() => <Text text={'Original: ' + value} />, [
value,
]);

const deferredChild = useMemo(
() => <Text text={'Deferred: ' + deferredValue} />,
[deferredValue],
);

return (
<div>
<div>{child}</div>
<div>{deferredChild}</div>
</div>
);
}

const root = ReactNoop.createRoot();

// Initial render
await act(async () => {
root.render(<App value={1} />);
expect(Scheduler).toFlushUntilNextPaint(['Original: 1', 'Deferred: 1']);
expect(root).toMatchRenderedOutput(
<div>
<div>Original: 1</div>
<div>Deferred: 1</div>
</div>,
);
});

await act(async () => {
startTransition(() => {
root.render(<App value={2} />);
});
expect(Scheduler).toFlushUntilNextPaint(['Original: 2', 'Deferred: 2']);
expect(root).toMatchRenderedOutput(
<div>
<div>Original: 2</div>
<div>Deferred: 2</div>
</div>,
);
});

await act(async () => {
root.render(<App value={3} />);
// In the regression, the memoized value was not updated during non-urgent
// updates, so this would flip the deferred value back to the initial
// value (1) instead of reusing the current one (2).
expect(Scheduler).toFlushUntilNextPaint(['Original: 3']);
expect(root).toMatchRenderedOutput(
<div>
<div>Original: 3</div>
<div>Deferred: 2</div>
</div>,
);
expect(Scheduler).toFlushUntilNextPaint(['Deferred: 3']);
expect(root).toMatchRenderedOutput(
<div>
<div>Original: 3</div>
<div>Deferred: 3</div>
</div>,
);
});
});
});

0 comments on commit bd08137

Please sign in to comment.