use-directives are called multiple times when elements inside a keyed block are reordered

[christianheine]

Description of the bug

• When attaching "use" directives inside a keyed block, they are called again (without being destroyed) if the elements of the block are reordered.

To Reproduce

• This REPL reproduces the issue: https://svelte.dev/repl/4b0807cd0596423b94e31a24bde9497a?version=3.20.1

Expected behavior

• Directives should be called only once when the component is mounted.
• Alternatively, if the element is remounted, "destroy" should be called beforehand (e.g. to free previously created resources).

Information about the Svelte project:

• Svelte version: 3.20.1 (bundled with Rollup)
• Browser: (latest) Chrome on MacOS

Severity

• Moderate to serious, since the application we are building is relying heavily on use-directives to leverage modularity. We noticed the issue when event listeners were mounted multiple times (leading to a memory leak as well as unexpected behavior and inconsistent internal state).

Additional context

• The issue only happens with keyed each blocks.

Previous analysis

• Here is where things break:

/* src/ListItem.svelte generated by Svelte v3.20.1 */
    function create_fragment(ctx) {
    	let div;
    	let t;
    	let directive_action;
    	let dispose;

    	return {
    		c() {
    			div = element("div");
    			t = text(/*id*/ ctx[0]);
    		},
    		m(target, anchor, remount) {
    			insert(target, div, anchor);
    			append(div, t);
    			if (remount) dispose(); // <-- remount is undefined
    			dispose = action_destroyer(directive_action = directive.call(null, div, { id: /*id*/ ctx[0] }));
    		},

It seems that remounting was considered, but the information (that the current operation is indeed a remount) gets lost due to an additional block in between. It's a bit hard to describe verbally. So here are the relevant parts of a debugger session (from the same app as in the example REPL):

[Conduitry]

This came up before in #2735, which was recently closed by its author who said it was now fixed - but perhaps it wasn't completely? I haven't looked closely at either repro or looked again at the PR that was said to resolve the other issue.

[christianheine]

Thanks. I did not see that one before (I had only searched for open issues...). This also explains where the (partial) solution came from.

Just had a look at the issue as well as the previous example. The previous issue actually seems to be fixed because in that example, the keyed each block directly contains a div with the actions attached to it.

Works:

{#each ids as id (id)}
    <div use:directive={{id}}>{id}</div>
{/each}

Does not work:

{#each ids as id (id)}
   <ListItem {id}/>
{/each}

I am not yet familiar with the Svelte compiler, but I believe it might be sufficient to change 3-4 areas:

1. Add remount as parameter to the mount call in additional circumstances (e.g. then this.type === 'each')
   

   svelte/src/compiler/compile/render_dom/Block.ts

   Line 298 in fe003b5

   properties.mount = x`function #mount(#target, anchor) {

2. Change the the signature of mount_component by adding remount and then pass the remount attribute down the chain to the next component.

   svelte/src/runtime/internal/Component.ts

   Line 55 in ff5f252

   export function mount_component(component, target, anchor) {

   
   Either with a default of false, or with an additional change a few lines down when a component is initially mounted.
   

   svelte/src/runtime/internal/Component.ts

   Line 159 in ff5f252

   mount_component(component, options.target, options.anchor);

3. Add remount here:
   

   svelte/src/compiler/compile/render_dom/wrappers/InlineComponent/index.ts

   Line 512 in fe003b5

   b`@mount_component(${name}, ${parent_node || '#target'}, ${parent_node ? 'null' : 'anchor'});`

4. Not sure about this one:
   

   svelte/src/compiler/compile/render_dom/wrappers/InlineComponent/index.ts

   Line 438 in fe003b5

   @mount_component(${name}, ${parent_node || '#target'}, ${parent_node ? 'null' : 'anchor'});

Today evening I already attempted to change 1-3 and it worked in my small example (destroy was called for each action), but a whole lot of tests did not pass any more after the change and I have not yet figured out why. @tanhauhau : Looks like you are (much) more familiar with the compiler internals. Maybe you can have a look if my hunch is going in the right direction or not.

[christianheine]

I had a second attempt at this today morning and got it working: All (non-JS) tests were passing and the issue was fixed in a test environment. So I continued to also update the JS tests (which compare the source code 1:1). Now all tests are passing again.

Also tried this on a rather large & complex app (70K+ LOC) and it worked exactly as expected (remark: I don't use SSR, but I assume the tests should cover this).

I guess it would be prudent to also write a test for this specific case (which I have not yet done), but before I take a shot at that and create a PR, it would be great if someone more experienced with the compiler internals could have a quick look at my changes:
master...christianheine:master

[christianheine]

After additional testing, I realized that there were still cases when remount was not passed along the whole (re)mounting chain. This was a bit trickier to find since it required to have a deeper (and more complex) hierarchy of elements.

Anyways, I added changes for these cases now as well (mainly src/compiler/compile/render_dom/wrappers/EachBlock.ts and src/compiler/compile/render_dom/wrappers/IfBlock.ts).

By the way: since #4569 seemed very related to this one, I also tried the test case which is provided in there. It seems like that would be fixed as well. @rohanrichards: This is the link to the fork, in case you also want to give this a spin.

As before: Any feedback from someone with more experience on the compiler on how they feel about the changes would be highly appreciated. Many thanks in advance!
Once I got more feedback I will try to create some decent test cases that cover these two issues.

In the meantime I will also continue use my fork for active development to see if anything else might be missing.

[pushkine]

yeah component fragments do not receive the remount prop, this is the correct fix for the current implementation

the real problem is that while local state in components is updating surgically, state in fragments is poorly managed and in most cases svelte simply pulls the old react on you & remounts everything as if the block was only just created

I'll try to address this problem & #4699 this week
In the meantime thumbs up for this fix, here is a test for the issue 08b3abb

this issue is unique to keyed each blocks as it is the only way in svelte to reorder nodes dynamically
there is no remount going on in unkeyed blocks as those have their props changed instead

eventually I believe that actions should not be recalled on mere order change
but in the meantime this is potentially a huge memory leak depending on the usecase

[nicolodavis]

Ran into this issue today. Thanks for working on the fix!

[Conduitry]

This has been fixed in 3.23.0 - https://svelte.dev/repl/4b0807cd0596423b94e31a24bde9497a?version=3.23.0