Skip to content

Commit

Permalink
[Fizz] Do not reinsert stylesheets after initial insert (facebook#27586)
Browse files Browse the repository at this point in the history 
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: facebook#27585
  • Loading branch information
@gnoff @AndyPengc12
gnoff authored and AndyPengc12 committed Apr 15, 2024
1 parent c70b3ce commit 553b3f6
Show file tree
Hide file tree
Showing 2 changed files with 141 additions and 60 deletions.
123 changes: 63 additions & 60 deletions packages/react-dom-bindings/src/client/ReactFiberConfigDOM.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2337,7 +2337,7 @@ function preinitStyle(
getStylesheetSelectorFromKey(key),
);
if (instance) {
state.loading = Loaded;
state.loading = Loaded & Inserted;
} else {
// Construct a new instance and insert it
const stylesheetProps = Object.assign(
Expand Down Expand Up @@ -2769,6 +2769,7 @@ export function acquireResource(
getStylesheetSelectorFromKey(key),
);
if (instance) {
resource.state.loading |= Inserted;
resource.instance = instance;
markNodeAsHoistable(instance);
return instance;
Expand Down Expand Up @@ -3360,71 +3361,73 @@ export function suspendResource(
return;
}
}
if (resource.instance === null) {
const qualifiedProps: StylesheetQualifyingProps = props;
const key = getStyleKey(qualifiedProps.href);
if ((resource.state.loading & Inserted) === NotLoaded) {
if (resource.instance === null) {
const qualifiedProps: StylesheetQualifyingProps = props;
const key = getStyleKey(qualifiedProps.href);

// Attempt to hydrate instance from DOM
let instance: null | Instance = hoistableRoot.querySelector(
getStylesheetSelectorFromKey(key),
);
if (instance) {
// If this instance has a loading state it came from the Fizz runtime.
// If there is not loading state it is assumed to have been server rendered
// as part of the preamble and therefore synchronously loaded. It could have
// errored however which we still do not yet have a means to detect. For now
// we assume it is loaded.
const maybeLoadingState: ?Promise<mixed> = (instance: any)._p;
if (
maybeLoadingState !== null &&
typeof maybeLoadingState === 'object' &&
// $FlowFixMe[method-unbinding]
typeof maybeLoadingState.then === 'function'
) {
const loadingState = maybeLoadingState;
state.count++;
const ping = onUnsuspend.bind(state);
loadingState.then(ping, ping);
// Attempt to hydrate instance from DOM
let instance: null | Instance = hoistableRoot.querySelector(
getStylesheetSelectorFromKey(key),
);
if (instance) {
// If this instance has a loading state it came from the Fizz runtime.
// If there is not loading state it is assumed to have been server rendered
// as part of the preamble and therefore synchronously loaded. It could have
// errored however which we still do not yet have a means to detect. For now
// we assume it is loaded.
const maybeLoadingState: ?Promise<mixed> = (instance: any)._p;
if (
maybeLoadingState !== null &&
typeof maybeLoadingState === 'object' &&
// $FlowFixMe[method-unbinding]
typeof maybeLoadingState.then === 'function'
) {
const loadingState = maybeLoadingState;
state.count++;
const ping = onUnsuspend.bind(state);
loadingState.then(ping, ping);
}
resource.state.loading |= Inserted;
resource.instance = instance;
markNodeAsHoistable(instance);
return;
}
resource.state.loading |= Inserted;
resource.instance = instance;
markNodeAsHoistable(instance);
return;
}

const ownerDocument = getDocumentFromRoot(hoistableRoot);

const stylesheetProps = stylesheetPropsFromRawProps(props);
const preloadProps = preloadPropsMap.get(key);
if (preloadProps) {
adoptPreloadPropsForStylesheet(stylesheetProps, preloadProps);
}
const ownerDocument = getDocumentFromRoot(hoistableRoot);

// Construct and insert a new instance
instance = ownerDocument.createElement('link');
markNodeAsHoistable(instance);
const linkInstance: HTMLLinkElement = (instance: any);
// This Promise is a loading state used by the Fizz runtime. We need this incase there is a race
// between this resource being rendered on the client and being rendered with a late completed boundary.
(linkInstance: any)._p = new Promise((resolve, reject) => {
linkInstance.onload = resolve;
linkInstance.onerror = reject;
});
setInitialProperties(instance, 'link', stylesheetProps);
resource.instance = instance;
}
const stylesheetProps = stylesheetPropsFromRawProps(props);
const preloadProps = preloadPropsMap.get(key);
if (preloadProps) {
adoptPreloadPropsForStylesheet(stylesheetProps, preloadProps);
}

if (state.stylesheets === null) {
state.stylesheets = new Map();
}
state.stylesheets.set(resource, hoistableRoot);
// Construct and insert a new instance
instance = ownerDocument.createElement('link');
markNodeAsHoistable(instance);
const linkInstance: HTMLLinkElement = (instance: any);
// This Promise is a loading state used by the Fizz runtime. We need this incase there is a race
// between this resource being rendered on the client and being rendered with a late completed boundary.
(linkInstance: any)._p = new Promise((resolve, reject) => {
linkInstance.onload = resolve;
linkInstance.onerror = reject;
});
setInitialProperties(instance, 'link', stylesheetProps);
resource.instance = instance;
}

const preloadEl = resource.state.preload;
if (preloadEl && (resource.state.loading & Settled) === NotLoaded) {
state.count++;
const ping = onUnsuspend.bind(state);
preloadEl.addEventListener('load', ping);
preloadEl.addEventListener('error', ping);
if (state.stylesheets === null) {
state.stylesheets = new Map();
}
state.stylesheets.set(resource, hoistableRoot);

const preloadEl = resource.state.preload;
if (preloadEl && (resource.state.loading & Settled) === NotLoaded) {
state.count++;
const ping = onUnsuspend.bind(state);
preloadEl.addEventListener('load', ping);
preloadEl.addEventListener('error', ping);
}
}
}
}
Expand Down
78 changes: 78 additions & 0 deletions packages/react-dom/src/__tests__/ReactDOMFloat-test.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2852,6 +2852,84 @@ body {
]);
});

// https://github.com/facebook/react/issues/27585
it('does not reinsert already inserted stylesheets during a delayed commit', async () => {
await act(() => {
renderToPipeableStream(
<html>
<body>
<link rel="stylesheet" href="first" precedence="default" />
<link rel="stylesheet" href="second" precedence="default" />
server
</body>
</html>,
).pipe(writable);
});

expect(getMeaningfulChildren(document)).toEqual(
<html>
<head>
<link rel="stylesheet" href="first" data-precedence="default" />
<link rel="stylesheet" href="second" data-precedence="default" />
</head>
<body>server</body>
</html>,
);

const root = ReactDOMClient.createRoot(document.body);
expect(getMeaningfulChildren(container)).toBe(undefined);
root.render(
<>
<link rel="stylesheet" href="first" precedence="default" />
<link rel="stylesheet" href="third" precedence="default" />
<div>client</div>
</>,
);
await waitForAll([]);
expect(getMeaningfulChildren(document)).toEqual(
<html>
<head>
<link rel="stylesheet" href="first" data-precedence="default" />
<link rel="stylesheet" href="second" data-precedence="default" />
<link rel="stylesheet" href="third" data-precedence="default" />
<link rel="preload" href="third" as="style" />
</head>
<body>
<div>client</div>
</body>
</html>,
);

// In a transition we add another reference to an already loaded resource
// https://github.com/facebook/react/issues/27585
React.startTransition(() => {
root.render(
<>
<link rel="stylesheet" href="first" precedence="default" />
<link rel="stylesheet" href="third" precedence="default" />
<div>client</div>
<link rel="stylesheet" href="first" precedence="default" />
</>,
);
});
await waitForAll([]);
// In https://github.com/facebook/react/issues/27585 the order updated
// to second, third, first
expect(getMeaningfulChildren(document)).toEqual(
<html>
<head>
<link rel="stylesheet" href="first" data-precedence="default" />
<link rel="stylesheet" href="second" data-precedence="default" />
<link rel="stylesheet" href="third" data-precedence="default" />
<link rel="preload" href="third" as="style" />
</head>
<body>
<div>client</div>
</body>
</html>,
);
});

xit('can delay commit until css resources error', async () => {
// TODO: This test fails and crashes jest. need to figure out why before unskipping.
const root = ReactDOMClient.createRoot(container);
Expand Down

0 comments on commit 553b3f6

Please sign in to comment.