fix: store DOM boundaries on effects

[Rich-Harris]

This is an alternative to #12182 and #11690; fixes #12177 and #12198. WIP because it needs tidying up and documenting, but I'll try and summarise the changes:

Instead of effect.dom being a node or an array of nodes, which is written to in an unpredictable order (leading to bugs like the aforementioned, unless you do a lot of splicing which I think we're better off avoiding), block/branch effects have an effect.nodes property with start, end and anchor properties.

This property is set when the effect's template is cloned. For example in the demo app's case...

var root = $.template(`<button> </button>`);

...the <button> element is the main effect's nodes.start and nodes.end. When it's time to destroy the effect, we just remove everything from nodes.start to nodes.end, inclusive.

In a trickier case...

{#if foo}
  <p>does this exist or not?</p>
{/if}

<p>this always exists</p>

...while the if block effect's nodes is straightforward — the <p> is both start and end — the effect containing the if block has a trickier job:

var root = $.template(`<!> <p>this always exists</p>`, 1);

During hydration we can just use the opening hydration boundary as the start, but in the mount case we can't do that — the first node might be the <p> inside the if, or it might be the if block's <!> anchor, depending on the value of foo. For that reason, the effect's start node is null, which means that get_first_node consults effect.first, recursively, until we figure out which is the first node.

Components and render tags are an even trickier case as in many cases they don't create an effect at all. For that reason if the first item inside an effect is one of those components/render tags, the start is undefined but can be replaced with null or a node when the item is rendered.

This is all quite complicated to describe but it actually makes most implementation details much simpler. For example dynamic elements no longer need to reach up inside their parent to muck about with parent_effect.dom, and {@html ...} tags have an easier time as well.

One notable change is that in addition to relinking each block items as we move them around, we also need to relink their effects. We probably should have been doing that anyway. Currently, this is slightly inefficient but we can probably do the relinking of both items and effects at the same time, minimising the number of operations.

A nice thing about this way of doing things is that it hopefully opens the door to a more efficient approach to hydration. Currently, we have to iterate over every NodeList in the hydrated DOM twice (once to build up the hydrate_nodes array that becomes effect.dom, once inside every sibling call), but if there's no effect.dom then it stands to reason that we could skip the first traversal in favour of populating effect.nodes.end at the end of the process, inside append. That's left as future work, however.

Before submitting the PR, please make sure you do the following

• It's really useful if your PR references an issue where it is discussed ahead of time. In many cases, features are absent for a reason. For large changes, please create an RFC: https://github.com/sveltejs/rfcs
• Prefix your PR title with feat:, fix:, chore:, or docs:.
• This message body should clearly illustrate what problems it solves.
• Ideally, include a test that fails without this PR but passes with it.

Tests and linting

• Run the tests with pnpm test and lint the project with pnpm lint

[changeset-bot]

⚠️ No Changeset found

Latest commit: 6ec803d

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

[dummdidumm]

why was this test removed?

[Rich-Harris]

With a template that starts with a <!> comment, the start node is null (or undefined in the static component/render tag case), and when it's time to remove nodes Svelte finds the actual start node from the first child effect.

Currently, <noscript> is also replaced with a <!> in the client-side template, but there's no corresponding child effect, so the start node is incorrect and nodes are left behind when the parent effect is destroyed.

The easy fix was to just leave <noscript> elements in the template (albeit without children), but that changed the result of the test. The alternative would be to add a new flag, but that feels like more complexity (and we're running out of room for flags — a bitmask can only hold 31)

[trueadm]

Nice work :)