Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

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

Closed
Kcwidman opened this issue Aug 9, 2023 · 7 comments · May be fixed by #9089
Closed

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

Kcwidman opened this issue Aug 9, 2023 · 7 comments · May be fixed by #9089
Labels
bug compiler Changes relating to the compiler

Comments

@Kcwidman
Copy link

Kcwidman commented Aug 9, 2023 •
edited
Loading

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
Copy link
Contributor

Seems related to #4265

@dummdidumm
Copy link
Member

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

@dummdidumm dummdidumm added bug compiler Changes relating to the compiler labels Aug 10, 2023
@Kcwidman
Copy link
Author

Kcwidman commented Aug 10, 2023 •
edited
Loading

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
Copy link
Contributor

adiguba commented Aug 10, 2023

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 teobgeno mentioned this issue Aug 10, 2023
Open
5 tasks
@teobgeno
Copy link
Contributor

teobgeno commented Aug 10, 2023 •
edited
Loading

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 sina-salahshour mentioned this issue Sep 12, 2023
Merged
@sina-salahshour
Copy link
Contributor

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

@Prinzhorn Prinzhorn mentioned this issue Sep 17, 2023
Closed
dummdidumm pushed a commit that referenced this issue Oct 19, 2023
@sina-salahshour
fix: ignore comments while comparing nodes in node_match (#9197)
6b9b8af 
related to issue #9088
it doesn't solve the main problem of dependencies getting invalidated whenever value of a variable gets changed.
but it fixes the behavior difference between the code with and without comments
kelvinsjk pushed a commit to kelvinsjk/svelte that referenced this issue Oct 19, 2023
@sina-salahshour @kelvinsjk
fix: ignore comments while comparing nodes in node_match (sveltejs#9197)
63eb414 
related to issue sveltejs#9088
it doesn't solve the main problem of dependencies getting invalidated whenever value of a variable gets changed.
but it fixes the behavior difference between the code with and without comments
@dummdidumm
Copy link
Member

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
bug compiler Changes relating to the compiler
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

fix: nodes match when right expression has trailing comments teobgeno/svelte
5 participants
@dummdidumm @adiguba @teobgeno @sina-salahshour @Kcwidman