Fix .contains() does not mean it's a child

[myieye]

Three things were wrong here:

• The logic here wasn't quite right. elem1.contains(elem2) checks all descendants, so it doesn't imply that elem2 is a child of elem1
• We shouldn't recalculate the target when the element is getting destroyed, because we won't necessarily get the current target if there have been DOM changes
• onDestroy was using the original options, which would be out of date if they had ever been changed (i.e. via update())

Due to a combination of these problems, when I navigate away from a route in my application I'm currently getting:

Uncaught (in promise) DOMException: Failed to execute 'removeChild' on 'Node': The node to be removed is not a child of this node.
    at destroy (http://localhost:5173/node_modules/.vite/deps/svelte-ux.js?v=ed6d092c:3364:16)

There are just so many little details that are wrong that it turns out there are a few ways to reproduce the bug, but what's crucial in my example is that: 1) The portal is initialized with {target: undefined} so that in onDestroy it queries for the nearest target. 2) There's a portal target (e.g. .PortalTarget) within the same {#if} as the portal content. Otherwise, .contains() will return false. The reason is that everything within the {#if} gets pulled out of the DOM, so root elements in the {#if} don't have a parentElement, but the elements within that detached subtree are still conntected to each other. 3) The portal content is not using that target i.e. it's not a direct child of it. It can simply be in its original parent (with enabled: false) or be in a different target that's also within the same {#if}. So, in the example you can trigger the error whether or not you've clicked "Move to target".

[changeset-bot[bot]]

🦋 Changeset detected

Latest commit: d4439c57bf69e383a5e4044acfe0f3b360f2a5a3

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

| Name | Type | | --------- | ----- | | svelte-ux | Patch |

Not sure what this means? Click here to learn what changesets are.

[Click here if you're a maintainer who wants to add another changeset to this PR](https://github.com/myieye/svelte-ux/new/bugfix/contains-does-not-mean-its-a-child?filename=.changeset/gorgeous-pandas-joke.md&value=---%0A%22svelte-ux%22%3A%20patch%0A---%0A%0AFix%20.contains()%20does%20not%20mean%20it's%20a%20child%0A)

[vercel[bot]]

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated (UTC)
svelte-ux	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Jun 14, 2024 1:50pm

[techniq]

@myieye This makes sense to me. Could you add an example of this use case (nested portal'd element) and run pnpm lint to make prettier happy 😁. Thanks!

[myieye]

@techniq sorry, I jumped the gun on this one. But, I eventually nailed it down and added an example that breaks without my second commit. I also updated the PR description to explain exactly what was going wrong.

[techniq]

@myieye I really appreciate the detailed write up and digging in. I've got 2 small asks :)

• I noticed you can not recreate the example after destroying due to the {#if !destory} around the whole example. Can this be scoped further down, or at least put the Recreate button in an {:else} block of this statement?
• Could you add a changeset (pnpm changeset) so this creates a new version?

Thanks for all the effort here!

[techniq]

@myieye Looks good! Ready to merge?

[myieye]

@techniq Yes 👍

[techniq]

Thanks @myieye. Publishing as 0.66.8. Should be live in a minute or so