Skip to content

Commit d5b54d0

Browse files
sebmarkbagesebmarkbage
authored
[SuspenseList] Fix bugs with dropped Promises (#17082)
* Transfer any pending promises from inner boundary to list For non-hidden modes, this boundary should commit so this shouldn't be needed but the nested boundary can make a second pass which forces these to be recreated without resuspending. In this case, the outer list assumes that it can collect the inner promises to still rerender if needed. * Propagate suspense "context" change to nested SuspenseLists This means that we always rerender any nested SuspenseLists together. This bug looks similar to the previous one but is not based on the lack of retry but that the retry only happens on the outer boundary but the inner doesn't get a retry ping since it didn't know about its own promise after the second pass.
1 parent 75955bf commit d5b54d0

File tree

3 files changed

+206
-17
lines changed

3 files changed

+206
-17
lines changed

‎packages/react-reconciler/src/ReactFiberBeginWork.js‎

Lines changed: 22 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -2121,6 +2121,20 @@ function updateDehydratedSuspenseComponent(
21212121
}
21222122
}
21232123

2124+
function scheduleWorkOnFiber(
2125+
fiber: Fiber,
2126+
renderExpirationTime: ExpirationTime,
2127+
) {
2128+
if (fiber.expirationTime < renderExpirationTime) {
2129+
fiber.expirationTime = renderExpirationTime;
2130+
}
2131+
let alternate = fiber.alternate;
2132+
if (alternate !== null && alternate.expirationTime < renderExpirationTime) {
2133+
alternate.expirationTime = renderExpirationTime;
2134+
}
2135+
scheduleWorkOnParentPath(fiber.return, renderExpirationTime);
2136+
}
2137+
21242138
function propagateSuspenseContextChange(
21252139
workInProgress: Fiber,
21262140
firstChild: null | Fiber,
@@ -2134,18 +2148,15 @@ function propagateSuspenseContextChange(
21342148
if (node.tag === SuspenseComponent) {
21352149
const state: SuspenseState | null = node.memoizedState;
21362150
if (state !== null) {
2137-
if (node.expirationTime < renderExpirationTime) {
2138-
node.expirationTime = renderExpirationTime;
2139-
}
2140-
let alternate = node.alternate;
2141-
if (
2142-
alternate !== null &&
2143-
alternate.expirationTime < renderExpirationTime
2144-
) {
2145-
alternate.expirationTime = renderExpirationTime;
2146-
}
2147-
scheduleWorkOnParentPath(node.return, renderExpirationTime);
2151+
scheduleWorkOnFiber(node, renderExpirationTime);
21482152
}
2153+
} else if (node.tag === SuspenseListComponent) {
2154+
// If the tail is hidden there might not be an Suspense boundaries
2155+
// to schedule work on. In this case we have to schedule it on the
2156+
// list itself.
2157+
// We don't have to traverse to the children of the list since
2158+
// the list will propagate the change when it rerenders.
2159+
scheduleWorkOnFiber(node, renderExpirationTime);
21492160
} else if (node.child !== null) {
21502161
node.child.return = node;
21512162
node = node.child;

‎packages/react-reconciler/src/ReactFiberCompleteWork.js‎

Lines changed: 9 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -1083,19 +1083,22 @@ function completeWork(
10831083
if (suspended !== null) {
10841084
workInProgress.effectTag |= DidCapture;
10851085
didSuspendAlready = true;
1086+
1087+
// Ensure we transfer the update queue to the parent so that it doesn't
1088+
// get lost if this row ends up dropped during a second pass.
1089+
let newThennables = suspended.updateQueue;
1090+
if (newThennables !== null) {
1091+
workInProgress.updateQueue = newThennables;
1092+
workInProgress.effectTag |= Update;
1093+
}
1094+
10861095
cutOffTailIfNeeded(renderState, true);
10871096
// This might have been modified.
10881097
if (
10891098
renderState.tail === null &&
10901099
renderState.tailMode === 'hidden'
10911100
) {
10921101
// We need to delete the row we just rendered.
1093-
// Ensure we transfer the update queue to the parent.
1094-
let newThennables = suspended.updateQueue;
1095-
if (newThennables !== null) {
1096-
workInProgress.updateQueue = newThennables;
1097-
workInProgress.effectTag |= Update;
1098-
}
10991102
// Reset the effect list to what it w as before we rendered this
11001103
// child. The nested children have already appended themselves.
11011104
let lastEffect = (workInProgress.lastEffect =

‎packages/react-reconciler/src/__tests__/ReactSuspenseList-test.internal.js‎

Lines changed: 175 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1826,6 +1826,181 @@ describe('ReactSuspenseList', () => {
18261826
);
18271827
});
18281828

1829+
it('eventually resolves a nested forwards suspense list', async () => {
1830+
let B = createAsyncText('B');
1831+
1832+
function Foo() {
1833+
return (
1834+
<SuspenseList revealOrder="together">
1835+
<SuspenseList revealOrder="forwards">
1836+
<Suspense fallback={<Text text="Loading A" />}>
1837+
<Text text="A" />
1838+
</Suspense>
1839+
<Suspense fallback={<Text text="Loading B" />}>
1840+
<B />
1841+
</Suspense>
1842+
<Suspense fallback={<Text text="Loading C" />}>
1843+
<Text text="C" />
1844+
</Suspense>
1845+
</SuspenseList>
1846+
<Suspense fallback={<Text text="Loading D" />}>
1847+
<Text text="D" />
1848+
</Suspense>
1849+
</SuspenseList>
1850+
);
1851+
}
1852+
1853+
ReactNoop.render(<Foo />);
1854+
1855+
expect(Scheduler).toFlushAndYield([
1856+
'A',
1857+
'Suspend! [B]',
1858+
'Loading B',
1859+
'Loading C',
1860+
'D',
1861+
// The second pass forces the fallbacks
1862+
'Loading A',
1863+
'Loading B',
1864+
'Loading C',
1865+
'Loading D',
1866+
]);
1867+
1868+
expect(ReactNoop).toMatchRenderedOutput(
1869+
<>
1870+
<span>Loading A</span>
1871+
<span>Loading B</span>
1872+
<span>Loading C</span>
1873+
<span>Loading D</span>
1874+
</>,
1875+
);
1876+
1877+
await B.resolve();
1878+
1879+
expect(Scheduler).toFlushAndYield(['A', 'B', 'C', 'D']);
1880+
1881+
expect(ReactNoop).toMatchRenderedOutput(
1882+
<>
1883+
<span>A</span>
1884+
<span>B</span>
1885+
<span>C</span>
1886+
<span>D</span>
1887+
</>,
1888+
);
1889+
});
1890+
1891+
it('eventually resolves a nested forwards suspense list with a hidden tail', async () => {
1892+
let B = createAsyncText('B');
1893+
1894+
function Foo() {
1895+
return (
1896+
<SuspenseList revealOrder="together">
1897+
<SuspenseList revealOrder="forwards" tail="hidden">
1898+
<Suspense fallback={<Text text="Loading A" />}>
1899+
<Text text="A" />
1900+
</Suspense>
1901+
<Suspense fallback={<Text text="Loading B" />}>
1902+
<B />
1903+
</Suspense>
1904+
</SuspenseList>
1905+
<Suspense fallback={<Text text="Loading C" />}>
1906+
<Text text="C" />
1907+
</Suspense>
1908+
</SuspenseList>
1909+
);
1910+
}
1911+
1912+
ReactNoop.render(<Foo />);
1913+
1914+
expect(Scheduler).toFlushAndYield([
1915+
'A',
1916+
'Suspend! [B]',
1917+
'Loading B',
1918+
'C',
1919+
'Loading C',
1920+
]);
1921+
1922+
expect(ReactNoop).toMatchRenderedOutput(<span>Loading C</span>);
1923+
1924+
await B.resolve();
1925+
1926+
expect(Scheduler).toFlushAndYield(['A', 'B', 'C']);
1927+
1928+
expect(ReactNoop).toMatchRenderedOutput(
1929+
<>
1930+
<span>A</span>
1931+
<span>B</span>
1932+
<span>C</span>
1933+
</>,
1934+
);
1935+
});
1936+
1937+
it('eventually resolves two nested forwards suspense list with a hidden tail', async () => {
1938+
let B = createAsyncText('B');
1939+
1940+
function Foo({showB}) {
1941+
return (
1942+
<SuspenseList revealOrder="forwards">
1943+
<SuspenseList revealOrder="forwards" tail="hidden">
1944+
<Suspense fallback={<Text text="Loading A" />}>
1945+
<Text text="A" />
1946+
</Suspense>
1947+
{showB ? (
1948+
<Suspense fallback={<Text text="Loading B" />}>
1949+
<B />
1950+
</Suspense>
1951+
) : null}
1952+
</SuspenseList>
1953+
<Suspense fallback={<Text text="Loading C" />}>
1954+
<Text text="C" />
1955+
</Suspense>
1956+
</SuspenseList>
1957+
);
1958+
}
1959+
1960+
ReactNoop.render(<Foo showB={false} />);
1961+
1962+
expect(Scheduler).toFlushAndYield(['A', 'C']);
1963+
1964+
expect(ReactNoop).toMatchRenderedOutput(
1965+
<>
1966+
<span>A</span>
1967+
<span>C</span>
1968+
</>,
1969+
);
1970+
1971+
// Showing the B later means that C has already committed
1972+
// so we're now effectively in "together" mode for the head.
1973+
ReactNoop.render(<Foo showB={true} />);
1974+
1975+
expect(Scheduler).toFlushAndYield([
1976+
'A',
1977+
'Suspend! [B]',
1978+
'Loading B',
1979+
'C',
1980+
'A',
1981+
'C',
1982+
]);
1983+
1984+
expect(ReactNoop).toMatchRenderedOutput(
1985+
<>
1986+
<span>A</span>
1987+
<span>C</span>
1988+
</>,
1989+
);
1990+
1991+
await B.resolve();
1992+
1993+
expect(Scheduler).toFlushAndYield(['B']);
1994+
1995+
expect(ReactNoop).toMatchRenderedOutput(
1996+
<>
1997+
<span>A</span>
1998+
<span>B</span>
1999+
<span>C</span>
2000+
</>,
2001+
);
2002+
});
2003+
18292004
it('can do unrelated adjacent updates', async () => {
18302005
let updateAdjacent;
18312006
function Adjacent() {

0 commit comments

Comments
 (0)