sveltejs / svelte

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

$: reactive shortcut is not current with multiple sync updates to a writeable store #6730

Closed arackaf closed 6 months ago

arackaf commented 3 years ago

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.

image

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

isaacHagoel commented 3 years ago

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.

rmunn commented 3 years ago

Regarding @isaacHagoel's REPL example, which does $count.a = 11 inside the reactive block, I've recently seen code that specifically depended on the fact that store assignment inside a reactive block does not trigger the block again. I'll post a link if I can remember where I found it, but it was something along the lines of:

$: {
  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 $count.a = 11 doesn't re-trigger the reactive block that uses $count, may be deliberate and just needs to be documented better. However, the more complex example from @arackaf's REPL is not the same thing, as it's entirely dependent on the order in which various $: blocks are called.

isaacHagoel commented 3 years ago

@rmunn looks like my issue happens regardless of stores. It is an issue with the reactivity itself. See this REPL

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. I wonder how I can work around this issue...

Prinzhorn commented 3 years ago

@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)

isaacHagoel commented 3 years ago

@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). Notice that if I add setTimeout it "works" which makes it even more confusing and still susceptible to infinite loops. If it is different I am happy to submit a new issue but I suspect the original issue would reproduce without a store (I will try to find time to try it out later or tomorrow).

Prinzhorn commented 3 years ago

I can easily defend from infinite loop within my reactive block

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 <script> block could be removed. If you can provide a more real world example I'm happy to show you a solution.

Notice that if I add setTimeout it "works" which makes it even more confusing and still susceptible to infinite loops.

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.

If it is different I am happy to submit a new issue

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 visible property is true. However, it will from then on keep rendering it even if visible becomes false again (this is from an intelligent tab component that will lazily render tabs but then keep them rendered to retain state like scroll position etc., it will only hide them via CSS)

<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 $: render = render || visible; would infinitely keep setting render because it depends on render and so on. I'm sure I have more complex examples in my code base, some I might not even realize that Svelte saves me.

rmunn commented 3 years ago

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 $: statements are compiled in the same order they're encountered, so that moving the $: sync(...) call up also moved it up in the compiled JS. I won't have time to dig into this further today, but I bet there's something about the order of subscriptions that is causing this behavior.

dummdidumm commented 3 years ago

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.

arackaf commented 3 years ago

@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.

arackaf commented 3 years ago

@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.

Conduitry commented 3 years ago

I haven't read the whole thread, but from @dummdidumm's comment, this sounds like the same question as came up in #5848.

rmunn commented 3 years ago

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).

arackaf commented 3 years ago

@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.

arackaf commented 3 years ago

@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.

isaacHagoel commented 3 years ago

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.

Rich-Harris commented 3 years ago

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}
Rich-Harris commented 3 years ago

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 $: (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 log frequently_changing_thing whenever infrequently_changing_thing changes, but you don't want to log frequently, then you can't do this...

$: 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 (1) in the example above as a result of (2) changing without altering the behaviour that the cleanup 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:

$: { fill, path } invalidate();
$: { infrequently_changing_thing } console.log({ infrequently_changing_thing, frequently_changing_thing });

The rule here is that if the $ labeled statement's body is a BlockStatement whose body is an ExpressionStatement whose expression is a SequenceExpression whose expressions is an array of Identifier 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...

// 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.

Conduitry commented 3 years ago

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?

arackaf commented 3 years ago

Reactive blocks don’t currently return anything, do they? Could a reactive block return the dependency array?

$: { 
  foo(a, b, c);
  return [a, b]
}
Rich-Harris commented 3 years ago

Good point re break. I guess you could solve it by swapping the order:

$: {
  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 break:

$: { y } $: {
  if (x) break $;
  console.log(y);
}

// or if you didn't want to reuse $
$: { y } fast: {
  if (x) break fast;
  console.log(y);
}

What happens with auto-declaring reactive declarations?

I was imagining they'd continue to work the same way — no change in syntactic validity.

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?

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.

Could a reactive block return the dependency array?

No, that's an invalid return statement because you're not inside a function body.

arackaf commented 3 years ago

@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.

arackaf commented 3 years ago

@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 …

Conduitry commented 3 years ago

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 break.

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.

arackaf commented 3 years ago

@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.

Rich-Harris commented 3 years ago

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

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 (1) doesn't know that it reads obj, it's that statement (2) doesn't know that it writes obj. The fundamental problem is one of ordering — Svelte can't sort the statements topologically if reads or writes are hidden from view — and there are two potential solutions that spring to mind:

  1. Track which reactive statements caused which bits to get dirty. Re-run all reactive declarations until there are no more changes, but skip any statements that caused themselves to become dirty
  2. Detect any 'hidden writes' and warn that the ordering is incorrect as a result, probably only in dev mode

(The third solution is to have a Sufficiently Smart Compiler that identifies that the call to go writes to obj, but that's a non-starter.)

Of those, I prefer the second. The actual solution to the problem is very simple — (1) and (2) should be swapped — and guiding the developer towards that solution is almost certainly better than the considerable book-keeping and wasted computation that the first would involve.

rmunn commented 3 years ago

For clarity, would $: { (a, b, c) } { console.log({ a, b }) } also be allowed? Depending on the length of the identifiers, some people might want to split it onto multiple lines, in which case the parentheses would be useful. If (a, b, c) turns into the same SequenceExpression in the AST that a, b, c does, then of course the answer is yes and this would come for free.

@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.

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 x changes, the block re-runs. Is that the logic I want?"), whereas putting the dependency list first means you only have to read the code block once, and the dependency list ends up working a little bit like a list of function parameters so you already have a mental framework in place to understand it.

Count me as a solid vote for the dependency list coming first, not last.

Conduitry commented 3 years ago

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 $: foo, bar, baz, do_something();. Would that start emitting warnings for code we've already been telling people to use?

arackaf commented 3 years ago

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.

arxpoetica commented 3 years ago

+1 for dependencies first.

arackaf commented 3 years ago

But really, if solving dep tracking at runtime is prohibitively hard in the general case, having it be a feature unique to explicit dep lists could be a decent middle ground.

It wouldn’t be a breaking change since it doesn’t exist yet. It would allow devs to both explicitly declare that foo is a dep of a reactive block, and also have that block explicitly re-run as needed if foo changes in ways unexpected to the Svelte compiler.

arxpoetica commented 3 years ago

What about:

$: {
  $$: [fill, path];
  invalidate();
}

or

$: { $$: [fill, path]; invalidate(); }

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...

arxpoetica commented 3 years ago

And to have it be non-breaking, we can search for the absence of $$ and then fallback to the implicit find-and-react-to-all-vars approach.

Rich-Harris commented 3 years ago

Does it make sense to try to come up with a syntax that lets component authors specify that as well?

I'm definitely not suggesting any of these, but they're all valid...

$: { a, b, c => d, e, f } {...}
$: { a, b, c > d, e, f } {...}
$: { a, b, c | d, e, f } {...}

(The first one isn't really workable, since c will show up as an unused parameter in the c => d arrow function.)

This would introduce an interesting new problem though, which is that you presumably need a way to distinguish between 'I haven't explicitly stated which variables are written to' and 'assume this statement doesn't write to any variables'. If we don't provide a way to explicitly state which variables are written to, then we don't have to solve that problem.

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?

Hmm. That could happen. Though it would only happen if the hidden assignment was to a variable that was also used in a separate reactive statement, which would consequently be executing with stale values. That feels like a very weird situation — is it a thing that happens in real code? Possibly, though I'm struggling to think of a use case.

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.

Pulling that off would make Svelte as simple to use at scale as it is in fun little demos.

Part of building a scalable system is providing developers with as few footguns as possible. What you're asking for is the ability to write this...

({ loaded } = obj);
if (foo) go();

...and run this...

({ loaded } = obj);
if (foo) go();
({ loaded } = obj);

...when what you meant was this:

if (foo) go();
({ loaded } = obj);

It's not terrible if ({ loaded } = obj) runs twice. But if that line was ({ loaded = expensive_function(obj) }) then it becomes a footgun. Running that code twice is absolutely pointless, and you shouldn't be disappointed if a framework has your back by steering you towards better outcomes.

Appeals to 'this is how useEffect behaves' won't work here: useEffect cascades are a terrible outcome, and what we're really talking about here is dependencies between statements, which are dealt with in React (as in any code that isn't topologically ordered) by... manually ordering those statements.

Rich-Harris commented 3 years ago

For clarity, would $: { (a, b, c) } { console.log({ a, b }) } also be allowed?

@rmunn Not sure I understand why the parens are useful? You can already split a sequence expression into multiple lines if needed:

$: {
  a,
  b,
  c
} {
  console.log({ a, b });
}
arxpoetica commented 3 years ago

There is actually an explicit reason why $: {} followed by {} is bad, as in this example:

$: {
  a,
  b,
  c
} {
  console.log({ a, b });
}

Let's say it actually looked like the following:

$: {
  a,
  b,
  c
}
{
  console.log({ a, b });
}

It's a slippery slope between same line and next line, and blocks are an ECMA5+ standard for creating new scope...someone could arguably want to use that for let and const statements that have their own scope but have nothing to do with the prior reactive block declaration statement.

arackaf commented 3 years ago

I’m ok with a framework steering me away from bad code, but not if the price is that I might have correct code that simply doesn’t work, because there’s an order the Svelte compiler is expecting my side effects to be listed in. Surely Svelte could warn me in the console if it detects weirdness like that?

Every react dev has seen the infinite loop error message. It’s not a big deal. Much better to see that once in awhile, and never, ever have to manually order lines of code to make the framework work correctly.

arackaf commented 3 years ago

To put a finer emphasis on this, I've not written code even 1/10th as complex in Svelte, compared to some of the React projects I've worked on over the years. Swyx once said that React was for apps, and Svelte for sites. If that's also the position of the Svelte team, then I think this behavior is acceptable. But if this framework is intended to tackle richly interactive web applications, the likes of which React has been handling for years, then I think hiding these topological ordering dependencies from the developer is essential.

Conduitry commented 3 years ago

I don't think Svelte needs to shy away from the possibility of being used for complicated apps in order to require that users think about what order the code they're writing will be run in, if that's what's being implied.

arxpoetica commented 3 years ago

After bikeshedding ruthlessly on this for at least an hour, lol, I'm leaning more into @Rich-Harris's proposal toward $: { x, y, z } { foo() }, with the weird caveat around break $.

arackaf commented 3 years ago

@Conduitry that’s stretching what I wrote a bit, no? Of course devs need to think about the order of their code. The question is, should devs need to think about the order of their seemingly unrelated reactive blocks, in order to satisfy the hidden rules of the compiler. As far as I can tell, Svelte is the only framework with such a requirement, and I think it will hurt its ability to be used on software at scale, with teams of (often junior) devs.

Rich-Harris commented 3 years ago

It's a slippery slope between same line and next line

Not really. There's no situation in which someone would write

$: { a, b, c }

as a standalone statement; it's meaningless. By extension, there's no ambiguity about whether the subsequent block 'belongs' to the label.

Surely Svelte could warn me in the console if it detects weirdness like that?

You mean like this — the proposal that you were 'disappointed' by an hour ago? 😉

  1. Detect any 'hidden writes' and warn that the ordering is incorrect as a result, probably only in dev mode
arackaf commented 3 years ago

@Rich-Harris to be clear, the best case scenario would be to warn after it executes the updates. Halt an infinite loop and error, perform weird update loops, and also warn. But I think the first priority should be correctness without devs needing to manually track references and writes, and re-order reactive blocks.

Rich-Harris commented 3 years ago

As far as I can tell, Svelte is the only framework with such a requirement

Svelte lets you write code like this...

$: d = a * 2;
$: a = b + c;

...because reactive statements are ordered topologically. In that regard it's inspired by things like Observable — you're freed from the requirement to author statements in a particular order. If you translate that directly to React, you end up with this:

const d = useMemo(() => a * 2, [a]);
const a = useMemo(() => b + c, [b, c]);

This is, of course, invalid code. How do we make it valid? By manually ordering those statements. It's not correct to say 'Svelte is the only framework with this requirement' — it's the only framework where topological ordering is even a possibility.

Now you might say that a more accurate comparison is useEffect:

const [d, setD] = useState(null);
const [a, setA] = useState(null);

useEffect(() => {
  setD(a * 2);
}, [a]);

useEffect(() => {
  setA(b + c);
}, [b, c]);

But you're not freed from having to think about the order in which code happens here either. You need to consider the initial state where a and d are both null, which could cause a rendering error in your template, or could cause an effect to fail. A component containing that code will render three times — once initially, when a and d are both null, once after both effects have run the first time, when d will confusingly be equal to 0 (because null * n === 0), and once with correct values. Per the React docs...

effects scheduled with useEffect don’t block the browser from updating the screen. This makes your app feel more responsive

...that's not guaranteed to all happen in the same tick, so it's possible that the user will see these incorrect and inconsistent values.

In short, while I appreciate that the React model is the one you're familiar with, it's in no way something we aspire to emulate. Making insulting comments about Svelte's scalability (it scales just fine, thanks) isn't going to persuade us to add footguns to the framework, and doesn't motivate people to go further out of their way to help you. The correct response to this issue is to make it easier for developers to manually order their statements — yes, just like you have to do in every other framework — in cases where dependencies are hidden from the compiler.

arackaf commented 3 years ago

No insult intended toward Svelte. Saying it's not as capable of working on software at scale, with a team (consisting of juniors) behind it, is not an insult if that's not what it's designed for.

The useEffect code is indeed the correct comparison, but I'm not sure why you think needing to specify an initial state is some sort of problem. You just ... specify initial state. And I'm pretty sure all effects do indeed run within the same tick. Do you have a source saying otherwise? The page you quoted merely noted that useEffect does not run in the same tick as the antecedent state update, with useLayoutEffect existing if you do need that (ie, if you need to run effects that affect ... layout, without jank).

https://reactjs.org/docs/hooks-effect.html

But whatever neat parlor tricks Svelte can do, ie

$: d = a * 2;
$: a = b + c;

might save a line or two of code, but that'll mean little when a mid-level dev is spending hours trying to figure out why a shared data-fetching library isn't updating exactly as expected.

arxpoetica commented 3 years ago

if that's not what it's designed for

I respect him deeply when I say, Swyx was just wrong about this one.

arackaf commented 3 years ago

@arxpoetica I want him to be wrong about this, but it's hard to imagine a serious web app: FinTech dashboard, a healthcare software suite, etc being written with a reactivity model that had this kind of fragility, requiring manual intervention to help the compiler out. That's just a non-starter.

Svelte is still a joy to work with for silly little side projects

https://github.com/arackaf/booklist

it's legit fun. Even after bumping into this very problem on THAT tiny, insignificant little app. Even in spite of that I still enjoy using it more than React. I just want it to be something I can use at work. That would be amazing

rmunn commented 3 years ago

This would introduce an interesting new problem though, which is that you presumably need a way to distinguish between 'I haven't explicitly stated which variables are written to' and 'assume this statement doesn't write to any variables'. If we don't provide a way to explicitly state which variables are written to, then we don't have to solve that problem.

Whatever syntax is chosen, perhaps putting a literal null in the part where you list the variables written to could mean "assume this statement doesn't write to any variables". A literal undefined might mean the same as null, but if for some reason there's a need to explicitly say "don't assume anything about writes, and instead analyze the code block", then I suppose putting undefined in place of a written-to list could have that meaning.

@rmunn Not sure I understand why the parens are useful?

For people coming from other languages, it might be comforting to have parens around a comma-separated list. They're not needed in Javascript, but I could see some devs wanting them if they're used to a different language's syntax. That's all I was thinking about.

isaacHagoel commented 3 years ago

I want him to be wrong about this, but it's hard to imagine a serious web app: FinTech dashboard, a healthcare software suite, etc being written with a reactivity model that had this kind of fragility, requiring manually intervention to help the compiler out. That's just a non-starter.

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.

rmunn commented 3 years ago

All of what @isaacHagoel and @arackaf want is available in stores, by the way. The issue is that the documentation led them (and presumably other devs who haven't spoken up about it) to believe that $: would work exactly the same way as stores do, and it has subtle differences.

Rich-Harris commented 3 years ago

You just ... specify initial state

I mean, sure, but...

const [d, setD] = useState(a * 2);
const [a, setA] = useState(b + c);

useEffect(() => {
  setD(a * 2);
}, [a]);

useEffect(() => {
  setA(b + c);
}, [b, c]);

...then you create a maintenance burden by repeating the expression (you have to be careful to keep it synced between init and update, contra the principles of declarative programming). And if it involves any expensive computation (or allocates stuff) then you need to be careful about putting it inside the function form of useState, so the computation/allocation doesn't happen on unrelated renders, which not everyone remembers to do. More footguns.

Do you have a source saying otherwise?

I'm saying that the three renders (where Svelte needs one) are not guaranteed to run in the same tick, not that the effects themselves run in separate ticks if they belong to the same commit.

@isaacHagoel it's not just about infinite loops, it's about the expected behaviour in other situations (see the render = render || visible or cleanup examples above). Adam's problem isn't that his code is only running once when it should run multiple times; it should only run once. The problem is that his code is running in the wrong order, and Svelte wasn't able to tell him as much. If he'd been given a console warning that said something like this...

The reactive declaration on line 15...

  $: ({ loaded } = obj);

...references `obj`, which is assigned indirectly by line 19:

  $: if (foo) go();

If the first statement should run after the second statement, you will need to swap them, as Svelte cannot 
order them topologically at compile time. See https://svelte.dev/blah for an explanation.

...then there would have been no problem. Do you have an example of a situation where being guided towards the correct order wouldn't solve the issues you've encountered?