-
-
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 shortcut is not current with multiple sync updates to a writeable store #6730
Comments
We've hit this one as well. I was about to create an issue and then I saw this. 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". |
Regarding @isaacHagoel's REPL example, which does $: {
if (typeof $cleanup === 'function') {
$cleanup();
}
// ... code that needs cleanup ...
$cleanup = () => { console.log("Cleaning up..."); };
// Assignment above does not trigger an infinite loop
} So what you're seeing in that example, where |
@rmunn looks like my issue happens regardless of stores. It is an issue with the reactivity itself. I wonder if it is possible to reproduce the original issue without store. I would bet it is. I agree that when there is a bug that there has been around for a long time there would be existing code relying on the buggy behaviour. |
@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) |
@Prinzhorn I can easily defend from infinite loop within my reactive block. I cannot workaround the current issue (maybe I can but haven't come up with a way yet). |
I'm sure you can, in the same way you can determine if a trivial turing machine will halt. But you can't do that for a non-trivial application. Your REPL is basically constant and the entire
There's a difference between a tight infinite loop and using a timer that happens on an entirely different event loop tick. The timer is no different from any other event (e.g. a click) and invalidates the variable as one would expect.
I think that would be better, because we shouldn't hijack this issue (unless you can show that it's actually the same issue). I personally rely on the behavior you describe and to me this is not a bug at all but absolutely required. Here's a simple example from my code base (simplified code). It implements a "LazyTab" component that will only render when the <script>
export let visible;
let render = false;
// Once render becomes true (via visible) it will stay true forever.
$: render = render || visible;
</script>
{#if render}
<slot />
{/if} If Svelte wouldn't do what it does, then |
To hep with debugging @arackaf's issue, I took the compiled JS code of his two REPL examples and diffed it. After removing the few line-number comments whose line numbers had changed, the core of the diff was as follows: --- example-1.js 2021-09-15 04:27:40.729899632 -0500
+++ example-2.js 2021-09-15 04:28:38.715858076 -0500
@@ -259,6 +259,15 @@
const click_handler = () => $$invalidate(0, currentNextPageKey = nextNextPageKey);
$$self.$$.update = () => {
+ if ($$self.$$.dirty & /*currentNextPageKey*/ 1) {
+ $: sync({
+ loading: true,
+ data: null,
+ loaded: false,
+ k: currentNextPageKey
+ });
+ }
+
if ($$self.$$.dirty & /*$queryState*/ 128) {
$: console.log("XXX ", $queryState);
}
@@ -278,15 +287,6 @@
}
}
}
-
- if ($$self.$$.dirty & /*currentNextPageKey*/ 1) {
- $: sync({
- loading: true,
- data: null,
- loaded: false,
- k: currentNextPageKey
- });
- }
};
return [ It seems that the |
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 |
@isaacHagoel - thanks a TON for simplifying my example. I knew there were simpler repro's, but it took me a few hours to get that one perfect, and I needed to move on. |
@dummdidumm oh yikes - that seems really familiar, and I fear you might be right about this being expected, but I certainly hope not :( I thought for sure updating the store anywhere, anytime would re-trigger the reactive block, completely apart from compile-time analysis. This is an extremely dangerous use case for non-demo, more realistic code. |
I haven't read the whole thread, but from @dummdidumm's comment, this sounds like the same question as came up in #5848. |
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 |
@Conduitry I fear it is. SO sorry to waste your time on this potential duplicate. This felt different, since I figured updating a store would break through that issue, but it seems not? I'd love to see a special case added to make this work with stores - dunno how feasible that is, though. |
@rmunn solid points - yeah, this definitely didn't seem like the same issue. I'm still hoping there's a potential patch for this specific issue. |
Thanks for the enlightening discussion above. I've created a separate issue and included my REPL examples there and what I am complaining about has nothing to do with the order of evaluation of reactive blocks and is reproduced with a single one. |
Without weighing into the issue at hand right now (it's honestly not clear to me what the correct solution is here — the points about infinite loops are well made, the current behaviour is intentional but unfortunate in the case here where there isn't actually an infinite loop, merely a hidden dependency that prevents Svelte from finding the correct topological ordering), it took me a long time of staring at Adam's frankly bonkers repro before I could make sense of it, so here's a simpler version: https://svelte.dev/repl/80a3e35ee61f42c0930b0a6d3f7115b1?version=3.42.5 <script>
// exported so that `foo` is reactive
export let foo = true;
const obj = { loaded: true };
function go() {
obj.loaded = false;
setTimeout(() => {
obj.loaded = false;
}, 1000);
}
// (1) runs first, because it doesn't _look_ like it depends on (2)
$: ({ loaded } = obj);
// (2) runs second, invalidates obj, but Svelte doesn't run
// reactive statements more than once per tick
$: if (foo) go();
</script>
{#if loaded}
<h1>this text should never be visible</h1>
{:else}
<h1>this text should be visible immediately</h1>
{/if} |
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... <Canvas>
<Shape fill="red" path={...}/>
</Canvas> <!-- Shape.svelte -->
<script>
import { getContext } from 'svelte';
export let path;
export let fill;
const { invalidate, add_command } = getContext('canvas');
add_command((ctx) => {
ctx.beginPath();
// some drawing happens
ctx.fillStyle = fill;
ctx.fill();
});
// when `fill` or `path` change, we need to invalidate the canvas,
// but `invalidate` doesn't need to know anything about `fill` or `path`
$: (fill, path, invalidate());
</script> That $: console.log({ infrequently_changing_thing, frequently_changing_thing }); ...you have to do this: function log_stuff(infrequently_changing_thing) {
console.log({ infrequently_changing_thing, frequently_changing_thing });
}
$: log_stuff(infrequently_changing_thing); 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 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: $: { fill, path } invalidate(); $: { infrequently_changing_thing } console.log({ infrequently_changing_thing, frequently_changing_thing }); The rule here is that if the Since The one gotcha is Prettier... // before
$: { a, b, c } { console.log({ a, b }) }
// after
$: {
a, b, c;
}
{
console.log({ a, b });
} ...but I'm fairly sure the plugin could solve that. |
Some other gotchas:
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? |
Reactive blocks don’t currently return anything, do they? Could a reactive block return the dependency array? $: {
foo(a, b, c);
return [a, b]
} |
Good point re $: {
if (x) break $;
console.log(y);
} { y } It's definitely less aesthetically appealing (to me at least) though. I guess this monstrosity could work for the cases where you need $: { y } $: {
if (x) break $;
console.log(y);
}
// or if you didn't want to reuse $
$: { y } fast: {
if (x) break fast;
console.log(y);
}
I was imagining they'd continue to work the same way — no change in syntactic validity.
You could certainly do this sort of thing... $: {
[fill, path];
invalidate();
} ...but it's arguably less clear (hard to tell that you could shorten the dependency list rather than lengthening it that way) and only works with block statements.
No, that's an invalid |
@Rich-Harris I think your last example is on to something. Have the dep array be the last expression in the block, and I think that would look pretty good. |
@Rich-Harris come to think of it, your first example is really, really good too. If the dev list is last that’s closer to what React currently does … |
As long as we'll carry over whatever label the user may have put on the next block into the compiled JS, I think requiring folks to explicitly add another label makes the most sense. I agree it's more confusing to put the dependencies after the main block, and I don't think we should inconvenience typical use for the sake of Returning from syntax back to functionality: Is your idea that we would keep the current "only run once, runs can't synchronously trigger additional runs" behavior, but that we would now take into account the overridden dependencies when topographically sorting the reactive blocks? That sounds sufficient to me, I think. I'm not a fan of the idea of cycling through the reactive blocks until everything settles. |
@Conduitry Rich did explicitly say that this would allow for potentially infinite reevaluations of the reactive blocks, which I think is absolutely essential (though he can speak for himself if I misunderstood). React (and other frameworks to my knowledge) do not give you ONE opportunity for your side effects to run, requiring careful ordering of them. |
Ideally yes — I reckon it's better that we settle on one rule for when reactive statements evaluate, and for this to be a non-breaking change it would have to be the current rule (which I think makes sense in any case). The wrinkle is that if we did want to later change the behaviour for blocks with explicit dependency lists, that would be a breaking change. So we probably want to be sure. But it's worth noting that explicit dependency lists wouldn't be a tool for solving the problem in @arackaf's example. The issue isn't that statement
(The third solution is to have a Sufficiently Smart Compiler that identifies that the call to Of those, I prefer the second. The actual solution to the problem is very simple — |
For clarity, would
Ugh, no, please. Putting the dependency array last is one of React's ugly bits. It's pretty much necessary if the dependency array is a function parameter, because it's hard (and in some cases impossible) to deal with optional parameters in non-final positions, so I understand why React did it that way. But putting the dependency list last makes you read the code block twice to ensure that you've fully understood it ("wait, let me double-check: when Count me as a solid vote for the dependency list coming first, not last. |
Ah. Yes, this new syntax would just let you override the dependencies for a given reactive block, but it wouldn't let you specify which ones it's writing to. I guess that logic would continue to work as it is now - whichever variables are assigned to within the reactive block? Does it make sense to try to come up with a syntax that lets component authors specify that as well? If we're detecting hidden writes in dev mode and issuing a warning, are you worried about that warning irritating people who already explicitly wrote their reactive blocks in such a way as to hide an assignment from the compiler? It's not officially documented anywhere, but we've already been pushing people towards writing code like |
I’m disappointed to hear the preference for (2) over (1). Having to keep track of the ordering of reactive blocks, and manually move them around would be a somewhat unique burden among JS frameworks. It’s worth noting that React will never, ever care which order your useEffect calls are listed in. Interestingly I had assumed the explicit dep lists would trigger behavior (1) for the explicitly listed deps. But if you’re even thinking of extending that to the general case, I hope you’ll reconsider whether that would be wasteful work. Pulling that off would make Svelte as simple to use at scale as it is in fun little demos. |
I think "freed from needing to fully understand the reactivity model" isn't quite right. Isaac's comment from before is worth repeating in part
"Understanding" the reactive model isn't the problem. It's getting it to behave in a consistent way. Currently the reactivity model allows for weird actions at a distance. Add a reactive block here, and another one stops working, for ostensibly baffling reasons. A warning might help, usually, but I still think for complex apps that warning would be insufficient (third party libraries like Apollo, etc could easily pollute those line numbers making it difficult for the app developer to know what the hell to do). I also think the perf risks are low. This problem happens rarely. Svelte is already massively faster than React, and React is already more than fast enough to be incredibly popular, and effective at building profitable software. I'd be shocked if some additional tracking bits affected this in any meaningful way, and would (imo) be a pretty good tradeoff to gain consistency in how the reactivity works. Trading a small amount of perf for better consistency is a good tradeoff. |
Which is why I addressed the bugs that cause inconsistencies
One of the reasons for that is that the Svelte team is disciplined about design questions just like this one. Each tradeoff seems fine by itself, at the time, but these effects accumulate. We're talking about penalizing every single user of Svelte apps in order to duct tape over a problem that a tiny minority of Svelte developers have encountered. I know it seems like a huge thing to you, but we have a larger constituency to take care of. Anyway, we're back making the same points in a loop, so I'm going to bow out of this thread. |
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. |
Just to leave on a positive note, I only care because I love Svelte. A lot. I'd love to see it get to a place where it could be used on complex apps at scale. That's why I'm pushing for this sort of change. If that's not what Svelte is striving for, then I'll keep loving Svelte for smaller, personal projects. React is a pain but getting paid 💵 💴 💶 💷💰 to use it has a nice way of numbing that pain 😂 |
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: $: a = b + c;
$: d = a * 2;
run: if (a > 10) {
console.log('resetting');
b = c = 0;
}
run: console.log({ a, b, c, d }); Here,
$: a = b + c;
$: d = a * 2;
$: tick().then(() => {
if (a > 10) {
console.log('resetting');
b = c = 0;
}
});
$: tick().then(() => {
console.log({ a, b, c, d });
}); In the original repro, the only thing that would change is this: -$: sync({ loading: true, data: null, loaded: false, k: currentNextPageKey });
+run: sync({ loading: true, data: null, loaded: false, k: currentNextPageKey }); All other Similarly, @isaacHagoel's repro could be updated thusly: -$: if ($state.items) {
+run: if ($state.items) { 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. |
Can't we just add:
example to docs? I think having two so similiar features would be confusing when to use which one. Mainly for begginers. |
@Rich-Harris that's fucking brilliant. I love it. Solves all of the problems here. It's exactly the sort of feature / escape hatch a framework should have. I'm sorry this took such a painful thread to get here, but I'm incredibly excited to see this thinking :) Bike shed, should it maybe be
to be more consistent??? Or, since this seems to be sugar for $: tick().then(() => {
code
}); maybe $tick: {
code
} might be another option. Really though I don't care. Seriously stoked to see this being considered. |
@Mlocik97 it's one more feature for beginners to learn, eventually, maybe (you can get extremely far without it), but it unlocks a lot, and simplifies a lot. |
@Rich-Harris I think your example works even today, without $: a = b + c;
$: d = a * 2;
const reset = () => b = c = 0;
run: if (a > 10) {
console.log('resetting');
reset();
}
run: console.log({ a, b, c, d }); |
This comment has been minimized.
This comment has been minimized.
I agree, but those two features would in a lot of case produce same result, and that would make junior devs (begginers) confused. I can already feel, how they would ask about difference between these two five times every day on Discord. |
@Mlocik97 I'm sure a good section in the docs would make that clear. Ie, for super advanced use cases, that you probably won't even need, ever, etc etc etc. I don't think good features should be left off because some juniors might have a bit of confusion around it. EDIT - actually, it looks like Rich is more or less saying run: would be a replacement for $: { blocks, which would leave Fine with me either way - I suspect that'd be a tough breaking change to make happen. Really I'm just ecstatic to see features being contemplated that would add the sort of consistency Isaac and myself have been talking about. |
@Rich-Harris @Mlocik97 @arackaf
Point 2: Point 3: Point 4: |
I can offer an explanation for one of your issues:
That's because |
@rmunn (continued) only rendering {a}{b}{c}{d} seem to make it run to its conclusion in any order of blocks but this still means it is not something that can be used in reality (we don't render every variable). |
I would add that I was curious how other frameworks would deal with this test case. 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? |
I want to point out that However, I do like the separation, because it solves #4933 + #4933 (comment) by making it clear that these are reactive declarations and hence immutable (?) |
I mean, there is already a lot of stuff that use |
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 |
oh I didn't know this,... I always was naming labels differently, if I used them (that was already rarely). Thanks for info, I learned something new again. |
Not sure if this is still on people's mind or if everyone moved on to other issues. |
@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. |
This will be fixed in Svelte 5. The runtime is more consistent now with listening to updates. To not break backwards-compatibility, the |
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
System Info
Severity
blocking an upgrade
The text was updated successfully, but these errors were encountered: