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

Snippets using props will error if rendered outside the component after its unmounted #14878

Open
lassebomh opened this issue Jan 2, 2025 · 2 comments

Comments

@lassebomh
Copy link

lassebomh commented Jan 2, 2025

Describe the bug

We're trying to upgrade to Svelte 5 at my company, but we ran into this bug when trying to migrate our portals implementation.
In short, the goal with portals is to allow a child component to insert one of its elements into an element of another component via an imported reference, instead of prop drilling. I'll spare you the other details and get right into the issue.

This is the bug:

First we have a file with an exported object containing a snippet:

// portals.svelte.js

export let headerPortal = $state({ snippet: null });

And our App.svelte renders this snippet, as well as containing a dynamic component:

<script>
  import { headerPortal } from './portals.svelte'

  let route = { component: ..., props: ... }
</script>

{#if headerPortal.snippet !== null}
  <header>
    {@render headerPortal.snippet()}
  </header>
{/if}

<route.component {...route.props} />

Then we have a component that sets headerPortal.snippet. The snippet has to contain a reference to a prop (2-levels deep):

<script>
  import { headerPortal } from "./portals.svelte";

  let { data } = $props();

  $effect.pre(() => {
    headerPortal.snippet = mySnippet;

    return () => {
      headerPortal.snippet = null;
    };
  });
</script>

{#snippet mySnippet()}
  <h1>{data.text}</h1>
{/snippet}

If we use our dynamic component in App.svelte to mount this one, and then unmount it, then we will get an error:

Uncaught TypeError: Cannot read properties of undefined (reading 'text')

Reproduction

Bug REPL

Open the console and click "Show page 2" and then "Show page 1". You should see an error, similar to the one above.

https://svelte.dev/playground/1175b32356ca41d4953c2896f71871ac?version=5.16.0

I haven't been able to reduce it any further.

Workaround REPL

https://svelte.dev/playground/c214d0b1cb5c4e6cba00c85a3704391f?version=5.16.0

It makes a change to the router:

<script>
  let route = ...
  let someKey = $state(0) // increment whenever route changes
</script>

{#if route !== null}
	{#each [route] as route (someKey)}
		<route.component {...route.props} />
	{/each}
{/if}

It's a complete hack. I expected {#key someKey } to work the same way, but it doesn't. It has to be a keyed each block

Logs

No response

System Info

System:
    OS: Windows 11 10.0.26100
    CPU: (16) x64 AMD Ryzen 9 6900HS with Radeon Graphics        
    Memory: 16.66 GB / 31.25 GB
  Binaries:
    Node: 20.12.2 - C:\Program Files\nodejs\node.EXE
    npm: 10.5.2 - C:\Program Files\nodejs\npm.CMD
    pnpm: 9.12.3 - C:\Program Files\nodejs\pnpm.CMD
  Browsers:
    Edge: Chromium (131.0.2903.112)
    Internet Explorer: 11.0.26100.1882

Severity

annoyance

@paoloricciuti
Copy link
Member

This is the issue with setting state in an effect: it can get out of sync. In this case you are setting the snippet to null too late so for a split second the component unmount, the prop became null and the snippet is still rendered. You can technically fix this by setting the snippet to null before changing the route but i don't know if it's feasible in your project.

@Leonidaz
Copy link

Leonidaz commented Jan 2, 2025

Yeah, the issue is when you're setting the props for Page1, the Page2 component is now supplied with empty props, which sets the reference to data in the snippet to undefined and that causes the contents of the snippet which is an effect based on the reactive prop data (props are always reactive) to rerun and at this point it errors since it expects data to be there.

So, either what @paoloricciuti recommended to set the snippet before the route change, or the following solutions:

Update Page2.svelte to add optional chaining when accessing props in snippets because the props are not provided when switching components. Or you can wrap the inside of a snippet in an {#if data}.

{#snippet myHeaderSnippet()}
  {data?.text}
{/snippet}

The second issue is that you have a memory leak in the component that you're setting the headerPortal.snippet.

That needs to be released when the component is destroyed. E.g. $effect that just returns a function or onDestroy. In the solution below, it would be done in the Page2.svelte.

So once that is solved, you can first unload the component and then on the next tick replace the route with the right component and props.

To unload a component, just set route.component = null.

I created a common function navigate(options) that takes route params and unloads the current page component and on the next tick sets the route.

Repl - with tick()

You could also pass in a callback property to each page component and let it fire when it's being destroyed and then the root component would load the next page component.

E.g. Repl - unload callback

It does require an additional prop to be passed in to each component. Next tick definitely seems more elegant.

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