Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix: Don't read primaryChild.childExpirationTime #18457

Merged
merged 1 commit into from
Apr 1, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 1 addition & 11 deletions packages/react-reconciler/src/ReactFiberBeginWork.js
Original file line number Diff line number Diff line change
Expand Up @@ -1646,16 +1646,9 @@ function shouldRemainOnFallback(
function getRemainingWorkInPrimaryTree(
current: Fiber,
workInProgress: Fiber,
currentPrimaryChildFragment: Fiber | null,
renderExpirationTime,
) {
const currentParentOfPrimaryChildren =
currentPrimaryChildFragment !== null
? currentPrimaryChildFragment
: current;
const currentChildExpirationTime =
currentParentOfPrimaryChildren.childExpirationTime;

const currentChildExpirationTime = current.childExpirationTime;
const currentSuspenseState: SuspenseState = current.memoizedState;
if (currentSuspenseState !== null) {
// This boundary already timed out. Check if this render includes the level
Expand Down Expand Up @@ -1945,7 +1938,6 @@ function updateSuspenseComponent(
primaryChildFragment.childExpirationTime = getRemainingWorkInPrimaryTree(
current,
workInProgress,
null,
renderExpirationTime,
);
workInProgress.memoizedState = updateSuspenseState(
Expand Down Expand Up @@ -2016,7 +2008,6 @@ function updateSuspenseComponent(
primaryChildFragment.childExpirationTime = getRemainingWorkInPrimaryTree(
current,
workInProgress,
currentPrimaryChildFragment,
renderExpirationTime,
);
// Skip the primary children, and continue working on the
Expand Down Expand Up @@ -2118,7 +2109,6 @@ function updateSuspenseComponent(
primaryChildFragment.childExpirationTime = getRemainingWorkInPrimaryTree(
current,
workInProgress,
null,
renderExpirationTime,
);
// Skip the primary children, and continue working on the
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3266,6 +3266,90 @@ describe('ReactSuspenseWithNoopRenderer', () => {
},
);

it(
'regression: primary fragment fiber is not always part of setState ' +
'return path (another case)',
async () => {
// Reproduces a bug where updates inside a suspended tree are dropped
// because the fragment fiber we insert to wrap the hidden children is not
// part of the return path, so it doesn't get marked during setState.
const {useState} = React;
const root = ReactNoop.createRoot();

function Parent() {
return (
<Suspense fallback={<Text text="Loading..." />}>
<Child />
</Suspense>
);
}

let setText;
function Child() {
const [text, _setText] = useState('A');
setText = _setText;
return <AsyncText text={text} />;
}

// Mount an initial tree. Resolve A so that it doesn't suspend.
await resolveText('A');
await ReactNoop.act(async () => {
root.render(<Parent />);
});
expect(Scheduler).toHaveYielded(['A']);
// At this point, the setState return path follows current fiber.
expect(root).toMatchRenderedOutput(<span prop="A" />);

// Schedule another update. This will "flip" the alternate pairs.
await resolveText('B');
await ReactNoop.act(async () => {
setText('B');
});
expect(Scheduler).toHaveYielded(['B']);
// Now the setState return path follows the *alternate* fiber.
expect(root).toMatchRenderedOutput(<span prop="B" />);

// Schedule another update. This time, we'll suspend.
await ReactNoop.act(async () => {
setText('C');
});
expect(Scheduler).toHaveYielded(['Suspend! [C]', 'Loading...']);

// Commit. This will insert a fragment fiber to wrap around the component
// that triggered the update.
await ReactNoop.act(async () => {
await advanceTimers(250);
});
// The fragment fiber is part of the current tree, but the setState return
// path still follows the alternate path. That means the fragment fiber is
// not part of the return path.
expect(root).toMatchRenderedOutput(
<>
<span hidden={true} prop="B" />
<span prop="Loading..." />
</>,
);

await ReactNoop.act(async () => {
// Schedule a normal pri update. This will suspend again.
setText('D');

// And another update at lower priority. This will unblock.
await resolveText('E');
Scheduler.unstable_runWithPriority(
Scheduler.unstable_IdlePriority,
() => {
setText('E');
},
);
});
// Even though the fragment fiber is not part of the return path, we should
// be able to finish rendering.
expect(Scheduler).toHaveYielded(['Suspend! [D]', 'E']);
expect(root).toMatchRenderedOutput(<span prop="E" />);
},
);

it(
'after showing fallback, should not flip back to primary content until ' +
'the update that suspended finishes',
Expand Down