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

Do not expose default slot let bindings to named slots #6049

Merged
merged 19 commits into from
May 26, 2023

Conversation

tanhauhau
Copy link
Member

@tanhauhau tanhauhau commented Mar 4, 2021

BREAKING CHANGE

Default slot let bindings are not available on named slots anymore and vice versa:

<script>
	import Nested from './Nested.svelte';
</script>

<Nested let:count>
	<p>
		count in default slot - is available: {count}
	</p>
	<p slot="bar">
		count in bar slot - is not available: {count}
	</p>
</Nested>

This makes slot bindings more consistent as the behavior is undefined when for example the default slot is from a list and the named slot is not.

To fix this, make the bindings you need available on the named slot (or default slot) and bind to them there.

<script>
	import Nested from './Nested.svelte';
</script>

<Nested let:count>
	<p>
		count in default slot - is available: {count}
	</p>
--	<p slot="bar">
++	<p slot="bar" let:count>
--		count in bar slot - is not available: {count}
++		count in bar slot - is now available: {count}
	</p>
</Nested>

PR description

Fixes #5865
Fixes #5684
Fixes #7245
Fixes #7709

@Rich-Harris this takes an opposite approach of #2419 from fixing #2320
where defining the let:value on Component, <Component let:value> will only apply to the default slot.

To make it clear,

  • it will not create a new scope on the <Component> level
    • using value in named slot will have "missing-declaration" warning
  • it will emit warnings, discouraging using let: on the <Component>
    • only if there's other named slots within <Component >

#2419 exposes the default slot data to named slot:

<!-- Component.svelte -->
<slot data="123" />
<slot name="foo" />

<!-- App.svelte -->
<Component let:data>
  <svelte:fragment slot="foo">
    {data}
  </svelte:fragment>
</Component>

This assumes that the default slot will always be there whenever foo slot exists, and there will be 1 default slot.
This immediately turns complicated and does not make sense if there's multiple of default <slot />

{#each [1,2,3] as item}
  <slot data={item} />
{/each}
<slot name="foo" />

This MR fixes #2320 with a separate direction, where specifying let:data in the <Component> attribute will be solely for default slot and will not be available to the named slot.

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

  • This message body should clearly illustrate what problems it solves.
  • Ideally, include a test that fails without this PR but passes with it.

Tests

  • Run the tests with npm test and lint the project with npm run lint

@tanhauhau tanhauhau changed the title Tanhauhau/gh 5865 Do not expose default slot let bindings to named slots Mar 4, 2021
@tanhauhau tanhauhau force-pushed the tanhauhau/gh-5865 branch 2 times, most recently from 12c9e62 to 76b3472 Compare March 4, 2021 02:08
@dummdidumm
Copy link
Member

If we decide to go with it we can only do so for v4, as it's a breaking change

@dummdidumm dummdidumm added this to the 4.x milestone Feb 22, 2023
@dummdidumm

This comment was marked as outdated.

@dummdidumm dummdidumm closed this Mar 31, 2023
@gtm-nayan gtm-nayan reopened this May 26, 2023
@vercel
Copy link

vercel bot commented May 26, 2023

@tanhauhau is attempting to deploy a commit to the Svelte Team on Vercel.

A member of the Team first needs to authorize it.

@gtm-nayan gtm-nayan changed the base branch from master to version-4 May 26, 2023 11:04
Copy link
Member

@dummdidumm dummdidumm left a comment

Choose a reason for hiding this comment

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

Looks good mostly, left a few comments

test/validator/samples/lets-on-component/warnings.json Outdated Show resolved Hide resolved
scripts/compile-test.js Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants