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

Astro + Solid issue with hydration: nested islands being rendered partially #6301

Closed
1 task
alexander-lozovsky opened this issue Feb 18, 2023 · 7 comments · Fixed by #7197
Closed
1 task
Assignees
Labels
- P4: important Violate documented behavior or significantly impacts performance (priority)

Comments

@alexander-lozovsky
Copy link

What version of astro are you using?

2.0.14

Are you using an SSR adapter? If so, which one?

node

What package manager are you using?

npm

What operating system are you using?

Mac

Describe the Bug

I'm building an application with Astro + Solid + node.js SSR, and I was facing very interesting issue with hydration

Sometimes in the app we're using nested islands, something like:

<Tabs client:load> // 
  <TabPanel>
     ...
     <Modal client:load>{/* modal content */}</Modal>
  </TabPanel>
  ...
<Tabs/>

And once we moved the app to different infrastructure with http caching enabled, we've started experiencing issues with such components being rendered partially in browsers (Chrome, Safari). In Chrome the issue happened from time to time, but stable reproducing in Safari

I managed to create a minimal reproducible example https://github.com/alexander-lozovsky/astro-solid-cache-bug, where I'm using node.js ssr with a custom express server and http cache configured for static files.

First time you open the page in Safari, it is rendered as expected:
image

But when you reload the page, .js files are loaded from the cache, and the page is not rendering correctly:
image

I was debugging hydration logic, and technically it happens because Astro starts hydration on Wrapper before Safari renders the NestedComponent: when it starts hydration on Wrapper, it takes innerHTML with <h1>Hello world 1</h1> and insert it to the page. So NestedComponent never renders

As this happened only when .js files are loaded from http cache, I assume it has something to do with browser event loop and how it prioritise tasks

In this example issue is reproducible only in Safari, but we observe similar things in Chrome as well. It looks like there are some gaps in how Astro hydrates such Solid islands, so when you enable http caching there are possible race conditions between hydration logic and browser rendering the page

I'm leaning towards to not use Solid nested islands in our app at all, as there are too many issues with it at the moment (#6263 one more issue I've raised). But since Astro allows to do this, it would be nice to improve hydration logic to avoid such race conditions if it is possible. If not - maybe framework can restrict this pattern or at least throw some warning that nested Solid islands might not safe to use

Link to Minimal Reproducible Example

https://github.com/alexander-lozovsky/astro-solid-cache-bug

Participation

  • I am willing to submit a pull request for this issue.
@JerryWu1234
Copy link
Contributor

Hello, Does the situation also happen in Google Chrome?

@alexander-lozovsky
Copy link
Author

@wulinsheng123 Hi, yes, it happened to us in Google Chrome as well. I didn't manage to build minimal reproducible example to reproduce it there, but on my project we first notice this issue in Chrome. And unlike Safari, in Chrome it happened from time to time.

@matthewp matthewp added the - P4: important Violate documented behavior or significantly impacts performance (priority) label Mar 24, 2023
@bluwy bluwy self-assigned this Apr 6, 2023
@bluwy
Copy link
Member

bluwy commented May 3, 2023

I tried to reproduce this in Safari (v16.4) and Chrome (v112), but it doesn't happen for me. I took a look at @astrojs/solid-js's hydration code, and it seems to properly handle slots. Is this still happening today?

@bluwy bluwy added the needs response Issue needs response from OP label May 3, 2023
@alexander-lozovsky
Copy link
Author

I checked and it's interesting:

  • without bumping any versions, the issue doesn't happen in Safari 16.4, but happens in Safari 16.3 and below
  • In Safari 16.3 and below the issue happens even after bumping all the versions (astro@2.4.1, astrojs/solid-js@2.1.1)

In Chrome, as I said above, it happened but it was very difficult to catch, and I wasn't been able to create a minimal reproducible example

@bluwy
Copy link
Member

bluwy commented May 24, 2023

Sorry for the late followup. I'm looking at this again, and since I can't get to use a lower version of Safari without installing from a potentially malicious link 😬, I'm reading the hydration source code now and trying to find clues where it could happen.

It looks like we had previous issues with Safari too that are maybe related: #3891 and #3873

What I think it is happening here is also the same as the linked PRs above, where the astro-island children aren't ready yet when we start hydrating. I've opened #7197 adding a setTimeout to maybe fix this.

Would appreciate if you can test this out with npm install astro@next--nested-hydrate. I'm not calling setTimeout in all the cases, so if it doesn't work I can also cut another release making setTimeout called everytime.

@alexander-lozovsky
Copy link
Author

I tested npm install astro@next--nested-hydrate in Xcode iOs simulator on iOS 16.0, and everything works as expected. No race conditions. You can try and test it too in the simulator, the issue is reproducible there

@bluwy
Copy link
Member

bluwy commented Jun 5, 2023

Holy crap, I completely missed your reply. Sorry about that 😅 Gonna move forward with the PR then, and thanks for testing it.

Using the simulator is a neat trick that I haven't thought of. Will try that for next time.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
- P4: important Violate documented behavior or significantly impacts performance (priority)
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants