🐛 BUG: Component children aren't hydrated if they were conditionally hidden during the build

[tony-sull]

What package manager are you using?

yarn

What operating system are you using?

Linux

Describe the Bug

If a React or Preact component takes in children and then conditionally hides/shows the children based on internal state, if the children are initially hidden during the build they won't be available for client-side hydration.

Steps to Reproduce

a. Clone the https://github.com/tony-sull/repro-react-conditional-hydrate
2. yarn && yarn start
d. Error! The react component hydrated, but the "Hello React!" message never shows because it wasn't included included in the built HTML

Alternate: copy the examples/framework-{preact/react} project. Update the counter to only show the message when the count is above 0, <div className="children">{count > 0 && children}</div>

Link to Minimal Reproducible Example (Optional)

https://github.com/tony-sull/repro-react-conditional-hydrate

[natemoo-re]

Thanks for filing an issue with a straightforward reproduction! 😍

I'm curious if you have any thoughts on how best to solve this... We can't really control how users choose to render their components, so this might just be a caveat we have to document.

Maybe instead of not rendering anything, users should render something like astro-placeholder?

[tony-sull]

Yeah it's definitely a tricky one to try to handle properly. I was also thinking it may just be a matter docs making it clear that you shouldn't conditionally hide children passed in from an Astro component.

Best I can tell this would only really hit if the children passed from an Astro component to a framework component are conditionally hidden. Any content hiding inside the react components would be covered in the generated react code.

That may not be a big deal - if that first level component needs to conditionally hide content then just pass it in as props instead of children

[jasikpark]

I feel like this should be supported no? Is this prevented by the hydration being fancy and not just naive "tell react to render with initial props" + hydrate = "tell react to render with initial props and keep running"?

It definitely seems like a case of Astro tree-shaking where it can't know that count will never be greater than zero, i.e. fully-static server side render is different from a static server side render meant for hydration.

[tony-sull]

Yeah, I'm also hoping we can fix this one, I should have time to dig into this one today.

From what I could tell it's a limitation of the client-side hydration leaning on the server rendered DOM to include the children elements. This isn't a problem normally, but if Astro passes in children that a framework purposely doesn't render on the server then neither Astro or the client framework ever knows those children existed in the template

[tony-sull]

@natemoo-re I think it we may actually be able to handle in the hydration scripts

Right now, here Astro setups up the hydration script to include passed in props and the server rendered astro-fragment. Each hydration script then just grabs innerHTML from the fragment and assumes the innerHTML were the original children

Thoughts on including an internal prop along the lines of __children that is the original children from the template before it was rendered? The hydration scripts could then use pull that out of the props when rehydrating and use that as children instead of innerHMTL

I don't want to jump the gun on the fix here, not sure if there's a risk of doing it this way or if it could be optimized. I'd be up for submitting a PR if we think this fix could make sense though

[tony-sull]

One optimization it could have is to only include __children if the rendered innerHTML for the fragment doesn't match the original children from the template.

Its probably more common that children are used as-is. In that case the hydration wouldn't have __children and could default to the current use of the rendered innerHTML

[tony-sull]

Here's an update to illustrate the challenge

When prepping the rendered HTML for client hydration, Astro currently includes whatever children the component rendered at build time. On the client, the hydration scripts assume that whatever <astro-fragment>'s inner HTML is are the children meant to be used based on the original template

If we want to handle this with more than just a build warning, I think there's two options

1. We could optimize this by comparing <astro-fragment> to the component's pre-rendered children, but at the cost of some build performance since we'd be doing that check for every component being prepped for client hydration

2. The other option would be to always include __children to avoid having to compare against <astro-fragment>, but at the cost of extra rendered HTML for the common case where children were unchanged in <astro-fragment>

Any thoughts on the tradeoffs here? Or do we just document it as a limitation?

[jasikpark]

This seems like a bug to revisit w/ the new compiler!

[jonathantneal]

Reproduction at https://stackblitz.com/edit/github-bbe8ox

[matthewp]

I have a fix for this. Going to add a <template> when a fragment is not rendered and pass that as children.