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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

HenriqueLimas
Copy link
Contributor

@HenriqueLimas HenriqueLimas commented Sep 29, 2024

Summary

When a 3rd party script adds a text node in the body, the hydration fails instead of trying to get the next sibling to compare. With this PR the canHydrateInstance() keeps looking for the nextSibling considering also text nodes.

The issue can be viewed in this repo.

It is related to the current opening issue regarding hydration issues #24430

Frameworks usually uses text nodes as a fragment since DocumentFragment loses the reference of their children as they are appended to the DOM.

How did you test this change?

  • Added a test case with issue
  • Changed with the same code implemented locally in the browser to fix the issue as shown bellow
Screen.Recording.2024-09-29.at.4.26.03.PM.mov

Copy link

vercel bot commented Sep 29, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
react-compiler-playground ✅ Ready (Inspect) Visit Preview 💬 Add feedback Sep 29, 2024 11:29pm

@react-sizebot
Copy link

The size diff is too large to display in a single comment. The GitHub action for this pull request contains an artifact called 'sizebot-message.md' with the full message.

Generated by 🚫 dangerJS against 7776122

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

@HenriqueLimas
Copy link
Contributor Author

HenriqueLimas commented Oct 3, 2024

Thank you @gnoff for the review, and your comment make sense. We were able to remove this use case of empty text around the div on our side, but I still think it should be handled properly as it is common for frameworks to use empty text node for fragments.

I have some other use cases that I am noticing causes hydration issues:

  • Empty text node added around elements (The use case that this PR was trying to solve)
  • A new element is prepended to the body with the same element nodeType
  • A new class was added to the body (this is a dev warning)

I added those use cases in this repo.

I was wondering if we could add a flag on the server on the elements that needs hydration and the hydration checks only those elements (like a d-rh="<random>" attribute). In that way during hydration we keep getting the next hydratable element also based on this attribute and ignore newly added elements.

<html>
  <script>window.__reactHydrateDataAttribute = 'a1b2c';</script>
  <head></head>
  <body data-rh="a1b2c">
     <div></div> <!-- Added by a script -->
     <div data-rh="a1b2c"></div>
  </body>
</html>

The random it is just to make sure it doesn't conflict, but it can be removed for simplicity. The downside is that now each element will have this, but we could also use only on the children of the body instead of all the elements.

@HenriqueLimas
Copy link
Contributor Author

@gnoff, it seems that Solid utilizes a data-hk attribute to indicate a hydration key, enabling fine-grained hydration based on sibling and child elements. While this addresses a different challenge and isn't necessary in React due to the virtual DOM's handling of such issues, a similar strategy could be implemented to identify elements that require hydration. This ensures that non-React elements remain untouched, in contrast to other frameworks that may remove them. Additionally, the hydration key can be generated per request and limited to direct children of the body, helping to minimize payload size. It would be worthwhile to test the performance impact of checking whether an element is a child of the body versus adding the attribute to all elements, which would increase the payload size.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants