Skip to content

Commit

Permalink
[SuspenseList] Bug fix: Reset renderState when bailing out (#16278)
Browse files Browse the repository at this point in the history
If there are adjacent updates we bail out of rendering the suspense list
at all but we may still complete the node. We need to reset the render
state in that case.

I restructured so that this is in the same code path so we don't forget it
in the future.
  • Loading branch information
sebmarkbage authored Aug 2, 2019
1 parent 0c1ec04 commit a53f5cc
Show file tree
Hide file tree
Showing 2 changed files with 72 additions and 21 deletions.
47 changes: 26 additions & 21 deletions packages/react-reconciler/src/ReactFiberBeginWork.js
Original file line number Diff line number Diff line change
Expand Up @@ -2782,29 +2782,26 @@ function beginWork(
const didSuspendBefore =
(current.effectTag & DidCapture) !== NoEffect;

const childExpirationTime = workInProgress.childExpirationTime;
if (childExpirationTime < renderExpirationTime) {
const hasChildWork =
workInProgress.childExpirationTime >= renderExpirationTime;

if (didSuspendBefore) {
if (hasChildWork) {
// If something was in fallback state last time, and we have all the
// same children then we're still in progressive loading state.
// Something might get unblocked by state updates or retries in the
// tree which will affect the tail. So we need to use the normal
// path to compute the correct tail.
return updateSuspenseListComponent(
current,
workInProgress,
renderExpirationTime,
);
}
// If none of the children had any work, that means that none of
// them got retried so they'll still be blocked in the same way
// as before. We can fast bail out.
pushSuspenseContext(workInProgress, suspenseStackCursor.current);
if (didSuspendBefore) {
workInProgress.effectTag |= DidCapture;
}
return null;
}

if (didSuspendBefore) {
// If something was in fallback state last time, and we have all the
// same children then we're still in progressive loading state.
// Something might get unblocked by state updates or retries in the
// tree which will affect the tail. So we need to use the normal
// path to compute the correct tail.
return updateSuspenseListComponent(
current,
workInProgress,
renderExpirationTime,
);
workInProgress.effectTag |= DidCapture;
}

// If nothing suspended before and we're rendering the same children,
Expand All @@ -2818,7 +2815,15 @@ function beginWork(
renderState.tail = null;
}
pushSuspenseContext(workInProgress, suspenseStackCursor.current);
break;

if (hasChildWork) {
break;
} else {
// If none of the children had any work, that means that none of
// them got retried so they'll still be blocked in the same way
// as before. We can fast bail out.
return null;
}
}
}
return bailoutOnAlreadyFinishedWork(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1826,4 +1826,50 @@ describe('ReactSuspenseList', () => {
</Fragment>,
);
});

it('can do unrelated adjacent updates', async () => {
let updateAdjacent;
function Adjacent() {
let [text, setText] = React.useState('-');
updateAdjacent = setText;
return <Text text={text} />;
}

function Foo() {
return (
<div>
<SuspenseList revealOrder="forwards">
<Text text="A" />
<Text text="B" />
</SuspenseList>
<Adjacent />
</div>
);
}

ReactNoop.render(<Foo />);

expect(Scheduler).toFlushAndYield(['A', 'B', '-']);

expect(ReactNoop).toMatchRenderedOutput(
<div>
<span>A</span>
<span>B</span>
<span>-</span>
</div>,
);

// Update the row adjacent to the list
ReactNoop.act(() => updateAdjacent('C'));

expect(Scheduler).toHaveYielded(['C']);

expect(ReactNoop).toMatchRenderedOutput(
<div>
<span>A</span>
<span>B</span>
<span>C</span>
</div>,
);
});
});

0 comments on commit a53f5cc

Please sign in to comment.