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

Astro.slot.render() with arguments does not work with Fragment #6683

Closed
1 task
BryceRussell opened this issue Mar 28, 2023 · 18 comments
Closed
1 task

Astro.slot.render() with arguments does not work with Fragment #6683

BryceRussell opened this issue Mar 28, 2023 · 18 comments
Assignees
Labels
- P2: nice to have Not breaking anything but nice to have (priority) needs triage Issue needs to be triaged

Comments

@BryceRussell
Copy link
Member

BryceRussell commented Mar 28, 2023

What version of astro are you using?

2.1.7

Are you using an SSR adapter? If so, which one?

None

What package manager are you using?

pnpm

What operating system are you using?

Windows

What browser are you using?

Firefox

Describe the Bug

When Astro.slots.render() is called with arguments it does not handle <Fragment>s correctly and throws an error.

// Slot.astro
<Fragment set:html={Astro.slots.render('name', ['arg'])}>

Does NOT Work:

<Slot>
  <Fragment slot="name">
    {arg => <span>{arg}</span>}
  </Fragment>
</Slot>

Works:

<Slot>
  <frag slot="name">
    {arg => <span>{arg}</span>}
  </frag>
</Slot>

This is the line that throws the error: https://github.com/withastro/astro/blob/main/packages/astro/src/core/render/result.ts#L106

It looks like the <Fragment> is being rendered into HTML before it can be rendered with the arguments from Astro.slots.render()

Link to Minimal Reproducible Example

https://stackblitz.com/edit/github-ucqonn?file=src/pages/index.astro

Participation

  • I am willing to submit a pull request for this issue.
@matthewp matthewp added the - P4: important Violate documented behavior or significantly impacts performance (priority) label Mar 28, 2023
@matthewp
Copy link
Contributor

Thanks!

@JerryWu1234
Copy link
Contributor

I will be looking this bug.

@JerryWu1234
Copy link
Contributor

this bug should be closed

@BryceRussell
Copy link
Member Author

BryceRussell commented Jun 28, 2023

@JerryWu1234 So far this bug has only been partially fixed, PR #6832 fixed this case:

<Fragment slot="name">
    Text
</Fragment>

But, this case has not been fixed:

<Fragment slot="name">
  {arg => <span>{arg}</span>}
</Fragment>

@JerryWu1234
Copy link
Contributor

${renderComponent($$result, "Slot", $$Slot, {}, { "name": ($$result2) => renderTemplate`${renderComponent($$result2, "Fragment", Fragment, { "slot": "name" }, { "default": ($$result3) => renderTemplate`${(arg) => renderTemplate`<p>${arg}</p>`}` })}` })}

Because of the nested element, there is a workaround that uses the original tags (‘div’, ‘span’).

such as

<div slot="name">
  {arg => <span>{arg}</span>}
</div>

@JerryWu1234
Copy link
Contributor

I think that it requires a lot of work if we want to fix this bug. Perhaps my idea is not a good one. So please share your idea if you have any idea to fix this bug.

this bug is derived from this line

yield* renderChild(child());

@natemoo-re
Copy link
Member

Thanks for looking into this one @JerryWu1234! I agree that this would be awesome to fix but it will take quite a bit of work.

One possible idea is to adjust our compiler output so that we generate an optimized statement for cases like this. That's probably the cleanest approach I can think of, since we'd need to deal with scope tracking across async boundaries otherwise.

@natemoo-re
Copy link
Member

For my own future reference, investigating this was Linear ticket PLT-556. Considering the "investigation" part done, but hoping we can circle back with a proper fix soon.

@lilnasy
Copy link
Contributor

lilnasy commented Oct 27, 2023

@BryceRussell What is the use case for <Fragment slot="name"> vs <frag slot="name">?

@BryceRussell
Copy link
Member Author

@lilnasy For me, <Fragment> feels more intuitive and semantically correct vs <frag> feeling more like a hack. I think most people trying to pass a function to a named slot would try to use <Fragment> first and when that doesn't work they would have to do some trial and error to discover that <frag> also works.

@lilnasy lilnasy added the - P2: nice to have Not breaking anything but nice to have (priority) label Oct 27, 2023
@lilnasy
Copy link
Contributor

lilnasy commented Oct 27, 2023

Yeah, that's fair. To clarify, the tag name "frag" is not special, any plain HTML tag works and they are documented with named slots. We don't document Fragment with it.

@lilnasy lilnasy removed the - P4: important Violate documented behavior or significantly impacts performance (priority) label Oct 27, 2023
@BryceRussell
Copy link
Member Author

BryceRussell commented Oct 27, 2023

Ya I was just using frag to standardize my examples, but the fact that it can be any HTML tag is what feels unintuitive/hacky to me. When using a HTML tag to pass a function it is implied that the tag will be included in the markup of the slot like it is when passing HTML, but it is not and it instead acts more like a <Fragment>. If you were able to use a <Fragment> to pass a function it would be more implicit that your passing the function itself and not the tag too.

@lilnasy lilnasy self-assigned this Feb 9, 2024
@lilnasy
Copy link
Contributor

lilnasy commented Feb 23, 2024

But, this case has not been fixed:

<Fragment slot="name">
  {arg => <span>{arg}</span>}
</Fragment>

In this case, the function is the default slot of the Fragment component. If astro's rendering was architected differently, it would be simple for the Fragment to "passthrough" the function to whatever the Fragment itself is slotted into, but it currently isn't.

Related: #7610 (comment)

Closing as a wontfix.

@lilnasy lilnasy closed this as not planned Won't fix, can't repro, duplicate, stale Feb 23, 2024
@BryceRussell
Copy link
Member Author

BryceRussell commented Feb 23, 2024

@lilnasy

This used to work (using a psuedo <Fragment> like <fragment>) in previous versions of Astro, is there a reason this is not supported anymore? I built a library of components that uses this principal of passing template functions to named slot to customize the rendering of a component https://github.com/BryceRussell/astro-headless-ui/wiki#named-slot-functions would this be considered a regression?

@lilnasy
Copy link
Contributor

lilnasy commented Feb 23, 2024

I'm still looking through to see if <Fragment slot="name"> behavior changed in recent versions, but the pseudo-rendering you mention does still work - instead of the Fragment component, the function could be placed within any plain html element that has a slot attribute.

@lilnasy
Copy link
Contributor

lilnasy commented Feb 23, 2024

I'm re-opening until we know whether/when the behavior changed.

@lilnasy lilnasy reopened this Feb 23, 2024
@github-actions github-actions bot added the needs triage Issue needs to be triaged label Feb 23, 2024
@lilnasy
Copy link
Contributor

lilnasy commented Feb 23, 2024

I see this output from 2.10 to 4.4. The whitespace in the resulting html changed a bit, but that's it.

image

@BryceRussell
Copy link
Member Author

BryceRussell commented Feb 23, 2024

@lilnasy I was doing some testing on this and I figured out that it was breaking on my end because I was using tags with dashes -:

  • <tag slot="named">{() => ...}: tag is NOT included in the markup, arguments ARE passed to slot function
  • <tag-with-dashes slot="named">{() => ...}: tag IS included in the markup, arguments are NOT passed to slot function

I think it is safe to close this issue, thanks!

@lilnasy lilnasy closed this as not planned Won't fix, can't repro, duplicate, stale Feb 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
- P2: nice to have Not breaking anything but nice to have (priority) needs triage Issue needs to be triaged
Projects
None yet
Development

No branches or pull requests

5 participants