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.

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

Conversation

paoloricciuti
Copy link
Member

Svelte 5 rewrite

No issue, it was reported by Theo on discord.

Please note that the Svelte codebase is currently being rewritten for Svelte 5. Changes should target Svelte 5, which lives on the default branch (main).

If your PR concerns Svelte 4 (including updates to svelte.dev.docs), please ensure the base branch is svelte-4 and not main.

Before submitting the PR, please make sure you do the following

  • It's really useful if your PR references an issue where it is discussed ahead of time. In many cases, features are absent for a reason. For large changes, please create an RFC: https://github.com/sveltejs/rfcs
  • Prefix your PR title with feat:, fix:, chore:, or docs:.
  • This message body should clearly illustrate what problems it solves.
  • Ideally, include a test that fails without this PR but passes with it.

Tests and linting

  • Run the tests with pnpm test and lint the project with pnpm lint

Copy link

changeset-bot bot commented Sep 26, 2024

🦋 Changeset detected

Latest commit: 1af292c

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
svelte Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

{/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

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

@dummdidumm dummdidumm merged commit d423004 into main Sep 27, 2024
9 checks passed
@dummdidumm dummdidumm deleted the fix-migrating-slot-in-custom-elements branch September 27, 2024 10:49
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.

3 participants