sveltejs / svelte

web development for the rest of us
https://svelte.dev
MIT License
79.6k stars 4.21k forks source link

In runes when a primitive is reassigned with its previous value run effects listening to it #10615

Closed FoHoOV closed 8 months ago

FoHoOV commented 8 months ago

Describe the problem

In svelte5 lets say we have a state that has a primitive value like this let primitiveState = $state("some value");. Then we try to listen to its changes with an $effect:

$effect(()=>{
      console.log(primitiveState);
      // log to somewhere
      // popup a notification
      // update a store
})

but if for whatever reason the primitiveState has been reassigned to the same value, the effect won't run. This at first was unexpected to me because if instead of primitiveState we had an object like let name = $state({primitiveState: "some value"}); and changed it with name.primitiveState = 'some value' we would get notified; but again if instead we get a derived state with const derivedState = $derived(name.primitiveState and listen to derivedState changes we will not get notified. This inconsistency kind of surprised me. There is another thing as well, the exact same thing works with form actions! why? lets see:

+page.server.ts

import type { Actions } from '@sveltejs/kit';

export const actions: Actions = {
    default: () => {
        return { text: 'static text' };
    }
};

+page.svelte

<script lang="ts">
    import { enhance } from '$app/forms';

    const { form } = $props();
    const derivedMessage = $derived(form?.text);

    $effect(()=>{
        console.log(derivedMessage);
    })
</script>

<form method="post" use:enhance>
    <button>
        submit to get changes
    </button>
</form>

Here when we click on the button at first form changes to undefined which makes the derivedMessage to be undefined at first and as soon the response comes in, form has the value static text. Since it didn't get assinged to static text back to back, we got notifined: instead of static text -> static text it went like static text -> undefined -> static text This is another inconsistency that I saw. For my use-case I first wrote a server side validation, which showed the errors in my form; then I called the exact same function in the client-side and it didn't work 🗡️ (now I know why). There is already a discussion about this in the comments of this bug report (at first I thought this was a bug). Please read the comments and visist the provided REPLs and SvelteLab links there to get a better idea of some other use cases.

I'm gonna mark some points from those discussions here. The provided solutions (I call them workarounds actually) were these:

You can use two-way bindings, an exported function, lists or separate component instances. There are many approaches and most of them should not even require $effect to achieve the desired result. (Would not really recommend using $effect, unless necessary, since it can fairly easily cause trouble.)

And my response was:

all the proposed solutions have some caveats:

  1. Two-way binding: The Notification component in the provided REPL should NOT be allowed to change the message. It should not require two-way data binding to work. Why force mutability when we shouldn't? (By the way, why does this work then? The string content is the same.)
  2. Exported functions: Exported functions are indeed a solution but require a ref, which requires bindings on the parent component. So, basically, to set a prop instead of going <SomeComp someProp=someValue, we have to first export a setter function in SomeComp, then bind a ref for whoever uses this component, then call this function whenever something changes.
  3. List: You basically combined Dispatcher and Alert into one component (erasing the problem), which reduces the reusability of Alert, which shows only one message. If I want to show one message, I need to know that for it to work, I am REQUIRED to pass a list.
  4. Separate component instances: Firstly, it requires a ref; then we need to manually mount and unmount instead of declaratively just saying <Notification .... On each click, we completely destroy the old instance, which destroys ALL INTERNAL STATE of that component (if we have onMount and other stuff, they will rerun as well, which might be a side effect we don't want). We also need to keep track of the introduced notifiation variable. (I'm not sure about this one but maybe when unmounting the parent component we should manually unmount the notification component if it is not unmounted yet to prevent memory leaks) Wouldn't it better that none of these workarounds were required? I want to put my emphasis on the difference in behaviour again as well; also the fact that you should be aware that does not behave as expected based on the reasons provided.

Describe the proposed solution

Primitive value changes should behave the same as objects. This reduces complexitity and improves DX and removes some GOTCHAS I faced.

Importance

nice to have

dummdidumm commented 8 months ago

There's no difference between primitive and object states, in this REPL none of the effects rerun. What you're experiencing is a consequence of a workaround to ensure form properties update correctly after a form reset - i.e. this is specific to SvelteKit's form actions (and a somewhat unfortunate workaround which we'd like to find a better solution for some day).

FoHoOV commented 8 months ago

@dummdidumm you seem to have missed the point, the actual REPL that represents my point is this one. In the linked issue there are other REPLs and SvelteLab examples that also elaborate on the requested feature as well. Assigment to self with the same object reference is not what i'm saying, assigning to the same value (different objects but same value) is what the actual point is.

FoHoOV commented 8 months ago

basically, if we want to be notified if something is changed (change could a different value or reassignment to the same value) we MUST have an object, this will not work with primitive values. As you can see in this REPL, I would expect both of them to have the same behaviour. I see you point, primitves are immutable in JS, but can there be a workaround to make this work?

dummdidumm commented 8 months ago

This actually works consistently, and whether that's intuitive or not depends on how you look at it. When reassigning the object, you're assigning a new instance of that object. So the object as a whole has changed. The $effect that does object.primitive does not only listen to primitive, it also listens to object - in general, it listens to all signals along the object chain. It does that to play it save - we can't possibly know whether you want the effect to rerun when the whole object changes, or only when the property at the end of the chain changes (different people have different opinions on that). By doing it this way, you can easily achieve the desired result by wrapping the object chain in a derived. The other way around wouldn't be that easy.

FoHoOV commented 8 months ago

@dummdidumm So in this REPL (which is also provided in this feature request and the linked issue) I've asked for other better svelte solutions. I've also listed the suggested solutions and the reasons I think they are not good (in this post). What would you suggest to be done instead? Also another question, wouldn't you be surprised that the effect in Alert is not getting called? I mean like now that I know the reason I will always have to be carefull if the prop or state is not a primitive otherwise it won't work with the same value being assigend to it, but isn't this counter intuitive? I want to know your thought process to improve myself to see it as you do so I can write better svelte applications.

FoHoOV commented 7 months ago

Can one of the maintainers please tell me the intended way to achieve what this issue is referring to? :( currently what I do is to use String objects instead of primitives. Take a look at this for instance while reassigning to the same "primitive" value actually reruns the effect.

setting the value here

let primitiveState = $state<Number>(0);
let toggle = 0;

const intervalId = setInterval(()=>{    
    if(toggle < 2){
        primitiveState = new Number(1);
        toggle+=1;
    } else {
        primitiveState = new Number(0.5);
        toggle = 0;
    }
}, 1000);

and passing it normaly like

<Button opacity={primitiveState}></Button>

Logs:

"primitive: 0"
2x "primitive: 1"
"primitive: 0.5"
dummdidumm commented 7 months ago

If you want to ensure the effect reruns, introduce a variable to it that is guaranteed to change, like an incrementing counter. Your way of using a new object instance also works.