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

Conditionally rendered default <slot {prop}> with unrelated named <slot> causes unexpected let:prop assignment #6901

Closed
darrylyeo opened this issue Nov 4, 2021 · 2 comments

Comments

@darrylyeo
Copy link

Describe the bug

I have a component that passes a variable to the default <slot>, which should only be rendered if the variable is defined. However, adding an additional unrelated named <slot> causes an error Cannot read property 'hello' of undefined whenever the variable is consumed with let: destructuring syntax.

I would expect let: to not be assigned to if the default <slot> isn't being rendered, regardless of the presence of additional named slots.

Reproduction

REPL: https://svelte.dev/repl/a7df02836b344eab9db7e3f01b7ec43f?version=3.44.1

App.svelte

<script>
	import HeaderAndData from './HeaderAndData.svelte'
</script>

<HeaderAndData let:data={{hello}}>
	<!-- Uncomment this (Cannot read property 'hello' of undefined) -->
	<!-- <h1 slot="header">Data:</h1> -->

	<p>{hello}</p>
</HeaderAndData>

HeaderAndData.svelte

<script>
	let data
	
	function getData(){
		data = {
			hello: 'world'
		}
	}
</script>

<button on:click={getData}>Get Data</button>

<slot name="header"></slot>

{#if data}
	<slot {data}></slot>
{/if}

Logs

Cannot read property 'hello' of undefined

System Info

3.44.1

Severity

annoyance

@rmunn
Copy link
Contributor

rmunn commented Nov 9, 2021

I would expect let: to not be assigned to if the default <slot> isn't being rendered, regardless of the presence of additional named slots.

I believe the reason why the let: is being processed when you uncomment the header slot is because anything set with let: in the component itself is actually made available to all your slot content, not just the default slot content. Let's take a look at the JS output of your REPL repro. The App.svelte file in your repro compiles to the following code:

headeranddata = new HeaderAndData({
  props: {
    $$slots: {
      default: [
        create_default_slot,
        ({ data: { hello } }) => ({ 0: hello }),
        ({ data: hello_hello }) => hello_hello ? 1 : 0
      ]
    },
    $$scope: { ctx }
  }
});

There's more to it, of course, but that's the relevant part. The slot data structure consists of an array of three functions; I haven't delved into what they do in detail, but you can see that the second function expects to receive an object with a data property, and that the data property will contain a hello property, which is destructured in the function params list. That destructuring is causing this issue; I'll get to how it's causing the issue a little later.

Now, if you uncomment the header slot, then you'll see the following in the compiled output:

headeranddata = new HeaderAndData({
  props: {
    $$slots: {
      header: [
        create_header_slot,
        ({ data: { hello } }) => ({ 0: hello }),
        ({ data: hello_hello }) => hello_hello ? 1 : 0
      ],
      default: [
        create_default_slot,
        ({ data: { hello } }) => ({ 0: hello }),
        ({ data: hello_hello }) => hello_hello ? 1 : 0
      ]
    },
    $$scope: { ctx }
  }
});

Note how the header slot has a different creation function, but the exact same data-receiving functions as the default slot. This shows how, when you have let:data={something} in the component call, the value of data is made available as something throughout all your slots, not just the default slot. I don't know with 100% certainty whether that's intended or not, but I believe it's probably intended, because it enables various kinds of functionality like:

<FancyListExample let:item={foo}>
  <div slot="header">{foo.title}</div>
  <div>{foo.body}</div>
  <div slot="footer">{foo.endnotes}</div>
</FancyListExample>

If the let:item={foo} assignment was not made available throughout the entire set of slots, then you'd have to repeat let:item={foo} on each slot, which would be tedious and error-prone if there are many slots.

Now, let's get back to your REPL example. It looks like as long as the default slot is the only slot, and it isn't being rendered in the component, the ({ data: { hello } }) => ({ 0: hello }) function is never called. But once you uncomment the header slot, then it gets rendered, and its ({ data: { hello } }) => ({ 0: hello }) function gets called as part of its rendering process. And because you have let data in the component, thereby giving it a default value of undefined, that means that the slot tries to fetch the .hello property of undefined in the parameter destructuring, which of course fails.

There are two ways you could solve this. The simplest way is to not deference { hello } in your component call, and instead deference it only in the default slot (which is only rendered if data is defined, and therefore is guaranteed to have a meaningful data.hello value). In other words, change let:data={{hello}} to let:data={data} as follows:

<HeaderAndData let:data={data}>
  <!-- Uncommented header line now does not throw exception -->
  <h1 slot="header">Data:</h1>

  <p>{data.hello}</p>
</HeaderAndData>

And then, of course, you could just use the let:data shortcut for let:data={data}.

The other thing you could do is assign a default value to data in your component. Just change let data in the component to let data = {} and there will be no more exceptions. You'll see undefined rendered, so you can fix that by changing the {#if data} test (which is now true since empty objects are truthy in Javascript) to {#if data?.hello} (which will be safe to test even if data happens to be undefined).

Which one of those two fixes is better for you depends on what your real code is doing, but in general I think the first approach (don't destructure {hello} in the let: assignment, so that you only access the .hello property where it's actually needed) is probably going to work best.

@gtm-nayan
Copy link
Contributor

Seems like this has been fixed in version 4... likely by #6049

@darrylyeo can you check if you're still around?

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

No branches or pull requests

3 participants