Xiphe / remix-island

utils to render remix into a dom-node instead of the whole document
MIT License
131 stars 7 forks source link

cleanup option of createHead causes double asset loading #2

Closed jozefmery closed 1 year ago

jozefmery commented 1 year ago

Issue description

Following the configuration steps in the provided readme causes the Head component to be rendered twice, also causing double loading of assets, and flashing (as the css is unloaded, and then immediately loaded again). First, Head is rendered into a string on the server, and again on the client upon hydration. Providing { cleanup: false } (true by default) to the createHead function fixes the double-rendering.

Notes

Originally I thought the readme is incorrectly rendering the Head both on the server and in the App component. Removing it from the App component fixes the issues, however the Head component is no longer managed by React, and is thus static. Rendering in the App component combined with disabled cleanup also seems to work while keeping the Head reactive. Furthermore, I was unable to figure out the point of having or enabling the cleanup option from source.

Xiphe commented 1 year ago

Hey @jozefmery thanks for reaching out about this. I've heard about the flashing problem with css a multiple times now and am not sure if there is a solution that can be utilized in this lib.

Flash of unstyled content

What I noticed in my apps that I have only one singe global css file that is used for all pages (meaning I don't use the "this route bings it's own css" feature of remix). So I moved the <link> that loads those styles out of my Remix/React App and input the static part of the entry.server.ts.

Do you experience other noticeable effects caused by this?

Why the cleanup?

If we don't do the cleanup, the server-rendered elements will stay in the head which may cause unwanted behavior. For example the user may load a subpage that brings extra styles then navigates away from that page and normally remix would remove the extra styles but now it doesn't.

By setting { cleanup: false } you basically say: "I'm ok with keeping the server header in here...".

Rendering Head on server

Originally I thought the readme is incorrectly rendering the Head both on the server and in the App component. Removing it from the App component fixes the issues, however the Head component is no longer managed by React, and is thus static.

Good observation and I understand your thinking. The Head component will "magically" opt out from rendering on the server and only render on the server within renderHeadToString. My goal with this design was to minimize boilerplate for apps but I understand that it might be irritating.

Possible solution

I'm wondering if it might solve the problem when the cleanup step is delayed a little bit. Maybe we need to wait until all resources are loaded before removing them from the head.

Do you think it might be worth investigating here? Do you have another idea?


I really hope that this package will not be needed in the near future because either react solved its hydration issues or remix has a buildin workaround.

Xiphe commented 1 year ago

I tinkered around a little bit with the flash of unstyled content problem and noticed that on my end the problem occurs only with a disabled cache.

Do you have your cache enabled or disabled?

jozefmery commented 1 year ago

Firstly, thank you for the detailed explanations.

Rendering style links on the server

I'm using UnoCSS in my project (in case you don't know, think Tailwind on steroids, highly recommended) meaning I also have one main css file, the other is just resets, so statically rendering them seems like a smart idea, but I haven't tried that approach yet (will try and comeback if I find any problems). Unfortunately, this solution may not be feasible for everyone.

Cache

I normally have cache enabled, sometimes I disable it for testing purposes. With the cache disabled, the flash of unstyled content happens consistently. Enabling it partially solves the issue as sometimes I still see the flash, however I don't know what causes it or how to meaningfully reproduce it. Thus I would consider caching like a 99% solution.

Final thoughts

I am by no means expert, and for now my page is a sort-of toy project, so my ideas may be completely useless. Despite that, I'm not sure either React or Remix will have a meaningful solution for this problem any time soon. React docs state that hydration mismatches, although only warning in development mode, should be treated as bugs and fixed, because validating the markup would be very expensive in most cases. One rough idea I had is to have two separate hydrateRoot calls. I haven't found a conflict in React docs related to this (they mention that usually there will only be one). Thus if the head contains client injected items, only the hydration of the head would fail and would need to be client rendered, at least that is how I understand it (I may be completely wrong). Unfortunately, this would almost definitely require changes to Remix itself, but I am in no position to further comment on that as I simply don't know. Delaying the cleanup might be worth exploring, if there is a mechanism for knowing that everything is loaded (if I'm not mistaken, fixed delay doesn't make sense due to varying connection speeds).

Xiphe commented 1 year ago

Cool!

Agree that the "moving css out of remix" is not a solution for everyone. But as I mentioned, this package also does not aim to provide a sustainable solution and become part of some sort of "standard". It helps me move forward with react+remix until they sort their stuff out and if it helps others along the way I'm more then happy.

Interesting that you still have flickers with cache enabled. I can imagine that it might be the time the browser takes to re-apply the styles to the page. Might be worth investigating if the problem becomes reproducible with massively big stylesheets. Another thing could be blocking @import statements of other stylesheets that can not be cached due to their headers (Typekit uses a @import statement for usage tracking of their fonts for example).

As with the double hydrateRoot - this would circumvent the issue but require a lot of new code to sync up the two react apps. The whole point of the current situation is to have the head and body synced so that remix can do it's magic.

Agree with that a fixed delay will not work reliably. I think it would work if we'd wait with the cleanup until the hydrated head is rendered and loaded but to me that feels like a too complex workaround for a problem that will hopefully be fixed at it's root in the near future.

jozefmery commented 1 year ago

Yes, hopefully a native solution is provided by the Remix, or even better, React itself. This solution, and I mean absolutely no disrespect, does feel rather "hacky".

My stylesheet does not have @import anywhere, and isn't large either. I have url references to google fonts though, which may or may not be related.

Anyway, you might want to close this issue as not planned for now, and thank you for the discussion.

Xiphe commented 1 year ago

I also consider this "hacky" ;)

My stylesheet does not have @import anywhere, and isn't large either. I have url references to google fonts though, which may or may not be related.

yeah, might be related to that - not sure either.

Anyway, you might want to close this issue as not planned for now, and thank you for the discussion.

Will do and thank you, too