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

Baffling loss of styles in Markdown #11079

Open
1 task
innermatrix opened this issue May 17, 2024 · 12 comments
Open
1 task

Baffling loss of styles in Markdown #11079

innermatrix opened this issue May 17, 2024 · 12 comments
Labels
needs discussion Issue needs to be discussed

Comments

@innermatrix
Copy link

innermatrix commented May 17, 2024

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.
@github-actions github-actions bot added the needs triage Issue needs to be triaged label May 17, 2024
@bluwy
Copy link
Member

bluwy commented May 17, 2024

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.

@bluwy bluwy added needs discussion Issue needs to be discussed and removed needs triage Issue needs to be triaged labels May 17, 2024
@innermatrix
Copy link
Author

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
Copy link
Author

innermatrix commented May 17, 2024

@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
Copy link
Author

innermatrix commented May 18, 2024

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
Copy link
Contributor

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
Copy link
Author

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
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Author

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
Copy link

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
Copy link
Author

@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
Copy link

mpstaton commented Sep 22, 2024

  • 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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs discussion Issue needs to be discussed
Projects
None yet
Development

No branches or pull requests

4 participants