Skip to content

Commit a998552

Browse files
gnoffgnoff
authored
[Fizz] Do not reinsert stylesheets after initial insert (#27586)
The loading state tracking for suspensey CSS is too complicated. Prior to this change it had a state it could enter into where a stylesheet was already in the DOM but the loading state did not know it was inserted causing a later transition to try to insert it again. This fix is to add proper tracking of insertions on the codepaths that were missing it. It also modifies the logic of when to suspend based on whether the stylesheet has already been inserted or not. This is not 100% correct semantics however because a prior commit could have inserted a stylesheet and a later transition should ideally be able to wait on that load before committing. I haven't attempted to fix this yet however because the loading state tracking is too complicated as it is and requires a more thorough refactor. Additionally it's not particularly valuable to delay a transition on a loading stylesheet when a previous commit also relied on that stylesheet but didn't wait for it b/c it was sync. I will follow up with an improvement PR later fixes: #27585
1 parent 51ffd35 commit a998552

File tree

2 files changed

+141
-60
lines changed

2 files changed

+141
-60
lines changed

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

Lines changed: 63 additions & 60 deletions
Original file line numberDiff line numberDiff line change
@@ -2337,7 +2337,7 @@ function preinitStyle(
23372337
getStylesheetSelectorFromKey(key),
23382338
);
23392339
if (instance) {
2340-
state.loading = Loaded;
2340+
state.loading = Loaded & Inserted;
23412341
} else {
23422342
// Construct a new instance and insert it
23432343
const stylesheetProps = Object.assign(
@@ -2769,6 +2769,7 @@ export function acquireResource(
27692769
getStylesheetSelectorFromKey(key),
27702770
);
27712771
if (instance) {
2772+
resource.state.loading |= Inserted;
27722773
resource.instance = instance;
27732774
markNodeAsHoistable(instance);
27742775
return instance;
@@ -3360,71 +3361,73 @@ export function suspendResource(
33603361
return;
33613362
}
33623363
}
3363-
if (resource.instance === null) {
3364-
const qualifiedProps: StylesheetQualifyingProps = props;
3365-
const key = getStyleKey(qualifiedProps.href);
3364+
if ((resource.state.loading & Inserted) === NotLoaded) {
3365+
if (resource.instance === null) {
3366+
const qualifiedProps: StylesheetQualifyingProps = props;
3367+
const key = getStyleKey(qualifiedProps.href);
33663368

3367-
// Attempt to hydrate instance from DOM
3368-
let instance: null | Instance = hoistableRoot.querySelector(
3369-
getStylesheetSelectorFromKey(key),
3370-
);
3371-
if (instance) {
3372-
// If this instance has a loading state it came from the Fizz runtime.
3373-
// If there is not loading state it is assumed to have been server rendered
3374-
// as part of the preamble and therefore synchronously loaded. It could have
3375-
// errored however which we still do not yet have a means to detect. For now
3376-
// we assume it is loaded.
3377-
const maybeLoadingState: ?Promise<mixed> = (instance: any)._p;
3378-
if (
3379-
maybeLoadingState !== null &&
3380-
typeof maybeLoadingState === 'object' &&
3381-
// $FlowFixMe[method-unbinding]
3382-
typeof maybeLoadingState.then === 'function'
3383-
) {
3384-
const loadingState = maybeLoadingState;
3385-
state.count++;
3386-
const ping = onUnsuspend.bind(state);
3387-
loadingState.then(ping, ping);
3369+
// Attempt to hydrate instance from DOM
3370+
let instance: null | Instance = hoistableRoot.querySelector(
3371+
getStylesheetSelectorFromKey(key),
3372+
);
3373+
if (instance) {
3374+
// If this instance has a loading state it came from the Fizz runtime.
3375+
// If there is not loading state it is assumed to have been server rendered
3376+
// as part of the preamble and therefore synchronously loaded. It could have
3377+
// errored however which we still do not yet have a means to detect. For now
3378+
// we assume it is loaded.
3379+
const maybeLoadingState: ?Promise<mixed> = (instance: any)._p;
3380+
if (
3381+
maybeLoadingState !== null &&
3382+
typeof maybeLoadingState === 'object' &&
3383+
// $FlowFixMe[method-unbinding]
3384+
typeof maybeLoadingState.then === 'function'
3385+
) {
3386+
const loadingState = maybeLoadingState;
3387+
state.count++;
3388+
const ping = onUnsuspend.bind(state);
3389+
loadingState.then(ping, ping);
3390+
}
3391+
resource.state.loading |= Inserted;
3392+
resource.instance = instance;
3393+
markNodeAsHoistable(instance);
3394+
return;
33883395
}
3389-
resource.state.loading |= Inserted;
3390-
resource.instance = instance;
3391-
markNodeAsHoistable(instance);
3392-
return;
3393-
}
33943396

3395-
const ownerDocument = getDocumentFromRoot(hoistableRoot);
3396-
3397-
const stylesheetProps = stylesheetPropsFromRawProps(props);
3398-
const preloadProps = preloadPropsMap.get(key);
3399-
if (preloadProps) {
3400-
adoptPreloadPropsForStylesheet(stylesheetProps, preloadProps);
3401-
}
3397+
const ownerDocument = getDocumentFromRoot(hoistableRoot);
34023398

3403-
// Construct and insert a new instance
3404-
instance = ownerDocument.createElement('link');
3405-
markNodeAsHoistable(instance);
3406-
const linkInstance: HTMLLinkElement = (instance: any);
3407-
// This Promise is a loading state used by the Fizz runtime. We need this incase there is a race
3408-
// between this resource being rendered on the client and being rendered with a late completed boundary.
3409-
(linkInstance: any)._p = new Promise((resolve, reject) => {
3410-
linkInstance.onload = resolve;
3411-
linkInstance.onerror = reject;
3412-
});
3413-
setInitialProperties(instance, 'link', stylesheetProps);
3414-
resource.instance = instance;
3415-
}
3399+
const stylesheetProps = stylesheetPropsFromRawProps(props);
3400+
const preloadProps = preloadPropsMap.get(key);
3401+
if (preloadProps) {
3402+
adoptPreloadPropsForStylesheet(stylesheetProps, preloadProps);
3403+
}
34163404

3417-
if (state.stylesheets === null) {
3418-
state.stylesheets = new Map();
3419-
}
3420-
state.stylesheets.set(resource, hoistableRoot);
3405+
// Construct and insert a new instance
3406+
instance = ownerDocument.createElement('link');
3407+
markNodeAsHoistable(instance);
3408+
const linkInstance: HTMLLinkElement = (instance: any);
3409+
// This Promise is a loading state used by the Fizz runtime. We need this incase there is a race
3410+
// between this resource being rendered on the client and being rendered with a late completed boundary.
3411+
(linkInstance: any)._p = new Promise((resolve, reject) => {
3412+
linkInstance.onload = resolve;
3413+
linkInstance.onerror = reject;
3414+
});
3415+
setInitialProperties(instance, 'link', stylesheetProps);
3416+
resource.instance = instance;
3417+
}
34213418

3422-
const preloadEl = resource.state.preload;
3423-
if (preloadEl && (resource.state.loading & Settled) === NotLoaded) {
3424-
state.count++;
3425-
const ping = onUnsuspend.bind(state);
3426-
preloadEl.addEventListener('load', ping);
3427-
preloadEl.addEventListener('error', ping);
3419+
if (state.stylesheets === null) {
3420+
state.stylesheets = new Map();
3421+
}
3422+
state.stylesheets.set(resource, hoistableRoot);
3423+
3424+
const preloadEl = resource.state.preload;
3425+
if (preloadEl && (resource.state.loading & Settled) === NotLoaded) {
3426+
state.count++;
3427+
const ping = onUnsuspend.bind(state);
3428+
preloadEl.addEventListener('load', ping);
3429+
preloadEl.addEventListener('error', ping);
3430+
}
34283431
}
34293432
}
34303433
}

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

Lines changed: 78 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2852,6 +2852,84 @@ body {
28522852
]);
28532853
});
28542854

2855+
// https://github.com/facebook/react/issues/27585
2856+
it('does not reinsert already inserted stylesheets during a delayed commit', async () => {
2857+
await act(() => {
2858+
renderToPipeableStream(
2859+
<html>
2860+
<body>
2861+
<link rel="stylesheet" href="first" precedence="default" />
2862+
<link rel="stylesheet" href="second" precedence="default" />
2863+
server
2864+
</body>
2865+
</html>,
2866+
).pipe(writable);
2867+
});
2868+
2869+
expect(getMeaningfulChildren(document)).toEqual(
2870+
<html>
2871+
<head>
2872+
<link rel="stylesheet" href="first" data-precedence="default" />
2873+
<link rel="stylesheet" href="second" data-precedence="default" />
2874+
</head>
2875+
<body>server</body>
2876+
</html>,
2877+
);
2878+
2879+
const root = ReactDOMClient.createRoot(document.body);
2880+
expect(getMeaningfulChildren(container)).toBe(undefined);
2881+
root.render(
2882+
<>
2883+
<link rel="stylesheet" href="first" precedence="default" />
2884+
<link rel="stylesheet" href="third" precedence="default" />
2885+
<div>client</div>
2886+
</>,
2887+
);
2888+
await waitForAll([]);
2889+
expect(getMeaningfulChildren(document)).toEqual(
2890+
<html>
2891+
<head>
2892+
<link rel="stylesheet" href="first" data-precedence="default" />
2893+
<link rel="stylesheet" href="second" data-precedence="default" />
2894+
<link rel="stylesheet" href="third" data-precedence="default" />
2895+
<link rel="preload" href="third" as="style" />
2896+
</head>
2897+
<body>
2898+
<div>client</div>
2899+
</body>
2900+
</html>,
2901+
);
2902+
2903+
// In a transition we add another reference to an already loaded resource
2904+
// https://github.com/facebook/react/issues/27585
2905+
React.startTransition(() => {
2906+
root.render(
2907+
<>
2908+
<link rel="stylesheet" href="first" precedence="default" />
2909+
<link rel="stylesheet" href="third" precedence="default" />
2910+
<div>client</div>
2911+
<link rel="stylesheet" href="first" precedence="default" />
2912+
</>,
2913+
);
2914+
});
2915+
await waitForAll([]);
2916+
// In https://github.com/facebook/react/issues/27585 the order updated
2917+
// to second, third, first
2918+
expect(getMeaningfulChildren(document)).toEqual(
2919+
<html>
2920+
<head>
2921+
<link rel="stylesheet" href="first" data-precedence="default" />
2922+
<link rel="stylesheet" href="second" data-precedence="default" />
2923+
<link rel="stylesheet" href="third" data-precedence="default" />
2924+
<link rel="preload" href="third" as="style" />
2925+
</head>
2926+
<body>
2927+
<div>client</div>
2928+
</body>
2929+
</html>,
2930+
);
2931+
});
2932+
28552933
xit('can delay commit until css resources error', async () => {
28562934
// TODO: This test fails and crashes jest. need to figure out why before unskipping.
28572935
const root = ReactDOMClient.createRoot(container);

0 commit comments

Comments
 (0)