melt-ui / runes

Experimenting what Melt will look like with Runes
44 stars 1 forks source link

Readme Previous Usage Example #8

Closed anatolzak closed 5 months ago

anatolzak commented 6 months ago

The previous usage example in the readme doesn't actually represent correct usage. The following code would not even work.

It seems with the code below as if the open prop is supposed to be a boolean because in onOpenChange we try to assign to it the next value. However when passing an open argument to createTooltip it's supposed to be a store, not a reactive variable.

<!-- previously -->
<script>
    export let open;

    const {
        elements: { trigger, content, arrow },
        states,
    } = createTooltip({
        open,
        onOpenChange: ({ curr, next }) => {
            open = next;
            return next;
        },
    });

    $: states.open.set(open);
</script>

<button use:melt={$trigger}>Trigger</button>

{#if open}
    <div use:melt={$content}>
        <div use:melt={$arrow} />
        <p>Hello world!</p>
    </div>
{/if}

So, since the only way to use an external open state is by using a store, the code becomes

<!-- previously using stores -->
<script>
    export let open;

    const { 
        elements: { trigger, content, arrow } 
    } = createTooltip({ open });
</script>

<button use:melt={$trigger}>Trigger</button>

{#if $open}
    <div use:melt={$content}>
        <div use:melt={$arrow} />
        <p>Hello world!</p>
    </div>
{/if}
<!-- now using runes -->
<script>
    let { open } = $props();

    const tooltip = new Tooltip({
        open: {
            get: () => open,
            set: (v) => (open = v),
        },
    });
</script>

<button {...tooltip.trigger()}>Trigger</button>

{#if open}
    <div {...tooltip.content()}>
        <div {...tooltip.arrow()} />
        <p>Hello world!</p>
    </div>
{/if}

So not as bad it the readme was trying to make it seem :) Don't get me wrong, I love svelte 5, but the before and after comparison currently is incorrect.

Also, just a side note, even if you could pass a reactive variable for the open state to createTooltip instead of a store, the code would still not work because boolean values are not passed by reference. So we would not be able to reassign the open prop's value. More precisely, we would be able to but it would not affect the reactive variable in the parent component.

abdel-17 commented 5 months ago

Oh yeah I forgot about the distinction between open and defaultOpen. Passing stores to components is not very ergonomic. The identical code would look more like this:

<script lang="ts">
    export let open: boolean;

    const { 
        elements: { trigger, content, arrow },
        states,
    } = createTooltip({
        defaultOpen: open,
        onOpenChange: ({ next }) => {
            open = next;
            return next;
        },
    });

    $: states.open.set(open);
</script>

With the upcoming overridable props refactor, it will look different yet again, so maybe we can just wait for that to get merged and update the example.

anatolzak commented 5 months ago

@abdel-17 This code will not work as expected. When state changes internally in melt, onOpenChange is called where we set the external open prop to next. This will not actually update the value of the open variable in the parent component that's rendering the current component. This is because in JavaScript boolean variables are not passed by reference, which is why when you try to update the open state from inside the child component, it will not update the parent's open value because those are 2 different variables technically.

Here is a simpler snippet showing what I mean

let count = 0;
function increment(countParam) {
    countParam++;
}
increment(count);
console.log(count); // 0

So the only way to actually use melt builders with the ability to keep the state externally and be able to pass that state with props, is to use svelte stores because primitive values like boolean are not passes by reference.

abdel-17 commented 5 months ago

You can update variables in a closure, even primitives. Try it for yourself in a Svelte REPL.

Bits UI does this https://github.com/huntabyte/bits-ui/blob/main/src/lib/bits/tooltip/components/tooltip.svelte

abdel-17 commented 5 months ago

Your example specifically doesn't work because the function parameter has the same name as the outer variable, so it shadows it.

anatolzak commented 5 months ago

Your example specifically doesn't work because the function parameter has the same name as the outer variable, so it shadows it.

@abdel-17 The parameter name doesn't matter. When you pass a primitive value to a function, it creates a copy of it, which is what you access inside the function as the parameter. So mutating the primitive parameter doesn't change the value of the primitive value passed to the function regardless of the name of the outer variable and function parameter name.

image

You can update variables in a closure, even primitives.

You are right about how closures work, however when passing primitive reactive values as props to a different component in Svelte, Svelte doesn't use closures, it acts just the way you pass primitive values to a function, which is the reason updating the value of a prop in the child component doesn't update the value of the the parent reactive variable.

I reproduced your code here where you can see that the parent reactive variable doesn't get updated https://stackblitz.com/edit/github-kgtuq4?file=src/routes/+page.svelte,src/lib/components/Tooltip.svelte

https://github.com/melt-ui/runes-experiment/assets/53095479/eb6cb440-0c55-44a1-9ab4-214a3422cff9

abdel-17 commented 5 months ago

It doesn't work because the parent component doesn't bind: to the child's open state.

Try this: <Tooltip bind:open />

anatolzak commented 5 months ago

ah, I see. Thanks for the clarification!