Repository: facebook/react
Pull request: [Fizz] Don't handle errors in completeBoundary instruction

Stacked on #33066 and #33068.

Currently we're passing `errorDigest` to `completeBoundary` if there is a client side error (only CSS loading atm). This only exists because of `completeBoundaryWithStyles`. Normally if there's a server-side error we'd emit the `clientRenderBoundary` instruction instead. This adds unnecessary code to the common case where all styles are in the head. This is about to get worse with batching because client render shouldn't be throttled but complete should be.

The first commit moves the client render logic inline into `completeBoundaryWithStyles` so we only pay for it when styles are used.

However, the approach I went with in the second commit is to reuse the `$RX` instruction instead (`clientRenderBoundary`). That way if you have both it ends up being amortized. However, it does mean we have to emit the `$RX` (along with the `$RC` helper if any `completeBoundaryWithStyles` instruction is needed.

Changed files:
filename: packages/react-dom-bindings/src/server/ReactFizzConfigDOM.js
patch: @@ -4482,14 +4482,14 @@ export function writeCompletedSegmentInstruction(
   }
 }
 
+const completeBoundaryScriptFunctionOnly = stringToPrecomputedChunk(
+  completeBoundaryFunction,
+);
 const completeBoundaryScript1Full = stringToPrecomputedChunk(
   completeBoundaryFunction + '$RC("',
 );
 const completeBoundaryScript1Partial = stringToPrecomputedChunk('$RC("');
 
-const completeBoundaryWithStylesScript1FullBoth = stringToPrecomputedChunk(
-  completeBoundaryFunction + styleInsertionFunction + '$RR("',
-);
 const completeBoundaryWithStylesScript1FullPartial = stringToPrecomputedChunk(
   styleInsertionFunction + '$RR("',
 );
@@ -4531,19 +4531,27 @@ export function writeCompletedBoundaryInstruction(
     writeChunk(destination, renderState.startInlineScript);
     writeChunk(destination, endOfStartTag);
     if (requiresStyleInsertion) {
+      if (
+        (resumableState.instructions & SentClientRenderFunction) ===
+        NothingSent
+      ) {
+        // The completeBoundaryWithStyles function depends on the client render function.
+        resumableState.instructions |= SentClientRenderFunction;
+        writeChunk(destination, clientRenderScriptFunctionOnly);
+      }
       if (
         (resumableState.instructions & SentCompleteBoundaryFunction) ===
         NothingSent
       ) {
-        resumableState.instructions |=
-          SentStyleInsertionFunction | SentCompleteBoundaryFunction;
-        writeChunk(destination, completeBoundaryWithStylesScript1FullBoth);
-      } else if (
+        // The completeBoundaryWithStyles function depends on the complete boundary function.
+        resumableState.instructions |= SentCompleteBoundaryFunction;
+        writeChunk(destination, completeBoundaryScriptFunctionOnly);
+      }
+      if (
         (resumableState.instructions & SentStyleInsertionFunction) ===
         NothingSent
       ) {
         resumableState.instructions |= SentStyleInsertionFunction;
-
         writeChunk(destination, completeBoundaryWithStylesScript1FullPartial);
       } else {
         writeChunk(destination, completeBoundaryWithStylesScript1Partial);
@@ -4608,6 +4616,9 @@ export function writeCompletedBoundaryInstruction(
   return writeBootstrap(destination, renderState) && writeMore;
 }
 
+const clientRenderScriptFunctionOnly =
+  stringToPrecomputedChunk(clientRenderFunction);
+
 const clientRenderScript1Full = stringToPrecomputedChunk(
   clientRenderFunction + ';$RX("',
 );

filename: packages/react-dom-bindings/src/server/fizz-instruction-set/ReactDOMFizzInstructionSetInlineCodeStrings.js
patch: @@ -4,9 +4,9 @@
 export const clientRenderBoundary =
   '$RX=function(b,c,d,e,f){var a=document.getElementById(b);a&&(b=a.previousSibling,b.data="$!",a=a.dataset,c&&(a.dgst=c),d&&(a.msg=d),e&&(a.stck=e),f&&(a.cstck=f),b._reactRetry&&b._reactRetry())};';
 export const completeBoundary =
-  '$RC=function(b,d,e){if(d=document.getElementById(d)){d.parentNode.removeChild(d);var a=document.getElementById(b);if(a){b=a.previousSibling;if(e)b.data="$!",a.setAttribute("data-dgst",e);else{e=b.parentNode;a=b.nextSibling;var f=0;do{if(a&&8===a.nodeType){var c=a.data;if("/$"===c||"/&"===c)if(0===f)break;else f--;else"$"!==c&&"$?"!==c&&"$!"!==c&&"&"!==c||f++}c=a.nextSibling;e.removeChild(a);a=c}while(a);for(;d.firstChild;)e.insertBefore(d.firstChild,a);b.data="$"}b._reactRetry&&b._reactRetry()}}};';
+  '$RC=function(a,d){if(d=document.getElementById(d))if(d.parentNode.removeChild(d),a=document.getElementById(a)){a=a.previousSibling;var f=a.parentNode,b=a.nextSibling,e=0;do{if(b&&8===b.nodeType){var c=b.data;if("/$"===c||"/&"===c)if(0===e)break;else e--;else"$"!==c&&"$?"!==c&&"$!"!==c&&"&"!==c||e++}c=b.nextSibling;f.removeChild(b);b=c}while(b);for(;d.firstChild;)f.insertBefore(d.firstChild,b);a.data="$";a._reactRetry&&a._reactRetry()}};';
 export const completeBoundaryWithStyles =
-  '$RM=new Map;\n$RR=function(r,t,w){function u(n){this._p=null;n()}for(var p=new Map,q=document,g,b,h=q.querySelectorAll("link[data-precedence],style[data-precedence]"),v=[],k=0;b=h[k++];)"not all"===b.getAttribute("media")?v.push(b):("LINK"===b.tagName&&$RM.set(b.getAttribute("href"),b),p.set(b.dataset.precedence,g=b));b=0;h=[];var l,a;for(k=!0;;){if(k){var e=w[b++];if(!e){k=!1;b=0;continue}var c=!1,m=0;var d=e[m++];if(a=$RM.get(d)){var f=a._p;c=!0}else{a=q.createElement("link");a.href=d;a.rel=\n"stylesheet";for(a.dataset.precedence=l=e[m++];f=e[m++];)a.setAttribute(f,e[m++]);f=a._p=new Promise(function(n,x){a.onload=u.bind(a,n);a.onerror=u.bind(a,x)});$RM.set(d,a)}d=a.getAttribute("media");!f||d&&!matchMedia(d).matches||h.push(f);if(c)continue}else{a=v[b++];if(!a)break;l=a.getAttribute("data-precedence");a.removeAttribute("media")}c=p.get(l)||g;c===g&&(g=a);p.set(l,a);c?c.parentNode.insertBefore(a,c.nextSibling):(c=q.head,c.insertBefore(a,c.firstChild))}Promise.all(h).then($RC.bind(null,\nr,t,""),$RC.bind(null,r,t,"Resource failed to load"))};';
+  '$RM=new Map;\n$RR=function(r,v,w){function t(n){this._p=null;n()}for(var p=new Map,q=document,g,b,h=q.querySelectorAll("link[data-precedence],style[data-precedence]"),u=[],k=0;b=h[k++];)"not all"===b.getAttribute("media")?u.push(b):("LINK"===b.tagName&&$RM.set(b.getAttribute("href"),b),p.set(b.dataset.precedence,g=b));b=0;h=[];var l,a;for(k=!0;;){if(k){var e=w[b++];if(!e){k=!1;b=0;continue}var c=!1,m=0;var d=e[m++];if(a=$RM.get(d)){var f=a._p;c=!0}else{a=q.createElement("link");a.href=d;a.rel=\n"stylesheet";for(a.dataset.precedence=l=e[m++];f=e[m++];)a.setAttribute(f,e[m++]);f=a._p=new Promise(function(n,x){a.onload=t.bind(a,n);a.onerror=t.bind(a,x)});$RM.set(d,a)}d=a.getAttribute("media");!f||d&&!matchMedia(d).matches||h.push(f);if(c)continue}else{a=u[b++];if(!a)break;l=a.getAttribute("data-precedence");a.removeAttribute("media")}c=p.get(l)||g;c===g&&(g=a);p.set(l,a);c?c.parentNode.insertBefore(a,c.nextSibling):(c=q.head,c.insertBefore(a,c.firstChild))}Promise.all(h).then($RC.bind(null,\nr,v),$RX.bind(null,r,"CSS failed to load"))};';
 export const completeSegment =
   '$RS=function(a,b){a=document.getElementById(a);b=document.getElementById(b);for(a.parentNode.removeChild(a);a.firstChild;)b.parentNode.insertBefore(a.firstChild,b);b.parentNode.removeChild(b)};';
 export const formReplaying =

filename: packages/react-dom-bindings/src/server/fizz-instruction-set/ReactDOMFizzInstructionSetShared.js
patch: @@ -47,7 +47,7 @@ export function clientRenderBoundary(
   }
 }
 
-export function completeBoundary(suspenseBoundaryID, contentID, errorDigest) {
+export function completeBoundary(suspenseBoundaryID, contentID) {
   const contentNode = document.getElementById(contentID);
   if (!contentNode) {
     // If the client has failed hydration we may have already deleted the streaming
@@ -70,52 +70,47 @@ export function completeBoundary(suspenseBoundaryID, contentID, errorDigest) {
   // Find the boundary around the fallback. This is always the previous node.
   const suspenseNode = suspenseIdNode.previousSibling;
 
-  if (!errorDigest) {
-    // Clear all the existing children. This is complicated because
-    // there can be embedded Suspense boundaries in the fallback.
-    // This is similar to clearSuspenseBoundary in ReactFiberConfigDOM.
-    // TODO: We could avoid this if we never emitted suspense boundaries in fallback trees.
-    // They never hydrate anyway. However, currently we support incrementally loading the fallback.
-    const parentInstance = suspenseNode.parentNode;
-    let node = suspenseNode.nextSibling;
-    let depth = 0;
-    do {
-      if (node && node.nodeType === COMMENT_NODE) {
-        const data = node.data;
-        if (data === SUSPENSE_END_DATA || data === ACTIVITY_END_DATA) {
-          if (depth === 0) {
-            break;
-          } else {
-            depth--;
-          }
-        } else if (
-          data === SUSPENSE_START_DATA ||
-          data === SUSPENSE_PENDING_START_DATA ||
-          data === SUSPENSE_FALLBACK_START_DATA ||
-          data === ACTIVITY_START_DATA
-        ) {
-          depth++;
+  // Clear all the existing children. This is complicated because
+  // there can be embedded Suspense boundaries in the fallback.
+  // This is similar to clearSuspenseBoundary in ReactFiberConfigDOM.
+  // TODO: We could avoid this if we never emitted suspense boundaries in fallback trees.
+  // They never hydrate anyway. However, currently we support incrementally loading the fallback.
+  const parentInstance = suspenseNode.parentNode;
+  let node = suspenseNode.nextSibling;
+  let depth = 0;
+  do {
+    if (node && node.nodeType === COMMENT_NODE) {
+      const data = node.data;
+      if (data === SUSPENSE_END_DATA || data === ACTIVITY_END_DATA) {
+        if (depth === 0) {
+          break;
+        } else {
+          depth--;
         }
+      } else if (
+        data === SUSPENSE_START_DATA ||
+        data === SUSPENSE_PENDING_START_DATA ||
+        data === SUSPENSE_FALLBACK_START_DATA ||
+        data === ACTIVITY_START_DATA
+      ) {
+        depth++;
       }
+    }
 
-      const nextNode = node.nextSibling;
-      parentInstance.removeChild(node);
-      node = nextNode;
-    } while (node);
-
-    const endOfBoundary = node;
+    const nextNode = node.nextSibling;
+    parentInstance.removeChild(node);
+    node = nextNode;
+  } while (node);
 
-    // Insert all the children from the contentNode between the start and end of suspense boundary.
-    while (contentNode.firstChild) {
-      parentInstance.insertBefore(contentNode.firstChild, endOfBoundary);
-    }
+  const endOfBoundary = node;
 
-    suspenseNode.data = SUSPENSE_START_DATA;
-  } else {
-    suspenseNode.data = SUSPENSE_FALLBACK_START_DATA;
-    suspenseIdNode.setAttribute('data-dgst', errorDigest);
+  // Insert all the children from the contentNode between the start and end of suspense boundary.
+  while (contentNode.firstChild) {
+    parentInstance.insertBefore(contentNode.firstChild, endOfBoundary);
   }
 
+  suspenseNode.data = SUSPENSE_START_DATA;
+
   if (suspenseNode['_reactRetry']) {
     suspenseNode['_reactRetry']();
   }
@@ -234,13 +229,8 @@ export function completeBoundaryWithStyles(
   }
 
   Promise.all(dependencies).then(
-    window['$RC'].bind(null, suspenseBoundaryID, contentID, ''),
-    window['$RC'].bind(
-      null,
-      suspenseBoundaryID,
-      contentID,
-      'Resource failed to load',
-    ),
+    window['$RC'].bind(null, suspenseBoundaryID, contentID),
+    window['$RX'].bind(null, suspenseBoundaryID, 'CSS failed to load'),
   );
 }
 

filename: packages/react-dom/src/__tests__/ReactDOMFizzStaticBrowser-test.js
patch: @@ -1630,14 +1630,14 @@ describe('ReactDOMFizzStaticBrowser', () => {
     // We are mostly just trying to assert that no preload for our stylesheet was emitted
     // prior to sending the segment the stylesheet was for. This test is asserting this
     // because the boundary complete instruction is sent when we are writing the
-    const instructionIndex = result.indexOf('$RC');
+    const instructionIndex = result.indexOf('$RX');
     expect(instructionIndex > -1).toBe(true);
-    const slice = result.slice(0, instructionIndex + '$RC'.length);
+    const slice = result.slice(0, instructionIndex + '$RX'.length);
 
     expect(slice).toBe(
       '<!DOCTYPE html><html><head><link rel="expect" href="#«R»" blocking="render"/></head>' +
         '<body>hello<!--$?--><template id="B:1"></template><!--/$--><template id="«R»"></template>' +
-        '<div hidden id="S:1">world<!-- --></div><script>$RC',
+        '<div hidden id="S:1">world<!-- --></div><script>$RX',
     );
   });
 

filename: packages/react-dom/src/__tests__/ReactDOMFloat-test.js
patch: @@ -1265,9 +1265,7 @@ body {
     const suspenseInstance = boundaryTemplateInstance.previousSibling;
 
     expect(suspenseInstance.data).toEqual('$!');
-    expect(boundaryTemplateInstance.dataset.dgst).toBe(
-      'Resource failed to load',
-    );
+    expect(boundaryTemplateInstance.dataset.dgst).toBe('CSS failed to load');
 
     expect(getMeaningfulChildren(document)).toEqual(
       <html>
@@ -1313,7 +1311,7 @@ body {
     );
     expect(errors).toEqual([
       'The server could not finish this Suspense boundary, likely due to an error during server rendering. Switched to client rendering.',
-      'Resource failed to load',
+      'CSS failed to load',
     ]);
   });