Skip to content

Commit 65c8ac5

Browse files
gnoffgnoff
committed
When inserting styles during a delayed commit we should never reinsert styles that are already in the document
1 parent 51ffd35 commit 65c8ac5

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)