From f85afe903e623f454cb034c98f13fd5817f2f40b Mon Sep 17 00:00:00 2001 From: Josh Story Date: Fri, 7 Jun 2024 16:53:24 -0700 Subject: [PATCH] [Float][Fiber] Assume stylesheets in document are already loaded (#29811) When we made stylesheets suspend even during high priority updates we exposed a bug in the loading tracking of stylesheets that are loaded as part of the preamble. This allowed these stylesheets to put suspense boundaries into fallback mode more often than expected because cases where a stylesheet was server rendered could now cause a fallback to trigger which was never intended to happen. This fix updates resource construction to evaluate whether the instance exists in the DOM prior to construction and if so marks the resource as loaded and inserted. One ambiguity that needed to be solved still is how to tell whether a stylesheet rendered as part of a late Suspense boundary reveal is already loaded. I updated the instruction to clear out the loading promise after successfully loading. This is useful because later if we encounter this same resource again we can avoid the microtask if it is already loaded. It also means that we can concretely understand that if a stylesheet is in the DOM without this marker then it must have loaded (or errored) already. --- .../src/client/ReactFiberConfigDOM.js | 72 ++++---- ...actDOMFizzInstructionSetExternalRuntime.js | 15 +- ...tDOMFizzInstructionSetInlineCodeStrings.js | 2 +- .../ReactDOMFizzInstructionSetInlineSource.js | 15 +- .../src/__tests__/ReactDOMFloat-test.js | 166 ++++++++++++++++++ 5 files changed, 225 insertions(+), 45 deletions(-) diff --git a/packages/react-dom-bindings/src/client/ReactFiberConfigDOM.js b/packages/react-dom-bindings/src/client/ReactFiberConfigDOM.js index 2fcf5bf9a57ff..7dd5b923bcc23 100644 --- a/packages/react-dom-bindings/src/client/ReactFiberConfigDOM.js +++ b/packages/react-dom-bindings/src/client/ReactFiberConfigDOM.js @@ -2411,7 +2411,7 @@ export function getResource( if (!resource) { // We asserted this above but Flow can't figure out that the type satisfies const ownerDocument = getDocumentFromRoot(resourceRoot); - resource = { + resource = ({ type: 'stylesheet', instance: null, count: 0, @@ -2419,15 +2419,34 @@ export function getResource( loading: NotLoaded, preload: null, }, - }; + }: StylesheetResource); styles.set(key, resource); + const instance = ownerDocument.querySelector( + getStylesheetSelectorFromKey(key), + ); + if (instance) { + const loadingState: ?Promise = (instance: any)._p; + if (loadingState) { + // This instance is inserted as part of a boundary reveal and is not yet + // loaded + } else { + // This instance is already loaded + resource.instance = instance; + resource.state.loading = Loaded | Inserted; + } + } + if (!preloadPropsMap.has(key)) { - preloadStylesheet( - ownerDocument, - key, - preloadPropsFromStylesheet(qualifiedProps), - resource.state, - ); + const preloadProps = preloadPropsFromStylesheet(qualifiedProps); + preloadPropsMap.set(key, preloadProps); + if (!instance) { + preloadStylesheet( + ownerDocument, + key, + preloadProps, + resource.state, + ); + } } } return resource; @@ -2520,28 +2539,21 @@ function preloadStylesheet( preloadProps: PreloadProps, state: StylesheetState, ) { - preloadPropsMap.set(key, preloadProps); - - if (!ownerDocument.querySelector(getStylesheetSelectorFromKey(key))) { - // There is no matching stylesheet instance in the Document. - // We will insert a preload now to kick off loading because - // we expect this stylesheet to commit - const preloadEl = ownerDocument.querySelector( - getPreloadStylesheetSelectorFromKey(key), - ); - if (preloadEl) { - // If we find a preload already it was SSR'd and we won't have an actual - // loading state to track. For now we will just assume it is loaded - state.loading = Loaded; - } else { - const instance = ownerDocument.createElement('link'); - state.preload = instance; - instance.addEventListener('load', () => (state.loading |= Loaded)); - instance.addEventListener('error', () => (state.loading |= Errored)); - setInitialProperties(instance, 'link', preloadProps); - markNodeAsHoistable(instance); - (ownerDocument.head: any).appendChild(instance); - } + const preloadEl = ownerDocument.querySelector( + getPreloadStylesheetSelectorFromKey(key), + ); + if (preloadEl) { + // If we find a preload already it was SSR'd and we won't have an actual + // loading state to track. For now we will just assume it is loaded + state.loading = Loaded; + } else { + const instance = ownerDocument.createElement('link'); + state.preload = instance; + instance.addEventListener('load', () => (state.loading |= Loaded)); + instance.addEventListener('error', () => (state.loading |= Errored)); + setInitialProperties(instance, 'link', preloadProps); + markNodeAsHoistable(instance); + (ownerDocument.head: any).appendChild(instance); } } diff --git a/packages/react-dom-bindings/src/server/fizz-instruction-set/ReactDOMFizzInstructionSetExternalRuntime.js b/packages/react-dom-bindings/src/server/fizz-instruction-set/ReactDOMFizzInstructionSetExternalRuntime.js index 419817b52e1f3..5e632aa36d48e 100644 --- a/packages/react-dom-bindings/src/server/fizz-instruction-set/ReactDOMFizzInstructionSetExternalRuntime.js +++ b/packages/react-dom-bindings/src/server/fizz-instruction-set/ReactDOMFizzInstructionSetExternalRuntime.js @@ -45,6 +45,11 @@ export function completeBoundaryWithStyles( const dependencies = []; let href, precedence, attr, loadingState, resourceEl, media; + function cleanupWith(cb) { + this['_p'] = null; + cb(); + } + // Sheets Mode let sheetMode = true; while (true) { @@ -80,18 +85,14 @@ export function completeBoundaryWithStyles( resourceEl.setAttribute(attr, stylesheetDescriptor[j++]); } loadingState = resourceEl['_p'] = new Promise((resolve, reject) => { - resourceEl.onload = resolve; - resourceEl.onerror = reject; + resourceEl.onload = cleanupWith.bind(resourceEl, resolve); + resourceEl.onerror = cleanupWith.bind(resourceEl, reject); }); // Save this resource element so we can bailout if it is used again resourceMap.set(href, resourceEl); } media = resourceEl.getAttribute('media'); - if ( - loadingState && - loadingState['s'] !== 'l' && - (!media || window['matchMedia'](media).matches) - ) { + if (loadingState && (!media || window['matchMedia'](media).matches)) { dependencies.push(loadingState); } if (avoidInsert) { diff --git a/packages/react-dom-bindings/src/server/fizz-instruction-set/ReactDOMFizzInstructionSetInlineCodeStrings.js b/packages/react-dom-bindings/src/server/fizz-instruction-set/ReactDOMFizzInstructionSetInlineCodeStrings.js index 1b0ba47378357..6e47c3b0658e3 100644 --- a/packages/react-dom-bindings/src/server/fizz-instruction-set/ReactDOMFizzInstructionSetInlineCodeStrings.js +++ b/packages/react-dom-bindings/src/server/fizz-instruction-set/ReactDOMFizzInstructionSetInlineCodeStrings.js @@ -6,7 +6,7 @@ export const clientRenderBoundary = export const completeBoundary = '$RC=function(b,c,e){c=document.getElementById(c);c.parentNode.removeChild(c);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 d=a.data;if("/$"===d)if(0===f)break;else f--;else"$"!==d&&"$?"!==d&&"$!"!==d||f++}d=a.nextSibling;e.removeChild(a);a=d}while(a);for(;c.firstChild;)e.insertBefore(c.firstChild,a);b.data="$"}b._reactRetry&&b._reactRetry()}};'; export const completeBoundaryWithStyles = - '$RM=new Map;\n$RR=function(r,t,w){for(var u=$RC,n=$RM,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&&n.set(b.getAttribute("href"),b),p.set(b.dataset.precedence,g=b));b=0;h=[];var l,a;for(k=!0;;){if(k){var f=w[b++];if(!f){k=!1;b=0;continue}var c=!1,m=0;var d=f[m++];if(a=n.get(d)){var e=a._p;c=!0}else{a=q.createElement("link");a.href=d;a.rel="stylesheet";for(a.dataset.precedence=\nl=f[m++];e=f[m++];)a.setAttribute(e,f[m++]);e=a._p=new Promise(function(x,y){a.onload=x;a.onerror=y});n.set(d,a)}d=a.getAttribute("media");!e||"l"===e.s||d&&!matchMedia(d).matches||h.push(e);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(u.bind(null,r,t,""),u.bind(null,r,t,"Resource failed to load"))};'; + '$RM=new Map;\n$RR=function(t,u,y){function v(n){this._p=null;n()}for(var w=$RC,p=$RM,q=new Map,r=document,g,b,h=r.querySelectorAll("link[data-precedence],style[data-precedence]"),x=[],k=0;b=h[k++];)"not all"===b.getAttribute("media")?x.push(b):("LINK"===b.tagName&&p.set(b.getAttribute("href"),b),q.set(b.dataset.precedence,g=b));b=0;h=[];var l,a;for(k=!0;;){if(k){var e=y[b++];if(!e){k=!1;b=0;continue}var c=!1,m=0;var d=e[m++];if(a=p.get(d)){var f=a._p;c=!0}else{a=r.createElement("link");a.href=\nd;a.rel="stylesheet";for(a.dataset.precedence=l=e[m++];f=e[m++];)a.setAttribute(f,e[m++]);f=a._p=new Promise(function(n,z){a.onload=v.bind(a,n);a.onerror=v.bind(a,z)});p.set(d,a)}d=a.getAttribute("media");!f||d&&!matchMedia(d).matches||h.push(f);if(c)continue}else{a=x[b++];if(!a)break;l=a.getAttribute("data-precedence");a.removeAttribute("media")}c=q.get(l)||g;c===g&&(g=a);q.set(l,a);c?c.parentNode.insertBefore(a,c.nextSibling):(c=r.head,c.insertBefore(a,c.firstChild))}Promise.all(h).then(w.bind(null,\nt,u,""),w.bind(null,t,u,"Resource 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 = diff --git a/packages/react-dom-bindings/src/server/fizz-instruction-set/ReactDOMFizzInstructionSetInlineSource.js b/packages/react-dom-bindings/src/server/fizz-instruction-set/ReactDOMFizzInstructionSetInlineSource.js index 511ee1db3fd9a..abf17903e3e83 100644 --- a/packages/react-dom-bindings/src/server/fizz-instruction-set/ReactDOMFizzInstructionSetInlineSource.js +++ b/packages/react-dom-bindings/src/server/fizz-instruction-set/ReactDOMFizzInstructionSetInlineSource.js @@ -47,6 +47,11 @@ export function completeBoundaryWithStyles( const dependencies = []; let href, precedence, attr, loadingState, resourceEl, media; + function cleanupWith(cb) { + this['_p'] = null; + cb(); + } + // Sheets Mode let sheetMode = true; while (true) { @@ -82,18 +87,14 @@ export function completeBoundaryWithStyles( resourceEl.setAttribute(attr, stylesheetDescriptor[j++]); } loadingState = resourceEl['_p'] = new Promise((resolve, reject) => { - resourceEl.onload = resolve; - resourceEl.onerror = reject; + resourceEl.onload = cleanupWith.bind(resourceEl, resolve); + resourceEl.onerror = cleanupWith.bind(resourceEl, reject); }); // Save this resource element so we can bailout if it is used again resourceMap.set(href, resourceEl); } media = resourceEl.getAttribute('media'); - if ( - loadingState && - loadingState['s'] !== 'l' && - (!media || window['matchMedia'](media).matches) - ) { + if (loadingState && (!media || window['matchMedia'](media).matches)) { dependencies.push(loadingState); } if (avoidInsert) { diff --git a/packages/react-dom/src/__tests__/ReactDOMFloat-test.js b/packages/react-dom/src/__tests__/ReactDOMFloat-test.js index 43926bfa160fe..01a6d6f13727f 100644 --- a/packages/react-dom/src/__tests__/ReactDOMFloat-test.js +++ b/packages/react-dom/src/__tests__/ReactDOMFloat-test.js @@ -3348,6 +3348,172 @@ body { ); }); + it('will assume stylesheets already in the document have loaded if it cannot confirm it is not yet loaded', async () => { + await act(() => { + renderToPipeableStream( + + + + + +
+ + , + ).pipe(writable); + }); + + const root = ReactDOMClient.createRoot(document.querySelector('#foo')); + + root.render( +
+ + + hello world + +
, + ); + + await waitForAll([]); + expect(getMeaningfulChildren(document)).toEqual( + + + + + +
+
hello world
+
+ + , + ); + }); + + it('will assume wait for loading stylesheets to load before continuing', async () => { + let ssr = true; + function Component() { + if (ssr) { + return null; + } else { + return ( + <> + +
hello client
+ + ); + } + } + + await act(() => { + renderToPipeableStream( + + +
+ + + +
hello world
+ + +
+
+ + + +
+ + , + ).pipe(writable); + }); + + expect(getMeaningfulChildren(document)).toEqual( + + + +
loading...
+
+ + , + ); + + await act(() => { + resolveText('reveal'); + }); + + expect(getMeaningfulChildren(document)).toEqual( + + + + + +
loading...
+
+ + + , + ); + + ssr = false; + + ReactDOMClient.hydrateRoot( + document, + + +
+ + + +
hello world
+ + +
+
+ + + +
+ + , + ); + await waitForAll([]); + + expect(getMeaningfulChildren(document)).toEqual( + + + + + +
loading...
+
+ + + , + ); + + await expect(async () => { + loadStylesheets(); + }).toErrorDev([ + "Hydration failed because the server rendered HTML didn't match the client.", + ]); + assertLog(['load stylesheet: foo']); + + expect(getMeaningfulChildren(document)).toEqual( + + + + + +
+
hello world
+
+
+
hello client
+
+ + + , + ); + }); + 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');