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

Slotted content less reactive when referencing external slot props in event handler #7440

Closed
brunnerh opened this issue Apr 10, 2022 · 5 comments · Fixed by #7448
Closed
Labels

Comments

@brunnerh
Copy link
Member

brunnerh commented Apr 10, 2022

Describe the bug

This is a very specific issue which only seems to happen when passing slot props into content slotted into another component, only using them in an event handler.

E.g.

<ItemDisplay content={item}>
  <svelte:fragment slot="default" let:content>
    <!-- ... -->

    <Wrapper>
      <!-- Passed value of "content" will be outdated if original source (item) changes -->
      <button type=button on:click={() => onEdit(content)}>Edit (Wrapped)</button>
    </Wrapper>

    <!-- Passed value of "content" will be up to date if original source (item) changes  -->
    <button type=button on:click={() => onEdit(content)}>Edit</button>
  </svelte:fragment>
</Table>

If the the content is additionally used in any other way, e.g. JSON stringified in the button content, the problem disappears.

Reproduction

REPL

The edit buttons are supposed to initialize an edit dialog with the current state of the item, that being whether it is checked.
So if an item is initially unchecked, the edit dialog should also have an unchecked checkbox. Is this changed and the dialog is opened again, the box should now be checked.

Concrete steps:

  • Click "Edit (Wrapped)" button
  • Check box in dialog, confirm with OK
  • Click the same edit button again, observe checked state in dialog
  • (Clicking the non-wrapped edit button should show the dialog in the correct state)

Logs

No response

System Info

System:
    OS: Windows 10 10.0.19044
    Memory: 13.37 GB / 31.92 GB
  Binaries:
    Node: 16.7.0 - C:\Program Files\nodejs\node.EXE
    Yarn: 1.22.17 - ~\AppData\Roaming\npm\yarn.CMD
    npm: 6.14.13 - C:\Program Files\nodejs\npm.CMD
  Browsers:
    Chrome: 100.0.4896.75
    Edge: Spartan (44.19041.1266.0), Chromium (100.0.1185.36)
    Internet Explorer: 11.0.19041.1566
  npmPackages:
    svelte: ^3.46.4 => 3.47.0
    webpack: ^5.70.0 => 5.70.0

Severity

annoyance

@mdynnl
Copy link

mdynnl commented Apr 10, 2022

You can workaround this by

  1. Subscribing to content inside the nested slot including reactive event handlers i.e on:click={(content, () => {})}.
    https://svelte.dev/repl/d4666b62f77640d1ad3c1a6a54bc647a?version=3.47.0

  2. Spreading $$props or $$restProps in parent slot (these are not recommended for perf reasons)
    https://svelte.dev/repl/b41c38532d2c46d9a8d81c732c4e551b?version=3.47.0

Edit: I think svelte should dirty check by default if a slot's let bindings are referenced by inner slots' function scopes, e.g. event handlers. This would lead to more correct code(IINW) and better compiler output than above approaches.

@brandonmcconnell
Copy link
Contributor

Hi @brunnerh. I'm noticing a couple of things here (and I'm rather new to Svelte, so forgive me if some of my comments are in error).

  • Shouldn't your svelte-fragment be svelte:fragment. Otherwise, as you can see by inspecting your REPL, the svelte-fragment custom element is actually rendered to the DOM.

  • The content slot-prop you created is on ItemDisplay, not the fragment, yet you are using the let: directive on your fragment instead of the ItemDisplay component instance. Instead, I believe this should be added to the ItemDisplay allowing you to forego your use of a fragment altogether.

    Here is how that would look:

    BEFORE (REPL)

    <ItemDisplay content={item}>
        <svelte-fragment slot="default" let:content>
            <input type=checkbox disabled checked={content.isChecked} />
            <Wrapper>
                <button type=button on:click={() => onEdit(content)}>Edit (Wrapped)</button>
            </Wrapper>
            <button type=button on:click={() => onEdit(content)}>Edit</button>
        </svelte-fragment>
    </ItemDisplay>

    AFTER (REPL)

    <ItemDisplay content={item} let:content>
        <input type=checkbox disabled checked={content.isChecked} />
        <Wrapper>
            <button type=button on:click={() => onEdit(content)}>Edit (Wrapped)</button>
        </Wrapper>
        <button type=button on:click={() => onEdit(content)}>Edit</button>
    </ItemDisplay>

@brunnerh
Copy link
Member Author

@brandonmcconnell The svelte-fragment was an error. It does not affect the bug, but it might be confusing, so I fixed it accordingly.

I know the slot props can be declared on the component, but slot props may not necessarily be available on every slot and the component already has a property called content. Separating it like this makes the scope a bit more explicit, but this is up to personal preference.

@Conduitry
Copy link
Member

This should be fixed in 3.48.0 - https://svelte.dev/repl/6e71c6198e044697b577f36a29a0c507?version=3.48.0

@brandonmcconnell
Copy link
Contributor

Thanks, @Conduitry. Another issue was recently opened which may be related, where slot prop contents returned by the default slot renders as undefined when passed into named slots:

Not sure if that should be working or not, but currently Svelte allows passing props into technically-neighboring slots without throwing any errors, but the value passed is undefined.

2 REPL examples of this behavior:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants