[Fiber] render boundary in fallback if it contains a new stylesheet during sync update

gnoff · gnoff · commit 1798c2c3329b · 2024-05-20T14:26:42.000-07:00
When we implemented Suspensey CSS we had a heuristic that if the update was sync we would ignore the loading states of any new stylesheets and just do the commit. But for a stylesheet capability to be useful it needs to reliably prevent FOUC and since the stylesheet api is opt-in through precedence we don't have to maintain backaward compat (old stylesheets do not block commit but then nobody really renders them because of FOUC anyway)

This update modifies the logic to put a boundary back into fallback if a sync update would lead to a stylesheet commiting before it loaded.
diff --git a/packages/react-art/src/ReactFiberConfigART.js b/packages/react-art/src/ReactFiberConfigART.js
@@ -471,8 +471,8 @@ export function maySuspendCommit(type, props) {
 }
 
 export function preloadInstance(type, props) {
-  // Return true to indicate it's already loaded
-  return true;
+  // Return 0 to indicate it's already loaded
+  return 0;
 }
 
 export function startSuspendingCommit() {}
diff --git a/packages/react-dom-bindings/src/client/ReactFiberConfigDOM.js b/packages/react-dom-bindings/src/client/ReactFiberConfigDOM.js
@@ -3153,21 +3153,24 @@ export function mayResourceSuspendCommit(resource: Resource): boolean {
   );
 }
 
-export function preloadInstance(type: Type, props: Props): boolean {
-  // Return true to indicate it's already loaded
-  return true;
+type RequiredTimeout = number;
+export function preloadInstance(type: Type, props: Props): RequiredTimeout {
+  // Return 0 to indicate it's already loaded
+  return 0;
 }
 
-export function preloadResource(resource: Resource): boolean {
+export function preloadResource(resource: Resource): RequiredTimeout {
   if (
     resource.type === 'stylesheet' &&
     (resource.state.loading & Settled) === NotLoaded
   ) {
-    // we have not finished loading the underlying stylesheet yet.
-    return false;
+    // Return Infinity to indicate it must still require loading
+    // and that React must wait forever
+    return Infinity;
   }
-  // Return true to indicate it's already loaded
-  return true;
+
+  // Return 0 to indicate it's already loaded
+  return 0;
 }
 
 type SuspendedState = {
diff --git a/packages/react-dom/src/__tests__/ReactDOMFloat-test.js b/packages/react-dom/src/__tests__/ReactDOMFloat-test.js
@@ -2946,25 +2946,8 @@ body {
       <link rel="preload" as="style" href="bar" />,
     ]);
 
-    // Try just this and crash all of Jest
     errorStylesheets(['bar']);
 
-    // // Try this and it fails the test when it shouldn't
-    // await act(() => {
-    //   errorStylesheets(['bar']);
-    // });
-
-    // // Try this there is nothing throwing here which is not really surprising since
-    // // the error is bubbling up through some kind of unhandled promise rejection thingy but
-    // // still I thought it was worth confirming
-    // try {
-    //   await act(() => {
-    //     errorStylesheets(['bar']);
-    //   });
-    // } catch (e) {
-    //   console.log(e);
-    // }
-
     loadStylesheets(['foo']);
     assertLog(['load stylesheet: foo', 'error stylesheet: bar']);
 
@@ -3197,6 +3180,131 @@ body {
     );
   });
 
+  it('will put a Suspense boundary into fallback if it contains a stylesheet not loaded during a sync update', async () => {
+    function App({children}) {
+      return (
+        <html>
+          <body>{children}</body>
+        </html>
+      );
+    }
+    const root = ReactDOMClient.createRoot(document);
+
+    await clientAct(() => {
+      root.render(<App />);
+    });
+    await waitForAll([]);
+
+    await clientAct(() => {
+      root.render(
+        <App>
+          <Suspense fallback="loading...">
+            <div>
+              hello
+              <link rel="stylesheet" href="foo" precedence="default" />
+            </div>
+          </Suspense>
+        </App>,
+      );
+    });
+    await waitForAll([]);
+
+    // Although the commit suspended, a preload was inserted.
+    expect(getMeaningfulChildren(document)).toEqual(
+      <html>
+        <head>
+          <link rel="preload" href="foo" as="style" />
+        </head>
+        <body>loading...</body>
+      </html>,
+    );
+
+    loadPreloads(['foo']);
+    assertLog(['load preload: foo']);
+    expect(getMeaningfulChildren(document)).toEqual(
+      <html>
+        <head>
+          <link rel="stylesheet" href="foo" data-precedence="default" />
+          <link rel="preload" href="foo" as="style" />
+        </head>
+        <body>loading...</body>
+      </html>,
+    );
+
+    loadStylesheets(['foo']);
+    assertLog(['load stylesheet: foo']);
+    expect(getMeaningfulChildren(document)).toEqual(
+      <html>
+        <head>
+          <link rel="stylesheet" href="foo" data-precedence="default" />
+          <link rel="preload" href="foo" as="style" />
+        </head>
+        <body>
+          <div>hello</div>
+        </body>
+      </html>,
+    );
+
+    await clientAct(() => {
+      root.render(
+        <App>
+          <Suspense fallback="loading...">
+            <div>
+              hello
+              <link rel="stylesheet" href="foo" precedence="default" />
+              <link rel="stylesheet" href="bar" precedence="default" />
+            </div>
+          </Suspense>
+        </App>,
+      );
+    });
+    await waitForAll([]);
+    expect(getMeaningfulChildren(document)).toEqual(
+      <html>
+        <head>
+          <link rel="stylesheet" href="foo" data-precedence="default" />
+          <link rel="preload" href="foo" as="style" />
+          <link rel="preload" href="bar" as="style" />
+        </head>
+        <body>
+          <div style="display: none;">hello</div>loading...
+        </body>
+      </html>,
+    );
+
+    loadPreloads(['bar']);
+    assertLog(['load preload: bar']);
+    expect(getMeaningfulChildren(document)).toEqual(
+      <html>
+        <head>
+          <link rel="stylesheet" href="foo" data-precedence="default" />
+          <link rel="stylesheet" href="bar" data-precedence="default" />
+          <link rel="preload" href="foo" as="style" />
+          <link rel="preload" href="bar" as="style" />
+        </head>
+        <body>
+          <div style="display: none;">hello</div>loading...
+        </body>
+      </html>,
+    );
+
+    loadStylesheets(['bar']);
+    assertLog(['load stylesheet: bar']);
+    expect(getMeaningfulChildren(document)).toEqual(
+      <html>
+        <head>
+          <link rel="stylesheet" href="foo" data-precedence="default" />
+          <link rel="stylesheet" href="bar" data-precedence="default" />
+          <link rel="preload" href="foo" as="style" />
+          <link rel="preload" href="bar" as="style" />
+        </head>
+        <body>
+          <div style="">hello</div>
+        </body>
+      </html>,
+    );
+  });
+
   it('can suspend commits on more than one root for the same resource at the same time', async () => {
     document.body.innerHTML = '';
     const container1 = document.createElement('div');
diff --git a/packages/react-native-renderer/src/ReactFiberConfigFabric.js b/packages/react-native-renderer/src/ReactFiberConfigFabric.js
@@ -501,8 +501,8 @@ export function maySuspendCommit(type: Type, props: Props): boolean {
   return false;
 }
 
-export function preloadInstance(type: Type, props: Props): boolean {
-  return true;
+export function preloadInstance(type: Type, props: Props): number {
+  return 0;
 }
 
 export function startSuspendingCommit(): void {}
diff --git a/packages/react-native-renderer/src/ReactFiberConfigNative.js b/packages/react-native-renderer/src/ReactFiberConfigNative.js
@@ -534,9 +534,9 @@ export function maySuspendCommit(type: Type, props: Props): boolean {
   return false;
 }
 
-export function preloadInstance(type: Type, props: Props): boolean {
-  // Return true to indicate it's already loaded
-  return true;
+export function preloadInstance(type: Type, props: Props): number {
+  // Return 0 to indicate it's already loaded
+  return 0;
 }
 
 export function startSuspendingCommit(): void {}
diff --git a/packages/react-noop-renderer/src/createReactNoop.js b/packages/react-noop-renderer/src/createReactNoop.js
@@ -586,11 +586,14 @@ function createReactNoop(reconciler: Function, useMutation: boolean) {
       );
     },
 
-    preloadInstance(type: string, props: Props): boolean {
+    preloadInstance(type: string, props: Props): number {
       if (type !== 'suspensey-thing' || typeof props.src !== 'string') {
         throw new Error('Attempted to preload unexpected instance: ' + type);
       }
 
+      const requiredTimeout =
+        typeof props.timeout === 'number' ? props.timeout : 100;
+
       // In addition to preloading an instance, this method asks whether the
       // instance is ready to be committed. If it's not, React may yield to the
       // main thread and ask again. It's possible a load event will fire in
@@ -609,14 +612,14 @@ function createReactNoop(reconciler: Function, useMutation: boolean) {
         if (typeof onLoadStart === 'function') {
           onLoadStart();
         }
-        return false;
+        return requiredTimeout;
       } else {
-        // If this is false, React will trigger a fallback, if needed.
-        return record.status === 'fulfilled';
+        // If this is zero React will not suspend the commit or trigger a fallback
+        return record.status === 'fulfilled' ? 0 : requiredTimeout;
       }
     },
 
-    preloadResource(resource: mixed): boolean {
+    preloadResource(resource: mixed): number {
       throw new Error(
         'Resources are not implemented for React Noop yet. This method should not be called',
       );
diff --git a/packages/react-reconciler/src/ReactFiberCompleteWork.js b/packages/react-reconciler/src/ReactFiberCompleteWork.js
@@ -128,6 +128,7 @@ import {
   setShallowSuspenseListContext,
   ForceSuspenseFallback,
   setDefaultShallowSuspenseListContext,
+  getSuspenseHandler,
 } from './ReactFiberSuspenseContext';
 import {popHiddenContext} from './ReactFiberHiddenContext';
 import {findFirstSuspended} from './ReactFiberSuspenseComponent';
@@ -534,33 +535,31 @@ function preloadInstanceAndSuspendIfNeeded(
   // loaded yet.
   workInProgress.flags |= MaySuspendCommit;
 
-  // Check if we're rendering at a "non-urgent" priority. This is the same
-  // check that `useDeferredValue` does to determine whether it needs to
-  // defer. This is partly for gradual adoption purposes (i.e. shouldn't start
-  // suspending until you opt in with startTransition or Suspense) but it
-  // also happens to be the desired behavior for the concrete use cases we've
-  // thought of so far, like CSS loading, fonts, images, etc.
-  //
+  // preload the instance if necessary. Even if this is an urgent render there
+  // could be benefits to preloading early.
+  // @TODO we should probably do the preload in begin work
+  const requiredTimeout = preloadInstance(type, props);
+
   // We check the "root" render lanes here rather than the "subtree" render
   // because during a retry or offscreen prerender, the "subtree" render
   // lanes may include additional "base" lanes that were deferred during
   // a previous render.
-  // TODO: We may decide to expose a way to force a fallback even during a
-  // sync update.
   const rootRenderLanes = getWorkInProgressRootRenderLanes();
   if (!includesOnlyNonUrgentLanes(rootRenderLanes)) {
-    // This is an urgent render. Don't suspend or show a fallback. Also,
-    // there's no need to preload, because we're going to commit this
-    // synchronously anyway.
-    // TODO: Could there be benefit to preloading even during a synchronous
-    // render? The main thread will be blocked until the commit phase, but
-    // maybe the browser would be able to start loading off thread anyway?
-    // Likely a micro-optimization either way because typically new content
-    // is loaded during a transition, not an urgent render.
+    // This is an urgent render. If our required timeout is infinite
+    // we suspend to allow the nearest fallback to commit instead.
+    // if the required timeout is finite we treat it like it is zero
+    // and allow the tree to commit as is.
+    if (requiredTimeout === Infinity) {
+      const nearestBoundary = getSuspenseHandler();
+      if (nearestBoundary !== null) {
+        // When there is a suspense boundary we can put into fallback we do
+        // If not we'll commit without waiting for the required resource
+        suspendCommit();
+      }
+    }
   } else {
-    // Preload the instance
-    const isReady = preloadInstance(type, props);
-    if (!isReady) {
+    if (requiredTimeout) {
       if (shouldRemainOnPreviousScreen()) {
         // It's OK to suspend. Mark the fiber so we know to suspend before the
         // commit phase. Then continue rendering.
@@ -588,12 +587,25 @@ function preloadResourceAndSuspendIfNeeded(
 
   workInProgress.flags |= MaySuspendCommit;
 
+  const requiredTimeout = preloadResource(resource);
   const rootRenderLanes = getWorkInProgressRootRenderLanes();
   if (!includesOnlyNonUrgentLanes(rootRenderLanes)) {
-    // This is an urgent render. Don't suspend or show a fallback.
+    // This is an urgent render. If our required timeout is infinite
+    // we suspend to allow the nearest fallback to commit instead.
+    // if the required timeout is finite we treat it like it is zero
+    // and allow the tree to commit as is.
+    if (requiredTimeout === Infinity) {
+      const nearestBoundary = getSuspenseHandler();
+      if (nearestBoundary !== null) {
+        // When there is a suspense boundary we can put into fallback we do
+        // If not we'll commit without waiting for the required resource
+        suspendCommit();
+      }
+    }
   } else {
-    const isReady = preloadResource(resource);
-    if (!isReady) {
+    // This is a transition. If the resource is not ready
+    // we can suspend to allow the nearest fallback to commit instead.
+    if (requiredTimeout) {
       if (shouldRemainOnPreviousScreen()) {
         workInProgress.flags |= ShouldSuspendCommit;
       } else {
diff --git a/packages/react-reconciler/src/ReactFiberWorkLoop.js b/packages/react-reconciler/src/ReactFiberWorkLoop.js
@@ -23,6 +23,7 @@ import type {
 } from './ReactFiberTracingMarkerComponent';
 import type {OffscreenInstance} from './ReactFiberActivityComponent';
 import type {RenderTaskFn} from './ReactFiberRootScheduler';
+import type {Resource} from './ReactFiberConfig';
 
 import {
   enableCreateEventHandleAPI,
@@ -75,6 +76,7 @@ import {
   startSuspendingCommit,
   waitForCommitToBeReady,
   preloadInstance,
+  preloadResource,
   supportsHydration,
   setCurrentUpdatePriority,
   getCurrentUpdatePriority,
@@ -2219,9 +2221,13 @@ function renderRootConcurrent(root: FiberRoot, lanes: Lanes) {
             break;
           }
           case SuspendedOnInstanceAndReadyToContinue: {
+            let resource: null | Resource = null;
             switch (workInProgress.tag) {
+              case HostHoistable: {
+                resource = workInProgress.memoizedState;
+              }
+              // intentional fallthrough
               case HostComponent:
-              case HostHoistable:
               case HostSingleton: {
                 // Before unwinding the stack, check one more time if the
                 // instance is ready. It may have loaded when React yielded to
@@ -2232,8 +2238,10 @@ function renderRootConcurrent(root: FiberRoot, lanes: Lanes) {
                 const hostFiber = workInProgress;
                 const type = hostFiber.type;
                 const props = hostFiber.pendingProps;
-                const isReady = preloadInstance(type, props);
-                if (isReady) {
+                const requiredTimeout = resource
+                  ? preloadResource(resource)
+                  : preloadInstance(type, props);
+                if (requiredTimeout === 0) {
                   // The data resolved. Resume the work loop as if nothing
                   // suspended. Unlike when a user component suspends, we don't
                   // have to replay anything because the host fiber
diff --git a/packages/react-reconciler/src/__tests__/ReactFiberHostContext-test.internal.js b/packages/react-reconciler/src/__tests__/ReactFiberHostContext-test.internal.js
diff --git a/packages/react-reconciler/src/__tests__/ReactSuspenseyCommitPhase-test.js b/packages/react-reconciler/src/__tests__/ReactSuspenseyCommitPhase-test.js
diff --git a/packages/react-test-renderer/src/ReactFiberConfigTestHost.js b/packages/react-test-renderer/src/ReactFiberConfigTestHost.js

Original file line number	Diff line number	Diff line change
`@@ -471,8 +471,8 @@ export function maySuspendCommit(type, props) {`
`471`	`471`	`}`
`472`	`472`
`473`	`473`	`export function preloadInstance(type, props) {`
`474`		`- // Return true to indicate it's already loaded`
`475`		`- return true;`
	`474`	`+ // Return 0 to indicate it's already loaded`
	`475`	`+ return 0;`
`476`	`476`	`}`
`477`	`477`
`478`	`478`	`export function startSuspendingCommit() {}`
Original file line number	Diff line number	Diff line change
`@@ -501,8 +501,8 @@ export function maySuspendCommit(type: Type, props: Props): boolean {`
`501`	`501`	`return false;`
`502`	`502`	`}`
`503`	`503`
`504`		`-export function preloadInstance(type: Type, props: Props): boolean {`
`505`		`- return true;`
	`504`	`+export function preloadInstance(type: Type, props: Props): number {`
	`505`	`+ return 0;`
`506`	`506`	`}`
`507`	`507`
`508`	`508`	`export function startSuspendingCommit(): void {}`
Original file line number	Diff line number	Diff line change
`@@ -534,9 +534,9 @@ export function maySuspendCommit(type: Type, props: Props): boolean {`
`534`	`534`	`return false;`
`535`	`535`	`}`
`536`	`536`
`537`		`-export function preloadInstance(type: Type, props: Props): boolean {`
`538`		`- // Return true to indicate it's already loaded`
`539`		`- return true;`
	`537`	`+export function preloadInstance(type: Type, props: Props): number {`
	`538`	`+ // Return 0 to indicate it's already loaded`
	`539`	`+ return 0;`
`540`	`540`	`}`
`541`	`541`
`542`	`542`	`export function startSuspendingCommit(): void {}`