Skip to content

Commit

Permalink
Reset lastEffect when resuming SuspenseList
Browse files Browse the repository at this point in the history
We store an effect pointer so we can backtrack in the effect list in some
cases. This is a stateful variable. If we interrupt a render we need to
reset it.

This field was added after the optimization was added and I didn't remember
to reset it here.

Otherwise we end up not resetting the firstEffect so it points to a stale
list. As a result children don't end up inserted like we think they were.
Then we try to remove them it errors.

It would be nicer to just get rid of the effect list and use the tree for
effects instead. Maybe we still need something for deletions tho.
  • Loading branch information
sebmarkbage committed Mar 28, 2020
1 parent 35a2f74 commit a86b002
Show file tree
Hide file tree
Showing 2 changed files with 186 additions and 0 deletions.
1 change: 1 addition & 0 deletions packages/react-reconciler/src/ReactFiberBeginWork.js
Original file line number Diff line number Diff line change
Expand Up @@ -3153,6 +3153,7 @@ function beginWork(
// update in the past but didn't complete it.
renderState.rendering = null;
renderState.tail = null;
renderState.lastEffect = null;
}
pushSuspenseContext(workInProgress, suspenseStackCursor.current);

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2300,4 +2300,189 @@ describe('ReactSuspenseList', () => {
// remount.
expect(previousInst).toBe(setAsyncB);
});

it('is able to re-suspend the last rows during an update with hidden', async () => {
let AsyncB = createAsyncText('B');

let setAsyncB;

function B() {
let [shouldBeAsync, setAsync] = React.useState(false);
setAsyncB = setAsync;

return shouldBeAsync ? (
<Suspense fallback={<Text text="Loading B" />}>
<AsyncB />
</Suspense>
) : (
<Text text="Sync B" />
);
}

function Foo({updateList}) {
return (
<SuspenseList revealOrder="forwards" tail="hidden">
<Suspense key="A" fallback={<Text text="Loading A" />}>
<Text text="A" />
</Suspense>
<B key="B" updateList={updateList} />
</SuspenseList>
);
}

ReactNoop.render(<Foo />);

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

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

let previousInst = setAsyncB;

// During an update we suspend on B.
ReactNoop.act(() => setAsyncB(true));

expect(Scheduler).toHaveYielded([
'Suspend! [B]',
'Loading B',
// The second pass is the "force hide" pass
'Loading B',
]);

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

// Before we resolve we'll rerender the whole list.
// This should leave the tree intact.
ReactNoop.act(() => ReactNoop.render(<Foo updateList={true} />));

expect(Scheduler).toHaveYielded(['A', 'Suspend! [B]', 'Loading B']);

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

await AsyncB.resolve();

expect(Scheduler).toFlushAndYield(['B']);

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

// This should be the same instance. I.e. it didn't
// remount.
expect(previousInst).toBe(setAsyncB);
});

it('is able to interrupt a partially rendered tree and continue later', async () => {
let AsyncA = createAsyncText('A');

let updateLowPri;
let updateHighPri;

function Bar() {
let [highPriState, setHighPriState] = React.useState(false);
updateHighPri = setHighPriState;
return highPriState ? <AsyncA /> : null;
}

function Foo() {
let [lowPriState, setLowPriState] = React.useState(false);
updateLowPri = setLowPriState;
return (
<SuspenseList revealOrder="forwards" tail="hidden">
<Suspense key="A" fallback={<Text text="Loading A" />}>
<Bar />
</Suspense>
{lowPriState ? <Text text="B" /> : null}
{lowPriState ? <Text text="C" /> : null}
{lowPriState ? <Text text="D" /> : null}
</SuspenseList>
);
}

ReactNoop.render(<Foo />);

expect(Scheduler).toFlushAndYield([]);

expect(ReactNoop).toMatchRenderedOutput(null);

ReactNoop.act(() => {
// Add a few items at the end.
updateLowPri(true);

// Flush partially through.
expect(Scheduler).toFlushAndYieldThrough(['B', 'C']);

// Schedule another update at higher priority.
Scheduler.unstable_runWithPriority(
Scheduler.unstable_UserBlockingPriority,
() => updateHighPri(true),
);

// That will intercept the previous render.
});

jest.runAllTimers();

expect(Scheduler).toHaveYielded(
__DEV__
? [
// First attempt at high pri.
'Suspend! [A]',
'Loading A',
// Re-render at forced.
'Suspend! [A]',
'Loading A',
// We auto-commit this on DEV.
// Try again on low-pri.
'Suspend! [A]',
'Loading A',
]
: [
// First attempt at high pri.
'Suspend! [A]',
'Loading A',
// Re-render at forced.
'Suspend! [A]',
'Loading A',
// We didn't commit so retry at low-pri.
'Suspend! [A]',
'Loading A',
// Re-render at forced.
'Suspend! [A]',
'Loading A',
],
);

expect(ReactNoop).toMatchRenderedOutput(<span>Loading A</span>);

await AsyncA.resolve();

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

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

0 comments on commit a86b002

Please sign in to comment.