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

Inconsistent behavior of let directive in combination with snippets #12087

Closed
sheijne opened this issue Jun 19, 2024 · 5 comments · Fixed by #12400
Closed

Inconsistent behavior of let directive in combination with snippets #12087

sheijne opened this issue Jun 19, 2024 · 5 comments · Fixed by #12400
Milestone

Comments

@sheijne
Copy link

sheijne commented Jun 19, 2024

Describe the bug

Snippets are my favourite new feature in Svelte 5, and I trying to use the let directive in combination with snippets to see if it was possible, and ran into some odd behavior. Consider the following scenario (don't mind the console.log in the template, it's just to portray the issue):

<!-- MyComponent.svelte -->
<script>
  let { children } = $props();
</script>

{@render children({ myProp: 'Hello' })}
<!-- App.svelte -->
<script>
  import MyComponent from './MyComponent.svelte';
</script>

<MyComponent let:myProp>
  {console.log(myProp)}
</MyComponent>

During SSR the server logs prints "Hello", as I was hoping would happen. In the browser however, it will log undefined. This made me wonder if this is supposed to work at all. Either way I assume it should either work for both server and client, or neither.

I've included a REPL to demonstrate the client-side behavior in this issue, let me know if you'd also like a reproduction for the server-side behavior.

Did a little poking around, and looking at the compiler output in the REPL it seems to be caused by the generated JS, the generated snippet when using the let directive is as follows:

$.wrap_snippet(($$anchor, $$slotProps) => {
	const myProp = $.derived_safe_equal(() => $$slotProps.myProp);
	var text = $.text($$anchor);

	$.template_effect(() => $.set_text(text, console.log($.get(myProp))));
	$.append($$anchor, text);
})

When updating the REPL to use a snippet instead the following JS is generated:

$.wrap_snippet(($$anchor, $$arg0) => {
	let myProp = () => $$arg0?.().myProp;

	myProp();

	var text_1 = $.text($$anchor);

	$.template_effect(() => $.set_text(text_1, console.log(myProp())));
	$.append($$anchor, text_1);
})

In the case of the let directive, the prop is referenced as $$slotProps.myProp, in the case of a snippet the prop is referenced as $$args0?.().myProp. So if $$slotProps is actually a function, like $$args0, then $$slotsProps.myProp would always evaluate to undefined.

I haven't been able to dive deeper into this yet, but I might find some time to dig a little deeper, and create a PR for whatever the desired functionality is.

Reproduction

https://svelte-5-preview.vercel.app/#H4sIAAAAAAAAE42PQUvFMBCE_8qyCG2h9N1jfShevAjvbj1Iu9XAJhs2eQ9KyH-XWihFLx5nmG-YyThbpojmLaP_cIQGn0LAFtMSVhFvxImwxShXHVenj6PakM6DB7AuiCZ4XZ7FBfHkE8wqDqrudPC6raS6H3x_2unB90eOKRm3XFTCT3MexUdh6lg-681vysofmDO26GSys6UJTdIrlXZ_ccj99w1Tggzjl-VJyUOBB7gLKiHWze_p-VHJT6R7us6wrTRQvRCzVFCa8nfge_kGCpyuQ3IBAAA=

Logs

No response

System Info

System:
    OS: macOS 14.5
    CPU: (10) arm64 Apple M1 Pro
    Memory: 325.09 MB / 32.00 GB
    Shell: 5.9 - /bin/zsh
  Binaries:
    Node: 20.11.1 - ~/.nvm/versions/node/v20.11.1/bin/node
    npm: 10.2.4 - ~/.nvm/versions/node/v20.11.1/bin/npm
    pnpm: 9.1.1 - ~/.nvm/versions/node/v20.11.1/bin/pnpm
    bun: 1.1.13 - ~/.bun/bin/bun
  Browsers:
    Chrome: 126.0.6478.62
    Edge: 126.0.2592.61
    Safari: 17.5

Severity

annoyance

@paoloricciuti
Copy link
Member

I think snippets and let props are just not meant to work together and given that you can use snippets even outside runes (in a legacy component) i can just refactor the slot to use a snippet instead.

@sheijne
Copy link
Author

sheijne commented Jun 19, 2024

If that's the case I'll refrain from trying to use the let directive, it thought it was "nice to have" as it's less verbose, but don't feel strongly about it. I would probably also argue it's better to have a single way of doing things, so I was a little conflicted about it anyway.

Regardless, would it be good to make the behavior consistent between the server and client, to prevent confusion? Interoperability between v5 and older version could be favorable, so perhaps it would be good if it is still possible to use the let directive. Otherwise it might be good to ensure the value also resolves to undefined on the server, or even throwing a ReferenceError. I think a warning would also help, when the let directive is used in components where runes are enabled, would also help prevent confusion. Or just a general deprecation warning when using the let directive in v5.

@paoloricciuti
Copy link
Member

If that's the case I'll refrain from trying to use the let directive, it thought it was "nice to have" as it's less verbose, but don't feel strongly about it. I would probably also argue it's better to have a single way of doing things, so I was a little conflicted about it anyway.

Regardless, would it be good to make the behavior consistent between the server and client, to prevent confusion? Interoperability between v5 and older version could be favorable, so perhaps it would be good if it is still possible to use the let directive. Otherwise it might be good to ensure the value also resolves to undefined on the server, or even throwing a ReferenceError. I think a warning would also help, when the let directive is used in components where runes are enabled, would also help prevent confusion. Or just a general deprecation warning when using the let directive in v5.

I agree we should probably have some runtime warn about this.

@Rich-Harris Rich-Harris added this to the 5.0 milestone Jul 9, 2024
@Rich-Harris
Copy link
Member

Perhaps the easiest fix would be this:

export default function App($$anchor) {
	MyComponent($$anchor, {
-		children: ($$anchor, $$slotProps) => {
+		children: ($$anchor, $$args) => {
+			if ($$args) e.invalid_default_snippet_argument(); // this would need to be created
-			const myProp = $.derived_safe_equal(() => $$slotProps.myProp);
			var p = root_1();
			var text = $.child(p);

			$.reset(p);
-			$.template_effect(() => $.set_text(text, $.get(myProp) ?? '...'));
+			$.template_effect(() => $.set_text(text, myProp ?? '...'));
			$.append($$anchor, p);
		},
		$$slots: { default: true },
		$$legacy: true
	});
}

@sheijne
Copy link
Author

sheijne commented Jul 12, 2024

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 a pull request may close this issue.

3 participants