search

sveltejs / svelte

Cybernetically enhanced web apps
https://svelte.dev
MIT License
76.89k stars 3.99k forks source link

Petition to NOT deprectate slots in Svelte v5 #11471

Closed webJose closed 3 weeks ago

webJose commented 3 weeks ago

Describe the problem

While snippets seem to be more versatile in general, replacing slots with snippets seems to be just plain awful, and I will share a Bootstrap example that, in my opinion, makes the problem very evident: Writing snippets for every slot makes code almost unbearable to read.

Bootstrap Card Example

We have a component library for Bootstrap, and in it we have Svelte components for every possible piece of a card (https://getbootstrap.com/docs/5.3/components/card/), such as header, foooter, body, top image, title, etc.

Authoring a card with the slotted components is straight forward:

<Card let:Body let:Title let:Header class="m-3">
  <Header>
    <Title>The Card's Title</Title>
  </Header>
  <Body>
    <p>
      This is some card content inside the body of the card.
    </p>
  </Body>
</Card>

Now, if I were to replace slots with properties for snippets, building the card would look like the following:

{#snippet myBody(Title, Header, Body)}
  {#snippet header()}
    <Title body={title} />
    {#snippet title()}
      The Card's Title
    {/snippet}
  {/snippet}
  <Header body={header} />
  {#snippet myInnerBody()}
    <p>
      This is some card content inside the body of the card.
    </p>
  {/snippet}
  <Body body={myInnerBody} />
{/snippet}
<Card body={myBody} class="m-3" />

NOTE: Both examples provide slot variables or parameters to the snippet for the "inner" components, such as the header, title and body components. This is a nice trick to avoid consumers to have to import 4 or more "card parts" every time.

The slotted example is 10 lines; the snippet'ed example is 16. 60% line increase. Yes, Probably in real scenarios the gap closes fast. This is most likely due to the opening and closing on snippets, which is more or less a fixed "cost". Still, significant for quick cards that I bet exist in the real world.

Then we move to code examination, which is what bothers me the most: The first example clearly gives an accurate representation of how the card will be laid out when rendered. The second example does not. Everything is backwards, we have to come up with extra names (the snippet names) and the general flow of the data does not represent the card's layout nearly as well as the slotted counterpart. Sure, I could put the line above the snippet and get it to look a bit better. It still is messier and harder to read. Now imagine more complex card with list groups, images and who knows what else. I think this strategy doesn't scale well at all.

Because of these reasons, I would like to request that slots remain un-deprecated in Svelte v5.

Describe the proposed solution

What if snippets were to be placed inside as if they were slotted content?

<Card>
  {#snippet body(Title, Header, Body)}
    <Header>
        {#snippet body()}
            <Title>
              {#snippet body()}
                The Card's Title
              {/snippet}
            </Title>
        {/snippet}
    </Header>
    <Body>
      {#snippet body()}
        <p>
          This is some card content inside the body of the card.
        </p>
      {/snippet}
    </Body>
  {/snippet}
</Card>

The snippets are all named body because that's the property's name in each of the components (Card, CardTitle, CardHeader and CardBody). The snippet name pair it to the property. This setup fixes the layout of the code. However, this is 20 lines!

So an improvement:

<Card(Title, Body, Header)>
  <Header>
    <Title>
      The Card's Title
    </Title>
  </Header>
  <Body>
    <p>
      This is some card content inside the body of the card.
    </p>
  </Body>
</Card>

This is 12 lines. All the {#snippet} lines gone. If the snippet provides parameters (or arguments, depending on where you stand) and there is only one in the list of props, the "HTML element" is given the function syntax to be able to name said parameters, which are used inside the "slot" (really the snippet contents as per compiler's magic). If the only snippet property doesn't provide parameters, then the syntax looks identical to that of a slotted content without slot variables.

For Cases with More than One Snippet Property

<ComponentMultiSnippet>
  <snippetPropertyName(params)>
  </snippetPropertyName>
</ComponentMultiSnippet>

Basically the same "function" syntax but over a sub-element whose name is the name of the snippet property.

All this would not preclude passing snippets around as props as they have been currently conceived. All this would be compiler sugar.

Importance

would make my life easier

webJose commented 3 weeks ago

BTW, the proposed syntax seems to eliminate the need for <svelte:fragment> to "slot" content, so I guess {@const} and probably others would need refactor to be allowed immediately after the pseudo element named after the snippet property.

webJose commented 3 weeks ago

Also, if you downvote, can I ask that you provide a comment explaining the reason? Why you disagree. Am I mistaken? Did I do this incorrectly and it is my own fault that I perceive this as a problem? Or perhaps you are perfectly content with the mess that snippets produce? I guess that's a valid opinion too.

CaptainCodeman commented 3 weeks ago

I don't think your examples are fairly representing how the code would look using snippets.

webJose commented 3 weeks ago

Thanks, @CaptainCodeman for your input. Could you provide a more fair example? Or perhaps refactor mine? I just learned about snippets 2 days ago. It would be nice if I were told the best practices around snippets as slots.

webJose commented 3 weeks ago

Real World Example

This issue is really running in circles in my head because I love Svelte so much. I decided to take a simple component from a project from work: A Bootstrap toolbar with merely 11 controls. Here I present the Svelte v4 template:

<ButtonToolbar class="py-1">
    <div class="row flex-fill g-2" style:z-index="5">
        <div class="col-auto flex-fill">
            <div class="d-flex gap-1 justify-content-center justify-content-xxl-start align-items-end">
                <Button
                    size="sm"
                    buttonStyle="secondary"
                    on:click={refreshData}
                >
                    <Icon name="refresh" />
                </Button>
                <UserSelect
                    items={filteredOwners}
                    bind:selectedUser={$currentOwner}
                    text="Search by owner:"
                    on:search={({ detail }) => filteredOwners = filterExistingOwners(detail)}
                    on:clear={() => filteredOwners = []}
                />
                <div class="d-flex flex-row gap-1 align-items-end">
                    <Button
                        size="sm"
                        buttonStyle="danger"
                        outlined
                        disabled={!$currentOwner}
                        on:click={() => dispatch('ownerAction', 'remove')}
                    >
                        Remove Owner
                    </Button>
                </div>
                <UserSelect
                    items={newOwnersResult}
                    placeholder="WWID or <last, first>"
                    minValueLength={4}
                    bind:selectedUser={$newOwner}
                    text="New owner:"
                    on:search={({ detail }) => newOwnersResult = searchForUsers(detail)}
                    on:clear={() => newOwnersResult = Promise.resolve([])}
                />
                <div class="d-flex flex-row align-items-end gap-1">
                    <Button
                        size="sm"
                        buttonStyle="info"
                        outlined
                        disabled={!$currentOwner || !$newOwner}
                        on:click={() => dispatch('ownerAction', 'replace')}
                    >
                        Replace Owner
                    </Button>
                    <Button
                        size="sm"
                        buttonStyle="success"
                        outlined
                        disabled={!$newOwner}
                        on:click={() => dispatch('ownerAction', 'add')}
                    >
                        Add Owner
                    </Button>
                </div>
            </div>
        </div>
        <div class="col-xxl-4">
            <div class="h-100 d-flex align-items-center align-items-xxl-end justify-content-center justify-content-xxl-end align-items-end">
                <div class="d-flex flex-row align-items-baseline gap-1">
                    <ButtonGroup label="Operations to submit" size="sm" class="rounded-3 summary">
                        <BareCheckbox
                            class="mb-0"
                            value="add"
                            bind:group={activeOperations}
                            style="success"
                        >
                            {aggregatedData.add} addition{aggregatedData.add !== 1 ? 's' : ''}
                        </BareCheckbox>
                        <BareCheckbox
                            class="mb-0"
                            value="replace"
                            bind:group={activeOperations}
                            style="info"
                        >
                            {aggregatedData.replace} replacement{aggregatedData.replace !== 1 ? 's' : ''}
                        </BareCheckbox>
                        <BareCheckbox
                            class="mb-0"
                            value="remove"
                            bind:group={activeOperations}
                            style="danger"
                        >
                            {aggregatedData.remove} deletion{aggregatedData.remove !== 1 ? 's' : ''}
                        </BareCheckbox>
                    </ButtonGroup>
                    <Button size="sm" buttonStyle="secondary" disabled={actionButtonDisabled} on:click={undoChangesHandler}>Undo</Button>
                    <Button size="sm" buttonStyle="primary" disabled={actionButtonDisabled}>Submit</Button>
                </div>
            </div>
        </div>
    </div>
</ButtonToolbar>
{#if $showRefreshConfirmation}
    <RefreshConfirmation on:close={({ detail }) => processRefreshConfirmation(detail)}/>
{/if}

The original template is 99 lines. Now the template after migrating slots to snippets:

{#snippet buttonToolbar()}
    <div class="row flex-fill g-2" style:z-index="5">
        <div class="col-auto flex-fill">
            <div class="d-flex gap-1 justify-content-center justify-content-xxl-start align-items-end">
                {#snippet btnRefresh()}
                    <Icon name="refresh" />
                {/snippet}
                <Button
                    size="sm"
                    buttonStyle="secondary"
                    on:click={refreshData}
                    body={btnRefresh}
                />
                <UserSelect
                    items={filteredOwners}
                    bind:selectedUser={$currentOwner}
                    text="Search by owner:"
                    on:search={({ detail }) => filteredOwners = filterExistingOwners(detail)}
                    on:clear={() => filteredOwners = []}
                />
                <div class="d-flex flex-row gap-1 align-items-end">
                    {#snippet btnRemoveOwner()}
                        Remove Owner
                    {/snippet}    
                    <Button
                        size="sm"
                        buttonStyle="danger"
                        outlined
                        disabled={!$currentOwner}
                        on:click={() => dispatch('ownerAction', 'remove')}
                        body={btnRemoveOwner}
                    />
                </div>
                <UserSelect
                    items={newOwnersResult}
                    placeholder="WWID or <last, first>"
                    minValueLength={4}
                    bind:selectedUser={$newOwner}
                    text="New owner:"
                    on:search={({ detail }) => newOwnersResult = searchForUsers(detail)}
                    on:clear={() => newOwnersResult = Promise.resolve([])}
                />
                <div class="d-flex flex-row align-items-end gap-1">
                    {#snippet btnReplaceOwner()}
                        Replace Owner
                    {/snippet}    
                    <Button
                        size="sm"
                        buttonStyle="info"
                        outlined
                        disabled={!$currentOwner || !$newOwner}
                        on:click={() => dispatch('ownerAction', 'replace')}
                        body={btnReplaceOwner}
                    />
                    {#snippet btnAddOwner()}
                        Add Owner
                    {/snippet}
                    <Button
                        size="sm"
                        buttonStyle="success"
                        outlined
                        disabled={!$newOwner}
                        on:click={() => dispatch('ownerAction', 'add')}
                    />
                </div>
            </div>
        </div>
        <div class="col-xxl-4">
            <div class="h-100 d-flex align-items-center align-items-xxl-end justify-content-center justify-content-xxl-end align-items-end">
                <div class="d-flex flex-row align-items-baseline gap-1">
                    {#snippet buttonGroup()}
                        {#snippet additionsCb()}
                            {aggregatedData.add} addition{aggregatedData.add !== 1 ? 's' : ''}
                        {/snippet}    
                        <BareCheckbox
                            class="mb-0"
                            value="add"
                            bind:group={activeOperations}
                            style="success"
                            body={additionsCb}
                        />
                        {#snippet replacementsCb()}
                            {aggregatedData.replace} replacement{aggregatedData.replace !== 1 ? 's' : ''}
                        {/snippet}
                        <BareCheckbox
                            class="mb-0"
                            value="replace"
                            bind:group={activeOperations}
                            style="info"
                            body={replacementsCb}
                        />
                        {#snippet deletionsCb()}
                            {aggregatedData.remove} deletion{aggregatedData.remove !== 1 ? 's' : ''}
                        {/snippet}        
                        <BareCheckbox
                            class="mb-0"
                            value="remove"
                            bind:group={activeOperations}
                            style="danger"
                            body={deletionsCb}
                        />
                    {/snippet}
                    <ButtonGroup label="Operations to submit" size="sm" class="rounded-3 summary" body={buttonGroup}>
                    {#snippet btnUndo()}
                        Undo
                    {/snippet}
                    <Button size="sm" buttonStyle="secondary" disabled={actionButtonDisabled} on:click={undoChangesHandler} body={btnUndo} />
                    {#snippet btnSubmit()}
                        Submit
                    {/snippet}
                    <Button size="sm" buttonStyle="primary" disabled={actionButtonDisabled} body={btnSubmit} />
                </div>
            </div>
        </div>
    </div>
{/snippet}
<ButtonToolbar class="py-1" body={buttonToolbar} />
{#if $showRefreshConfirmation}
    <RefreshConfirmation on:close={({ detail }) => processRefreshConfirmation(detail)} />
{/if}

This is 120 lines. A 20% increase in code when counted by lines, on a real-world example, so let's say we dropped from 60% to 20%. There go the gains in code reduction when compared to React. 😢 (I have calculated in the past some 30% code reduction moving from React to Svelte).

What's Terrible In This

I find particularly terrible to have to add 3 lines of code just to put the word "Undo" inside a button. I went from:

                    <Button size="sm" buttonStyle="secondary" disabled={actionButtonDisabled} on:click={undoChangesHandler}>Undo</Button>

To:

                    {#snippet btnUndo()}
                        Undo
                    {/snippet}
                    <Button size="sm" buttonStyle="secondary" disabled={actionButtonDisabled} on:click={undoChangesHandler} body={btnUndo} />

Is there really no more concise syntax? What happened to the Svelte of days that combatted boilerplate? Let me quote learn.svelte.dev - Auto-subscriptions lesson:

It starts to get a bit boilerplatey though, especially if your component subscribes to multiple stores. Instead, Svelte has a trick up its sleeve — you can reference a store value by prefixing the store name with $:

Then I hate the fact that I have to come up with unique names for snippets. I am the kind of guy that embeds the <input> elements inside the <label> elements just so I don't have to come up with an ID.

I'm not against snippets. I'm against deprecating slots in favor of snippets in their current form. Svelte is a compiler: Let Svelte convert my slots to snippets. I realize I came late to the feedback party, so I ask that you at least don't deprecate slots.

brunnerh commented 3 weeks ago

This is a nice trick to avoid consumers to have to import 4 or more "card parts" every time.

It is not a nice trick, it is an abuse of syntax.

If you change it to imports, you just get this with snippets:

<Card class="m-3">
  <Header>
    <Title>The Card's Title</Title>
  </Header>
  <Body>
    <p>
      This is some card content inside the body of the card.
    </p>
  </Body>
</Card>

Contents of components implicitly create a children snippet that can be retrieved from $props.

Also, instead of abusing slot props to save imports, you could also store sub-components as properties of the main component:

<Card class="m-3">
  <Card.Header>
    <Card.Title>The Card's Title</Card.Title>
  </Card.Header>
  <Card.Body>
    <p>
      This is some card content inside the body of the card.
    </p>
  </Card.Body>
</Card>
webJose commented 3 weeks ago

Hello, @brunnerh. Thanks for stopping by.

It is not a nice trick, it is an abuse of syntax.

It is nicer than importing all the time, but granted: You have a nicer trick. 😄 How do you do the sub-components as properties? Like this?

<script lang="ts" context="module">
    export const Body = CardBody;
</script>

I like it.

Ok, children is something that appears to solve the default slot for sure. The automatic behavior is what I was asking for, and it seems children does it!! I'm so glad!

Question: I have components with up to 7 slots, I think. For the cases where we have named slots, will I be forced to write snippets like I did in the examples I presented? In other words, is the grievance I feel isolated to named slots?

brunnerh commented 3 weeks ago

For named slots you will still have to define snippets explicitly, but you can also put them inside the component and they will automatically be passed as property with the name of the snippet.

To have components as properties of others, using a separate file might be a good idea. E.g. something like this:

// card/index.js
import Card from './card.svelte';
import Body from './body.svelte';
...

Card.Body = Body;

export default Card;

(With TS, additional typing will be necessary.)

webJose commented 3 weeks ago

@brunnerh what about default slot with slot variables? I don't see any mention of syntax that replaces the let:XXX properties (I'm reading this page). Please don't tell me that children only works with no properties. That would make children a half-solution. 😞 If my default slots pass variables, then I must add {#snippet snippetName(vars)}, or so it seems. Yes?

By the way, I realize that if I had found that page before I tested (and panicked about) snippets, I would not have been as scandalized. I guess I used an old or incomplete article to read about them. I think you alleviated most of my concerns. Thank you for that.

brunnerh commented 3 weeks ago

If children needs arguments, you would have to declare it to define the arguments. It is a bit more verbose but gets rid of the weird scoping behavior of let props and is more explicit in general.

webJose commented 3 weeks ago

Ok, I'm satisfied with my new testing. I panicked over incomplete information. Thank you very much to those that corrected my shortcomings. All hail Svelte!!