Svelte: Wrapping element breaks "portals"

Created on 2 Dec 2019  路  10Comments  路  Source: sveltejs/svelte

Describe the bug
Wrapping a "portal" component in a div breaks the app, making the portal and it's contents permanent citizens of the DOM.

This also leads to duplicated content on each cycle of tries to show/hide the portal.

To Reproduce
https://svelte.dev/repl/7da6fc9ebae64ff6b2011417f37588cb?version=3.16.0

Remove the wrapping (commented in code) div to fix the demo.

Expected behavior
When I click "Teleport" the second time, I'd expect the content inside Portal to be removed from DOM.

Severity
馃挴 - a simple {#if} in template doesn't do what it's expected to.

Additional context
Portals are undocumented and left to be solved in user-land, but the code in my REPL has been suggested as the way to handle this scenario since Svelte V2.

Also, adding the onDestroy hook to the Portal component like suggested in #1849 ...

onDestroy(() => {
  document.body.removeChild(portalEl);
});

...seems to "fix" the issue. However, if there is no wrapping div around the Portal, the element and it's children get removed from the DOM by Svelte without the need for a onDestroy hook to do it manually.

Additional suggestions

As this is a somewhat common thing in a modern web app, it might be valuable to

a. figure out the proper way to handle a Portal scenario
b. document it
c. make a test for it

pending clarification

Most helpful comment

I think it would make more sense to add a target attribute to the existing svelte:component e.g.

<svelte:component this={someComponent} target={document.body} />

I agree with @arggh though, in SSR it should be ignored. However needing to understand that would add unnecessary cognitive load (consider someone who expects it to be rendered SSR and cant figure out why it doesn't). That alone seems to go against most svelte design philosophy.

Perhaps a more fitting solution is to just allow components placed inside of <svelte:body> and append them to the body similar to <svelte:head>. That shouldn't complicate SSR, it doesn't allow for specifying DOM targets but I think that would be sufficient for most use cases.

All 10 comments

For completeness I would like to point out that there is another pattern that can be used for portals. Initializing the component manually. Though I'm not sure how to pass slot content.

https://svelte.dev/repl/3c73b9f9510b4ec383a9399046ab5858?version=3.16.0

Neither of these solutions are elegant and are really a hack. I would love to see a language feature to handle portals.

If you include the onDestroy from the link it will work properly with the div.

onDestroy(() => {
  document.body.removeChild(portalEl)
});

https://svelte.dev/repl/331b576a6bfc4a3fb06bb791ae82a61e?version=3.16.0

This has to do with the if statement destroying the element directly when it becomes false. In reality though, you don't want the div around the portal as it doesn't make any sense (the portal is being pulled out of the dom tree and placed on the body). What this means is with your <div><Portal /></div> example when the portal is created there is also an empty div inside of the div.stuff I think the correct way to handle this is really to not wrap the portal.
E.G.
https://svelte.dev/repl/63b8ed3acb9544a0b77e38a2c0f92db3?version=3.16.0

This gets you the same result without the breaking issue.

However, somewhat related wrapping something with an if seems to fire onDestroy after the element is removed from the dom.
https://svelte.dev/repl/d1966ff4279c45158226cd83b5239e7d?version=3.16.0

I would like to propose a new svelte component to handle portals.

<svelte:target this={document.body}>
  <h1>Hello world</h1>
</svelte:target>

The primary issue with that - and with other related proposals that are very much tied to the DOM - is what to do in SSR. Just ignore it?

My gut feeling is, that 99% of portals are used for modals, popovers and similar overlaid elements in reaction to user events. For all our use cases, ignoring them in SSR would be the desired behavior.

I think it would make more sense to add a target attribute to the existing svelte:component e.g.

<svelte:component this={someComponent} target={document.body} />

I agree with @arggh though, in SSR it should be ignored. However needing to understand that would add unnecessary cognitive load (consider someone who expects it to be rendered SSR and cant figure out why it doesn't). That alone seems to go against most svelte design philosophy.

Perhaps a more fitting solution is to just allow components placed inside of <svelte:body> and append them to the body similar to <svelte:head>. That shouldn't complicate SSR, it doesn't allow for specifying DOM targets but I think that would be sufficient for most use cases.

I just want to add to this issue, that when making portals using the DOM API as seen here:
https://github.com/sveltejs/svelte/issues/3088 (ThomasJuster's code),
binding to dimensions e.g. bind:clientWidth no longer works :'(

Note that I can't demonstrate that using a REPL, because in a REPL sandbox the binding to dimensions don't work.

<script>
  import {onMount} from 'svelte';

  let dialogNode;
  let clientWidth;
  let portal;

  $: {
    console.log('clientWidth', clientWidth);
  }

  // Change the width of `dialogNode`. When using a portal, `clientWidth` will not change
  $: if (dialogNode) {
    dialogNode.style.width = '200px';
  }

  onMount(() => {
    // COMMENT THE CODE BELOW TO SEE THE CORRECT LOG FOR CLIENT WIDTH
    portal = document.createElement('div');
    portal.className = 'portal';
    document.body.appendChild(portal);
    portal.appendChild(dialogNode);
  });
</script>

<style>
  /* alternative is to use a css solution such as position fixed */
  /* .dialog { position: fixed } */
</style>

<div>
  <dialog class="dialog" open bind:this={dialogNode} bind:clientWidth={clientWidth}>
    foo
  </dialog>
</div>

Just dropping by to mention that all of the Svelte portal workarounds/hacks are very brittle and break easily and we won't have proper portals in Svelte until they are added as first-class citizens to the framework itself. I think this issue is a subset of this one https://github.com/sveltejs/svelte/issues/1133 and we should be working to provide this API in Svelte: https://github.com/sveltejs/svelte/issues/1133#issuecomment-654440756

https://github.com/romkor/svelte-portal does a very good job of providing portals to svelte.

Here's a surprisingly simple solution using just actions
https://svelte.dev/repl/8364bc976f0c4ff9b83adf6e7a3c19fd?version=3.29.4

<div use:createPortal="portal-key" />
md5-a88799e2c69a2094af3e53988f202c93


Was this page helpful?
0 / 5 - 0 ratings

Related issues

mmjmanders picture mmjmanders  路  3Comments

davidcallanan picture davidcallanan  路  3Comments

robnagler picture robnagler  路  3Comments

thoughtspile picture thoughtspile  路  3Comments

ricardobeat picture ricardobeat  路  3Comments