Skip to content

Commit 946168d

Browse files
gnoffgnoff
committed
[Fiber] render boundary in fallback if it contains a new stylesheet during 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.
1 parent 9d76c95 commit 946168d

File tree

7 files changed

+180
-57
lines changed

7 files changed

+180
-57
lines changed

Diff for: ‎packages/react-art/src/ReactFiberConfigART.js

+2-2
Original file line numberDiff line numberDiff line change
@@ -471,8 +471,8 @@ export function maySuspendCommit(type, props) {
471471
}
472472

473473
export function preloadInstance(type, props) {
474-
// Return true to indicate it's already loaded
475-
return true;
474+
// Return 0 to indicate it's already loaded
475+
return 0;
476476
}
477477

478478
export function startSuspendingCommit() {}

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

+11-8
Original file line numberDiff line numberDiff line change
@@ -3153,21 +3153,24 @@ export function mayResourceSuspendCommit(resource: Resource): boolean {
31533153
);
31543154
}
31553155

3156-
export function preloadInstance(type: Type, props: Props): boolean {
3157-
// Return true to indicate it's already loaded
3158-
return true;
3156+
type RequiredTimeout = number;
3157+
export function preloadInstance(type: Type, props: Props): RequiredTimeout {
3158+
// Return 0 to indicate it's already loaded
3159+
return 0;
31593160
}
31603161

3161-
export function preloadResource(resource: Resource): boolean {
3162+
export function preloadResource(resource: Resource): RequiredTimeout {
31623163
if (
31633164
resource.type === 'stylesheet' &&
31643165
(resource.state.loading & Settled) === NotLoaded
31653166
) {
3166-
// we have not finished loading the underlying stylesheet yet.
3167-
return false;
3167+
// Return Infinity to indicate it must still require loading
3168+
// and that React must wait forever
3169+
return Infinity;
31683170
}
3169-
// Return true to indicate it's already loaded
3170-
return true;
3171+
3172+
// Return 0 to indicate it's already loaded
3173+
return 0;
31713174
}
31723175

31733176
type SuspendedState = {

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

+125-17
Original file line numberDiff line numberDiff line change
@@ -2946,25 +2946,8 @@ body {
29462946
<link rel="preload" as="style" href="bar" />,
29472947
]);
29482948

2949-
// Try just this and crash all of Jest
29502949
errorStylesheets(['bar']);
29512950

2952-
// // Try this and it fails the test when it shouldn't
2953-
// await act(() => {
2954-
// errorStylesheets(['bar']);
2955-
// });
2956-
2957-
// // Try this there is nothing throwing here which is not really surprising since
2958-
// // the error is bubbling up through some kind of unhandled promise rejection thingy but
2959-
// // still I thought it was worth confirming
2960-
// try {
2961-
// await act(() => {
2962-
// errorStylesheets(['bar']);
2963-
// });
2964-
// } catch (e) {
2965-
// console.log(e);
2966-
// }
2967-
29682951
loadStylesheets(['foo']);
29692952
assertLog(['load stylesheet: foo', 'error stylesheet: bar']);
29702953

@@ -3197,6 +3180,131 @@ body {
31973180
);
31983181
});
31993182

3183+
it('will put a Suspense boundary into fallback if it contains a stylesheet not loaded during a sync update', async () => {
3184+
function App({children}) {
3185+
return (
3186+
<html>
3187+
<body>{children}</body>
3188+
</html>
3189+
);
3190+
}
3191+
const root = ReactDOMClient.createRoot(document);
3192+
3193+
await clientAct(() => {
3194+
root.render(<App />);
3195+
});
3196+
await waitForAll([]);
3197+
3198+
await clientAct(() => {
3199+
root.render(
3200+
<App>
3201+
<Suspense fallback="loading...">
3202+
<div>
3203+
hello
3204+
<link rel="stylesheet" href="foo" precedence="default" />
3205+
</div>
3206+
</Suspense>
3207+
</App>,
3208+
);
3209+
});
3210+
await waitForAll([]);
3211+
3212+
// Although the commit suspended, a preload was inserted.
3213+
expect(getMeaningfulChildren(document)).toEqual(
3214+
<html>
3215+
<head>
3216+
<link rel="preload" href="foo" as="style" />
3217+
</head>
3218+
<body>loading...</body>
3219+
</html>,
3220+
);
3221+
3222+
loadPreloads(['foo']);
3223+
assertLog(['load preload: foo']);
3224+
expect(getMeaningfulChildren(document)).toEqual(
3225+
<html>
3226+
<head>
3227+
<link rel="stylesheet" href="foo" data-precedence="default" />
3228+
<link rel="preload" href="foo" as="style" />
3229+
</head>
3230+
<body>loading...</body>
3231+
</html>,
3232+
);
3233+
3234+
loadStylesheets(['foo']);
3235+
assertLog(['load stylesheet: foo']);
3236+
expect(getMeaningfulChildren(document)).toEqual(
3237+
<html>
3238+
<head>
3239+
<link rel="stylesheet" href="foo" data-precedence="default" />
3240+
<link rel="preload" href="foo" as="style" />
3241+
</head>
3242+
<body>
3243+
<div>hello</div>
3244+
</body>
3245+
</html>,
3246+
);
3247+
3248+
await clientAct(() => {
3249+
root.render(
3250+
<App>
3251+
<Suspense fallback="loading...">
3252+
<div>
3253+
hello
3254+
<link rel="stylesheet" href="foo" precedence="default" />
3255+
<link rel="stylesheet" href="bar" precedence="default" />
3256+
</div>
3257+
</Suspense>
3258+
</App>,
3259+
);
3260+
});
3261+
await waitForAll([]);
3262+
expect(getMeaningfulChildren(document)).toEqual(
3263+
<html>
3264+
<head>
3265+
<link rel="stylesheet" href="foo" data-precedence="default" />
3266+
<link rel="preload" href="foo" as="style" />
3267+
<link rel="preload" href="bar" as="style" />
3268+
</head>
3269+
<body>
3270+
<div style="display: none;">hello</div>loading...
3271+
</body>
3272+
</html>,
3273+
);
3274+
3275+
loadPreloads(['bar']);
3276+
assertLog(['load preload: bar']);
3277+
expect(getMeaningfulChildren(document)).toEqual(
3278+
<html>
3279+
<head>
3280+
<link rel="stylesheet" href="foo" data-precedence="default" />
3281+
<link rel="stylesheet" href="bar" data-precedence="default" />
3282+
<link rel="preload" href="foo" as="style" />
3283+
<link rel="preload" href="bar" as="style" />
3284+
</head>
3285+
<body>
3286+
<div style="display: none;">hello</div>loading...
3287+
</body>
3288+
</html>,
3289+
);
3290+
3291+
loadStylesheets(['bar']);
3292+
assertLog(['load stylesheet: bar']);
3293+
expect(getMeaningfulChildren(document)).toEqual(
3294+
<html>
3295+
<head>
3296+
<link rel="stylesheet" href="foo" data-precedence="default" />
3297+
<link rel="stylesheet" href="bar" data-precedence="default" />
3298+
<link rel="preload" href="foo" as="style" />
3299+
<link rel="preload" href="bar" as="style" />
3300+
</head>
3301+
<body>
3302+
<div style="">hello</div>
3303+
</body>
3304+
</html>,
3305+
);
3306+
});
3307+
32003308
it('can suspend commits on more than one root for the same resource at the same time', async () => {
32013309
document.body.innerHTML = '';
32023310
const container1 = document.createElement('div');

Diff for: ‎packages/react-native-renderer/src/ReactFiberConfigFabric.js

+1-1
Original file line numberDiff line numberDiff line change
@@ -502,7 +502,7 @@ export function maySuspendCommit(type: Type, props: Props): boolean {
502502
}
503503

504504
export function preloadInstance(type: Type, props: Props): boolean {
505-
return true;
505+
return 0;
506506
}
507507

508508
export function startSuspendingCommit(): void {}

Diff for: ‎packages/react-native-renderer/src/ReactFiberConfigNative.js

+3-3
Original file line numberDiff line numberDiff line change
@@ -534,9 +534,9 @@ export function maySuspendCommit(type: Type, props: Props): boolean {
534534
return false;
535535
}
536536

537-
export function preloadInstance(type: Type, props: Props): boolean {
538-
// Return true to indicate it's already loaded
539-
return true;
537+
export function preloadInstance(type: Type, props: Props): number {
538+
// Return 0 to indicate it's already loaded
539+
return 0;
540540
}
541541

542542
export function startSuspendingCommit(): void {}

Diff for: ‎packages/react-reconciler/src/ReactFiberCompleteWork.js

+35-23
Original file line numberDiff line numberDiff line change
@@ -128,6 +128,7 @@ import {
128128
setShallowSuspenseListContext,
129129
ForceSuspenseFallback,
130130
setDefaultShallowSuspenseListContext,
131+
getSuspenseHandler,
131132
} from './ReactFiberSuspenseContext';
132133
import {popHiddenContext} from './ReactFiberHiddenContext';
133134
import {findFirstSuspended} from './ReactFiberSuspenseComponent';
@@ -534,33 +535,31 @@ function preloadInstanceAndSuspendIfNeeded(
534535
// loaded yet.
535536
workInProgress.flags |= MaySuspendCommit;
536537

537-
// Check if we're rendering at a "non-urgent" priority. This is the same
538-
// check that `useDeferredValue` does to determine whether it needs to
539-
// defer. This is partly for gradual adoption purposes (i.e. shouldn't start
540-
// suspending until you opt in with startTransition or Suspense) but it
541-
// also happens to be the desired behavior for the concrete use cases we've
542-
// thought of so far, like CSS loading, fonts, images, etc.
543-
//
538+
// preload the instance if necessary. Even if this is an urgent render there
539+
// could be benefits to preloading early.
540+
// @TODO we should probably do the preload in begin work
541+
const requiredTimeout = preloadInstance(type, props);
542+
544543
// We check the "root" render lanes here rather than the "subtree" render
545544
// because during a retry or offscreen prerender, the "subtree" render
546545
// lanes may include additional "base" lanes that were deferred during
547546
// a previous render.
548-
// TODO: We may decide to expose a way to force a fallback even during a
549-
// sync update.
550547
const rootRenderLanes = getWorkInProgressRootRenderLanes();
551548
if (!includesOnlyNonUrgentLanes(rootRenderLanes)) {
552-
// This is an urgent render. Don't suspend or show a fallback. Also,
553-
// there's no need to preload, because we're going to commit this
554-
// synchronously anyway.
555-
// TODO: Could there be benefit to preloading even during a synchronous
556-
// render? The main thread will be blocked until the commit phase, but
557-
// maybe the browser would be able to start loading off thread anyway?
558-
// Likely a micro-optimization either way because typically new content
559-
// is loaded during a transition, not an urgent render.
549+
// This is an urgent render. If our required timeout is infinite
550+
// we suspend to allow the nearest fallback to commit instead.
551+
// if the required timeout is finite we treat it like it is zero
552+
// and allow the tree to commit as is.
553+
if (requiredTimeout === Infinity) {
554+
const nearestBoundary = getSuspenseHandler();
555+
if (nearestBoundary !== null) {
556+
// When there is a suspense boundary we can put into fallback we do
557+
// If not we'll commit without waiting for the required resource
558+
suspendCommit();
559+
}
560+
}
560561
} else {
561-
// Preload the instance
562-
const isReady = preloadInstance(type, props);
563-
if (!isReady) {
562+
if (requiredTimeout) {
564563
if (shouldRemainOnPreviousScreen()) {
565564
// It's OK to suspend. Mark the fiber so we know to suspend before the
566565
// commit phase. Then continue rendering.
@@ -588,12 +587,25 @@ function preloadResourceAndSuspendIfNeeded(
588587

589588
workInProgress.flags |= MaySuspendCommit;
590589

590+
const requiredTimeout = preloadResource(resource);
591591
const rootRenderLanes = getWorkInProgressRootRenderLanes();
592592
if (!includesOnlyNonUrgentLanes(rootRenderLanes)) {
593-
// This is an urgent render. Don't suspend or show a fallback.
593+
// This is an urgent render. If our required timeout is infinite
594+
// we suspend to allow the nearest fallback to commit instead.
595+
// if the required timeout is finite we treat it like it is zero
596+
// and allow the tree to commit as is.
597+
if (requiredTimeout === Infinity) {
598+
const nearestBoundary = getSuspenseHandler();
599+
if (nearestBoundary !== null) {
600+
// When there is a suspense boundary we can put into fallback we do
601+
// If not we'll commit without waiting for the required resource
602+
suspendCommit();
603+
}
604+
}
594605
} else {
595-
const isReady = preloadResource(resource);
596-
if (!isReady) {
606+
// This is a transition. If the resource is not ready
607+
// we can suspend to allow the nearest fallback to commit instead.
608+
if (requiredTimeout) {
597609
if (shouldRemainOnPreviousScreen()) {
598610
workInProgress.flags |= ShouldSuspendCommit;
599611
} else {

Diff for: ‎packages/react-test-renderer/src/ReactFiberConfigTestHost.js

+3-3
Original file line numberDiff line numberDiff line change
@@ -334,9 +334,9 @@ export function maySuspendCommit(type: Type, props: Props): boolean {
334334
return false;
335335
}
336336

337-
export function preloadInstance(type: Type, props: Props): boolean {
338-
// Return true to indicate it's already loaded
339-
return true;
337+
export function preloadInstance(type: Type, props: Props): number {
338+
// Return 0 to indicate it's already loaded
339+
return 0;
340340
}
341341

342342
export function startSuspendingCommit(): void {}

0 commit comments

Comments
 (0)