@samitouri / QOS-React-2 / commits / d5b54d0c35

[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.

Sebastian Markbåge committed Oct 14, 2019 at 09:42 UTC d5b54d0c35fa7c1b1e5989239478b365493965df
3 files changed +206 -17
packages/react-reconciler/src/ReactFiberBeginWork.js
+22 -11
@@ -2121,6 +2121,20 @@ function updateDehydratedSuspenseComponent(
2121 }
2122 }
2123
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 +
2138 function propagateSuspenseContextChange(
2139 workInProgress: Fiber,
2140 firstChild: null | Fiber,
@@ -2134,18 +2148,15 @@ function propagateSuspenseContextChange(
2148 if (node.tag === SuspenseComponent) {
2149 const state: SuspenseState | null = node.memoizedState;
2150 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);
2152 }
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);
2160 } else if (node.child !== null) {
2161 node.child.return = node;
2162 node = node.child;
packages/react-reconciler/src/ReactFiberCompleteWork.js
+9 -6
@@ -1083,6 +1083,15 @@ function completeWork(
1083 if (suspended !== null) {
1084 workInProgress.effectTag |= DidCapture;
1085 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 +
1095 cutOffTailIfNeeded(renderState, true);
1096 // This might have been modified.
1097 if (
@@ -1090,12 +1099,6 @@ function completeWork(
1099 renderState.tailMode === 'hidden'
1100 ) {
1101 // 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 - }
1102 // Reset the effect list to what it w as before we rendered this
1103 // child. The nested children have already appended themselves.
1104 let lastEffect = (workInProgress.lastEffect =
packages/react-reconciler/src/__tests__/ReactSuspenseList-test.internal.js
+175
@@ -1826,6 +1826,181 @@ describe('ReactSuspenseList', () => {
1826 );
1827 });
1828
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 +
2004 it('can do unrelated adjacent updates', async () => {
2005 let updateAdjacent;
2006 function Adjacent() {