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

rules for slot attribute and let directive #18

Closed
wants to merge 1 commit into from

Conversation

tanhauhau
Copy link
Member

@tanhauhau tanhauhau commented Mar 1, 2020

@tanhauhau tanhauhau changed the title initial rfc for slot attribute rules rules for slot attribute and let directive Mar 1, 2020
@arxpoetica
Copy link
Member

Remind me where we're supposed to comment on RFCs? Here?

@tanhauhau
Copy link
Member Author

@arxpoetica yup. free to comment it here. 👍

@rixo
Copy link

rixo commented Mar 15, 2020

Sorry for being late to the party, this had escaped my vigilance.

As I understand the RFC, this is mostly increasing stability by removing buggy edge cases, and adding more meaningful compiler errors. Of course, I'm all in favor of this.

There is just this point:

  1. No nesting of slot attribute:

Is there a hard technical reason for this? Like it is really too complicated to implement in a sane way...

Because I feel there could be some fun applications in userland wit slot nesting. Typically, when publishing components for other to use, I think there might be interesting patterns allowing to provide advanced default contents, and default contents of default contents, etc.

I think I'd prefer if it remained possible. Now, if the current implementation is already buggy, and it is hard to overcome, I understand it is the right move to disallow it clearly and pop an informative error message. At least for now...

And, more importantly, I want to say I can't wait to see something like this supported:

<Component>
  <svelte:slot let:a slot="foo">
    <OtherComponent>{a}</OtherComponent>
  </svelte:slot>
</Component>

I have no special attachment to the syntax ({slot foo} or ` would work equally well for me), but I think it would be a huge improvement if we can remove the limitation that requires to have an element with named slots (i.e. #1037).

It's the single feature I miss the most in Svelte currently. It would really eases things up for integration of third party libraries, where a precise element structure is often expected. And component publishers too. And, generally, it would open new perspectives for what we can do with slots. The upvotes in #1037 shows that I'm not alone in thinking this.

@tanhauhau Any chance supporting this is in your plans? As I understand it, your related PR would open the way for it?

Oh, and thanks a lot for all your bug smash and all. Removing all those pesty edge cases makes Svelte really better for all of us ❤️

@tanhauhau
Copy link
Member Author

tanhauhau commented Mar 16, 2020

I think there might be interesting patterns allowing to provide advanced default contents

You can provide content for the default content https://svelte.dev/repl/0820cef07e3d4a41a4bccbb2040493f6?version=3.20.0

I still don't see a reason for having nested slot template, (element with slot="..." or <svelte:slot>)

Any chance supporting this is in your plans?

yes, with sveltejs/svelte#4556, you should be able to write

<Component>
  <svelte:slot slot="foo">
      <div>hello</div>
      <div>world</div>
  </svelte:slot>
</Component>

<Component>
  <svelte:slot slot="foo">
      <OtherComponent />
  </svelte:slot>
</Component>

@rixo
Copy link

rixo commented Mar 25, 2020

Sorry for taking so long to come back at this...

So, you're right, your REPL is exactly what I had in mind, "content for the default content". I misread the RFC. And I also totally agree with your opinion and proposed solutions about nesting slots on the consumer side.

And yes, your 2nd example with OtherComponent is precisely what I was hoping for.

So... Thanks again! I hope the PR will land soon :)

@AlexxNB
Copy link

AlexxNB commented May 2, 2020

As we already have elements {@html .. } and {@debug} - may we use new {@slot...} tag instead <svelte:slot...>? As for me it may be more svelty and less boilerplate.

Instead <svelte:slot slot="foo"> we may use {@slot "foo"} or even better {@slot:foo} to avoid using expression as slot name.

Common examples will be look like this:

<Component>
   Content for default slot
</Component>

<Component>
  {@slot:foo}Content of named slot{/slot}
  Content for default slot
</Component>

<Component>
  {@slot:foo}
      <div>hello</div>
      <div>world</div>
  {/slot}
</Component>

<Component>
  {@slot:foo}<OtherComponent/>{/slot}
</Component>

<!-- let-derictive replacement -->
<Component>
  {@slot:foo a,b,c}
      <OtherComponent>A is {a}</OtherComponent>
      <OtherComponent>B is {b}</OtherComponent>
      <OtherComponent>C is {c}</OtherComponent>
  {/slot}
</Component>

<!-- they are same -->
<Component let:a>
  A is {a}
</Component>
<!-- and -->
<Component>
  {@slot a}A is {a}{/slot}
</Component>

@milkbump
Copy link

I personally prefer the syntax suggestion by @AlexxNB, but for consistency's sake we should probably use {#slot:foo} instead of {@slot:foo} if this syntax is adopted.

@rixo
Copy link

rixo commented May 23, 2020

The problem with using a custom syntax like {#slot} is that we lose the parallel that already exists with native (custom elements) <slot />, and where slot="name", like Svelte currently, also has to be assigned to an element (e.g. <div slot="name">).

Slots can already be pretty confusing to comprehend, I think it is better to stick to what people (may) already know.

In fact, even <svelte:slot /> feels a bit confusing because it introduces a new kind of slot, where the concept is already a bit crowded (there the <slot /> in the parent component, and the target slot="name" for the slot content).

Maybe something more neutral just meaning a virtual element / no-element container would better express the intention? And regarding the syntax, maybe it would also feel less repetitive / boilerplaty than <svelte:slot slot="name" />... Maybe something like <svelte:fragment slot="name"> or <svelte:virtual slot="name">?

@milkbump
Copy link

@rixo I agree with your first point about the presumed complexity of slots. But it's precisely because of your second point about the "boilerplate-y" nature of <svelte:slot slot="name" with "slot" used twice that I prefer {#slot:foo}.

That said, if we use the <svelte: tag syntax, I don't think using <svelte:virtual or <svelte:fragment agrees with your argument about slot complexity. I think <svelte:slot is much easier to understand and teach.

@tanhauhau
Copy link
Member Author

The reason I am proposing <svelte:slot slot="foo"> over {#slot:foo} is that, it's more familiar to have let: attribute and possibly bind: (sveltejs/svelte#3617) in future:

<svelte:slot slot="foo" let:a={b}>
</svelte:slot>

vs

{#slot:foo let:a=b} <-- not sure how to express attribute in a more familiar syntax
{/slot}

@Rich-Harris
Copy link
Member

quick thoughts:

  • definitely agree it needs to be something element-ish, i.e. <svelte:xxx>. Also agree that <svelte:slot> is perhaps a little confusing since it replaces the slot attribute rather than the slot element, so <svelte:fragment> would make more sense
  • I'd suggest that the rule shouldn't be against nesting slots specifically, but rather using slot="foo" anywhere other than as a direct child of a component should be considered invalid
  • That probably shouldn't apply to slots inside custom elements? Not 100% sure what the rules are there, but we should presumably stick to them
  • Basically agree with this whole RFC but since some aspects are technically breaking changes we will probably need to just warn loudly for now, and continue with the current behaviour if someone does nest slots or have duplicates or whatever. Unless we choose to use the semver ju-jitsu of calling the changes a bugfix

@dummdidumm
Copy link
Member

Snippets will replace slots (they are still around but deprecated) in Svelte 5 and will remove the inconsistencies present with slots. Therefore closing this RFC - thank you.

@dummdidumm dummdidumm closed this Nov 16, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants