svelte: $: reactive shortcut is not current with multiple sync updates to a writeable store
Describe the bug
$: does not reliably fire with a writeable store, while store.subscribe does.
It seems like all initial writeable store updates within the same tick are gathered, with only the last, most recent one firing for the $:
reactive handler, usually. However, it seems like it can be possible for the $:
handler to fire pre-maturely, when there are still other state updates to happen within the same tick, which wind up going un-reported.
See repro link below.
Reproduction
https://svelte.dev/repl/57dbee8266d840d09611daebee226b91?version=3.42.5
I get the following in the dev console. Note that the {loading: "true"}
update is not reported from the $: handler, with the XXX, until the NEXT update happens. This results in the wrong state being reported, initially.
Note that moving the call to sync up, fixes this. It seems like Svelte’s analysis of the code is missing an odd edge case.
https://svelte.dev/repl/e88c70d6fd224b0d84656f83afd7e63c?version=3.42.5
Logs
N/A
System Info
REPL
Severity
blocking an upgrade
About this issue
- Original URL
- State: closed
- Created 3 years ago
- Reactions: 4
- Comments: 111 (95 by maintainers)
To me this REPL still feels kind of backwards. I would’ve written it differently probably, avoiding the need for a rerun.
Still, I think having a possibility to opt in (not out) of reruns would be a good addition to the framework. We can’t expect people to write code like we want them to at all times, and when they realize they coded themselves into a situation where they need that rerun, it should be available.
Regarding syntax: I don’t like
$: { deps } { doStuff() }
. Reasons:prettier-plugin-svelte
” -> not true, it would be a huge undertaking to change this print.prettier-plugin-svelte
delegates formatting of script tags to the core Prettier formatters, so we either need to fork that (impossible to maintain), or inspect the resulting docs array (highly discouraged by the Prettier team, could break frequently if inner doc structure changes). The “Prettier prints it in a less connected way” also is a hint towards point 1.I actually really like @arxpoetica’s solution to make this a inner label, although I would maybe name it to make it more clear:
This would also make it possible to add more labels for more fine-tuned behavior later on. For example this could mean “these are my deps, but don’t rerun when one of them change while calculating this”:
Whereas
$rerunOn
means “rerun this block when any of the listed variables are invalidated, even if this block already ran”. There could be situations where you want both$deps
and$rerunOn
to be defined, and that would be possible now.Regardless of how this discussion turns out, we really should have an “advanced docs” section going into more details for things like this and for providing best practices.
I would add that I was curious how other frameworks would deal with this test case. First I tried Solid because it has a reputation for having one of the most, well, solid reactive systems (that self infers the dependencies like Svelte does). Seem to be behaving as expected with no surprises and very consistently. See here. It is not sensitive to the order of the effects and always logs 6 times.
React also resolves correctly although it has some sensitivity (number of operations) to the order of the effects. See here
How can we get svelte (which, generally speaking, has far superior DX relative to both of the above) to the same level of predictability and robustness?
Not sure if this is still on people’s mind or if everyone moved on to other issues. In the meanwhile, I wrote this post on dev.to. Dev.to articles have good presence in search engines so folks googling around would hopefully bump into it until the official docs cover these issues (or until they are addressed otherwise). Cheers. cc @arackaf
We’ve hit this one as well. I was about to create an issue and then I saw this. Here is a very simple REPL reproducing the issue (might be a bit simpler than the one provided). The variable
isSmallerThan10
should be false but it is true.This seems to exist in older versions of Svelte down to 3.0.0 as far as I can tell. So severity should be higher than “blocking an upgrade”. @arackaf I downgraded your example to v3.0.0 and it seems to produce the same result. See here and if so please increase the priority.
Going to share some current thinking. I think what this all boils down to is the fact that Svelte conflates two things, which are actually quite different:
In CommandQuerySeparation Martin Fowler writes:
I think we can alleviate a lot of confusion by separating the two things at the level of syntax. And I think we can even do it in a backwards-compatible way, that would provide an escape hatch for stuff that you really do want to run until it ‘settles’ while still steering people towards better outcomes:
Here,
$:
retains its existing meaning. (In Svelte 4 we could arguably make a breaking change by insisting that it always be followed by an assignment, but there are some scenarios where that would be annoying.) As long as you don’t violate command-query separation, things will work exactly as you expect.run:
, on the other hand, would happen after those updates (even if$:
andrun:
are interleaved in your source code). It would loosely be equivalent to this:In the original repro, the only thing that would change is this:
All other
$:
lines are calculate something statements and would therefore be unchanged.Similarly, @isaacHagoel’s repro could be updated thusly:
I haven’t considered the full details of this proposal yet, but it seems like something that would give developers the ability to write code that needs to run in a loop without compromising on performance for everyone else.
@Rich-Harris @arackaf First of all thank you. Here is a summary of my current thoughts: Reactive blocks seem too good to be true and maybe they are. The “cooler” example (although this code looks very contrived to me given what it is trying to achieve, it doesn’t matter) as well as Rich’s suggested solutions to the example I have provided make me realise that. Splitting my example into two reactive blocks (one would run the find in it’s condition) but putting them in the wrong order also doesn’t work (like in Adam’s original example).
If the rules were clear and consistent (and/or the control mechanisms could be made explicit) then I would have no issue, even if the rules weren’t to my liking, but currently they don’t allow one to build a coherent mental model of how reactivity would behave, even if you are an expert, without always being on the guard, worrying and testing. The fact that #4933 and this are from last year and were basically ignored (I hope I am not being unfair) concerns me a lot. They show many other inconsistencies that contribute to why it is so hard to make a solid mental model.
As much as I love and enjoy Svelte, I do think that this is of a non-starter for new complex projects, if things stay as they are, not only for me but for any technical decision maker who would realise that these issues exist. Beyond some threshold of complexity (that is not that high) you need control and consistency maybe more than anything else. I am willing to invest resources from my team (and my own time) in helping drive this to a better place if that helps, as I have stated before, because I truely believe in Svelte and do not want to go back to React.
A side: I think there is a big difference between implicit dependencies in reactive blocks vs. dependencies that are stated explicitly at the top, usually using a gating $:if statement (but could also be a one line ternary assignment etc.). We always use the later as implicit dependencies are very hard to reason about (you need to read the whole block of code) and they also stop working if the block gets big and you decide to extract some of it to a function (very common). I would be totally fine with a solution that means that I cannot use implicit dependencies at all. Not sure it helps.
@Rich-Harris @Mlocik97 @arackaf
mmm… I spent some hours playing with
$: tick().then(() =>
vs.watch
(derived stores) vs. Rich’suseEffect
and I think I am starting to go a bit insane 😺 Point 1: When I try Rich’s example code (forrun
) with these 3 mechanisms, here is what I get:$: tick().then(() =>
doesn’t compile, complains about a cyclical dependency. See hereuseEffect
doesn’t console the final result, so it’s not running all the way through. Changing the order of the blocks doesn’t seem to fix it. See here. Changing the order of the blocks changes the output so no good.watch
prints the correct output but changing the order of the block has a pretty drastic effect on the amount of printouts. This is because every state change causes it to start again from the top. See herePoint 2: I tried other examples and
$: tick().then(() =>
seems to be behaving correctly for everything I tried. Even though it doesn’t “start from the top” every time likewatch
does, it seems to always get to the correct end result (more efficiently thatwatch
), which makes me wonder whether always using it instead of$:
would solve the problem in practice. Maybe there is a subtle issue I haven’t tested for but is going to bite in reality (?). If this pattern makes everything run to resolution, no matter the order or anything ( == it is consistent and predictable), then I am okay with documenting it and going back to our lives as @Mlocik97 proposed.Point 3: Not sure I fully wrapped my head around the implications of combining normal
$:
blocks (for assignment only) with$: tick().then(() =>
(orrun
) blocks. Could the “one per tick” nature of the normal$:
blocks get things out of sync somehow even if all they do is assignments? Does it make the mental model simpler or more complicated? easier or harder to reason about? I am not sure… I tend to want everything to be the same.Point 4: I think I would personally prefer just one keyword/syntax but being able to provide options to it. So
$:
stays the entry point to reactivity but maybe it take an options object somehow, something like what @dummdidumm said here and @rmunn seems to also support. This approach also makes it more easily extensible. The defaults for all of the options would be the current behaviour so it is all nice and backwards compatible.I can offer an explanation for one of your issues:
That’s because
useEffect
in that REPL is usingafterUpdate
, which is designed to run after the component’s visible fields are updated. With no HTML in that component, nothing happens. If I add{a}
to the HTML, so that an update is triggered every time the value ofa
changes, then the final result is logged to the console.Obviously I agree with Isaac’s thoughts here. I’d only add that I, too, am willing to invest some of my own time toward a solution here, where “solution” is defined as some mechanism to make reactive blocks behave consistently and reliably, without ever needing to think about ordering.
If that means explicit dependency lists which opt devs into automatic re-runs, cool. If that means Rich’s incredibly ambitious idea above, where reactive blocks automatically re-run as needed (and ignore situations where they trigger updates of their own dependencies from within the block) even better.
Either way I still love Svelte, and I’d still use it for any side project I start. I do agree there is a complexity threshold above which these problems make it a non-starter, but that wouldn’t stop me from using it in my own projects one bit.
I will just add one thing that came out of a conversation with Adam — this might not be the best place for it but it’s also not the worst — we occasionally run into situations where you want explicit control over the dependency list for reactive statements.
For example you might have something like this…
That
$: (fill, path, invalidate())
line is a bit weird. Conversely, if you want to ignore some dependency, you need to ‘mask’ it by hiding it inside a closure. If you want to logfrequently_changing_thing
wheneverinfrequently_changing_thing
changes, but you don’t want to log frequently, then you can’t do this……you have to do this:
This is all tangentially related to the issue in this thread because Adam suggested that if we did have a mechanism for expressing those dependencies, it could be treated as a signal to Svelte that reactive declarations should always re-run when those explicit dependencies change, infinite loop potential be damned. I actually don’t think that’s necessary — I reckon we could probably figure out a way to re-run
(1)
in the example above as a result of(2)
changing without altering the behaviour that thecleanup
example above depends on — but it’s worth noting in any case.So. Here’s my idea for a way to (optionally, for those rare cases where you really do need to give explicit instructions to the compiler) declare dependencies for a reactive statement:
The rule here is that if the
$
labeled statement’sbody
is aBlockStatement
whosebody
is anExpressionStatement
whoseexpression
is aSequenceExpression
whoseexpressions
is an array ofIdentifier
nodes, it is treated as a dependency list, and the next node in the AST takes its place as the reactive statement.Since
$: { a, b, c }
is meaningless code, I think you could even make the case that it’s a non-breaking change.The one gotcha is Prettier…
…but I’m fairly sure the plugin could solve that.
Yes, I believe this is essentially a duplicate of #5848, though it wouldn’t have been obvious right away that it was a duplicate. The docs say “Only values which directly appear within the
$:
block will become dependencies of the reactive statement,” but that short sentence doesn’t seem to be enough to help people grasp all the subtle implications. A couple examples, like the REPL examples in this issue and #5848, would probably be good for showing all the different ways you can get muddled with$:
statements that have “hidden” dependencies (dependencies in a function called from the$:
block but not visible directly in the$:
block itself).Label names are a separate namespace from variable/function/etc names, so there would be no conflict: https://svelte.dev/repl/eb2a4726339840ea90a7d421cbb29355?version=3.43.0
Having said that, I would prefer something prefixed with
$
like$run
, or even$runUntilSettled
to be more explicit about what it does.Can’t we just add:
$: tick().then(() => {});
example to docs? I think having two so similiar features would be confusing when to use which one. Mainly for begginers.
It’s worth noting that this is a tiny minority of Svelte users right now. If Svelte continues to gain marketshare, and get used more often in the larger, more complex apps that React is currently the de facto choice for, this will come up more and more.
This is exactly why I asked if Svelte should not be considered for apps like that. If Svelte is not designed for those sorts of complex apps, then that’s fine. But if Svelte is in theory designed to handle the same serious apps that React handles today, it is absolutely imperative that it behave consistently. This is what Isaac was trying to get across when he said “As much as I love and enjoy Svelte, I do think that this is of a non-starter for new complex projects.” I agree with that, and I imagine most engineers who’ve maintained these sorts of apps at scale would, too.
This. So much this. Please guys, (partial) protection infinite loops is nice and all but it cannot be prioritised over program correctness and predictability. Devs deal with the risk of infinite loops routinely. They are usually very easy to detect and fix. If the new behaviour (perform all updates) is opt-in via some kind of configuration (compiler flag or whatever) than it won’t be a breaking change.
What about:
or
That
$$
could give one a clue as a new developer that there’s something here related to the original$
.I suppose you could use curlies too…
@isaacHagoel This looks like a great write-up - thanks!
I know Rich is on vacation right now, but it sounds like he’s open to improving the reactivity system to get it to a place where it’ll scale to apps at your level, so I’m looking forward to seeing what comes of this.
@Rich-Harris I think your example works even today, without
:run
. Would I be correct in assuming (hopefully) that it would still work if you changed it to this:Knowing the path and walking the path are different. Tracking invalidations is one thing (relatively straightforward, we just need to blank out the
dirty
bitmask before running each statement, and inspect it afterwards), but re-running statements is a different kettle of fish.All of this stuff adds overhead. A warning by itself is a dev-mode-only thing; we can ditch that tracking for production builds. But if we also change the behaviour then every single production Svelte app that uses reactive statements becomes marginally less efficient, just so that developers are freed from needing to fully understand the reactivity model. I still don’t view that as a good tradeoff.
@Rich-Harris hahaha - someone in the other thread just pointed out the risk to memory leaks from effect 😃
I like console warnings, and I think it might be a path toward a sort of best of all worlds. From my (simplistic, uninformed) view, if you know enough to issue a console warning, you probably (I think?) know enough to just re-run the block, right?
Maybe in a perfect world the console warning could say “reactive block at line X was just re-run because … If you move that block up before the reactive block on line Y Svelte won’t have to re-run it, and your perf will improve”
Adam, again: this is insulting. Svelte can handle complex apps just fine: the reactivity model doesn’t ‘break down’ above some ‘complexity threshold’; we’ve yet to see a repro that shows an actual flaw in the reactivity model beyond ‘I didn’t fully grok how it works’.
In the past I’ve shown this code to React developers and asked them why it doesn’t work:
You already know the answer — it’s because the closure gets stale. But anecdotally, fewer than one in twenty React developers see that, even after they’ve been prompted. Do we consider that a flaw in React’s reactivity model? No, we don’t — we add a lint rule that, in development, warns you that you need to include
count
in the dependency array. (Of course at that point you’d be better off with asetTimeout
than asetInterval
, but that’s irrelevant for now.)Two observations:
To apply it to your case: if you, having been presented with this console warning (or an improved version)…
…still weren’t able to get the code to work as you expect, that’s not on the framework, that’s on you.
@isaacHagoel Thank you, this repro is very helpful. I whittled it down to the bare essentials here. It’s true that unlike Adam’s repro this couldn’t be fixed by reordering reactive statements, since there’s only one.
In this case I could imagine us printing a console warning like this:
I’m absolutely sympathetic to the view that it should ‘just work’. Viewed from one’s individual perspective, of course things should just work! But the semantics that cause one person’s code to just work can make another person’s code fail — take the
cleanup
example above, or something like this, in which re-running statements until they ‘settle’ would cause the temperature to always be reported as getting ‘cooler’:So a framework has to take a position on semantics that may cause some code to not just work. (Before Adam chimes in with ‘but React’, if you truly want React’s semantics in Svelte then go nuts. To anticipate another response: yes, we could make these things configurable, but that has compounding costs that really are best avoided.) And this is where I have to offer some perspective that informs my thinking on this issue, and it may seem tone-deaf or paternalistic or annoying, but I think it will help elucidate the different positions in this thread:
The purpose of a framework is to guide you towards the best outcomes.
That’s because we serve users first and developers second. Viewed through this lens, the problem here isn’t that Svelte isn’t doing what you expect, it’s that Svelte isn’t offering useful guidance. That’s why I’m advocating for warnings rather than semantic changes.
Now, a reasonable response might be ‘screw you, my code is perfectly fine’. I would understand that! But let’s consider the relevant part of the simplified repro I linked above:
If the block did execute repeatedly, then we’d be running that
.find(...)
twice (the second time, it has to search the entire array) when once would do. Maybe that’s fine in this case (it’s not hot code, the array will be small enough, it’s a cheap operation, etc) but in the general case it’s obviously suboptimal. Here’s a fixed version:Perhaps this feels like cheating since I’m operating on a simplified repro, so let’s take the original version:
Again, if this block ran repeatedly the
.find(...)
would happen more times than needed, but there’s actually a much worse problem:buildDisplayGroups
runs whenever things are kosher, rather than whenever they become kosher. So a$state.foo = 42
in some distant part of the app would causebuildDisplayGroups
to re-run even thoughdisplayGroups
is totally fine. Probably doesn’t matter in this case, but again, in the general case that’s an undesirable outcome.Fixed version:
(I should note here that if we implemented the console warnings we’ve been talking about, they would happen for this code as much as for the previous code, which perhaps we wouldn’t want. My thinking on all this stuff is evolving as we discuss it, please don’t take any of what I’m saying as a final word.)
I hope this doesn’t come across as ‘just write different code’, which is unhelpful advice. (I also realise that fixing a repro isn’t the same as fixing a real app, though I do think that the principles shown above are generally applicable.) Rather, I’m trying to illustrate that writing code on Svelte’s terms will yield better outcomes than trying to bend Svelte to the terms of each of its users. In that way it’s arguably a bit like TypeScript — I often have the experience (and see others have the experience) of getting mad at TypeScript because it doesn’t like the JavaScript that I’m adding types to, but when I eventually capitulate and rewrite the code on TypeScript’s terms, it’s almost invariably better than what I had before. The big difference is that TypeScript is very helpful at guiding you towards that outcome; Svelte is not.
@Rich-Harris unfortunately I do. I created #6732 because of it. My team at Pearson is creating pretty sophisticated interactive experiences, for example the recently launched virtual labs. We’ve went all in on Svelte for the most complex parts of the experience (because it is awesome in general) but were encountering puzzling behaviours because of this issue. Some of our stores are connected to the server so they are async which exposes us to infinite loops and all (in those cases). Then, for example when we added optimistic updates to one of those stores to keep up with the rapid rate of changes, suddenly things would not update and we found ourselves having to add ticks in “random” places and getting all sorts of hard to debug, silent failures and inconsistencies.
When the app is complex and “fast” (as in can do multiple things in the same tick) - it is very natural to have Reactive blocks implicitly communicating via state changes.
Some other gotchas:
break $;
if people want to bail out of a particular reactive block, which just automatically works because we leave the$:
label on the surrounding block in the generated JS. However, Acorn will complain if there’s abreak
statement referring to a label it can’t see on a containing block - and the label will now be on the previous block.Is there some other syntax where the explicit list of dependencies can occur within the reactive block, but do so in a way we can sneak in in an effectively non-breaking way?
I think this is the expected behavior.
The order of reactive statements is defined, and when triggered they run once. A reactive statement triggering another reactive statement will not make that statement rerun in the same tick in order to prevent endless loops (@Conduitry please correct me if I’m wrong).
In the example, there’s no connection between
$: ({ loaded, loading, data } = $queryState);
and$: sync({ loading: true, data: null, loaded: false, k: currentNextPageKey })
, which means Svelte will not reorder them. This means$: .. = $queryState);
runs first, and$: sync ...
runs afterwards. Because of the “only run once”-behavior,sync
’s update to$queryState
will not make that reactive assignment run again.@isaacHagoel Isn’t this a feature to prevent endless “reactive” loops/recursions? This is basically the halting problem, Svelte cannot determine (at compile time) if it’s save to keep running the same reactive block again when the dependency that triggers it changes within the same block.
Edit: Basically what @rmunn said, I think that’s different from the issue that @arackaf describes (which seems like a valid bug, changing the order of the blocks shouldn’t matter, but I didn’t dig into that REPL)