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

Wrapping element breaks "portals" #4036

Closed
arggh opened this issue Dec 2, 2019 · 12 comments
Closed

Wrapping element breaks "portals" #4036

arggh opened this issue Dec 2, 2019 · 12 comments
Labels
awaiting submitter needs a reproduction, or clarification temp-stale

Comments

@arggh
Copy link
Contributor

arggh commented Dec 2, 2019

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

@RedHatter
Copy link
Contributor

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.

@vipero07
Copy link

vipero07 commented Dec 3, 2019

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

@RedHatter
Copy link
Contributor

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

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

@Conduitry
Copy link
Member

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?

@arggh
Copy link
Contributor Author

arggh commented Dec 5, 2019

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.

@vipero07
Copy link

vipero07 commented Dec 5, 2019

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.

@Conduitry Conduitry added the awaiting submitter needs a reproduction, or clarification label Jan 9, 2020
@thojanssens
Copy link

thojanssens commented Jul 1, 2020

I just want to add to this issue, that when making portals using the DOM API as seen here:
#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>

@pretzelhammer
Copy link

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 #1133 and we should be working to provide this API in Svelte: #1133 (comment)

@bfanger
Copy link
Contributor

bfanger commented Oct 25, 2020

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

@pushkine
Copy link
Contributor

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

<div use:createPortal="portal-key" />
<div use:portal="portal-key"> Hello from Component ! </div>

@stale
Copy link

stale bot commented Jun 26, 2021

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@dummdidumm
Copy link
Member

Closing in favor of #7082

@dummdidumm dummdidumm closed this as not planned Won't fix, can't repro, duplicate, stale Oct 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
awaiting submitter needs a reproduction, or clarification temp-stale
Projects
None yet
Development

No branches or pull requests

10 participants