sveltejs / svelte

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

Svelte 5: $effect is unusable (produce circular dependencies and endless updates) #9944

Closed dm-de closed 8 months ago

dm-de commented 9 months ago

Describe the bug

This is a serious issue with $effect - it produces a lot of problems. See working example Svelte 4 and broken Svelte 5 version. It is impossible for me to so solve it with v5.

In Checks.svelte you will find a commented out $effect code, that I expect to work. But it does not work.

Then i added another $effect code below - that works, but not fully. I must use untrack - but it makes no sense here. Otherwise you will get endless iterations. The main problem seems to be with value.includes().

With this half working code, you can see 3 issues in REPL.

Additionally: one line is still commented out - I can not get it working. //if (!value) { value = []; return } //STILL DON'T WORK!!!

It is REALLY hard to use $effect as it is (specially compared with $: in v4). It needs to be improved, otherwise it will frustrate many people.

Reproduction

Svelte 4: https://svelte.dev/repl/9c8fbbe0824e4f4598a6c32e85119483?version=4.2.8

Svelte 5: LINK

Logs

No response

System Info

Svelte 5 Next 25

Severity

blocking all usage of svelte

trueadm commented 9 months ago

Would something like this work?

dm-de commented 9 months ago

Would something like this work?

I had same idea - but as you see it dosn't work (now).

edit: see invalid values not filtered out

TGlide commented 9 months ago

Working solution

Reposting what I said in Discord:

You are mutating state in an effect that reruns every time that state changes, of course it'll provide infinite loops. Similar to how a recursive function needs a stopgap (I forgot the technical term), these types of effects need a check to determine if they should run again or not (reassign again, in this case).

I used the lightweight lib dequal for this, although you could implement an equality check yourself if you wanted to.

trueadm commented 9 months ago

The other issue is passing in undefined for the value. Instead, an array value should always be passed in, especially if it's expected at the other side. Defaulting a bound value from undefined to something else in a child component to the parent again is just really difficult to understand and a cause for water-falling of updates which causes jank.

I'd even consider breaking out this logic further – having the filtering of the array in the parent that uses a common utility function that can do this for you. Then you likely won't need so much indirection at all. The trick here is to not think of these patterns with a Svelte 4 mind, but to look at the problem differently given the new primitives you have in Svelte 5.

Something like this

dm-de commented 9 months ago

@trueadm - thank you, but the functionality of your solution is different and filtering is moved to parent.

@TGlide, you solved it perfectly - I've tried this for DAYS with v5! But solved the same in few minutes in v4.

But this issue point it out, that $effect ist HARD to understand and the result is not intuitive expectable. My suggestion is not to close this issue, but to improve $effect.

I post some part of the code to see, what we need to do today:

v4:

$: if (disabled || !value) value = []
$: value = items2.filter(
    item => !item.disabled && value.includes(item.value)
).map(
    item => item.value
)

equivalent in v5 (updated & simplified):

$effect(() => {
    let to_set = (disabled || !value) ? [] : filtered_items.filter(
        item => !item.disabled && value.includes(item.value)
    ).map(
        item => item.value
    )
    if (!dequal(to_set, value ? [...value] : value)) value = to_set
})
trueadm commented 9 months ago

@dm-de We don't think $effect needs to be "improved", more that we just need to do a better job documenting how it should be used and what to watch out for. It's actually a very simple bit of logic – it re-runs when something inside it changes (sync). This is how it works in all other signal based libraries too.

The things you're stumbling across here are more likely related to how things are done with Svelte 4 in regards to bound props. My point I was trying to make is that in scenarios like this, it's actually simpler to redesign the parent -> child relationship in terms of their dataflow. Simplifying it like I showed above can make it easier to debug and reason with. Granted that this isn't how things worked in Svelte 4, but that's actually a design flaw in Svelte 4 and something we've been actively trying to tackle. For the cases where you need to have to do this, we offer untrack – however, I didn't see the need in your case.

If you use untrack then you can simplify the above solution a bit:

    $effect(() => {
        if (disabled || !value) value = []
    });
    $effect(() => {
        value = filtered_items.filter(
            item => !item.disabled && untrack(() => value).includes(item.value)
        ).map(
            item => item.value
        )
    })

As shown here.

dm-de commented 9 months ago

@trueadm Again, it's HARD! If you test your code, you will see that untrack does not work as expected. Invalid values are not filtered out. (invalid values = hidden or disabled items)

The main idea was to have a checkbox group that bind always valid values (and correct if necessary) and do not delegate this job to parent. Let's say you use 10 times this checkbox group everywhere - you will repeat it every time.

It is simular, if you have a date picker and only valid date-ranges are allowed. Should your parent do this job (please no!) or your date picker?

This is a real world example from my code - and I struggled with it a lot.

I believe, if $effect stay as it is (without error/warning from compiler) - this will be a unpredictable nightmare in Svelte 5. Alternative would be automatic untrack (but one version that works!).

trueadm commented 9 months ago

@dm-de I posted the old URL, my bad. Check it again :)

dm-de commented 9 months ago

@trueadm strange, it do not work like expected:

you see disabled checkboxes (they can not be valid) and invalid value: grafik

trueadm commented 9 months ago

Ah okay. I mistook the bug there, this should work better then?

That has got me wondering if there's something we can do here to guide folks towards this path or another better one. Leave it with me.

dm-de commented 9 months ago

No... Error: ERR_SVELTE_TOO_MANY_UPDATES: Maximum update depth exceeded. If i click issue 3 button

you see... this is try and error - it is not expectable

Even changing last line from working (TGlide) code in $effect produce different results: if (!dequal(to_set, value ? [...value] : value)) value = to_set //working! if (!dequal(to_set, value)) value = to_set //not working - I do not know why - Too many updates

trueadm commented 9 months ago

@dm-de If you de-couple the effects it's fine. I was avoiding the untrack here. I updated the link.

dm-de commented 9 months ago

Ha ha - this is a beast! it do not work click issue3 and then issue1 button Same image like above.

TGlide commented 9 months ago

Even changing last line from working (TGlide) code in $effect produce different results:

That's because value is a proxy, while to_set is a normal array. So I convert the proxy to a normal array before.

TGlide commented 9 months ago

I believe, if $effect stay as it is (without error/warning from compiler) - this will be a unpredictable nightmare in Svelte 5. Alternative would be automatic untrack (but one version that works!).

It's not unpredictable. The effect runs anytime a variable inside it changes. Value is changed inside the effect, but it is also read from inside it, so it is quite clear why an infinite loop happens. Having automatic untracks or similar approaches are not predictable. We've seen this with the current approach to state in Svelte (screenshot below).

Screenshot 2023-12-17 at 19 16 29
TGlide commented 9 months ago

Nonetheless, I'll try and make it simpler. But just because something has more LOC, doesn't mean its worse, especially when we get the whole context behind these decisions.

TGlide commented 9 months ago

Simpler code.

This is, IMO, simpler than the Svelte 4 example, easier to reason about (as in Svelte 4, it's not clear why the reactive block does not run infinitely), safer, while maintaining almost the same amount of LOC.

Comparison:

// Svelte 4
$: if (disabled || !value) value = []
$: value = items2.filter(
    item => !item.disabled && value.includes(item.value)
).map(
    item => item.value
)
// Svelte 5
const is_valid_key = (key) => !!filtered_items.find(i => i.value === key && !i.disabled)

$effect(() => {
  if (value?.length === 0) return
  if (disabled || !value) return value = []
  if (value.some(v => !is_valid_key(v))) {
      value = value.filter(is_valid_key)
  }
})

If you exclude the start and end of the effect ($effect(() => { and })), it's the same amount of LOC. While avoiding all the issues that reactive blocks present, that I mentioned in previous comments.

dm-de commented 9 months ago

ufff....

I thought first if (value?.length === 0) return is useless - or simple optimization

But without this line, it will not work. What a strange thing - I don't get it...

brunnerh commented 9 months ago

It's a dependency tracking issue.

If there were no read of value, then in the disabled == true case, disabled would become the sole dependency of the $effect, hence subsequent changes to value would not trigger it.

With this logic, first the value is checked and disabled only becomes relevant if there are any checked items.

brunnerh commented 9 months ago

This would be one of those use cases for a property change hook which I mentioned elsewhere.

The point of this logic is to ensure a consistent internal component state, when properties are changed from the outside. There is no way to make this distinction and thus an $effect will run into loops when updating the state/props unless a lot of care is taken.

If there were an external change hook, which would not be an effect but just an event, you should be able to simply use the Svelte 4 logic:

onPropChange(() => {
  if (disabled || !value)
    value = [];

  value = items
    .filter(i => !i.disabled && value.includes(i.value))
    .map(i => i.value);
});

(Could be more granular if the handler is supplied information about what property was changed.)

dm-de commented 9 months ago

I found out another working solution, which is more like original v4 code.

LINK

important part from this is:

    let items2 = $derived(
        items ? items.filter(item => !item.hidden && item.value) : []
    )

    let tmp = $derived(
        (disabled || !value) ? [] : items2.filter(
            item => !item.disabled && value.includes(item.value)
        ).map(
            item => item.value
        )
    )

    $effect(() => {
        if (!dequal(tmp, value ? [...value] : value)) value = tmp
    })

but i'm still not happy with it. some data duplication

Bad part is that even reading state can cause loops: I can not use this code: if (!dequal(tmp, value)) value = tmp

dm-de commented 9 months ago

This is my expected code, that should run - but it's wrong:

$effect(() => {
    if (disabled || !value) { value = []; return }
    value = items2.filter(
        item => !item.disabled && value.includes(item.value)
    ).map(
        item => item.value
    )
})

Value is watched in this $effect - and set at the same time = endless loop. Same philosophy worked in v4 without any problems.

Here are ideas to solve this in Svelte:

1) write manual code like posted solution

2) auto-detect this situation and print error message Pseudo: list1 of watch states: disabled, value, items2 list2 of change states: value list3 of conflicts: value (some items of list2 are in list1) any conflicts? then print error

3) auto-detect this situation and add auto-workaround - simular to posted solutions. Pseudo: detected conflicts and for conflicts: do not change directly, but only if values (or arrays/objects) are different. deep check with dequal is required

This feels BAD

4) add a general soluton, so code like if (!dequal(tmp, value)) value = tmp will run without endless loops

5) something else... more smart/advanced/different?

brunnerh commented 9 months ago

See my previous comment. There is nothing wrong with $effect, it's just the wrong tool for this.

PP-Tom commented 9 months ago

Reading though this, I must agree with OP. While nothing is logically wrong with $effect, I've come across this myself when converting Svelte 4 to Svelte 5 and it added a lot of complexity to the application.

My concerns isn't so much about the right or wrongs of $effect but when comparing it to Svelte 4's $: it adds complexity that may catch people out. As an experienced programmer, that is fine, but considering some of the reasons for people choosing Svelte it concerns me.

Svelte has always brought in programmers at all different experiences because "it just works", simple logic like "when a value changes do this" and it working is what has brought joy to programmers of all experiences. I worry that adding this complexity would be a net negative to Svelte for DX.

I personally worry about people seeing these changes happening to appease enterprise programmers while forgetting about the hobbyist.

TGlide commented 9 months ago

I personally worry about people seeing these changes happening to appease enterprise programmers while forgetting about the hobbyist.

The problem is that the static analysis that the reactivity system relied on previously was catching everyone off-guard, beginner and seasoned programmers alike. Reactive blocks would not run at times, or the order would that they are stated would drastically alter the output. There are several issues others have raised in GH that get fixed with runes (e.g. #6732).

Sure, compared to something that tried and "magically" do things for you, you're now forced to come up with more responsibility, and it may feel odd. But it's not out of the blue, things weren't working out as intended, reactivity was quite leaky at times.

dm-de commented 9 months ago

I want to share more information, about behavior in some other frameworks for comparison.

Summary: Effect produce endless loop in: Svelte 5, React, SolidJS Effect produce NO endless loop in: Svelte 4, Vue

This is an identical, but "useless" example to test endless loops.

Svelte 4: https://svelte.dev/repl/4d7c44fb38fd4baaa15f103c32b3ac7e?version=4.2.8

Svelte 5: LINK

React: run on https://playcode.io/react

import React, { useState, useEffect } from 'react';

export function App(props) {
  const [count, setCount] = useState(0);

  useEffect(() => {
    //setCount(count+1) //endless loop :-(
    if (count>10) setCount(0)
  });

  return (<button onClick={()=>setCount(count+1)}>{count}</button>);
}

Vue: LINK

Solid-JS: https://playground.solidjs.com/anonymous/d06ed5ca-e0b0-4f40-be18-153dae755800

TGlide commented 9 months ago

@dm-de One important point you're missing though. The loop "prevention" can be actively harmful.

Svelte 5 - Loop 👼 Svelte 4- No loop 👿

popapaul commented 9 months ago

I find that the use cases where $effect in the current state is beneficial, are very limited.

In svelte 4, the same result can be achieved with a setTimeout which in my opinion is much less of a hassle than having to write additional checks to avoid loops.

I already loathe the time where I will upgrade all my apps to runes and be greeted by infinite loops.

dm-de commented 9 months ago

I already loathe the time where I will upgrade all my apps to runes and be greeted by infinite loops.

These are rare codes that do this.

There are currently 8 effects in my library. 3 effects (identical code) would create such a loop and with this help I was able to solve it.

My other effect code - I naively assumed it was analogous to Svelte 4 - also triggered endless loops for me. There I had simply reset an object to default. Like this:

...
let obj = {}

$effect(()=>{
//lets reset obj first...
obj = {w:0, h:0, x:0, y:0}

//now set on conditions...
if (...) obj.w=...
if (...) obj.h=...
if (...) obj.x=...
if (...) obj.y=...
})
...
TGlide commented 9 months ago

In svelte 4, the same result can be achieved with a setTimeout which in my opinion is much less of a hassle than having to write additional checks to avoid loops.

For the simple use case I demonstrated, that may work. But this is about predictability, which is important to ensure your code is reliable.

The $effect rune is predictable: if an internal state changes, it will re-run, that's it. A reactive block with $: however, is unpredictable. If an internal state changes, it may re-run.

Also, what if it gets more complicated, with multiple setTimeouts?

EDIT: Of course these effects are wonky. They're just to show that a setTimeout yields different results.

dm-de commented 9 months ago

After looking into the issue, I think I did something similar to the obj before in my previous codes in React. The component was probably running in the loop. However, I never noticed that it updated itself.

If an error occurs (like in Svelte 5 or SolidJS), then I find that better than executing a loop unnoticed.

Even better if the compiler recognizes it and warns you.

Later... we need a collection of pitfalls in the docs, So nobody will run into such issues. Specially after Svelte 4 to Svelte 5 migration. $effect ist simple NOT a replacement for $: or afterUpdate etc... It's different.

popapaul commented 9 months ago

Of course these effects are wonky. They're just to show that a setTimeout yields different results.

That is true, setTimeout is not a true alternative.

Maybe instead we could have another variant of $effect which follows the naming scheme as $effect.pre, $effect.active, $effect.root , that is similar to the behaviour of $:

In that case we won`t need the debate anymore.

TGlide commented 9 months ago

Of course these effects are wonky. They're just to show that a setTimeout yields different results.

That is true, setTimeout is not a true alternative.

Maybe instead we could have another variant of $effect which follows the naming scheme as $effect.pre, $effect.active, $effect.root , that is similar to the behaviour of $:

In that case we won`t need the debate anymore.

That could be quite the API pollution IMO. It would be super confusing to have two separate effects functions that do almost the same thing. At least active and root serve a different, and pre works the same, just happens at a different point in time.

Still, maybe it could be valuable. Up to the maintainers :)

benmccann commented 9 months ago

You can avoid using $effect by using a reactive class instead. This is much easier to read and understand as well. I don't know if it matches the original 100% because I found the original to be extremely hard to follow and there aren't any clearly documented requirements, but I think it's at least close enough that it demonstrates the approach works and it could be tweaked as desired.

TGlide commented 9 months ago

You can avoid using $effect by using a reactive class instead. This is much easier to read and understand as well. I don't know if it matches the original 100% because I found the original to be extremely hard to follow and there aren't any clearly documented requirements, but I think it's at least close enough that it demonstrates the approach works and it could be tweaked as desired.

The problem with relying on the getter, is that the state doesn't actually change, so if you go, say, from config 1 to config 2 and back again,a is selected again.

To produce the same results you'd have to do something like this

benmccann commented 9 months ago

Not really a problem with getter, but just with me understanding the requirements :smile: It looks like your version uses getters and works just fine!

TGlide commented 9 months ago

Not really a problem with getter, but just with me understanding the requirements 😄 It looks like your version uses getters and works just fine!

That's fair! 😄

Yeah, I said to mean that the bulk of the calculation can't be only on the getter, but you're right that it depends on the requirements. Nonetheless it's a great example.

jonolo6 commented 8 months ago

I'm just curious - sorry to jump in here - so feel free to tell me to go away ;) But relating to the reactive class example

Is there a reason to use getters over exposed derived (other than the fact that it allows set() customisation)? I.e. rather than

get items() {
   return (this.#items ?? []).filter(i => !i.hidden && i.value)
}

have this:

items = $derived(this.#items ?? []).filter(i => !i.hidden && i.value)
ptrxyz commented 8 months ago

After playing around with runes for a bit, I am also a bit confused. It looks super weird to me that $derived blocks allow you to easily create self-depending loops:

let uuid = $state('');

$effect(() => {
  uuid = Math.random().toString(36).substring(7);
  if (uuid.includes("x")) console.log("X included")
});

Sure, I could go with untrack whenever you read a variable that's also a dependency but that's pretty verbose and probably error prone in more complicated $effects. I could see myself basically having to wrap everything in untrack to avoid unintended loops/updates:

$effect(() => {
  dependency1;
  dependency2;
  dependency3;
  untrack(() => {
    // complicated code goes here
  }) 
})

I would assume that in most cases you would like to not have infinite update loops and wouldn't it be easy to detect those cases: If the $effect block depends on a thing it also assigns to, make it not depend on that something.

Am I missing something or am I the only one thinking this is a bit clunky?

dummdidumm commented 8 months ago

$effect is not a derived, $derived is a derived, and whenever you derive something, you should use $derived. Closing in favor of #10193

dm-de commented 8 months ago

@dummdidumm...

Can you please post an example how you can solve described problem with $derived? I bet that's impossible Impossible, because you can not change bounded prop with $derived

benmccann commented 6 months ago

Here it is with $derived.by. The key is that you need to use a callback prop rather than bind:.