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.

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

Cycle detection is broken #3459

Closed
ricochet1k opened this issue Aug 24, 2019 · 5 comments · Fixed by #3516
Closed

Cycle detection is broken #3459

ricochet1k opened this issue Aug 24, 2019 · 5 comments · Fixed by #3516
Labels

Comments

@ricochet1k
Copy link

I sometimes get a cycle detection error when there is no cycle. There are diamonds in the graph but no cycles, and statement order in the source code matters, i.e. sometimes re-ordering the reactive statements in the code doesn't give a cycle error.

To Reproduce
https://svelte.dev/repl/c58d52de222546a0a743fe6fb158568c?version=3.9.1

Expected behavior
No cycle detection bug error for any variant of this problem

Severity
I cannot use Svelte when it is giving spurious and incorrect error messages.

@ricochet1k
Copy link
Author

To make this easy to follow, I renamed variables so that every variable only refers to variables before it in the alphabet, which makes it easy to see the lack of cycles.

@bwbroersma
Copy link
Contributor

bwbroersma commented Aug 26, 2019

I simplified your example even more:

<script>
  $: c = a + b;

  $: a = 10;
  $: b = a;

  // putting the reactive declarations $: c = a + b; here won't generate a cyclical dependency error
</script>

See REPL.

@kylecordes
Copy link

To make it easier to find/fix cycle dependency problem, here is a lesson learned from past (totally unrelated to Svelte) situations:

Ban the use of error messages that just say "Cyclical dependency detected" or which point out one bit of the cycle. Require that any such message describe the whole cycle. This tends to yield much more insight when it goes awry "in the field".

@Conduitry Conduitry added the bug label Aug 26, 2019
@ricochet1k
Copy link
Author

Yes, please describe the whole cycle. This was a pain to debug and reduce because I had no idea what it was talking about in the beginning.

@Rich-Harris
Copy link
Member

released 3.10 with the fix, thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants