using `Astro.slots.render` breaks children hydration

y-nk commented 1 year ago

Astro info

Astro version            v2.10.14
Package manager          npm
Platform                 linux
Architecture             x64
Adapter                  Couldn't determine.
Integrations             @astrojs/react

What browser are you using?

stackblitz

Describe the Bug

In a component, make use of Astro.slots.render to pre-render a slot (no matter which). If a child of this slot has a client directive, it will be ignored.

What's the expected result?

Hydration should happen normally

Link to Minimal Reproducible Example

https://stackblitz.com/edit/withastro-astro-wkjve2?file=src/pages/index.astro,src/components/Working.astro,src/components/NotWorking.astro

Usecase

I'm making a carousel. The carousel has a slot[name=slides] to collect all the slides of the carousel, for which i'd like to compute the children.length so that it doesn't have to come from userland. The length is used to generate pagination ui.

Participation

[x] I am willing to submit a pull request for this issue ...but i don't have much time at the moment. i'll be available around september.

y-nk commented 8 months ago

🤞

natemoo-re commented 8 months ago

@y-nk I upgraded everything to the latest and took another look at this. The problem seems to be using both Astro.slots.render("default") and rendering the <slot> element inside the component. But... I did find a workaround!

Check this updated repro. The only change I made (besides upgrading deps) was to the NotWorking.astro component. Instead of discarding the return value of await Astro.slots.render('default'), we're storing it in a variable and replacing the template's <slot /> usage with <Fragment set:html={markup} />.

Since there is a workaround (even thought it's unintuitive), this is probably going to be bumped down from a P4 priority. My guess is that our approach for script deduplication is causing this issue. The first time we render a component for a given Request, we'll add the necessary scripts for hydration to that markup. The second time we render it, we assume that the markup we rendered previously ended up in the Response, so we don't add the required scripts an additional time. Since Astro.slots.render('default') is technically the first render and <slot /> is technically the second render, the <slot /> value does not have the expected scripts.

y-nk commented 8 months ago

@natemoo-re thank you for looking into this. the explanation does make sense, the repo as well.

instead of not inserting the associated scripts on the 2nd render, would that make sense to rather keep them all and dedupe them after? not sure the cost of it, but i guess a key could be computed for calculation?

natemoo-re commented 8 months ago

That seems like a reasonable solution, if we can figure out how to implement it!

The root problem here is that our rendering process doesn't necessarily check the Response stream for the actual chunks that have been flushed, but instead sets the state with the assumption that rendered chunks will eventually be flushed. If we can find a way to hook the state into the actual Response stream that would help!

One quicker fix here might be to make an exception for Astro.slots.render(). Maybe we could somehow automatically wire up the <slot /> element with the result of the prior Astro.slots.render() call? Essentially adding some special handling for this particular case where both constructs are used in the same component.

withastro / astro