Skip to content

fix: avoid migrating slots in custom elements #13406

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.

Sign up for GitHub

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

Merged
merged 2 commits into from
Sep 27, 2024
Merged

fix: avoid migrating slots in custom elements #13406

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
ffa1527
fix: avoid migrating slots in custom elements
paoloricciuti Sep 26, 2024
1af292c
chore: better test
paoloricciuti Sep 26, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changeset/eight-waves-mate.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'svelte': patch
---

fix: avoid migrating slots in custom elements
1 change: 1 addition & 0 deletions packages/svelte/src/compiler/migrate/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -583,6 +583,7 @@ const template = {
next();
},
SlotElement(node, { state, next }) {
if (state.analysis.custom_element) return;
let name = 'children';
let slot_name = 'default';
let slot_props = '{ ';
Expand Down
23 changes: 23 additions & 0 deletions packages/svelte/tests/migrate/samples/slots-custom-element/input.svelte
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
<svelte:options customElement="my-element" />

<script>
// to show that it doesn't bail out from the whole migration
let count = 0;
</script>

<button on:click={()=>count++}><slot /></button>

{count}

{#if foo}
<slot name="foo" {foo} />
{/if}

{#if $$slots.bar}
{$$slots}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This begs an interesting question ... what about $$slots in the context of custom elements? $$slots is also legacy/deprecated syntax and as such shouldn't be used anymore. Is it even feasible/necessary to use that in the context of custom elements? @Theo-Steiner any real world use cases from your end?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had exactly the same question but when i tried to use $$slots in a custom element it's actually populated so it might be used? I don't think anyone uses it but i would still leave it untouched in case of custom elements because the migration removes it in favor of checking the prop for falsy values which will not be here if we don't migrate slots.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's a bit unfortunate because it may mean that after $$slots are gone people will have to migrate away from it, using something like https://developer.mozilla.org/en-US/docs/Web/API/HTMLSlotElement/assignedNodes instead. As you say it's probably super rare in practise, but it's something to keep in mind.

Copy link
Contributor

@Theo-Steiner Theo-Steiner Sep 27, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this just means
$$slots.bar
will become
$host().shadowRoot.querySelector("slot[name=bar]").assignedNodes()
which is not too bad either IMO

EDIT: forgot the assignedNodes

<slot name="bar" />
{/if}

{#if $$slots.default}foo{/if}

<slot name="dashed-name" />
23 changes: 23 additions & 0 deletions packages/svelte/tests/migrate/samples/slots-custom-element/output.svelte
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
<svelte:options customElement="my-element" />

<script>
// to show that it doesn't bail out from the whole migration
let count = $state(0);
</script>

<button onclick={()=>count++}><slot /></button>

{count}

{#if foo}
<slot name="foo" {foo} />
{/if}

{#if bar}
{$$slots}
<slot name="bar" />
{/if}

{#if children}foo{/if}

<slot name="dashed-name" />