-
-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
Reactive blocks run only once per tick, losing changes and allowing fast pace apps to get out of sync #6732
Comments
Note that your REPL example can be simplified even further: the same behavior is seen without needing an object property. In other words, when you write let isSmallerThan10 = true;
let count = 1;
$: if (count) {
if (count < 10) {
console.error("smaller", count);
// this should trigger this reactive block again and enter the "else" but it doesn't
count = 11;
} else {
console.error("larger", count);
isSmallerThan10 = false;
}
} |
Digging into this, I found the following statement in #4586 (comment):
More explanation in #4714 (comment):
Note that the workaround suggested in #4586 (comment) is to use a derived store. In your simplified example, one fix would have simply been to make $: isSmallerThan10 = count.a < 10; But I suspect that in your real code, there might be more going on and let count = writable(0);
let isSmallerThan10 = derived(count, $count => $count.a < 10);
// Now use `$count` and `$isSmallerThan10` in the rest of your code Derived stores are called synchronously whenever their dependencies are updated, rather than batched. Here's an alternate version of let isSmallerThan10 = derived(count, $count => {
console.log('count updated to', $count.a);
return ($count.a < 10);
}); Put that in your example and you'll see these three lines in the console log:
You can see that the |
You mentioned wanting guidance as to what to change to change this behavior. I think you might be able to change it with a one-line (well, two-line) fix: remove the If that's not enough, then you'll want to look into extract_reactive_declarations() in Component.ts, especially how it sets I'm not going to make a PR removing However, I fully agree that there should be an update to documentation around |
@rmunn thanks a lot for the detailed responses. I know that having Can you please explain further how derived stores solve this? What am I missing? |
Finally, here's a REPL showing how derived stores can safely update their "parent" store, causing the dependent store to run again. Go to https://svelte.dev/repl/59dc4577ca5a44688535e794b723d8df?version=3.42.6, click the Update button once, and check the console logs to see that the derived store was able to update its parent store and was re-run after the parent store updated. Note three things about this example:
|
@rmunn thanks for the example. I greatly appreciate your effort. |
One reason why having
Derived stores solve this because they are not batched, but always run when the parent is updated, even if that's several times. See the example REPL I just posted: the derived store Note that that does not mean that the derived store always has to update when the parent updates. There's a version of derived stores where the callback takes two parameters, not just one. Then the second parameter is a let even = derived(count, ($count, set) => {
if ($count % 2) {
console.log('Odd, not updating');
} else {
console.log('Even, good');
set($count);
}
}); Plug that into my REPL in place of the
I'd say a good rule of thumb would be:
Also, if your reactive block is calling any functions that update variables it's looking for, that's going to fail. This one is already mentioned in the docs: see the |
Yes, they can. Just use closures. Here's an example: https://svelte.dev/repl/149caba183d84ca8bfaf05ea4ca9d896?version=3.42.6
Agreed, which is why the documentation needs to be updated to clarify. But there is plenty of existing code that depends on the fact that Edit: I missed this part of your question:
I don't know; I'm not part of the core team and can't speak to their state of mind. |
@rmunn Thanks for pointing out closures (although they bring stale closures into the mix as well, hurray 😄 ). Maybe it is a valid way forward for us, even if verbose. Will need to think about it some more/ try it out with real code. I would hate to give up reactive blocks altogether. I think that even if the docs get updated people would still trip on it because it is hard to tell whether updates happen in the same tick or not unless you really analyse it and when this behaviour happens it can be quite hard to debug. Is there a way to ask someone from the core team whether they would be willing to accept such a change (option to avoid the optimisation that's causing this behaviour)? |
P.S. On the other hand, derived stores can't depend reactively on component-local state, but you might be able to get around that by introducing a special-purpose store that updates itself whenever you need the derived store to fire an update. Something like this: const updatePlease = writable(0);
const complexStore = derived([a, b, c, d, updatePlease], ($a, $b, $c, $d, _) => {
// ... code that uses stores a, b, c and d, and also some localStateA and localStateB
return 'Some value';
});
let localStateA;
let localStateB;
$: { let _a = localStateA; updatePlease.update(x => x+1); }
$: { let _b = localStateB; updatePlease.update(x => x+1); } The Basically, you'll have to convert to derived stores every time you need updates that cannot be batched, but you can safely use component-local variables and the Oh, and to answer this question that you posted while I was writing this:
According to sveltejs/kit#2100 (comment), the Svelte Discord at https://svelte.dev/chat is probably the right place, specifically in the "contributing" channel. |
Thanks. I will try my luck on Discord. |
You know, I think I've thought of a better rule of thumb for when to use
For presentation state, it doesn't matter if you miss an intermediate update, because the human eye isn't that fast anyway. So if you have, say, a live graph that updates its bar-chart columns according to the data in a store, it won't matter if some of the height updates get batched: if they're happening so close to each other, the human eye wouldn't notice the height transition from 3 to 5 if it's immediately followed by another transition from 5 to 8, so having the bar go directly from 3 to 8 will look just the same to the user. (Especially if you're animating the bar height with a tweened store). For data-model state, it certainly does matter if you miss intermediate updates. Because if you're also calculating the average of all the bars and triggering something (say, writing to a log) when the average is above a certain threshhold, going from 3 to 5 and then from 5 to 8 can have very different effects than going from 3 straight to 8. If going to 5 was enough to pass the threshhold, then 3->5 and 5->8 would write to the log twice, while 3->8 would only write to the log once. So if the piece of state you're looking at is intended to change the UI, then it's presentation state and you can use |
The thing is it is not intermediate updates that are missed but all updates besides the first (in my original example) |
Looking at Clean example without a store REPL, I don't understand why we would want the reactive block to re-run again, it could easily lead to infinite loops during runtime since there's no robust way to check if the old and new values are equal, e.g. for I think the current behaviour is feasible and could be documented in the docs for anyone who likes a deeper dive into Svelte's reactivity. But regarding workarounds, while the store technique works, it feels like it would open up risks for infinite loops as well. Maybe a better workaround is to refactor the code and inverse the control of reactive variables, so instead of Extra context: We heavily use Svelte in my company project too and we rarely hit this issue much, so I don't think this is a big blocking issue within Svelte. |
@bluwy sure there are ways to deep compare objects, maybe not super cheap ways (and i wouldn't want them to apply automatically but could be a good idea for another opt in feature). This happens regardless of objects (see the first comment). Infinite loops are usually very easy to notice (because your browser gets stuck), debug and fix, and anyway this is a very partial and un-needed defence because real app stores tend to be connected to servers and then you have asynchrony in the mix and this protection stops protecting you (see my example with timeout), which adds to the confusion. |
Thanks for the explanation @isaacHagoel. You're right about reactive programming should ensure program correctness, and there are many ways to achieve that. I think Svelte does ensure that, but in a different way by relying on IoC. If you do insist on making an optional feature, there are some hurdles I can see:
IMO this path would as well cause some subtle bugs, and is just one of the many ways to ensure program correctness. So from my POV, you could try that if you want, but it isn't a silver bullet too. Also: The maintainers have not forgotten about this! We're still discussing asynchronously the best path forward. |
There's a related RFC from another community member about this: sveltejs/rfcs#40 |
thanks @dummdidumm . I will read it in details. |
I think of reactive variables like cells in a spreadsheet. In a spreadsheet, you can't have a cell reference itself. The example in this issue seems like problematic code to write because the reactive block is both reading and writing the same variable value and it's unclear to me exactly how that should work. You could write that code in a much clearer way with far fewer lines and without the use of any stores like this: https://svelte.dev/repl/909e0ee9b2484dc3b9618dea80f82c86?version=3.42.6 |
@benmccann this is a toy example. I agree with you that in this case the code could have been different and better. Not so much in real life scenarios that we have. I could make an example that uses this kind of mechanism for a few reactive blocks to communicate with each other via state changes. The problem here is not the self referencing nature of the code (which is btw, legit - recursion is a very common pattern). The problem is that the block has only one chance to run per tick and that it does so silently without providing any warning or indication. |
Yeah, I definitely get that it's a toy example. But I think what I suggested would apply to any more complex example as well. Recursion is fine in JavaScript, but not a spreadsheet. I'm not saying to avoid recursion, but that I personally wouldn't choose recursive reactivity, and code is clearer if recursion is done outside the reactive code. E.g. this code is far nicer:
Whereas this code is not as nice:
I agree with your point that it should at least be documented. But personally I'd try to avoid that code and think a lint rule warning it's ugly might be even more useful. |
I see your point but as i said the same issue occurs when the communication
is across more than one reactive block through state.
I can try make an example if it helps.
…On Mon, Sep 20, 2021 at 9:29 AM Ben McCann ***@***.***> wrote:
Yeah, I definitely get that it's a toy example. But I think what I
suggested would apply to any more complex example as well.
Recursion is fine in JavaScript, but not a spreadsheet. I'm not saying to
avoid recursion, but that I personally wouldn't choose recursive
reactivity, and code is clearer if recursion is done outside the reactive
code.
E.g. this code is far nicer:
let y = 10;
$: x = fibonacci(y);
Whereas this code is not as nice:
let y = 10;
let x = y;
$: = {
if (y > 1) {
y -= 1;
x = x * y;
}
}
I agree with your point that it should at least be documented. But
personally I'd try to avoid that code and think a lint rule warning it's
ugly might be even more useful.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#6732 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AE4OZC2WRKVUKE7EBMSQ4GDUCZW4HANCNFSM5EDNRPSA>
.
Triage notifications on the go with GitHub Mobile for iOS
<https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675>
or Android
<https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.
|
Yeah, an example would help especially if it demonstrates a real use case |
@benmccann there is a discussion happening on #6730 so I am putting a new example there. I still used a single reactive block because I felt it is already getting too big even though I simplified it down as much as I could. At its core it represents a real use case that broke for us. |
@rmunn based on your advice, I was playing with derived stores a bit. See this REPL. Uncomment line 18 and it starts working. |
It's not that the store is optimized away if you don't include the store in the markup, it's that derived stores don't run their callback when nothing has subscribed to them. Actually, derived stores don't even set up their own subscriptions until someone subscribes to the derived store. This is so that derived stores work similarly to readable stores (in fact, derived stores are implemented as The good news for you is that all you have to do is set up a whole chain of derived stores, and subscribing to the last store will cause the whole chain to activate. I.e., import { writable, derived } from 'svelte/store'
const a = writable(0);
const aPlus1 = derived(a, x => x+1);
const aPlus2 = derived(aPlus1, x => x+1);
const aPlus3 = derived(aPlus2, x => x+1);
const aPlus4 = derived(aPlus3, x => x+1);
const aPlus5 = derived(aPlus4, x => x+1);
aPlus5.subscribe(x => {
console.log('a+5 = ', x); // Will log 'a + 5 = 5'
});
$a = 13; // Will log 'a + 5 = 18' Here's a REPL to prove that to you, along with some extra console logging at every step so you can see that each step is activating, not just the final step. https://svelte.dev/repl/7373bbf6d17a44d88676544ae3697aee?version=3.43.0 |
Also, looking at the Javascript code for your REPL, even without uncommenting line 18 the JS code for creating the Therefore, you don't necessarily need to subscribe to a derived store in the same component that creates it. If the derived store is being exported from the component as a module export, or being saved in component context with |
Here's an updated version of your REPL that shows that simply subscribing to the derived store is enough to make it run, and you don't actually need it in the markup: https://svelte.dev/repl/66c6820032614a66be7d739abc70d7be?version=3.42.6 But look at the console log of that REPL and notice that the phrase "Derived callback fired" is only printed once no matter how many times you click the Update button. That's because of another optimization that you'll need to look out for if you're using derived stores to trigger side effects like writing to other stores: the value returned from a derived store's callback is used to set the value of the derived store, using You can see that in action by editing the REPL I just linked in this comment and adding However, for derived stores that derive from two or more parent stores, you'd have to do something like let complexStoreToggle = false;
const complexStore = derived([parentA, parentB, parentC, parentD], ([a, b, c, d] => {
console.log('Doing some side effect with a, b, c and/or d');
complexStoreToggle = ! complexStoreToggle;
return complexStoreToggle;
}); Now the value of I hope that makes sense. If anything I've said doesn't make sense, please ask and I'll try to explain. It might take a few days as I have a busy weekend coming up, but I'll get back to you next week if I can't do so sooner. |
@arackaf - You might be interested in the above comments as well, since it's likely that some of your issues with reactive statements might also be solved by converting them to derived stores. |
@rmunn thanks. your insight about the need to subscribe in order to activate the store really helped! |
The lack of a return value in And if it isn't garbage collected, then you have the opposite problem, a memory leak, because you never saved the return value of the |
This may got a little lost in the other thread, but here's another approach by Rich for a simulated |
@rmunn @dummdidumm Thank you both for taking the time to interact with me. I do not take it for granted. |
@rmunn @dummdidumm I take it back about ticks. I have been playing a bit and it doesn't seem like they cause discrepancies. It looks like |
The docs say that |
This will be fixed in Svelte 5. The runtime is more consistent now with listening to updates. To not break backwards-compatibility, the |
@dummdidumm Is this issue related to goto() failing when conditional? |
Describe the bug
TLDR: Reactive blocks that need to stay up to speed with multiple state changes during the same tick basically break the application in subtle, hard to reason about ways (because things "randomly" go out of sync). This bug breaks the basic guarantee of reactivity.
More Info and context (skip to the next section for REPLs):
Reproduction
Expected behaviour: isSmallerThan10 should be false.
Actual behaviour: isSmallerThan10 stays true, which is out of sync with the app state (and breaks the contract of reactivity)
Logs
System Info
Severity
blocking all usage of svelte
The text was updated successfully, but these errors were encountered: