Start prerendering Suspense retries immediately (facebook#30934)

When a component suspends and is replaced by a fallback, we should start prerendering the fallback immediately, even before any new data is received. During the retry, we can enter prerender mode directly if we're sure that no new data was received since we last attempted to render the boundary. To do this, when completing the fallback, we leave behind a pending retry lane on the Suspense boundary. Previously we only did this once a promise resolved, but by assigning a lane during the complete phase, we will know that there's speculative work to be done. Then, upon committing the fallback, we mark the retry lane as suspended — but only if nothing was pinged or updated in the meantime. That allows us to immediately enter prerender mode (i.e. render without skipping any siblings) when performing the retry.
jackpope · Sep 11, 2024 · d6cb4e7 · d6cb4e7
1 parent 1bb0563
commit d6cb4e7
Show file tree

Hide file tree

Showing 29 changed files with 1,906 additions and 462 deletions.
diff --git a/packages/react-cache/src/__tests__/ReactCacheOld-test.internal.js b/packages/react-cache/src/__tests__/ReactCacheOld-test.internal.js
@@ -18,6 +18,7 @@ let Suspense;
 let TextResource;
 let textResourceShouldFail;
 let waitForAll;
+let waitForPaint;
 let assertLog;
 let waitForThrow;
 let act;
@@ -37,6 +38,7 @@ describe('ReactCache', () => {
     waitForAll = InternalTestUtils.waitForAll;
     assertLog = InternalTestUtils.assertLog;
     waitForThrow = InternalTestUtils.waitForThrow;
+    waitForPaint = InternalTestUtils.waitForPaint;
     act = InternalTestUtils.act;
 
     TextResource = createResource(
@@ -119,7 +121,12 @@ describe('ReactCache', () => {
     const root = ReactNoop.createRoot();
     root.render(<App />);
 
-    await waitForAll(['Suspend! [Hi]', 'Loading...']);
+    await waitForAll([
+      'Suspend! [Hi]',
+      'Loading...',
+
+      ...(gate('enableSiblingPrerendering') ? ['Suspend! [Hi]'] : []),
+    ]);
 
     jest.advanceTimersByTime(100);
     assertLog(['Promise resolved [Hi]']);
@@ -138,7 +145,12 @@ describe('ReactCache', () => {
     const root = ReactNoop.createRoot();
     root.render(<App />);
 
-    await waitForAll(['Suspend! [Hi]', 'Loading...']);
+    await waitForAll([
+      'Suspend! [Hi]',
+      'Loading...',
+
+      ...(gate('enableSiblingPrerendering') ? ['Suspend! [Hi]'] : []),
+    ]);
 
     textResourceShouldFail = true;
     let error;
@@ -148,15 +160,7 @@ describe('ReactCache', () => {
       error = e;
     }
     expect(error.message).toMatch('Failed to load: Hi');
-    assertLog([
-      'Promise rejected [Hi]',
-      'Error! [Hi]',
-      'Error! [Hi]',
-
-      ...(gate('enableSiblingPrerendering')
-        ? ['Error! [Hi]', 'Error! [Hi]']
-        : []),
-    ]);
+    assertLog(['Promise rejected [Hi]', 'Error! [Hi]', 'Error! [Hi]']);
 
     // Should throw again on a subsequent read
     root.render(<App />);
@@ -187,15 +191,27 @@ describe('ReactCache', () => {
 
     if (__DEV__) {
       await expect(async () => {
-        await waitForAll(['App', 'Loading...']);
+        await waitForAll([
+          'App',
+          'Loading...',
+
+          ...(gate('enableSiblingPrerendering') ? ['App'] : []),
+        ]);
       }).toErrorDev([
         'Invalid key type. Expected a string, number, symbol, or ' +
           "boolean, but instead received: [ 'Hi', 100 ]\n\n" +
           'To use non-primitive values as keys, you must pass a hash ' +
           'function as the second argument to createResource().',
+
+        ...(gate('enableSiblingPrerendering') ? ['Invalid key type'] : []),
       ]);
     } else {
-      await waitForAll(['App', 'Loading...']);
+      await waitForAll([
+        'App',
+        'Loading...',
+
+        ...(gate('enableSiblingPrerendering') ? ['App'] : []),
+      ]);
     }
   });
 
@@ -212,13 +228,17 @@ describe('ReactCache', () => {
         <AsyncText ms={100} text={3} />
       </Suspense>,
     );
-    await waitForAll(['Suspend! [1]', 'Loading...']);
+    await waitForPaint(['Suspend! [1]', 'Loading...']);
     jest.advanceTimersByTime(100);
     assertLog(['Promise resolved [1]']);
-    await waitForAll([1, 'Suspend! [2]', 1, 'Suspend! [2]', 'Suspend! [3]']);
+    await waitForAll([1, 'Suspend! [2]']);
+
+    jest.advanceTimersByTime(100);
+    assertLog(['Promise resolved [2]']);
+    await waitForAll([1, 2, 'Suspend! [3]']);
 
     jest.advanceTimersByTime(100);
-    assertLog(['Promise resolved [2]', 'Promise resolved [3]']);
+    assertLog(['Promise resolved [3]']);
     await waitForAll([1, 2, 3]);
 
     await act(() => jest.advanceTimersByTime(100));
@@ -233,25 +253,18 @@ describe('ReactCache', () => {
       </Suspense>,
     );
 
-    await waitForAll([1, 'Suspend! [4]', 'Loading...']);
-
-    await act(() => jest.advanceTimersByTime(100));
-    assertLog([
-      'Promise resolved [4]',
-
+    await waitForAll([
       1,
-      4,
-      'Suspend! [5]',
+      'Suspend! [4]',
+      'Loading...',
       1,
-      4,
+      'Suspend! [4]',
       'Suspend! [5]',
-
-      'Promise resolved [5]',
-      1,
-      4,
-      5,
     ]);
 
+    await act(() => jest.advanceTimersByTime(100));
+    assertLog(['Promise resolved [4]', 'Promise resolved [5]', 1, 4, 5]);
+
     expect(root).toMatchRenderedOutput('145');
 
     // We've now rendered values 1, 2, 3, 4, 5, over our limit of 3. The least
@@ -271,24 +284,14 @@ describe('ReactCache', () => {
       // 2 and 3 suspend because they were evicted from the cache
       'Suspend! [2]',
       'Loading...',
-    ]);
-
-    await act(() => jest.advanceTimersByTime(100));
-    assertLog([
-      'Promise resolved [2]',
 
       1,
-      2,
-      'Suspend! [3]',
-      1,
-      2,
+      'Suspend! [2]',
       'Suspend! [3]',
-
-      'Promise resolved [3]',
-      1,
-      2,
-      3,
     ]);
+
+    await act(() => jest.advanceTimersByTime(100));
+    assertLog(['Promise resolved [2]', 'Promise resolved [3]', 1, 2, 3]);
     expect(root).toMatchRenderedOutput('123');
   });
 
@@ -363,7 +366,12 @@ describe('ReactCache', () => {
       </Suspense>,
     );
 
-    await waitForAll(['Suspend! [Hi]', 'Loading...']);
+    await waitForAll([
+      'Suspend! [Hi]',
+      'Loading...',
+
+      ...(gate('enableSiblingPrerendering') ? ['Suspend! [Hi]'] : []),
+    ]);
 
     resolveThenable('Hi');
     // This thenable improperly resolves twice. We should not update the

diff --git a/packages/react-devtools-shared/src/__tests__/TimelineProfiler-test.js b/packages/react-devtools-shared/src/__tests__/TimelineProfiler-test.js
@@ -15,6 +15,17 @@ import {
   normalizeCodeLocInfo,
 } from './utils';
 
+import {ReactVersion} from '../../../../ReactVersions';
+import semver from 'semver';
+
+// TODO: This is how other DevTools tests access the version but we should find
+// a better solution for this
+const ReactVersionTestingAgainst = process.env.REACT_VERSION || ReactVersion;
+const enableSiblingPrerendering = semver.gte(
+  ReactVersionTestingAgainst,
+  '19.0.0',
+);
+
 describe('Timeline profiler', () => {
   let React;
   let Scheduler;
@@ -1651,7 +1662,11 @@ describe('Timeline profiler', () => {
             </React.Suspense>,
           );
 
-          await waitForAll(['suspended']);
+          await waitForAll([
+            'suspended',
+
+            ...(enableSiblingPrerendering ? ['suspended'] : []),
+          ]);
 
           Scheduler.unstable_advanceTime(10);
           resolveFn();
@@ -1662,9 +1677,38 @@ describe('Timeline profiler', () => {
           const timelineData = stopProfilingAndGetTimelineData();
 
           // Verify the Suspense event and duration was recorded.
-          expect(timelineData.suspenseEvents).toHaveLength(1);
-          const suspenseEvent = timelineData.suspenseEvents[0];
-          expect(suspenseEvent).toMatchInlineSnapshot(`
+          if (enableSiblingPrerendering) {
+            expect(timelineData.suspenseEvents).toMatchInlineSnapshot(`
+              [
+                {
+                  "componentName": "Example",
+                  "depth": 0,
+                  "duration": 10,
+                  "id": "0",
+                  "phase": "mount",
+                  "promiseName": "",
+                  "resolution": "resolved",
+                  "timestamp": 10,
+                  "type": "suspense",
+                  "warning": null,
+                },
+                {
+                  "componentName": "Example",
+                  "depth": 0,
+                  "duration": 10,
+                  "id": "0",
+                  "phase": "mount",
+                  "promiseName": "",
+                  "resolution": "resolved",
+                  "timestamp": 10,
+                  "type": "suspense",
+                  "warning": null,
+                },
+              ]
+            `);
+          } else {
+            const suspenseEvent = timelineData.suspenseEvents[0];
+            expect(suspenseEvent).toMatchInlineSnapshot(`
                       {
                         "componentName": "Example",
                         "depth": 0,
@@ -1678,10 +1722,13 @@ describe('Timeline profiler', () => {
                         "warning": null,
                       }
                   `);
+          }
 
           // There should be two batches of renders: Suspeneded and resolved.
           expect(timelineData.batchUIDToMeasuresMap.size).toBe(2);
-          expect(timelineData.componentMeasures).toHaveLength(2);
+          expect(timelineData.componentMeasures).toHaveLength(
+            enableSiblingPrerendering ? 3 : 2,
+          );
         });
 
         it('should mark concurrent render with suspense that rejects', async () => {
@@ -1708,7 +1755,11 @@ describe('Timeline profiler', () => {
             </React.Suspense>,
           );
 
-          await waitForAll(['suspended']);
+          await waitForAll([
+            'suspended',
+
+            ...(enableSiblingPrerendering ? ['suspended'] : []),
+          ]);
 
           Scheduler.unstable_advanceTime(10);
           rejectFn();
@@ -1719,9 +1770,39 @@ describe('Timeline profiler', () => {
           const timelineData = stopProfilingAndGetTimelineData();
 
           // Verify the Suspense event and duration was recorded.
-          expect(timelineData.suspenseEvents).toHaveLength(1);
-          const suspenseEvent = timelineData.suspenseEvents[0];
-          expect(suspenseEvent).toMatchInlineSnapshot(`
+          if (enableSiblingPrerendering) {
+            expect(timelineData.suspenseEvents).toMatchInlineSnapshot(`
+              [
+                {
+                  "componentName": "Example",
+                  "depth": 0,
+                  "duration": 10,
+                  "id": "0",
+                  "phase": "mount",
+                  "promiseName": "",
+                  "resolution": "rejected",
+                  "timestamp": 10,
+                  "type": "suspense",
+                  "warning": null,
+                },
+                {
+                  "componentName": "Example",
+                  "depth": 0,
+                  "duration": 10,
+                  "id": "0",
+                  "phase": "mount",
+                  "promiseName": "",
+                  "resolution": "rejected",
+                  "timestamp": 10,
+                  "type": "suspense",
+                  "warning": null,
+                },
+              ]
+            `);
+          } else {
+            expect(timelineData.suspenseEvents).toHaveLength(1);
+            const suspenseEvent = timelineData.suspenseEvents[0];
+            expect(suspenseEvent).toMatchInlineSnapshot(`
                       {
                         "componentName": "Example",
                         "depth": 0,
@@ -1735,10 +1816,13 @@ describe('Timeline profiler', () => {
                         "warning": null,
                       }
                   `);
+          }
 
           // There should be two batches of renders: Suspeneded and resolved.
           expect(timelineData.batchUIDToMeasuresMap.size).toBe(2);
-          expect(timelineData.componentMeasures).toHaveLength(2);
+          expect(timelineData.componentMeasures).toHaveLength(
+            enableSiblingPrerendering ? 3 : 2,
+          );
         });
 
         it('should mark cascading class component state updates', async () => {

diff --git a/packages/react-dom/src/__tests__/ReactDOMForm-test.js b/packages/react-dom/src/__tests__/ReactDOMForm-test.js
@@ -1459,13 +1459,23 @@ describe('ReactDOMForm', () => {
         </Suspense>,
       ),
     );
-    assertLog(['Suspend! [Count: 0]', 'Loading...']);
+    assertLog([
+      'Suspend! [Count: 0]',
+      'Loading...',
+
+      ...(gate('enableSiblingPrerendering') ? ['Suspend! [Count: 0]'] : []),
+    ]);
     await act(() => resolveText('Count: 0'));
     assertLog(['Count: 0']);
 
     // Dispatch outside of a transition. This will trigger a loading state.
     await act(() => dispatch());
-    assertLog(['Suspend! [Count: 1]', 'Loading...']);
+    assertLog([
+      'Suspend! [Count: 1]',
+      'Loading...',
+
+      ...(gate('enableSiblingPrerendering') ? ['Suspend! [Count: 1]'] : []),
+    ]);
     expect(container.textContent).toBe('Loading...');
 
     await act(() => resolveText('Count: 1'));

diff --git a/packages/react-dom/src/__tests__/ReactDOMSuspensePlaceholder-test.js b/packages/react-dom/src/__tests__/ReactDOMSuspensePlaceholder-test.js
@@ -160,7 +160,13 @@ describe('ReactDOMSuspensePlaceholder', () => {
     });
 
     expect(container.textContent).toEqual('Loading...');
-    assertLog(['A', 'Suspend! [B]', 'Loading...']);
+    assertLog([
+      'A',
+      'Suspend! [B]',
+      'Loading...',
+
+      ...(gate('enableSiblingPrerendering') ? ['A', 'Suspend! [B]', 'C'] : []),
+    ]);
     await act(() => {
       resolveText('B');
     });