sveltejs / svelte

Cybernetically enhanced web apps
https://svelte.dev
MIT License
78.83k stars 4.14k forks source link

Bug: assigning a value to a readable store inside the template masks the actual store value within the rest of the template #6802

Open mdynnl opened 3 years ago

mdynnl commented 3 years ago

Describe the bug

Probaby, nobody would code svelte in this way and I can't think of any realworld use cases right now. But svetle currently allows assigning any kind of stores in the template and doesn't compile them with set_store_value, so {$readableStore += 1} is valid Svelte code. By compiling them with set_store_value, this will not be valid as does currently, which should the correct behavior. Edit: update the REPL with more important details

Reproduction

https://svelte.dev/repl/291bf77f6f8949ca8ccc0349d6f4f690?version=3.43.1

Logs

No response

System Info

N/A

Severity

annoyance

Prinzhorn commented 3 years ago

I don't think this is related to stores at all. You can do {v+=1} with any variable.

https://svelte.dev/repl/443f2863daf84f92a1b32d2c1a47e0b7?version=3.43.1

I personally think all these things should not compile with v4 and throw. Having side-effects inside {} is dangerous and an anti-pattern. Due to the surgical updates, elements re-use and things like keyed each it's basically undefined behavior what your variable will eventually hold. The statements don't run to to bottom as you'd expect in a classic templating language.

I personally also believe {foo()} should not compile, unless foo itself is not a constant and can be replaced. To me it's an anti-pattern to pull data from inside the template and people expect foo() to be called when it's not. But {foo(bar)} is a different story, because it will update when bar changes (things like {formatDistance(date, $now)} are common). But my feelings on that are not as strong as with {foo = bar}. Maybe a lint rule would be enough here.

mdynnl commented 3 years ago

I'm aware this also applys to variables and also agree Svelte v4 not compiling them at all and throw.

But this is currently allowed and $stores don't get treated as they should be. $readableStore = 1 would throw in the instance but it doesn't in the template.

If v4 still allows this regardless of a lint-rule, store assignments should be wrapped like I said. But this is low-prior.

Prinzhorn commented 3 years ago

But this is currently allowed and $stores don't get treated as they should be. $readableStore = 1 would throw in the instance but it doesn't in the template.

I missed that part. Maybe make it clear in the title and description that this is specifically an issue with readable stores. It's definitely weird that $readable = 9 "works". It doesn't update the store (how could it, there is not set) but only updates the local context, masking the store value.

Bug: assigning a value to a readable store inside the template masks the actual store value within the rest of the template

I think this REPL makes it more obvious what this is about

https://svelte.dev/repl/0b790800a8eb4f0babef778590c4fd35?version=3.43.1