fix(rendering): prevent removal of necessary <astro-slot> elements

[lilnasy]

Changes

• Fixes slot content disappears upon hydration when using polymorphic tags #8247
• The check for whether to remove <astro-slot> happens multiple times - once per parent.
• When a distant parent is not to be hydrated, <astro-slot> gets removed, even though a direct parent needs it.
• The solution is to mark the tag as "definitely needed", so that distant parents don't evaluate it.
• This is done by replacing <astro-slot> with <astro-preserved-slot> in the HTML temporarily.
• <astro-preserved-slot> is converted back into <astro-slot> once there's no potential of being evaluated for removal - in the code for RenderDestination.
• Solution: fix(rendering): prevent removal of necessary <astro-slot> elements #10317 (comment)

Testing

• Added a case to a pre-existing E2E fixture.

Docs

• Does not affect usage.

[changeset-bot]

🦋 Changeset detected

Latest commit: 1ff765a

The changes in this PR will be included in the next version bump.

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

[lilnasy]

This test was checking for the conditions that cause the bug. We need <astro-slot> elements to exist to allow slots to render after hydration.

[ematipico]

The fix seems reasonable to me, but it would be great if @matthewp or @bluwy could weigh in

[ematipico]

"parent of a parent" -> "grandparent"?

[matthewp]

nice explanation!

Given that this is a sticky part of the code, I think we should release this in its own patch so it's easier to spot regressions if they happen.

[bluwy]

Alternating the slots feels like working around the issue and could have a performance impact compared to if we can render it directly correctly. Could this be fixed instead by only stripping away <astro-slot> for non-slot/child templates? e.g.

astro/packages/astro/src/runtime/server/render/component.ts

Lines 281 to 287 in 1d9fb15

	const childSlots = Object.values(children).join('');
	const renderTemplateResult = renderTemplate`<${Tag}${internalSpreadAttributes(
	props
	)}${markHTMLString(
	childSlots === '' && voidElementNames.test(Tag) ? `/>` : `>${childSlots}</${Tag}>`
	)}`;

Could we only replace <astro-slot> for non childSlots rendered strings? Is this issue also affected by those html that're assigned by renderToStaticMarkup too?

[lilnasy]

@bluwy Do you mean to make an exception for the "tagname as component" case?

[bluwy]

I mean that html can be derived from several ways, e.g. renderToStaticMarkup, "tagname as component", etc. And I noticed the tests only tested for "tagname as component", and I wonder if the issue only arises in this way?

If so, I was thinking we can make the fix directly in the if block for the "tagname as component" handling. Or if not, then I think this PR is probably the easiest fix, since I'm not sure if there's a reliable way to say "this chunk belongs to a slot, and we skip replacing astro-slots in it. For other chunks, we do the replacement"

[lilnasy]

I noticed the tests only tested for "tagname as component", and I wonder if the issue only arises in this way?

I don't think the issue is limited to it. but I'll take another look.

[natemoo-re]

Works for me! If there's a cleaner way to do the transformations as Blu suggested, I'm all for it. But this solution is aligned with our current hacky code, so I'm cool if the RegExp approach is the one that gets merged.

[lilnasy]

@bluwy

First-party framework components are not affected because they signal using "supportsAstroStaticSlots" that they will render astro-static-slot for the non-hydration case.

A much simpler 90% fix could be assuming that renderers do that by default.

[lilnasy]

The simpler fix works.

Required slots could still be removed if there's a third-party renderer that explicitly sets the "supportsAstroStaticSlots" flag to false. Qwik sets the flag to true, and analog doesn't set it, so it's more like a 99% fix.

For Astro 5.0, we should remove the flag to assume renderers that will always distinguish between hydratable and non-hydratable slots on their own. This was originally intended for Astro 3.0, but wasn't tracked.

[bluwy]

Awesome! Thanks for looking into it. This looks a ton simpler and makes sense to me too.