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

Client render dehydrated Suspense boundaries on document load #31620

Merged
merged 1 commit into from
Dec 3, 2024
Merged

Client render dehydrated Suspense boundaries on document load #31620

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
20 changes: 20 additions & 0 deletions packages/internal-test-utils/ReactJSDOM.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
const JSDOMModule = jest.requireActual('jsdom');

const OriginalJSDOM = JSDOMModule.JSDOM;

module.exports = JSDOMModule;
module.exports.JSDOM = function JSDOM() {
let result;
if (new.target) {
result = Reflect.construct(OriginalJSDOM, arguments);
} else {
result = JSDOM.apply(undefined, arguments);
}

require('./ReactJSDOMUtils').setupDocumentReadyState(
result.window.document,
result.window.Event,
);

return result;
};
33 changes: 33 additions & 0 deletions packages/internal-test-utils/ReactJSDOMUtils.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
export function setupDocumentReadyState(
document: Document,
Event: typeof Event,
) {
let readyState: 0 | 1 | 2 = 0;
Object.defineProperty(document, 'readyState', {
get() {
switch (readyState) {
case 0:
return 'loading';
case 1:
return 'interactive';
case 2:
return 'complete';
}
},
set(value) {
if (value === 'interactive' && readyState < 1) {
readyState = 1;
document.dispatchEvent(new Event('readystatechange'));
} else if (value === 'complete' && readyState < 2) {
readyState = 2;
document.dispatchEvent(new Event('readystatechange'));
document.dispatchEvent(new Event('DOMContentLoaded'));
} else if (value === 'loading') {
// We allow resetting the readyState to loading mostly for pragamtism.
// tests that use this environment don't reset the document between tests.
readyState = 0;
}
},
configurable: true,
});
}
22 changes: 21 additions & 1 deletion 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 @@ -194,6 +194,8 @@ const SUSPENSE_FALLBACK_START_DATA = '$!';
const FORM_STATE_IS_MATCHING = 'F!';
const FORM_STATE_IS_NOT_MATCHING = 'F';

const DOCUMENT_READY_STATE_COMPLETE = 'complete';

const STYLE = 'style';

opaque type HostContextNamespace = 0 | 1 | 2;
Expand Down Expand Up @@ -1262,7 +1264,11 @@ export function isSuspenseInstancePending(instance: SuspenseInstance): boolean {
export function isSuspenseInstanceFallback(
instance: SuspenseInstance,
): boolean {
return instance.data === SUSPENSE_FALLBACK_START_DATA;
return (
instance.data === SUSPENSE_FALLBACK_START_DATA ||
(instance.data === SUSPENSE_PENDING_START_DATA &&
instance.ownerDocument.readyState === DOCUMENT_READY_STATE_COMPLETE)
);
}

export function getSuspenseInstanceFallbackErrorDetails(
Expand Down Expand Up @@ -1303,6 +1309,20 @@ export function registerSuspenseInstanceRetry(
instance: SuspenseInstance,
callback: () => void,
) {
const ownerDocument = instance.ownerDocument;
if (ownerDocument.readyState !== DOCUMENT_READY_STATE_COMPLETE) {
ownerDocument.addEventListener(
Copy link
Collaborator

Choose a reason for hiding this comment

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

Whenever we add listeners in the render phase I need to look closer because it’s risky. For example that it won’t ever clean up.

We already do this with the retry but at least that overrides an existing callback which frees the previous one and it automatically frees once the dom node itself is removed.

For every attempt that has to be given up, we end up closing over the whole fiber tree that was attempted that we normally just gc. The clean up mechanism we have as a safety for leaking large trees won’t even kick in because it never commits so there’s no unmount. (This would be fixed by the big refactor idea to split the Fibers into two but unclear if that will ever happen.)

So it’s absolutely critical that this event actually fires and the closure gets cleaned up. However even then it might leak some attempted trees in the meantime which can leak to more gc work too since they now move to older generations.

The ideal would be to either move this whole thing to the commit phase or maybe lazily scan the fiber tree.

'DOMContentLoaded',
() => {
if (instance.data === SUSPENSE_PENDING_START_DATA) {
callback();
}
},
{
once: true,
},
Copy link
Collaborator

Choose a reason for hiding this comment

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

It’s probably fine to rely on this these days but I’m always paranoid of new features. If this is an old browser it would forever leak potentially very large trees.

An alternative to this is to explicitly just remove the same listener when called.

);
}
instance._reactRetry = callback;
}

Expand Down
Loading
Loading