Skip to content

Commit cf49d3a

Browse files
gnoffgnoff
committed
[Float][Fiber] Assume stylesheets in document are already loaded
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 20841f9 commit cf49d3a

File tree

5 files changed

+214
-38
lines changed

5 files changed

+214
-38
lines changed

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

Lines changed: 31 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -2412,17 +2412,30 @@ export function getResource(
24122412
if (!resource) {
24132413
// We asserted this above but Flow can't figure out that the type satisfies
24142414
const ownerDocument = getDocumentFromRoot(resourceRoot);
2415-
resource = {
2415+
resource = ({
24162416
type: 'stylesheet',
24172417
instance: null,
24182418
count: 0,
24192419
state: {
24202420
loading: NotLoaded,
24212421
preload: null,
24222422
},
2423-
};
2423+
}: StylesheetResource);
24242424
styles.set(key, resource);
2425-
if (!preloadPropsMap.has(key)) {
2425+
const instance = ownerDocument.querySelector(
2426+
getStylesheetSelectorFromKey(key),
2427+
);
2428+
if (instance) {
2429+
const loadingState: ?Promise<mixed> = (instance: any)._p;
2430+
if (loadingState) {
2431+
// This instance is inserted as part of a boundary reveal and is not yet
2432+
// loaded
2433+
} else {
2434+
// This instance is already loaded
2435+
resource.instance = instance;
2436+
resource.state.loading = Loaded | Inserted;
2437+
}
2438+
} else if (!preloadPropsMap.has(key)) {
24262439
preloadStylesheet(
24272440
ownerDocument,
24282441
key,
@@ -2601,26 +2614,21 @@ function preloadStylesheet(
26012614
) {
26022615
preloadPropsMap.set(key, preloadProps);
26032616

2604-
if (!ownerDocument.querySelector(getStylesheetSelectorFromKey(key))) {
2605-
// There is no matching stylesheet instance in the Document.
2606-
// We will insert a preload now to kick off loading because
2607-
// we expect this stylesheet to commit
2608-
const preloadEl = ownerDocument.querySelector(
2609-
getPreloadStylesheetSelectorFromKey(key),
2610-
);
2611-
if (preloadEl) {
2612-
// If we find a preload already it was SSR'd and we won't have an actual
2613-
// loading state to track. For now we will just assume it is loaded
2614-
state.loading = Loaded;
2615-
} else {
2616-
const instance = ownerDocument.createElement('link');
2617-
state.preload = instance;
2618-
instance.addEventListener('load', () => (state.loading |= Loaded));
2619-
instance.addEventListener('error', () => (state.loading |= Errored));
2620-
setInitialProperties(instance, 'link', preloadProps);
2621-
markNodeAsHoistable(instance);
2622-
(ownerDocument.head: any).appendChild(instance);
2623-
}
2617+
const preloadEl = ownerDocument.querySelector(
2618+
getPreloadStylesheetSelectorFromKey(key),
2619+
);
2620+
if (preloadEl) {
2621+
// If we find a preload already it was SSR'd and we won't have an actual
2622+
// loading state to track. For now we will just assume it is loaded
2623+
state.loading = Loaded;
2624+
} else {
2625+
const instance = ownerDocument.createElement('link');
2626+
state.preload = instance;
2627+
instance.addEventListener('load', () => (state.loading |= Loaded));
2628+
instance.addEventListener('error', () => (state.loading |= Errored));
2629+
setInitialProperties(instance, 'link', preloadProps);
2630+
markNodeAsHoistable(instance);
2631+
(ownerDocument.head: any).appendChild(instance);
26242632
}
26252633
}
26262634

‎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+
delete this['_p'];
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+
delete this['_p'];
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)