Baffling loss of styles in Markdown

innermatrix commented 6 months ago

Astro Info

Astro                    v4.8.5
Node                     v18.18.0
System                   Linux (x64)
Package Manager          unknown
Output                   static
Adapter                  none
Integrations             none

If this issue only occurs in one browser, which browser is a problem?

No response

Describe the Bug

I promise I spent hours reducing this to a MWE and I think every part that is left in the example is needed for repro at this point:

There is a Markdown file test-md.md using layout TestA and an Astro file test-astro.astro using the component TestA. The Markdown file is broken (as described below) while the Astro file works fine
TestA passes the default slot to TestB as well as a <div/> in slot test
TestB passes an empty slot head to TestC
TestB passes RenderTest to TestC default slot, with the test slot of RenderC filled with the test slot of TestB
TestC puts the head slot into a <head>
RenderTest calls slots.render('test')

And if you do all of that… then the HTML produced from the Markdown file has no styles:

<!DOCTYPE html><html data-astro-cid-yjwvcixp> <head></head> <body data-astro-cid-yjwvcixp><div data-astro-cid-yjwvcixp>   <p>Some content here</p>    </div></body></html>

but the HTML produced from the Astro file has styles as expected:

<!DOCTYPE html><html data-astro-cid-yjwvcixp> <head><style>div[data-astro-cid-yjwvcixp]{background:red}
</style></head> <body data-astro-cid-yjwvcixp><div data-astro-cid-yjwvcixp>   
Some content here
    </div></body></html>

What's the expected result?

Styles should be present in output from both Markdown and Astro.

Link to Minimal Reproducible Example

https://stackblitz.com/edit/github-ttu457

Participation

[ ] I am willing to submit a pull request for this issue.

bluwy commented 6 months ago

The issue is that when you call const hasContent = !/^\s*$/.test(await Astro.slots.render("test"));, the styles are included in the rendered string. Styles are hoisted to the top of the head, so it should only render once, and it rendered once in the Astro.slots.render. Hence it didn't render again in the actual <slot/>.

A workaround is to put some sort of HTML element in TestA.astro at the top, like <div></div> or <head></head>. This way Astro has a chance to render the head first knowing it is safe to render the head there.

As for a fix, maybe we can skip the "render once" check when we render via Astro.slots.render? But that could be tricky to implement.

innermatrix commented 6 months ago

Thanks; I'l try the workaround later.

As I am sure aware, this whole thing is coming up because I am trying to do what I expected slots.has would do. So maybe another option would be to create an API on Astro.slots (or add a flag to slots.render) who purpose would be for the caller to indicate that it wants to inspect the content, not include it in the output, which I think would allow Astro to decide that the styles should be included somewhere else when the content is rendered to be included.

Like Astro.slots.render = produce the slot content and include its style in the output (if it's rendered a 2nd time later, its styles will not be included for a 2nd time); but Astro.slots.prefetch = produce the slot content and don't include its styles in the output (you have to separately call render, directly or indirectly, to get the styles pulled in).

innermatrix commented 6 months ago

@bluwy your proposed workaround produces a malformed document:

<!DOCTYPE html><style>div[data-astro-cid-yjwvcixp]{background:red}
</style><div></div> <html data-astro-cid-yjwvcixp> <head></head> <body data-astro-cid-yjwvcixp><div data-astro-cid-yjwvcixp>   <p>Some content here</p>    </div></body></html>

The issue here is that I already have a head — it's in TestC — so adding anything to either TestA or TestB that causes Astro to put the styles there just results in styles before head, which doesn't work.

As I understand it, the crux of my setup is

TestA uses TestB which uses TestC
TestC has the head
TestB also involves a slots.render
slots.render in TestB happens before head in TestC → styles are discarded

Any other ideas? Obviously in the MWE I could refactor this to move the head into TestA but in my actual code that's not really a good option.

innermatrix commented 6 months ago

Okay, so the best that I have come up with for a workaround is that

If I have a component X
and the rendered tree of X contains head
and X references any component that might call slots.render and not include the result in its output
then X must make sure that head is rendered before the first call to slots.render that might throw away its result.

For example

Good:

<head/>
<SomeComponent>
<CallSlotsRenderInsideThis/>
</SomeComponent>

Also good:

<SomeComponentThatOutputsHead/>
<SomeComponent>
<CallSlotsRenderInsideThis/>
</SomeComponent>

Bad:

<SomeComponentThatOutputsHead>
<CallSlotsRenderInsideThis/>
</SomeComponentThatOutputsHead>

Because in that last case CallSlotsRenderInsideThis will be evaluated before SomeComponentThatOutputsHead and therefore before head, resulting in loss of styles; in the other two cases, CallSlotsRenderInsideThis happens after head.

So with that knowledge, I have found the minimal refactor that I can make in my code, and I can live with it for now.

However, in general this means that calling slots.render inside a component has really weird effects on how that component can be used in the render tree, because slots.render cannot safely precede head in the render order. This is very surprising, and makes slots.render challenging to use robustly.

matthewp commented 5 months ago

I think slots.render() is over-used in general. It should be a pretty rare thing that you need to mutate the HTML of another component. Most frameworks don't give you the ability to do that.

I know that a lot of people use it to check if a slot was passed, to render something different if not. I think this can also be implemented like this:

<div class="content"><slot /></div>
<div class="fallback">Fallback here...</div>

<style>
  .fallback {
    display: none;
  }

  .content:empty + .fallback {
    display: block;  
  }
</style>

In any event, I can't think of a way to fix the issue you're experiencing here.

innermatrix commented 5 months ago

Using client-side CSS to determine if a non-empty slot was passed is antithetical to static rendering, which is the whole point of using Astro. I understand why you would offer that as a solution (because it's better than nothing), but let's just acknowledge it for a gross hack that it is.

Maybe I am missing something but the obvious way to fix the issue I am experiencing here is to make slots.has work, instead of it being useless and then people having to use slots.render to work around it, only to be told that slots.render should not be used because of issues like this one.

matthewp commented 5 months ago

We can't know if a slot is going to result in no HTML unless we render it. We can't "fix" this in a way that won't just result in the same problem being described in this issue.

matthewp commented 5 months ago

It might be fixable by rethinking the slots APIs altogether though. It would be great to understand the use case more clearly. Is the use case purely about having fallbacks when the slot contents are empty? If that's the case we might be able to come up with something.

Any solution probably means getting rid of conditional slot rendering (getting rid of slots.render() I think though.

innermatrix commented 5 months ago

It would be great to understand the use case more clearly. Is the use case purely about having fallbacks when the slot contents are empty? If that's the case we might be able to come up with something.

My specific use case is 100% about trying to get a component with a slot to use the slot fallback content when the slot is passed in but its content is empty. I believe the specific situation I ran into this was along the lines of:

<X>
<Fragment slot="slot">
{predicate ? <Thing/> : undefined}
</Fragment>
</X>

(but with a a bunch of additional intervening layers, so the obvious solution of moving the entire slot inside the ternary was not available) where IIRC predicate being false leads to whitespace being passed into the slot, which then causes slot fallback to not be rendered. I don't have a MWE handy rn but I can try to put one together later.

mpstaton commented 1 month ago

Good:
<head/>
<SomeComponent>
<CallSlotsRenderInsideThis/>
</SomeComponent>
Also good:
<SomeComponentThatOutputsHead/>
<SomeComponent>
<CallSlotsRenderInsideThis/>
</SomeComponent>
Bad:
<SomeComponentThatOutputsHead>
<CallSlotsRenderInsideThis/>
</SomeComponentThatOutputsHead>
Because in that last case CallSlotsRenderInsideThis will be evaluated before SomeComponentThatOutputsHead and therefore before head, resulting in loss of styles; in the other two cases, CallSlotsRenderInsideThis happens after head.

So with that knowledge, I have found the minimal refactor that I can make in my code, and I can live with it for now.

However, in general this means that calling slots.render inside a component has really weird effects on how that component can be used in the render tree, because slots.render cannot safely precede head in the render order. This is very surprising, and makes slots.render challenging to use robustly.

Can you explain this for a Noob? I'm getting the same problem and your explanation is above my paygrade.

innermatrix commented 1 month ago

@mpstaton sure. Do whatever it takes to rearrange your component tree so that your use of slots.render is

Not a descendant of the <head> and
Not a descendant of the component that directly contains the <head>

Or to put yet differently: if <head> is in the component X, then move slots.render out of X and out of anything that might be inside X.

mpstaton commented 1 month ago

Not a descendant of the <head> and

Not a descendant of the component that directly contains the <head>

How do I look to see if something is a descendant of the <head>? I don't see any head tag anywhere.
I just Googled how to find the Head in Astro and that doesn't have anything, first result is https://www.eurogamer.net/fortnite-astro-heads-locations-7013

withastro / astro