svelte icon indicating copy to clipboard operation
svelte copied to clipboard

Svelte 5 does not call action update method when mutating arrays

Open isaacfink opened this issue 1 year ago • 12 comments

Describe the bug

In this component

<script>
	let items = $state([]);

	function registerItem(item) {
		console.log(item);
	}

	export function action(node, items) {
		return {
			update: (items) => {
				console.log('got update');
				for (const item of items) {
					registerItem(item);
				}
			},
			destroy: () => {}
		};
	}

	function addItem() {
		items.push(items.length);
	}
        function addItemOldWay() {
		items = [...items, items.length]
	}
	function removeItem() {
		items.pop();
	}
</script>

<button on:click={addItem}>add item</button>
<button on:click={addItemOldWay}>add item</button>
<button on:click={removeItem}>remove item</button>

{#each items as item}
	<div>{item}</div>
{/each}

<div use:action={items}></div>

I only see logs when using the old way of assignment reactivity, since we can use push now I expected this to work in actions too, it seems like it doesn't

Reproduction

https://svelte-5-preview.vercel.app/#H4sIAAAAAAAAE41SQW-0IBD9KxO-JtXEuHfrmvTYU489rHugMu6SKhgYN90Y_nsDaHW3TfNBoo_hzbzHwMRa2aFlxWFiivfICvY8DCxjdB38wl6wI2QZs3o0jY-UtjFyoKpWNXVIIAl7C3t4sMQJk8MxrZXfa0fVkNQKDJ6kJTQvhH3i2enk92tqtLK6w7zTpxj3YRez8XPQhuC7CA-_RGmBWZRMYaoVAED8GqTRqCXmxzgITlgsBiuYZePcij-eNM30x3Qt4EerDSSeGw8Kup3Vb4r5-fOYtwy3FnbZigVaMvpaQJLOHrfym5wI3VOt3F2DuRBBdPEUDObDaM9JhB2qE5237b3Pfe3EG7_eVoA9HPI8D3ju-VzpuFTaXnKvL_ibDT0ki3K5W9-OKt9HIq1Aq6LpZPOxn2YvruJCBL1yFznVH-zo_D9zVpeuivg-p1bTP-TNeX413AYQzloKeammsCx3HvvgtPPs0FW_D6PFIr7VfaBaV0Uyy1ivhWwlClaQGdEd3RdyNdiBfQMAAA==

Logs

No response

System Info

repl with runes

Severity

blocking an upgrade

isaacfink avatar Feb 12 '24 17:02 isaacfink

Update: I can get around this by using $effect but is this the intended behaviour?

isaacfink avatar Feb 12 '24 17:02 isaacfink

It looks like actions only update when their parameter gets reassigned.

That seems to be consistent with svelte 4, but I'm not entirely sure whether that is entirely intended in either svelte version

MotionlessTrain avatar Feb 12 '24 20:02 MotionlessTrain

@MotionlessTrain in svelte 4 reactivity is only triggered on reassignment so this is expected

isaacfink avatar Feb 12 '24 20:02 isaacfink

This is the expected behaviour. Svelte's reactivity is fine-grain, so changing a property of an object passed to a parameter will not cause it to update to action. You can use $effect inside action – they're designed to be used this way :)

trueadm avatar Feb 13 '24 11:02 trueadm

While it's explainable why this happens, this is still very surprising to someone migrating their code. Moreover, just putting $state around a state variable might make things go from "works" to "broken": If you were doing something like foo.bar = .. then in legacy mode foo is updated and the action is notified, but when using $state(..) foo is no longer updated, so the code stops working. I'm tempted to say that we need to find a way to deeply react to any changes within the object to call update when expected.

dummdidumm avatar Feb 13 '24 13:02 dummdidumm

@dummdidumm I personally feel that doing so will open up pandoras box. I also don't think there should be an expectation that things should be the same in runes mode. I definitely wouldn't want the behaviour that my action would update every time something pushes a new entry to my array.

trueadm avatar Feb 13 '24 13:02 trueadm

But that's the contract of the current action API - call update when something changes within that object. We could make it so that when you're not returning update in your action (because you're using $effect instead for example) that we're not doing the deep watch, because you don't need it then.

dummdidumm avatar Feb 13 '24 13:02 dummdidumm

Or maybe update is not needed anymore in actions, it could be based on $effect only, this will be hard to implement for existing libraries because it will break with svelte 4 but it looks like a cleaner approach

isaacfink avatar Feb 13 '24 13:02 isaacfink

Or maybe update is not needed anymore in actions, it could be based on $effect only, this will be hard to implement for existing libraries because it will break with svelte 4 but it looks like a cleaner approach

I agree, effects are cleaner than the update and destroy methods.

levibassey avatar Feb 13 '24 14:02 levibassey

@dummdidumm I personally feel that doing so will open up pandoras box. I also don't think there should be an expectation that things should be the same in runes mode. I definitely wouldn't want the behaviour that my action would update every time something pushes a new entry to my array.

then what is update for. This really is gotcha.

FoHoOV avatar Feb 15 '24 10:02 FoHoOV

This issue highlights that that the "all or nothing" approach of the action parameter is not state of the art and does not "feel Svelte" in so many ways. Svelte 5 would be the chance to deprecate it and rework actions to make them more reactive and surgical. I want to use this opportunity to point towards https://github.com/sveltejs/rfcs/pull/41 again. Manually tracking changes in the parameter and juggling things like addEventListener/removeEventListener, requestAnimationFrame/cancelAnimationFrame etc. is not fun.

This is the expected behaviour. Svelte's reactivity is fine-grain, so changing a property of an object passed to a parameter will not cause it to update to action. You can use $effect inside action – they're designed to be used this way :)

But now my action can only handle $state and not a plain object as parameter. Depending on the situation and component the data for the action can come from different sources.

Prinzhorn avatar Feb 18 '24 09:02 Prinzhorn

@Prinzhorn oh wow that’s actually an interesting RFC would be pretty interesting seeing how that can be updated with the new Svelte 5 conventions especially surrounding props and callback functions instead of event dispatchers

Antonio-Bennett avatar Feb 19 '24 18:02 Antonio-Bennett