sveltejs / svelte

Cybernetically enhanced web apps
https://svelte.dev
MIT License
78.66k stars 4.13k forks source link

Inconsistent behavior of `let` directive in combination with snippets #12087

Closed sheijne closed 2 months ago

sheijne commented 3 months ago

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 commented 3 months ago

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 commented 3 months ago

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 commented 3 months ago

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 commented 2 months ago

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 commented 2 months ago

It seems https://github.com/sveltejs/svelte/pull/12400/files#diff-e6527b55735d1694bca3cd7e3eef522b936ddd63022f038c67759321200dc39bR21 is triggering the error in certain cases, where I guess it shouldn't:

https://svelte-5-preview.vercel.app/#H4sIAAAAAAAAE8WST0_DMAzFv4oXkLZJ_XMfXQc3jhy4EQ5d627R0iRyXMZU9bujdixsIBC33fKS95yfLXeiVhq9WLx0whQNioV4cE5Egg9uEP4NNaOIhLctlcNN5ktSjnNpJKvGWWJ4Ivt-gJpsA9MkHVVyDE7vpMnSr4TJJnEMz1vlgUltNkgeCgNIZAniOJcmG-Nj9e7GG-UcMtSF1uui3M3KrdIVoZn3g0Fytm6ZrcmPSnJ3T2gqJDgZV8kseNMzc5d-1h4ejeRH1NrC3pKuJgNyoDgj3lvaeaiVwauh_gVWaG-vS3f51ykZcpcz_q0xEYnGVqpWWIkFU4t9FFZz9PxjOUEXZrOUgr0UuTQAGhm60H0U2CJIksSRdR56WMLteJzNv29tmMapwiq5mOBP5tf-A2Lqtt9aAwAA

This was the simplest reproduction I could think of. My suspicion is that $$slots.default is set to true on the props of a component when not explicitly using a snippet. So when trying to render the snippet from the child component the error is triggered.