Skip to content

Commit 5de5b61

Browse files
authored
Bugfix: memo drops lower pri updates on bail out (#18091)
Fixes a bug where lower priority updates on a components wrapped with `memo` are sometimes left dangling in the queue without ever being processed, if they are preceded by a higher priority bailout. Cause ----- The pending update priority field is cleared at the beginning of `beginWork`. If there is remaining work at a lower priority level, it's expected that it will be accumulated on the work-in-progress fiber during the begin phase. There's an exception where this assumption doesn't hold: SimpleMemoComponent contains a bailout that occurs *before* the component is evaluated and the update queues are processed, which means we don't accumulate the next priority level. When we complete the fiber, the work loop is left to believe that there's no remaining work. Mitigation ---------- Since this only happens in a single case, a late bailout in SimpleMemoComponent, I've mitigated the bug in that code path by restoring the original update priority from the current fiber. This same case does not apply to MemoComponent, because MemoComponent fibers do not contain hooks or update queues; rather, they wrap around an inner fiber that may contain those. However, I've added a test case for MemoComponent to protect against a possible future regression. Possible next steps ------------------- We should consider moving the update priority assignment in `beginWork` out of the common path and into each branch, to avoid similar bugs in the future.
1 parent abfbae0 commit 5de5b61

File tree

2 files changed

+88
-1
lines changed

2 files changed

+88
-1
lines changed

packages/react-reconciler/src/ReactFiberBeginWork.js

+19-1
Original file line numberDiff line numberDiff line change
@@ -523,6 +523,20 @@ function updateSimpleMemoComponent(
523523
) {
524524
didReceiveUpdate = false;
525525
if (updateExpirationTime < renderExpirationTime) {
526+
// The pending update priority was cleared at the beginning of
527+
// beginWork. We're about to bail out, but there might be additional
528+
// updates at a lower priority. Usually, the priority level of the
529+
// remaining updates is accumlated during the evaluation of the
530+
// component (i.e. when processing the update queue). But since since
531+
// we're bailing out early *without* evaluating the component, we need
532+
// to account for it here, too. Reset to the value of the current fiber.
533+
// NOTE: This only applies to SimpleMemoComponent, not MemoComponent,
534+
// because a MemoComponent fiber does not have hooks or an update queue;
535+
// rather, it wraps around an inner component, which may or may not
536+
// contains hooks.
537+
// TODO: Move the reset at in beginWork out of the common path so that
538+
// this is no longer necessary.
539+
workInProgress.expirationTime = current.expirationTime;
526540
return bailoutOnAlreadyFinishedWork(
527541
current,
528542
workInProgress,
@@ -3103,7 +3117,11 @@ function beginWork(
31033117
didReceiveUpdate = false;
31043118
}
31053119

3106-
// Before entering the begin phase, clear the expiration time.
3120+
// Before entering the begin phase, clear pending update priority.
3121+
// TODO: This assumes that we're about to evaluate the component and process
3122+
// the update queue. However, there's an exception: SimpleMemoComponent
3123+
// sometimes bails out later in the begin phase. This indicates that we should
3124+
// move this assignment out of the common path and into each branch.
31073125
workInProgress.expirationTime = NoWork;
31083126

31093127
switch (workInProgress.tag) {

packages/react-reconciler/src/__tests__/ReactMemo-test.internal.js

+69
Original file line numberDiff line numberDiff line change
@@ -419,6 +419,75 @@ describe('memo', () => {
419419
'Invalid prop `inner` of type `boolean` supplied to `Inner`, expected `number`.',
420420
]);
421421
});
422+
423+
it('does not drop lower priority state updates when bailing out at higher pri (simple)', async () => {
424+
const {useState} = React;
425+
426+
let setCounter;
427+
const Counter = memo(() => {
428+
const [counter, _setCounter] = useState(0);
429+
setCounter = _setCounter;
430+
return counter;
431+
});
432+
433+
function App() {
434+
return (
435+
<Suspense fallback="Loading...">
436+
<Counter />
437+
</Suspense>
438+
);
439+
}
440+
441+
const root = ReactNoop.createRoot();
442+
await ReactNoop.act(async () => {
443+
root.render(<App />);
444+
});
445+
expect(root).toMatchRenderedOutput('0');
446+
447+
await ReactNoop.act(async () => {
448+
setCounter(1);
449+
ReactNoop.discreteUpdates(() => {
450+
root.render(<App />);
451+
});
452+
});
453+
expect(root).toMatchRenderedOutput('1');
454+
});
455+
456+
it('does not drop lower priority state updates when bailing out at higher pri (complex)', async () => {
457+
const {useState} = React;
458+
459+
let setCounter;
460+
const Counter = memo(
461+
() => {
462+
const [counter, _setCounter] = useState(0);
463+
setCounter = _setCounter;
464+
return counter;
465+
},
466+
(a, b) => a.complexProp.val === b.complexProp.val,
467+
);
468+
469+
function App() {
470+
return (
471+
<Suspense fallback="Loading...">
472+
<Counter complexProp={{val: 1}} />
473+
</Suspense>
474+
);
475+
}
476+
477+
const root = ReactNoop.createRoot();
478+
await ReactNoop.act(async () => {
479+
root.render(<App />);
480+
});
481+
expect(root).toMatchRenderedOutput('0');
482+
483+
await ReactNoop.act(async () => {
484+
setCounter(1);
485+
ReactNoop.discreteUpdates(() => {
486+
root.render(<App />);
487+
});
488+
});
489+
expect(root).toMatchRenderedOutput('1');
490+
});
422491
});
423492
}
424493
});

0 commit comments

Comments
 (0)