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

Flyout: not showing up on hydrated pages #278

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

Flyout: not showing up on hydrated pages #278

LecrisUT opened this issue Mar 18, 2024 · 22 comments
Labels
Needed: more information A reply from issue author is required

Comments

@LecrisUT
Copy link

LecrisUT commented Mar 18, 2024

Details

I have enabled both Addons and Flyout enabled in the project dashboard, but the menu seems to not reliably show up for me. It pops up for a millisecond and then dissapears. I am not sure if it requires rebuilding when you enable/disable this feature, but I have tried it on a few other builds with similar results.

Potential caveat, I am using noscript, but I have enabled all javascripts links and I have tested in incognito with noscript off, and the same results show up. I did not check that thoroughly when re-building the builds though.

@humitos
Copy link
Member

humitos commented Mar 18, 2024

Thanks for opening this issue. I'm transferring it to the addons repository since it fits better there.

I am not sure if it requires rebuilding when you enable/disable this feature, but I have tried it on a few other builds with similar results.

No, it does not require re-building.

It pops up for a millisecond and then dissapears.

I'm experiencing the same behavior at https://mystmd-temp.readthedocs.io/latest/

It seems the page is failing with some react issues in the console. Also, I found the DOM is re-written completely in some weird way after loading the page completely, because there is a lot of things removed from it after loading. It seems that removes the addons and all the other Read the Docs features.

Screenshot_2024-03-18_11-23-48

It seems this is an issue of that particular theme and you should report it there. Keep us posted, please.

@humitos humitos added the Support Support question label Mar 18, 2024
@humitos humitos changed the title flyout-menu: Not showing up reliably Flyout: not showing up reliably Mar 18, 2024
@humitos humitos transferred this issue from readthedocs/readthedocs.org Mar 18, 2024
@humitos humitos added the Needed: more information A reply from issue author is required label Mar 18, 2024
@LecrisUT
Copy link
Author

LecrisUT commented Mar 18, 2024

Upstream PR where the RTD is implemented. executablebooks/mystmd#982

Do you think it's an issue with the theme or the site generator? For debugging, we could try different themes, do you have a suggestion on which ones to try? I'm thinking of rtd theme (nvm, it's not available) and then book?

(Also to avoid confusion for other readers this is mystmd and not myst-parser + sphinx)

@humitos
Copy link
Member

humitos commented Mar 18, 2024

Do you think it's an issue with the theme or the site generator?

I'm not sure how mystmd works and how its themes are implemented --so I don't have a suggestion here. However, its homepage has the same React issue, and its DOM is also completely rewritten in some weird way. Take a look at the JS console while loading https://mystmd.org/

@LecrisUT
Copy link
Author

Ok, I've opened an upstream issue. I have tried to change the theme, but it doesn't use the sphinx theme, and it is already on book theme, so there is no change that can be done right now.

@LecrisUT
Copy link
Author

LecrisUT commented Mar 18, 2024

Got some feedback. We are suspecting it's related to react/hydrate: https://react.dev/reference/react-dom/client/hydrateRoot

React will attach to the HTML that exists inside the domNode, and take over managing the DOM inside it. An app fully built with React will usually only have one hydrateRoot call with its root component.

Also relevant comment: executablebooks/mystmd#188 (comment)

This limitation is currently in Remix (we are building a web application, and then making a static html export of it).

@LecrisUT LecrisUT changed the title Flyout: not showing up reliably Flyout: not showing up on hydrated pages Mar 19, 2024
@humitos
Copy link
Member

humitos commented Mar 21, 2024

In executablebooks/mystmd#997 (comment) I noted that this hydrated issue may not be 100% related to Read the Docs since it happens on their home page which is not hosted in our platform as well.

@LecrisUT
Copy link
Author

Indeed, but it actually doesn't happen to me. In more investigation, I've seen that this is an issue with anything that injects contents in pages, and most probably in your case it's to do with browser plugins? The issue with the hydration is that it requires the content on the server to be identical with that on the client, since it takes over and re-renders all the elements under its hydrated node.

But, it seems that the hydration can be limited to a sub-node, so if all the page content can be wrapped in a container node, so that the hydration is not on the html root, in principle, it should allow for at least the flyout compatibility

@humitos
Copy link
Member

humitos commented Mar 21, 2024

I've seen that this is an issue with anything that injects contents in pages, and most probably in your case it's to do with browser plugins? The issue with the hydration is that it requires the content on the server to be identical with that on the client, since it takes over and re-renders all the elements under its hydrated node.

😞

Do you know what is hydrated useful for? I'm not sure to understand its goal. Does it make sense to disable that feature?

But, it seems that the hydration can be limited to a sub-node, so if all the page content can be wrapped in a container node, so that the hydration is not on the html root, in principle, it should allow for at least the flyout compatibility

This could be a good intermediate solution --not only of Read the Docs addons, but for any user that has plugins installed in their browser.

@LecrisUT
Copy link
Author

Do you know what is hydrated useful for? I'm not sure to understand its goal. Does it make sense to disable that feature?

Unfortunately that seems to be a staple of react and its their way of getting interactive UI elements. This also means that you will probably encounter similar feedback when another react based projects wants to integrate. The issue is that since the html content is not templatized, but instead it's generated from the json/js data, I don't see a way around the react backend right now.

A related issue could be the static-site-generation, since right now it is done very naively on mystmd by simply downloading the pages from a temporary server. This is a limitation that remix backend has. I have found that next.js does have a ssg, but I did not look deeply into how it works. If it can do without hydration, or at least make it more intelligent (maybe only affecting tagged elements), than maybe there is some hope for a simpler integration.

@rowanc1
Copy link

rowanc1 commented Apr 18, 2024

@humitos we recently switched to using Netlify for the same content, and I was surprised that their server-side additions to html served worked without issue on exactly the same content. I did a cursory look at the differences in where the modifications were made: (1) they didn't change the head or meta-tags, (2) they only added a single div at the end of the body (same with the loom chrome extension I am using, which also caused no problems); (3) they waited some amount of time (~2s) to execute the scripts and make any other client-side modifications to the dom. These resulted in no errors, and the collaboration/version bar fading in nicely.

So these things should all be possible on RTD as well:

My brief comparison to RTD, there were changes in the head (new meta tags and script tags for ads), the scripts executed before our hydration is run, and there are multiple places where the html is modified.

@humitos
Copy link
Member

humitos commented Apr 18, 2024

Hey @rowanc1, this is great information! Thanks a lot for taking the time to compare them and write this down here. Everything you mention seems doable from our side, I guess. So, I will try to give it a try in the following days locally and see if I find a way to reproduce it and fixing it after adapting our code.

By the way, do you know if Netlify has this process documented somewhere? Maybe that can help me understand why they wait ~2s and similar things.

@humitos
Copy link
Member

humitos commented Apr 18, 2024

I was able to reproduce this issue locally. The manipulation that makes React to complain is this line, https://github.com/readthedocs/common/blob/bd497c8a5b383e2059de2655b0b8527c76695dd8/dockerfiles/force-readthedocs-addons.js#L18 that adds the script tag into the head.

@humitos
Copy link
Member

humitos commented Apr 18, 2024

External changing data without sending a snapshot of it along with the HTML.
It can also happen if the client has a browser extension installed which messes with the HTML before React loaded.

(from https://react.dev/errors/418?invariant=418)

So, this is definitely an issue with "Hydrating server-rendered HTML" React's feature. I suppose it makes sense to disable this feature on the doctool completely because even if we figure it out how to solve the Read the Docs' issue --there will be plenty of browser extensions that will make the docs to break.

@humitos
Copy link
Member

humitos commented Apr 18, 2024

there will be plenty of browser extensions that will make the docs to break.

Meh, in fact, I was just bitten by this. I have an extension on Firefox and it was making the documentation to fail no matter what change I made. Testing on Chrome without any extension I see no hydrated issues when building on Read the Docs.

@LecrisUT
Copy link
Author

I was able to reproduce this issue locally. The manipulation that makes React to complain is this line, https://github.com/readthedocs/common/blob/bd497c8a5b383e2059de2655b0b8527c76695dd8/dockerfiles/force-readthedocs-addons.js#L18 that adds the script tag into the head.

Oh interesting, that means it affects all RTD addons.

At least for flyout, I was thinking that it should still be possible on the myst side to import rtd-addon and inject the element before the hydration (somewhere in myst-to-react). @humitos You've mentioned somewhere how this is (soon/now) possible to get the metadata dynamically, but I can't find the issue to add the link to it here as well.

@humitos
Copy link
Member

humitos commented Apr 19, 2024

they waited some amount of time (~2s) to execute the scripts and make any other client-side modifications to the dom

I tried using 100ms timeout and 99% of the times I don't get the error. So, I understand that if the DOM is changed after React hydration validation happens everything works fine together.

This is the diff I used for my tests (note that it's required to not have any browser extension too):

diff --git a/src/init.js b/src/init.js
index 2a5f454..f0572ad 100644
--- a/src/init.js
+++ b/src/init.js
@@ -1,3 +1,3 @@
 import * as readthedocs from "./index";
 
-readthedocs.setup();
+setTimeout(readthedocs.setup, 100);

I think this is not an acceptable solution anyways. Adding a random timeout to all the projects to workaround this issue that only affects React projects that have hydration enabled doesn't seems to be the right direction here.

I still need to understand how hydration works and why it's required to be enabled in a documentation tool like mystmd. I'd appreciate any guidance on these topics. Also, would you consider disabling this feature? I'm asking as a regular user with a browser with extensions, since all the sites generated with mystmd will break for me, even if they are hosted outside Read the Docs.

@humitos
Copy link
Member

humitos commented Apr 19, 2024

I was able to reproduce this issue locally. The manipulation that makes React to complain is this line, readthedocs/common@bd497c8/dockerfiles/force-readthedocs-addons.js#L18 that adds the script tag into the head.

Today I noted that the injection of the script via Wrangler is not the problem. Injecting that chunk of HTML and making that .js file it points to to return 404 doesn't make React hydration to complain.

@LecrisUT
Copy link
Author

About hydration, my understanding is that that is react's method of making UI elements be interactable. mystmd currently has only 2 html themes and these are using myst-to-react. There is no particular reason why sphinx cannot be used as the production end, but that will take time to develop.

I tried using 100ms timeout and 99% of the times I don't get the error. So, I understand that if the DOM is changed after React hydration validation happens everything works fine together.

So to confirm, even when the script is injected in the main body, as long as the RTD script is run sufficiently after the hydration it works? Does it work after the interacting with the UI elements, e.g. expanding the TOC?

I think this is not an acceptable solution anyways. Adding a random timeout to all the projects to workaround this issue that only affects React projects that have hydration enabled doesn't seems to be the right direction here.

Is it possible to detect when you are in a react/hydrating environment and add the timeout than? It should be a predictable script that is being added by most react environments.

@humitos
Copy link
Member

humitos commented Apr 19, 2024

So to confirm, even when the script is injected in the main body, as long as the RTD script is run sufficiently after the hydration it works? Does it work after the interacting with the UI elements, e.g. expanding the TOC?

It seems it works fine, yes.

Peek 2024-04-19 19-25

Is it possible to detect when you are in a react/hydrating environment and add the timeout than? It should be a predictable script that is being added by most react environments.

I don't know how to detect that. I don't have experience with React 🤷🏼

@humitos humitos removed the Support Support question label Apr 19, 2024
@rowanc1
Copy link

rowanc1 commented Apr 19, 2024

A quick note on hydration: this is used in a lot of modern web apps and is how the server/client communicate. There are lots of docs on react hydration - this blog gives a decent overview. "Hydration expects the initial client side render to match the server side render exactly - and mutating the DOM like this during page load will always cause issues" (from remix-run/remix#9016). All interactivity in mystmd (jupyter widgets, hover, table of contents, etc.) is driven by react, and that needs to be hydrated to become interactive. Hydration issues around plugins are something that the React community knows about (e.g. facebook/react#24430). If unknown HTML is encountered (e.g. through a plugin), the app doesn't break from the user experience, it rehydrates and completely overwrites the DOM, and there is a flicker and an error message in the console. For example, this myst page on read the docs production build, does have an error and does flicker when it removes the unknown RTD html, but recovers after the initial differences in expected html are removed:
https://mystmd.readthedocs.io/interactive-notebooks.html

For detecting the timeout, you could look at if (window.__remixContext) { timeout = 200; } else { timeout = 0}. On mystmd's side we could also emit a loaded event on the window if that would be helpful as well as a flag or something to trigger the delay. I haven't found a way that is outside of react to hook into knowing when hydration is complete.

@humitos
Copy link
Member

humitos commented Apr 22, 2024

For detecting the timeout, you could look at if (window.__remixContext) { timeout = 200; } else { timeout = 0}. On mystmd's side we could also emit a loaded event on the window if that would be helpful as well as a flag or something to trigger the delay. I haven't found a way that is outside of react to hook into knowing when hydration is complete.

Yeah, we would need an event to subscribe to so we can perform our own setup after that event is triggered since the a timeout won't be reliable since it could be network connection issues or similar that take longer than the timeout we are defining.

@LecrisUT
Copy link
Author

Could the event be created on myst-theme side where we detect if the build is inside an RTD environment? @rowanc1 it should be possible to inject post-hydration scripts to run to accommodate for that right? I am not a web guy so I am not sure if the event's data needs to be known or is it like webhooks where it just blindly sends an even to whatever integration listens for it? Is there no react standard event for that?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needed: more information A reply from issue author is required
Projects
None yet
Development

No branches or pull requests

3 participants