Strange cleanup bug when using {#each} block

[webJose]

Describe the bug

I don't know if this relates directly to the use of createSubscriber or not. I have a class that uses this function. The class takes some options that can only be set during construction, so the consumer of the class uses a $derived to construct a new instance every time the desired options change.

The store contains an array. I don't know if other types of data trigger this issue, or not, but array is what I have at work, so the reproduction uses an array.

Just by using an {#each} block to iterate the contents of the store is enough to make the previous store instance to fail cleanup.

Not shown in the repro, but if you manually access each item in the array as opposed of using the {#each} block, the problem doesn't happen and the previous store instance cleans up just fine.

Reproduction

REPL

Logs

No response

System Info

REPL

Severity

blocking an upgrade

[leonidaz]

@trueadm very strange, it seems like the cleanup callbacks of template effects inside the each are not running.

[paoloricciuti]

I think it's because you are reassigning state inside a derived...I tried to change the example without reassigning and it seemed to work

[webJose]

Hello! Thank you both for your interest in my disgraces. 😄 Any suggestions? Or is this truly a bug that needs fixing? Or should I attempt a different approach? What do you guys think?

[leonidaz]

@paoloricciuti could you post your repl?

I tried with an effect and also just wrapping the variable holding the instance in a signal and inside onclick setting it to a new instance but same thing.

[Thiagolino8]

I think it's because you are reassigning state inside a derived...I tried to change the example without reassigning and it seemed to work

I don't think that's the problem, see

[trueadm]

So it looks like this behaviour is intentional – the cleanup will never fire because of this logic:

https://github.com/sveltejs/svelte/blob/main/packages/svelte/src/reactivity/create-subscriber.js#L67-L69

Given we're always making a new MyStore(storeOptions), it means that the subscriber count never hits 0 because of the tick().then logic, meaning we increment the subscriber count just before the tick resolves. This ultimately means that the cleanup for the subscription will never occur here when the derived runs again.

[paoloricciuti]

So it looks like this behaviour is intentional – the cleanup will never fire because of this logic:

https://github.com/sveltejs/svelte/blob/main/packages/svelte/src/reactivity/create-subscriber.js#L67-L69

Given we're always making a new MyStore(storeOptions), it means that the subscriber count never hits 0 because of the tick().then logic, meaning we increment the subscriber count just before the tick resolves. This ultimately means that the cleanup for the subscription will never occur here when the derived runs again.

But the cleanup does fire if no each is there...in my investigation yesterday i found that the cleanup will fire commenting this line

svelte/packages/svelte/src/internal/client/dom/blocks/each.js

Lines 251 to 257 in dc8ae82

	// When we mount the each block for the first time, the collection won't be
	// connected to this effect as the effect hasn't finished running yet and its deps
	// won't be assigned. However, it's possible that when reconciling the each block
	// that a mutation occurred and it's made the collection MAYBE_DIRTY, so reading the
	// collection again can provide consistency to the reactive graph again as the deriveds
	// will now be `CLEAN`.
	get_collection();

Obviously that line is there for a reason but i think is at least partially the culprit

[webJose]

Interesting. But why does this only happen when using an {#each} block? For example, doing this instead:

<pre>{myStore.data[0]}</pre>
<pre>{myStore.data[1]}</pre>

Solves the cleanup problem. What's special about the {#each} block in this context?

[leonidaz]

So, what I'm seeing in the debugger in the each for some reason this line:

{#each myStore.data as _, index} which becomes $.each(node, 17, () => $.get(myStore).data, $.index, ($$anchor, _, index) => {

The $.get(myStore).data fires twice vs once based on the subscribe, very odd.

and then once for each iteration of the array, 2 more times since the array has two elements.

[trueadm]

Obviously that line is there for a reason but i think is at least partially the culprit

Removing that line doesn't seem to fix the issue for me.

[webJose]

Obviously that line is there for a reason but i think is at least partially the culprit

Removing that line doesn't seem to fix the issue for me.

Weird: You did not get cleanup for the first instance, but you did for the second one.

[trueadm]

Interestingly making myStore.data a derived and using that in the each block fixes the problem.

[paoloricciuti]

I think the reason the first didn't cleanup is the reason why that line is there: the collection is not attached to the effect so it technically doesn't have a subscriber...I didn't go much further but there's definitely something fishy