Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Float][Fiber] Assume stylesheets in document are already loaded #29811

Merged
merged 1 commit into from
Jun 7, 2024
Merged

[Float][Fiber] Assume stylesheets in document are already loaded #29811

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
72 changes: 42 additions & 30 deletions packages/react-dom-bindings/src/client/ReactFiberConfigDOM.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2412,23 +2412,42 @@ export function getResource(
if (!resource) {
// We asserted this above but Flow can't figure out that the type satisfies
const ownerDocument = getDocumentFromRoot(resourceRoot);
resource = {
resource = ({
type: 'stylesheet',
instance: null,
count: 0,
state: {
loading: NotLoaded,
preload: null,
},
};
}: StylesheetResource);
styles.set(key, resource);
const instance = ownerDocument.querySelector(
getStylesheetSelectorFromKey(key),
);
if (instance) {
const loadingState: ?Promise<mixed> = (instance: any)._p;
if (loadingState) {
// This instance is inserted as part of a boundary reveal and is not yet
// loaded
} else {
// This instance is already loaded
resource.instance = instance;
resource.state.loading = Loaded | Inserted;
}
}

if (!preloadPropsMap.has(key)) {
preloadStylesheet(
ownerDocument,
key,
preloadPropsFromStylesheet(qualifiedProps),
resource.state,
);
const preloadProps = preloadPropsFromStylesheet(qualifiedProps);
preloadPropsMap.set(key, preloadProps);
if (!instance) {
preloadStylesheet(
ownerDocument,
key,
preloadProps,
resource.state,
);
}
}
}
if (currentProps && currentResource === null) {
Expand Down Expand Up @@ -2599,28 +2618,21 @@ function preloadStylesheet(
preloadProps: PreloadProps,
state: StylesheetState,
) {
preloadPropsMap.set(key, preloadProps);

if (!ownerDocument.querySelector(getStylesheetSelectorFromKey(key))) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this check removed? If there is an existing style sheet why would we preload it?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We only call this function in one place and I changed it to gate the call on the instance not existing so we don't need to duplicate the check here

// There is no matching stylesheet instance in the Document.
// We will insert a preload now to kick off loading because
// we expect this stylesheet to commit
const preloadEl = ownerDocument.querySelector(
getPreloadStylesheetSelectorFromKey(key),
);
if (preloadEl) {
// If we find a preload already it was SSR'd and we won't have an actual
// loading state to track. For now we will just assume it is loaded
state.loading = Loaded;
} else {
const instance = ownerDocument.createElement('link');
state.preload = instance;
instance.addEventListener('load', () => (state.loading |= Loaded));
instance.addEventListener('error', () => (state.loading |= Errored));
setInitialProperties(instance, 'link', preloadProps);
markNodeAsHoistable(instance);
(ownerDocument.head: any).appendChild(instance);
}
const preloadEl = ownerDocument.querySelector(
getPreloadStylesheetSelectorFromKey(key),
);
if (preloadEl) {
// If we find a preload already it was SSR'd and we won't have an actual
// loading state to track. For now we will just assume it is loaded
state.loading = Loaded;
} else {
const instance = ownerDocument.createElement('link');
state.preload = instance;
instance.addEventListener('load', () => (state.loading |= Loaded));
instance.addEventListener('error', () => (state.loading |= Errored));
setInitialProperties(instance, 'link', preloadProps);
markNodeAsHoistable(instance);
(ownerDocument.head: any).appendChild(instance);
}
}

Expand Down
15 changes: 8 additions & 7 deletions ...dom-bindings/src/server/fizz-instruction-set/ReactDOMFizzInstructionSetExternalRuntime.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,11 @@ export function completeBoundaryWithStyles(
const dependencies = [];
let href, precedence, attr, loadingState, resourceEl, media;

function cleanupWith(cb) {
this['_p'] = null;
cb();
}

// Sheets Mode
let sheetMode = true;
while (true) {
Expand Down Expand Up @@ -80,18 +85,14 @@ export function completeBoundaryWithStyles(
resourceEl.setAttribute(attr, stylesheetDescriptor[j++]);
}
loadingState = resourceEl['_p'] = new Promise((resolve, reject) => {
resourceEl.onload = resolve;
resourceEl.onerror = reject;
resourceEl.onload = cleanupWith.bind(resourceEl, resolve);
resourceEl.onerror = cleanupWith.bind(resourceEl, reject);
});
// Save this resource element so we can bailout if it is used again
resourceMap.set(href, resourceEl);
}
media = resourceEl.getAttribute('media');
if (
loadingState &&
loadingState['s'] !== 'l' &&
(!media || window['matchMedia'](media).matches)
) {
if (loadingState && (!media || window['matchMedia'](media).matches)) {
dependencies.push(loadingState);
}
if (avoidInsert) {
Expand Down
2 changes: 1 addition & 1 deletion ...m-bindings/src/server/fizz-instruction-set/ReactDOMFizzInstructionSetInlineCodeStrings.js
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

15 changes: 8 additions & 7 deletions ...ct-dom-bindings/src/server/fizz-instruction-set/ReactDOMFizzInstructionSetInlineSource.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,11 @@ export function completeBoundaryWithStyles(
const dependencies = [];
let href, precedence, attr, loadingState, resourceEl, media;

function cleanupWith(cb) {
this['_p'] = null;
cb();
}

// Sheets Mode
let sheetMode = true;
while (true) {
Expand Down Expand Up @@ -82,18 +87,14 @@ export function completeBoundaryWithStyles(
resourceEl.setAttribute(attr, stylesheetDescriptor[j++]);
}
loadingState = resourceEl['_p'] = new Promise((resolve, reject) => {
resourceEl.onload = resolve;
resourceEl.onerror = reject;
resourceEl.onload = cleanupWith.bind(resourceEl, resolve);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It might be smaller to just inline the function like an arrow function. We probably compile them out but if we ever stop.

Copy link
Collaborator Author

@gnoff gnoff Jun 7, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

need bind b/c i mutate the resourceEl the binding. not doing so leads to closure spitting out very large code b/c it gives the variable some enormous loop name

resourceEl.onerror = cleanupWith.bind(resourceEl, reject);
});
// Save this resource element so we can bailout if it is used again
resourceMap.set(href, resourceEl);
}
media = resourceEl.getAttribute('media');
if (
loadingState &&
loadingState['s'] !== 'l' &&
(!media || window['matchMedia'](media).matches)
) {
if (loadingState && (!media || window['matchMedia'](media).matches)) {
dependencies.push(loadingState);
}
if (avoidInsert) {
Expand Down
166 changes: 166 additions & 0 deletions packages/react-dom/src/__tests__/ReactDOMFloat-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3348,6 +3348,172 @@ body {
);
});

it('will assume stylesheets already in the document have loaded if it cannot confirm it is not yet loaded', async () => {
await act(() => {
renderToPipeableStream(
<html>
<head>
<link rel="stylesheet" href="foo" data-precedence="default" />
</head>
<body>
<div id="foo" />
</body>
</html>,
).pipe(writable);
});

const root = ReactDOMClient.createRoot(document.querySelector('#foo'));

root.render(
<div>
<Suspense fallback="loading...">
<link rel="stylesheet" href="foo" precedence="default" />
hello world
</Suspense>
</div>,
);

await waitForAll([]);
expect(getMeaningfulChildren(document)).toEqual(
<html>
<head>
<link rel="stylesheet" href="foo" data-precedence="default" />
</head>
<body>
<div id="foo">
<div>hello world</div>
</div>
</body>
</html>,
);
});

it('will assume wait for loading stylesheets to load before continuing', async () => {
let ssr = true;
function Component() {
if (ssr) {
return null;
} else {
return (
<>
<link rel="stylesheet" href="foo" precedence="default" />
<div>hello client</div>
</>
);
}
}

await act(() => {
renderToPipeableStream(
<html>
<body>
<div>
<Suspense fallback="loading...">
<BlockedOn value="reveal">
<link rel="stylesheet" href="foo" precedence="default" />
<div>hello world</div>
</BlockedOn>
</Suspense>
</div>
<div>
<Suspense fallback="loading 2...">
<Component />
</Suspense>
</div>
</body>
</html>,
).pipe(writable);
});

expect(getMeaningfulChildren(document)).toEqual(
<html>
<head />
<body>
<div>loading...</div>
<div />
</body>
</html>,
);

await act(() => {
resolveText('reveal');
});

expect(getMeaningfulChildren(document)).toEqual(
<html>
<head>
<link rel="stylesheet" href="foo" data-precedence="default" />
</head>
<body>
<div>loading...</div>
<div />
<link rel="preload" href="foo" as="style" />
</body>
</html>,
);

ssr = false;

ReactDOMClient.hydrateRoot(
document,
<html>
<body>
<div>
<Suspense fallback="loading...">
<BlockedOn value="reveal">
<link rel="stylesheet" href="foo" precedence="default" />
<div>hello world</div>
</BlockedOn>
</Suspense>
</div>
<div>
<Suspense fallback="loading 2...">
<Component />
</Suspense>
</div>
</body>
</html>,
);
await waitForAll([]);

expect(getMeaningfulChildren(document)).toEqual(
<html>
<head>
<link rel="stylesheet" href="foo" data-precedence="default" />
</head>
<body>
<div>loading...</div>
<div />
<link rel="preload" href="foo" as="style" />
</body>
</html>,
);

await expect(async () => {
loadStylesheets();
}).toErrorDev([
"Hydration failed because the server rendered HTML didn't match the client.",
]);
assertLog(['load stylesheet: foo']);

expect(getMeaningfulChildren(document)).toEqual(
<html>
<head>
<link rel="stylesheet" href="foo" data-precedence="default" />
</head>
<body>
<div>
<div>hello world</div>
</div>
<div>
<div>hello client</div>
</div>
<link rel="preload" href="foo" as="style" />
</body>
</html>,
);
});

it('can suspend commits on more than one root for the same resource at the same time', async () => {
document.body.innerHTML = '';
const container1 = document.createElement('div');
Expand Down