Skip to content

Commit f85afe9

Browse files
gnoffgnoff
committed
[Float][Fiber] Assume stylesheets in document are already loaded (facebook#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.
1 parent f994737 commit f85afe9

File tree

5 files changed

+225
-45
lines changed

5 files changed

+225
-45
lines changed

‎packages/react-dom-bindings/src/client/ReactFiberConfigDOM.js

Lines changed: 42 additions & 30 deletions
Original file line numberDiff line numberDiff line change
@@ -2411,23 +2411,42 @@ export function getResource(
24112411
if (!resource) {
24122412
// We asserted this above but Flow can't figure out that the type satisfies
24132413
const ownerDocument = getDocumentFromRoot(resourceRoot);
2414-
resource = {
2414+
resource = ({
24152415
type: 'stylesheet',
24162416
instance: null,
24172417
count: 0,
24182418
state: {
24192419
loading: NotLoaded,
24202420
preload: null,
24212421
},
2422-
};
2422+
}: StylesheetResource);
24232423
styles.set(key, resource);
2424+
const instance = ownerDocument.querySelector(
2425+
getStylesheetSelectorFromKey(key),
2426+
);
2427+
if (instance) {
2428+
const loadingState: ?Promise<mixed> = (instance: any)._p;
2429+
if (loadingState) {
2430+
// This instance is inserted as part of a boundary reveal and is not yet
2431+
// loaded
2432+
} else {
2433+
// This instance is already loaded
2434+
resource.instance = instance;
2435+
resource.state.loading = Loaded | Inserted;
2436+
}
2437+
}
2438+
24242439
if (!preloadPropsMap.has(key)) {
2425-
preloadStylesheet(
2426-
ownerDocument,
2427-
key,
2428-
preloadPropsFromStylesheet(qualifiedProps),
2429-
resource.state,
2430-
);
2440+
const preloadProps = preloadPropsFromStylesheet(qualifiedProps);
2441+
preloadPropsMap.set(key, preloadProps);
2442+
if (!instance) {
2443+
preloadStylesheet(
2444+
ownerDocument,
2445+
key,
2446+
preloadProps,
2447+
resource.state,
2448+
);
2449+
}
24312450
}
24322451
}
24332452
return resource;
@@ -2520,28 +2539,21 @@ function preloadStylesheet(
25202539
preloadProps: PreloadProps,
25212540
state: StylesheetState,
25222541
) {
2523-
preloadPropsMap.set(key, preloadProps);
2524-
2525-
if (!ownerDocument.querySelector(getStylesheetSelectorFromKey(key))) {
2526-
// There is no matching stylesheet instance in the Document.
2527-
// We will insert a preload now to kick off loading because
2528-
// we expect this stylesheet to commit
2529-
const preloadEl = ownerDocument.querySelector(
2530-
getPreloadStylesheetSelectorFromKey(key),
2531-
);
2532-
if (preloadEl) {
2533-
// If we find a preload already it was SSR'd and we won't have an actual
2534-
// loading state to track. For now we will just assume it is loaded
2535-
state.loading = Loaded;
2536-
} else {
2537-
const instance = ownerDocument.createElement('link');
2538-
state.preload = instance;
2539-
instance.addEventListener('load', () => (state.loading |= Loaded));
2540-
instance.addEventListener('error', () => (state.loading |= Errored));
2541-
setInitialProperties(instance, 'link', preloadProps);
2542-
markNodeAsHoistable(instance);
2543-
(ownerDocument.head: any).appendChild(instance);
2544-
}
2542+
const preloadEl = ownerDocument.querySelector(
2543+
getPreloadStylesheetSelectorFromKey(key),
2544+
);
2545+
if (preloadEl) {
2546+
// If we find a preload already it was SSR'd and we won't have an actual
2547+
// loading state to track. For now we will just assume it is loaded
2548+
state.loading = Loaded;
2549+
} else {
2550+
const instance = ownerDocument.createElement('link');
2551+
state.preload = instance;
2552+
instance.addEventListener('load', () => (state.loading |= Loaded));
2553+
instance.addEventListener('error', () => (state.loading |= Errored));
2554+
setInitialProperties(instance, 'link', preloadProps);
2555+
markNodeAsHoistable(instance);
2556+
(ownerDocument.head: any).appendChild(instance);
25452557
}
25462558
}
25472559

‎packages/react-dom-bindings/src/server/fizz-instruction-set/ReactDOMFizzInstructionSetExternalRuntime.js

Lines changed: 8 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -45,6 +45,11 @@ export function completeBoundaryWithStyles(
4545
const dependencies = [];
4646
let href, precedence, attr, loadingState, resourceEl, media;
4747

48+
function cleanupWith(cb) {
49+
this['_p'] = null;
50+
cb();
51+
}
52+
4853
// Sheets Mode
4954
let sheetMode = true;
5055
while (true) {
@@ -80,18 +85,14 @@ export function completeBoundaryWithStyles(
8085
resourceEl.setAttribute(attr, stylesheetDescriptor[j++]);
8186
}
8287
loadingState = resourceEl['_p'] = new Promise((resolve, reject) => {
83-
resourceEl.onload = resolve;
84-
resourceEl.onerror = reject;
88+
resourceEl.onload = cleanupWith.bind(resourceEl, resolve);
89+
resourceEl.onerror = cleanupWith.bind(resourceEl, reject);
8590
});
8691
// Save this resource element so we can bailout if it is used again
8792
resourceMap.set(href, resourceEl);
8893
}
8994
media = resourceEl.getAttribute('media');
90-
if (
91-
loadingState &&
92-
loadingState['s'] !== 'l' &&
93-
(!media || window['matchMedia'](media).matches)
94-
) {
95+
if (loadingState && (!media || window['matchMedia'](media).matches)) {
9596
dependencies.push(loadingState);
9697
}
9798
if (avoidInsert) {

‎packages/react-dom-bindings/src/server/fizz-instruction-set/ReactDOMFizzInstructionSetInlineCodeStrings.js

Lines changed: 1 addition & 1 deletion
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎packages/react-dom-bindings/src/server/fizz-instruction-set/ReactDOMFizzInstructionSetInlineSource.js

Lines changed: 8 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -47,6 +47,11 @@ export function completeBoundaryWithStyles(
4747
const dependencies = [];
4848
let href, precedence, attr, loadingState, resourceEl, media;
4949

50+
function cleanupWith(cb) {
51+
this['_p'] = null;
52+
cb();
53+
}
54+
5055
// Sheets Mode
5156
let sheetMode = true;
5257
while (true) {
@@ -82,18 +87,14 @@ export function completeBoundaryWithStyles(
8287
resourceEl.setAttribute(attr, stylesheetDescriptor[j++]);
8388
}
8489
loadingState = resourceEl['_p'] = new Promise((resolve, reject) => {
85-
resourceEl.onload = resolve;
86-
resourceEl.onerror = reject;
90+
resourceEl.onload = cleanupWith.bind(resourceEl, resolve);
91+
resourceEl.onerror = cleanupWith.bind(resourceEl, reject);
8792
});
8893
// Save this resource element so we can bailout if it is used again
8994
resourceMap.set(href, resourceEl);
9095
}
9196
media = resourceEl.getAttribute('media');
92-
if (
93-
loadingState &&
94-
loadingState['s'] !== 'l' &&
95-
(!media || window['matchMedia'](media).matches)
96-
) {
97+
if (loadingState && (!media || window['matchMedia'](media).matches)) {
9798
dependencies.push(loadingState);
9899
}
99100
if (avoidInsert) {

‎packages/react-dom/src/__tests__/ReactDOMFloat-test.js

Lines changed: 166 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3348,6 +3348,172 @@ body {
33483348
);
33493349
});
33503350

3351+
it('will assume stylesheets already in the document have loaded if it cannot confirm it is not yet loaded', async () => {
3352+
await act(() => {
3353+
renderToPipeableStream(
3354+
<html>
3355+
<head>
3356+
<link rel="stylesheet" href="foo" data-precedence="default" />
3357+
</head>
3358+
<body>
3359+
<div id="foo" />
3360+
</body>
3361+
</html>,
3362+
).pipe(writable);
3363+
});
3364+
3365+
const root = ReactDOMClient.createRoot(document.querySelector('#foo'));
3366+
3367+
root.render(
3368+
<div>
3369+
<Suspense fallback="loading...">
3370+
<link rel="stylesheet" href="foo" precedence="default" />
3371+
hello world
3372+
</Suspense>
3373+
</div>,
3374+
);
3375+
3376+
await waitForAll([]);
3377+
expect(getMeaningfulChildren(document)).toEqual(
3378+
<html>
3379+
<head>
3380+
<link rel="stylesheet" href="foo" data-precedence="default" />
3381+
</head>
3382+
<body>
3383+
<div id="foo">
3384+
<div>hello world</div>
3385+
</div>
3386+
</body>
3387+
</html>,
3388+
);
3389+
});
3390+
3391+
it('will assume wait for loading stylesheets to load before continuing', async () => {
3392+
let ssr = true;
3393+
function Component() {
3394+
if (ssr) {
3395+
return null;
3396+
} else {
3397+
return (
3398+
<>
3399+
<link rel="stylesheet" href="foo" precedence="default" />
3400+
<div>hello client</div>
3401+
</>
3402+
);
3403+
}
3404+
}
3405+
3406+
await act(() => {
3407+
renderToPipeableStream(
3408+
<html>
3409+
<body>
3410+
<div>
3411+
<Suspense fallback="loading...">
3412+
<BlockedOn value="reveal">
3413+
<link rel="stylesheet" href="foo" precedence="default" />
3414+
<div>hello world</div>
3415+
</BlockedOn>
3416+
</Suspense>
3417+
</div>
3418+
<div>
3419+
<Suspense fallback="loading 2...">
3420+
<Component />
3421+
</Suspense>
3422+
</div>
3423+
</body>
3424+
</html>,
3425+
).pipe(writable);
3426+
});
3427+
3428+
expect(getMeaningfulChildren(document)).toEqual(
3429+
<html>
3430+
<head />
3431+
<body>
3432+
<div>loading...</div>
3433+
<div />
3434+
</body>
3435+
</html>,
3436+
);
3437+
3438+
await act(() => {
3439+
resolveText('reveal');
3440+
});
3441+
3442+
expect(getMeaningfulChildren(document)).toEqual(
3443+
<html>
3444+
<head>
3445+
<link rel="stylesheet" href="foo" data-precedence="default" />
3446+
</head>
3447+
<body>
3448+
<div>loading...</div>
3449+
<div />
3450+
<link rel="preload" href="foo" as="style" />
3451+
</body>
3452+
</html>,
3453+
);
3454+
3455+
ssr = false;
3456+
3457+
ReactDOMClient.hydrateRoot(
3458+
document,
3459+
<html>
3460+
<body>
3461+
<div>
3462+
<Suspense fallback="loading...">
3463+
<BlockedOn value="reveal">
3464+
<link rel="stylesheet" href="foo" precedence="default" />
3465+
<div>hello world</div>
3466+
</BlockedOn>
3467+
</Suspense>
3468+
</div>
3469+
<div>
3470+
<Suspense fallback="loading 2...">
3471+
<Component />
3472+
</Suspense>
3473+
</div>
3474+
</body>
3475+
</html>,
3476+
);
3477+
await waitForAll([]);
3478+
3479+
expect(getMeaningfulChildren(document)).toEqual(
3480+
<html>
3481+
<head>
3482+
<link rel="stylesheet" href="foo" data-precedence="default" />
3483+
</head>
3484+
<body>
3485+
<div>loading...</div>
3486+
<div />
3487+
<link rel="preload" href="foo" as="style" />
3488+
</body>
3489+
</html>,
3490+
);
3491+
3492+
await expect(async () => {
3493+
loadStylesheets();
3494+
}).toErrorDev([
3495+
"Hydration failed because the server rendered HTML didn't match the client.",
3496+
]);
3497+
assertLog(['load stylesheet: foo']);
3498+
3499+
expect(getMeaningfulChildren(document)).toEqual(
3500+
<html>
3501+
<head>
3502+
<link rel="stylesheet" href="foo" data-precedence="default" />
3503+
</head>
3504+
<body>
3505+
<div>
3506+
<div>hello world</div>
3507+
</div>
3508+
<div>
3509+
<div>hello client</div>
3510+
</div>
3511+
<link rel="preload" href="foo" as="style" />
3512+
</body>
3513+
</html>,
3514+
);
3515+
});
3516+
33513517
it('can suspend commits on more than one root for the same resource at the same time', async () => {
33523518
document.body.innerHTML = '';
33533519
const container1 = document.createElement('div');

0 commit comments

Comments
 (0)