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

React/DOM: hydrated page incompatible with RTD flyout menu #997

Open
LecrisUT opened this issue Mar 18, 2024 · 12 comments
Open

React/DOM: hydrated page incompatible with RTD flyout menu #997

LecrisUT opened this issue Mar 18, 2024 · 12 comments
Labels
bug Something isn't working

Comments

@LecrisUT
Copy link
Contributor

LecrisUT commented Mar 18, 2024

Description

This might be either a template or generator issue. It is hard to describe the issue, but there is more discussion here.

A screenshot of the console issue:
Screenshot_20240318_110444

@rowanc1
Copy link
Collaborator

rowanc1 commented Mar 18, 2024

Can you please re-upload the screenshot of the console and let us know which site the issues occur on?

I suspect this is a hydration error if the DOM is being modified in unexpected ways. When react loads this will overwrite any unexpected changes to the dom and complain. You can read more about react hydration here. This can sometimes be the case with chrome extensions and is likely also happening if RTD is changing the HTML, which is not supported by React hydration out of the box.

@LecrisUT
Copy link
Contributor Author

There was supposed to be a screenshot associated but it seems it's not visible. If you look at the linked issue or this comment you'll see the console log. For a full site to investigate, you can check: https://mystmd-temp.readthedocs.io/latest/

This can sometimes be the case with chrome extensions and is likely also happening if RTD is changing the HTML

Indeed that is the case iiuc. They are injecting the flyout javascript, probably at every page.

@LecrisUT LecrisUT changed the title bug: React/DOM issue React/DOM: hydrated page incompatible with RTD flyout menu Mar 19, 2024
@LecrisUT
Copy link
Contributor Author

I was trying to find where the hydration occurs in the code, but I couldn't find anything. I've found at least that the flyout is injected at the body level 1. The hydration says that it can be scoped to a specific element, so I wonder if the contents of the page can be moved to a sub-node and hydrate only that? Not a nice solution though :/

This is independent of remix backend being used, and it's intrinsic to any node/js backend?

Footnotes

  1. https://github.com/readthedocs/addons/blob/049913ef467d2c6c88c2c7647f3f654277171082/src/flyout.js#L362-L369

@agoose77
Copy link
Contributor

I don't know enough to provide suggestions about resolving this from a standpoint of "fix MyST".

However, it might also just make sense to disable these features on RTD and implement React components for them in our theme. Then we can aim for better integration (although I note the disadvantages of such an approach).

@LecrisUT
Copy link
Contributor Author

However, it might also just make sense to disable these features on RTD and implement React components for them in our theme.

I did think about it, but it's hard to get the appropriate config for the constructor 1

Footnotes

  1. https://github.com/readthedocs/addons/blob/049913ef467d2c6c88c2c7647f3f654277171082/src/flyout.js#L351-L357

@humitos
Copy link

humitos commented Mar 21, 2024

Hi 👋🏼 . I'm one of the Read the Docs developers and I've been following some of the issues related to the hydrated error and also some other related to Read the Docs.

I wanted to chime in here because I noticed the hydrated issue you noticed is not strictly related to Read the Docs but with something else. Note that going to https://mystmd.org/ (not hosted on Read the Docs) shows the same error in the console:

Screenshot_2024-03-21_10-49-19

I did think about it, but it's hard to get the appropriate config for the constructor

We are working on an approach to expose this config data to theme authors so they can build components as they need. This is an example of how a Sphinx theme would use it: readthedocs/sphinx_rtd_theme#1526. I'd appreciate any feedback you may have about it.

However, I wouldn't follow this integration path before understanding the React error first since as I mentioned before it's not strictly related to the Read the Docs addons.

@agoose77
Copy link
Contributor

@humitos could you report your browser? I don't see such an error on https://mystmd.org

@LecrisUT
Copy link
Contributor Author

LecrisUT commented Mar 21, 2024

@agoose77 this is most likely an issue with browser plugins that would similarly inject contents, e.g. auto-dark-mode.

From what I understood from the react hydration documentation, changing where the hydration happens can fix most of the RTD addons and other browser plugins. But I couldn't find where that hydration happens. Maybe it's controlled indirectly by remix, but even there I couldn't navigate to the page creation api.

Another thought I had was to check how other backends like next.js handles ssg and/or if it can do the hydartion more systematically, e.g. only affecting the generated content elements. The code is not strictly linked to remix, but it is strictly linked to react, right?

Edit: Digging a bit more, is this issue ultimately with the themes? Is it technically possible to create the pages outside of a react ecosystem?

@humitos
Copy link

humitos commented Mar 21, 2024

@humitos could you report your browser?

I'm using Firefox with a few plugins (figured it out this could be an issues thanks to @LecrisUT). It doesn't happens on Chrome with a fresh session.

@agoose77 is hydrated strictly required here? can it be disabled? I assume lot of users will have this issue since it's pretty common to use browser plugins these days.

BTW, what's the advantage/goal of using hydrated? I'm not sure to 100% its benefits. I'd love if you can expand on this so I can understand this issue better.

@humitos
Copy link

humitos commented Apr 19, 2024

I was trying to find where the hydration occurs in the code, but I couldn't find anything

@LecrisUT I understand this happens in the myst-theme repository at https://github.com/executablebooks/myst-theme/blob/65a2bab24ed2936f2a8cedb8bd076fe7a3f26cf9/themes/book/app/entry.client.tsx

I see the whole document is considered for hydration. We may be able to change that for another inner HTML element (e.g. document.querySelector("#inner-div-container-inside-body") or similar) to avoid this issues with browser extensions.

diff --git a/themes/book/app/entry.client.tsx b/themes/book/app/entry.client.tsx
index 62da6d15..afd745a9 100644
--- a/themes/book/app/entry.client.tsx
+++ b/themes/book/app/entry.client.tsx
@@ -5,7 +5,7 @@ import { hydrateRoot } from 'react-dom/client';
 function hydrate() {
   startTransition(() => {
     hydrateRoot(
-      document,
+      document.querySelector("#inner-div-container-inside-body"),
       <StrictMode>
         <RemixBrowser />
       </StrictMode>,

I'm not sure how to use my modifications here, but I think this could be a good test case.

@LecrisUT
Copy link
Contributor Author

@humitos I was thinking of using #1129 to experiment and during the debug period, simply have the packages.json point to some branch on the myst-theme repo with potential fixes

@rowanc1
Copy link
Collaborator

rowanc1 commented Apr 19, 2024

Remix is also handling scripts, metatags, and classes on the html, so the hydrated root needs to be the full html when using remix (there is remix-island, and some other work arounds, so it is kinda possible to do this). Considering other frameworks is possible (note that nextjs will also likely have similar issues, bare bones react looses out on SSR and other things provided by a framework like routing). I also left a comment over here about hydration.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

4 participants