Adding a comment changes the semantics of the code significantly, as well a related potential reactivity bug

[Kcwidman]

Describe the bug

Adding a comment changes the semantics of the code. In fact, adding a comment fixes another presumed reactivity bug.

Let's walk through the original presumed bug first, then I'll explain the bizarre comment behavior.

For the rest of this report, I'll be referring to this REPL which replicates the issue.

Note: I have also replicated this with a running svelte-kit application as well.

Replication:

1. some prop (so far I've only gotten it to work with a list) get's passed to a component
2. a copy or a mapping of that component is made
3. the copy is displayed in markup (I'm guessing if it's not, then the compiler doesn't deem it necessary to update it)
4. at some later point, a function manually triggers reactivity on the copy (in this case the clickAction handler)

Bug:
The original list will have reactivity triggered on it even though it's been copied by value. The expected behavior is that the original list should not have reactivity triggered on it all. This is validated in the JS output, showing that the clickAction handler invalidates both names and namesCopy.

Findings:

1. this only happens if names is a prop, if you toggle the comments on the lines defining the names list in DataTable.svelte,
   the behavior works as expected
2. adding a comment to the side of the line that manually triggers reactivity on the copy changes the behavior of the
   code (this is also reflected in the JS output)

Reproduction

Replication is mentioned in tandem with bug description.

Logs

No response

System Info

System:
    OS: Linux 4.18 CentOS Stream 8
    CPU: (16) x64 11th Gen Intel(R) Core(TM) i9-11900K @ 3.50GHz
    Memory: 44.25 GB / 62.44 GB
    Container: Yes
    Shell: 4.4.20 - /bin/bash
  Binaries:
    Node: 18.16.0 - /usr/bin/node
    npm: 9.5.1 - /usr/bin/npm
    pnpm: 8.4.0 - /usr/bin/pnpm
  Browsers:
    Chrome: 114.0.5735.90

Severity

annoyance

[teobgeno]

Seems related to #4265

[dummdidumm]

The export let is a duplicate of #4265, the comment thing is very strange

[Kcwidman]

The export let is a duplicate of #4265, the comment thing is very strange

@dummdidumm,
Thank you for finding that similar issue. I looked, but didn't find it since the symptoms of my version surface in a different way. While I agree the behavior seems very related, do you think the export let part of this issue might constitute it's own distinct issue? #4265 is triggering a reactivity twice where it should trigger once. This issue here triggers reactivity once where it should trigger it zero times. Semantically this bug is presenting itself quite differently.

Not opposed either way if it's still decided the reactivity part of this bug is a duplicate, just voicing my additional thoughts. Thanks for the help!

edit: I found #7749 which does seem to be a duplicate of my reactivity issue, minus the comment weirdness. However, it has remained unresolved/unassigned since August, 2022. After seeing the long history of #4265 without any confirmation of a fix, is there any way to get this issue recognized by someone on the team? This bug has lead to many lost hours by 10's of developers by the looks of it.

[adiguba]

Hello,

First, it seem that invalidate a reactive declaration will invalidate all his dependencies.

Exemple :

// This line
namesCopy = namesCopy;
// is converted to
($$invalidate(0, namesCopy), $$invalidate(2, names));
// but IMHO, it should be :
$$invalidate(0, namesCopy);

I don't understand why, but it seems to be intentional, based on this code :

svelte/packages/svelte/src/compiler/compile/render_dom/invalidate.js

Lines 129 to 140 in f4c4d99

	// if this is a reactive declaration, invalidate dependencies recursively
	const deps = new Set([name]);
	deps.forEach((name) => {
	const reactive_declarations = renderer.component.reactive_declarations.filter((x) =>
	x.assignees.has(name)
	);
	reactive_declarations.forEach((declaration) => {
	declaration.dependencies.forEach((name) => {
	deps.add(name);
	});
	});
	});

And the different behavior with the comment seems to come from there :

svelte/packages/svelte/src/compiler/compile/render_dom/invalidate.js

Lines 50 to 57 in f4c4d99

	if (
	node.type === 'AssignmentExpression' &&
	node.operator === '=' &&
	nodes_match(node.left, node.right) &&
	tail.length === 0
	) {
	return get_invalidated(head, node);
	}

=> The nodes_match() will be falsy with a comment, since it is only present on one of the nodes.
Maybe nodes_match() should ignore trailingComments ?

[teobgeno]

Hello @Kcwidman
As @adiguba mention a reactive declaration will invalidate all his dependencies.
I suppose that as long as the variable namesCopy has a reactive declaration and a dependency with the variable names, svelte will invalidate both variables. I got your point about the spread operator but seems, If I'm not mistaken, that is not working that way. I do not know if you are searching a solution for this but a possible workaround would be not to declare the variable in a reactive declarations. Taking your example from REPL I would modify someComponent.svelte like below

export let names
let namesCopy = []

$: console.log("This should not have reactivity triggered repeatedly...", names)

const clickAction = () => {
	namesCopy = [...names]
}
</script>
<button on:click={clickAction}>click me!</button>
{namesCopy}

[sina-salahshour]

apart from the comment problem, you can prevent the names from being dependency of namesCopy, by moving the copy logic to a separate function and then calling it on a reactive block like this:
https://svelte.dev/repl/615c6a8fcd504ba99e0b94aafe651f87?version=4.1.2

[dummdidumm]

Since the comment problem is resolved, I'm closing this as a duplicate of #4265 and #7749