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.

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

Consider Text node in the root hydration #31097

Closed
Closed
Changes from all commits
Commits
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
9 changes: 5 additions & 4 deletions packages/react-dom-bindings/src/client/ReactFiberConfigDOM.js
Original file line number Diff line number Diff line change
@@ -1047,7 +1047,10 @@ export function canHydrateInstance(
props: Props,
inRootOrSingleton: boolean,
): null | Instance {
while (instance.nodeType === ELEMENT_NODE) {
while (
instance.nodeType === ELEMENT_NODE ||
(inRootOrSingleton && instance.nodeType === TEXT_NODE)
Copy link
Collaborator

Choose a reason for hiding this comment

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

This isn't quite right b/c the instance is refined to Element below and text nodes aren't elements. But I see what are you are trying to do. As commented below we're being somewhat conservative here but if you could make the case that there is widespread use of extensions/scripts that insert text nodes in the body before primary content then we can consider changing the heuristic

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah so frameworks uses text nodes as a reference to a fragment because DocumentFragment loses their children reference when appended in the dom. Or also the React portal adds text around portal nodes.
Our use case is that pieces of the page like a header are rendered outside of React as inline scripts. We use suppressHydrationWarning for those DOM tree, but they also have portal similar implementation to render elements on the body causing the issue when the script is loaded.

example of React Portal rendered tree from the documentation as an example of the approach used
Screenshot 2024-09-30 at 4 44 59 PM

) {
const element: Element = (instance: any);
const anyProps = (props: any);
if (element.nodeName.toLowerCase() !== type.toLowerCase()) {
@@ -1177,11 +1180,9 @@ export function canHydrateInstance(
}
instance = nextInstance;
}
// This is a suspense boundary or Text node or we got the end.
// This is a suspense boundary or we got the end.
// Suspense Boundaries are never expected to be injected by 3rd parties. If we see one it should be matched
// and this is a hydration error.
// Text Nodes are also not expected to be injected by 3rd parties. This is less of a guarantee for <body>
// but it seems reasonable and conservative to reject this as a hydration error as well
return null;
}

Original file line number Diff line number Diff line change
@@ -788,4 +788,20 @@ describe('ReactDOMServerHydration', () => {

expect(ref.current).toBe(button);
});

it('ignores text nodes on root when hydrating', async () => {
const root = document.createElement('div');

root.innerHTML = '<button>Click</button>';
const button = root.firstChild;

const emptyText = document.createTextNode('');
root.insertBefore(emptyText, button);

const ref = React.createRef();
await act(() => {
ReactDOMClient.hydrateRoot(root, <button ref={ref}>Click</button>);
});
expect(ref.current).toBe(button);
});
});