[Fiber] render boundary in fallback if it contains a new stylesheet d…

…uring sync update 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.
facebook · May 1, 2024 · da0b480 · da0b480
1 parent 4508873
commit da0b480
Show file tree

Hide file tree

Showing 3 changed files with 152 additions and 30 deletions.
diff --git a/packages/react-dom-bindings/src/client/ReactFiberConfigDOM.js b/packages/react-dom-bindings/src/client/ReactFiberConfigDOM.js
@@ -3158,16 +3158,27 @@ export function preloadInstance(type: Type, props: Props): boolean {
  return true;
 }
 
-export function preloadResource(resource: Resource): boolean {
+export function preloadResource(resource: Resource): {
+ ready: boolean,
+ required: boolean,
+} {
+ let ready = true;
+ let required = false;
  if (
  resource.type === 'stylesheet' &&
  (resource.state.loading & Settled) === NotLoaded
  ) {
  // we have not finished loading the underlying stylesheet yet.
- return false;
+ ready = false;
+ required = true;
  }
- // Return true to indicate it's already loaded
- return true;
+
+ // It is important that we return this in tail position to ensure
+ // closure will elide the object when building the bundle
+ return {
+ ready,
+ required,
+ };
 }
 
 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-reconciler/src/ReactFiberCompleteWork.js b/packages/react-reconciler/src/ReactFiberCompleteWork.js
@@ -588,15 +588,18 @@ function preloadResourceAndSuspendIfNeeded(
 
  workInProgress.flags |= MaySuspendCommit;
 
- const rootRenderLanes = getWorkInProgressRootRenderLanes();
- if (!includesOnlyNonUrgentLanes(rootRenderLanes)) {
- // This is an urgent render. Don't suspend or show a fallback.
- } else {
- const isReady = preloadResource(resource);
- if (!isReady) {
- if (shouldRemainOnPreviousScreen()) {
- workInProgress.flags |= ShouldSuspendCommit;
- } else {
+ const {ready, required} = preloadResource(resource);
+ if (!ready) {
+ if (shouldRemainOnPreviousScreen()) {
+ // We can stay on the current screen until the resource is ready
+ // so we mark this fiber as suspending the commit
+ workInProgress.flags |= ShouldSuspendCommit;
+ } else {
+ const rootRenderLanes = getWorkInProgressRootRenderLanes();
+ if (!includesOnlyNonUrgentLanes(rootRenderLanes) && required) {
+ // We can't stay on the current screen so we suspend
+ // but this resource is required to show the boundary content
+ // so we suspend the boundary
  suspendCommit();
  }
  }