sveltejs / svelte

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

Allow opt-in explicit dependency tracking for `$effect` #9248

Open ottomated opened 8 months ago

ottomated commented 8 months ago

Describe the problem

In Svelte 4, you can use this pattern to trigger side-effects:

<script>
  let num = 0;
  let timesChanged = 0;

  $: num, timesChanged++;
</script>

<h1>Num: {num}</h1>
<h2>Times changed: {timesChanged}</h2>
<button on:click={() => (num++)}>Increment</button>

However, this becomes unwieldy with runes:

<script>
  import { untrack } from 'svelte';
  let num = $state(0);
  let timesChanged = $state(0);

  $effect(() => {
    // I don't like this syntax - it's unclear what it's doing
    num;
    // If you forget the untrack(), you get an infinite loop
    untrack(() => {
      timesChanged++;
    });
  });
</script>

<h1>Num: {num}</h1>
<h2>Times changed: {timesChanged}</h2>
<button on:click={() => (num++)}>Increment</button>

Describe the proposed solution

Allow an opt-in manual tracking of dependencies:

(note: this could be a different rune, like $effect.explicit)

<script>
  let num = $state(0);
  let timesChanged = $state(0);

  $effect(() => {
    timesChanged++;
  }, [num]);
</script>

<h1>Num: {num}</h1>
<h2>Times changed: {timesChanged}</h2>
<button on:click={() => (num++)}>Increment</button>

Alternatives considered

would make my life easier

dummdidumm commented 8 months ago

You can implement this in user land:

function explicitEffect(fn, depsFn) {
  $effect(() => {
    depsFn();
    untrack(fn);
  });
}

// usage
explicitEffect(
  () => {
     // do stuff 
  },
  () => [dep1, dep2, dep3]
);
pbvahlst commented 8 months ago

I use a similar pattern when e.g. a prop change, this would also benifit from being able to opt in on what to track instead of remembering to untrack everything.

export let data;
let a;
let b;

$: init(data);

function init(data) {
   a = data.x.filter(...);
   b = data.y.toSorted();
} 

I'm only interrested in calling init() when data change, not when a or b does, but a and b is used in the markup so they you still update the DOM. Furthermore with $effect() this would not work for ssr if I understand correctly?

ottomated commented 8 months ago

You can implement this in user land:

function explicitEffect(fn, depsFn) {
  $effect(() => {
    depsFn();
    untrack(fn);
  });
}

// usage
explicitEffect(
  () => {
     // do stuff 
  },
  () => [dep1, dep2, dep3]
);

Good approach. Don't love the syntax, but I suppose it's workable.

ottomated commented 8 months ago

@pbvahlst

I think you want

const { data } = $props();
const a = $derived(data.x.filter(...));
const b = $derived(data.y.toSorted());
pbvahlst commented 8 months ago

Thanks @ottomated, that seems reasonable. The inverse of $untrack() would still make sense though as you initially suggested. Maybe something like

$track([dep1, dep2], () => {
    // do stuff
});

But let's see what happens πŸ™‚

crisward commented 8 months ago

FWIW I currently make stuff update by passing in parameter into a reactive function call eg

<script>
let c = 0, a = 0, b = 0

function update(){
  c = a+b
}

S: update(a,b)
</sctipt>

Perhaps effect could take parameters on the things to watch, defaulting to all

$effect((a,b) => {
    c = a + b
});

That's got to be better that the 'untrack' example here https://svelte-5-preview.vercel.app/docs/functions#untrack

Also I think the $effect name is a very "computer sciencie", I'd personally call it watch, because that's what it feels like its doing. I think the below code says what it does.

$watch((a,b) => {
    c = a + b
});
Evertt commented 8 months ago

@crisward I personally love the syntax you came up with, but I'm sure there's gonna be a lot of complaints that "that's not how javascript actually / normally works".

brunnerh commented 8 months ago

It could cause issues/confusion when intentionally using recursive effects that assign one of the dependencies. It would not make much sense to re-assign a function argument as that change would commonly be gone after the function scope.

A side note on the names: $effect keeps the association with side effects which is lost with $watch. This rune is coupled to the component life cycle and will only execute in the browser; "watch" to me sounds more general than it actually is.

I am in favor of having explicit tracking, maybe even as the recommended/intended default; as of now the potential for errors with $effect seems huge, given that it can trigger on "invisible" signals if any functions are called that contain them (related issue).

Artxe2 commented 8 months ago

How about adding a syntax like $effect.last, which only runs once at the end in a microtask, like the existing $ label syntax, and does not track changes caused by it? It seems like $effect may need some fine-grained behaviors, like $effect.pre, which behaves like beforeMount.

Not-Jayden commented 8 months ago

I'd opt for using a config object over an array if this were to be implemented. e.g.

 $effect(() => {
    timesChanged++;
  }, {
    track: [num],
  });

More explicit, and makes it more future-proof to other config that might potentially be wanted. e.g. there could be a desire for an untrack option as well to explicitly opt out of specific dependencies that might be nested in the effect, or you could define a pre boolean instead of needing $effect.pre

Blackwidow-sudo commented 7 months ago

Perhaps effect could take parameters on the things to watch, defaulting to all

$effect((a,b) => {
    c = a + b
});

But that would mean that we cant pass callbacks like it was intended before.

function recalc() {
  c = a + b
}

$effect(recalc)

Im not really in favor of untrack but i dont see a better solution. Passing an array of things to track is also very annoying, isn't that what react does?

aradalvand commented 6 months ago

The API shapes I'd propose:

$effect.on(() => {
    // do stuff...
}, dep1, dep2, dep3);

Or

$watch([dep1, dep2, dep3], () => {
    // do stuff
});

I actually prefer $watch, primarily because we'll also need to have a .pre variation for this, which would make it look a little weird: $effect, $effect.pre, $effect.on, $effect.on.pre.

With $watch, on the other hand, we'll have: $effect, $effect.pre <=> $watch, $watch.pre. Nice and symmetrical.

Plus, the word watch makes a lot of sense for this, I feel.

opensas commented 3 months ago

I think something like this would be pretty useful. Usually when I needed to manually control which variables triggered reactivity it was easier for me to think about which variables to track, and not which ones to untrack. To avoid adding new runes (and new stuff to teach and learn) I guess it would be easier to let $effect track every referenced variable by default, but allowing to explicitly specify dependencies, either with an extra deps array or with a config object.

eddiemcconkie commented 3 months ago

I'd be in favor of a $watch rune like @aradalvand proposed, as I ran into a use case for it today. I have several inputs on a page (id, name, description, location) and some input groups (tags and contents) which are kept in an array of states. I have the following code to mark the save state as "dirty" when any of the inputs change:

let { id, name, description, location, tags, contents } = $state(data.container);
let saveState = $state<'saved' | 'saving' | 'dirty'>('saved');
$effect(() => {
    [id, name, description, location, ...tags, ...contents];
    untrack(() => {
        saveState = 'dirty';
    });
});

I'm using the array to reference all the dependencies. The tags and contents both need to be spread so that the effect triggers when the individual array items change. Then I need to wrap saveState = 'dirty' in untrack so that it doesn't cause an infinite loop. With a $watch rune, it could look like this:

$watch([id, name, description, location, ...tags, ...contents], () => {
    saveState = 'dirty';
})

I think it makes it more obvious why the array is being used, and it simplifies the callback function since untrack is no longer needed.

opensas commented 3 months ago

@eddiemcconkie that's a great example of a good opportunity to explicitly define dependencies

we could also add an options parameter like this

$effect(() => saveState = 'dirty', { track: [id, name, description, location, ...tags, ...contents] })

which I think has the advantage of reflecting that it really does the same as a regular $effect with explicit dependecies

eddiemcconkie commented 3 months ago

@opensas would that still track dependencies based on what's referenced in the callback? I think the difference with the $watch proposal is that it wouldn't automatically track dependencies

opensas commented 3 months ago

@opensas would that still track dependencies based on what's referenced in the callback? I think the difference with the $watch proposal is that it wouldn't automatically track dependencies

no, in case dependencies are explicitly passed, references in callback should be ignored. you are telling the compiler "let me handle dependencies".

Evertt commented 3 months ago

Just to add my 2 cents. I find an extra $watch rune less confusing in this case. I'd rather have two runes that serve a similar purpose, but use two very different ways of achieving that purpose. Than to have a single rune whose behavior you can drastically change when you add a second parameter to it.

dummdidumm commented 3 months ago

I want to reiterate that it's really easy to create this yourself: https://github.com/sveltejs/svelte/issues/9248#issuecomment-1732246774 Given that it's so easy I'm not convinced this warrants a new (sub) rune

FeldrinH commented 2 months ago

I want to reiterate that it's really easy to create this yourself: #9248 (comment) Given that it's so easy I'm not convinced this warrants a new (sub) rune

On the other hand, if every project starts to define similar helpers, possibly with different argument orders and names, then this will harm the readability of code for anyone not familiar with the helpers used in that specific project and will add mental overhead to switching between projects. I feel that having an effect with a fixed set of dependencies tracked is common enough that it really should be part of the core library.

opensas commented 2 months ago

I want to reiterate that it's really easy to create this yourself: #9248 (comment) Given that it's so easy I'm not convinced this warrants a new (sub) rune

On the other hand, if every project starts to define similar helpers, possibly with different argument orders and names, then this will harm the readability of code for anyone not familiar with the helpers used in that specific project and will add mental overhead to switching between projects. I feel that having an effect with a fixed set of dependencies tracked is common enough that it really should be part of the core library.

I wholeheartedly agree with @FeldrinH, it's not just whether it's easy or difficult to achieve such outcome, but providing an idiomatic and standard way to perform something that is common enough that it's worth having it included in the official API, instead of expecting everyone to develop it's own very-similar-but-not-quite-identical solution.

Anyway, providing in the docs the example provided by @dummdidumm would really encourage every one to adopt the same solution.

Gin-Quin commented 2 months ago

Actually I would do it like this in Svelte 4:

<script>
  let num = 0;
  let timesChanged = 0;

  $: incrementTimesChanged(num);

  function incrementTimesChanged() {
    timesChanged++
  }
</script>

Which you can do the same way in Svelte 5 (EDIT: you can't, see comment below):

<script>
  let num = $state(0);
  let timesChanged = $state(0);

  $effect(() => incrementTimesChanged(num));

  function incrementTimesChanged() {
     timesChanged++
  }
</script>

But I agree that a watch function / rune would be awesome and add much readability for this use case:

<script>
  let num = $state(0);
  let timesChanged = $state(0);

  $watch([num], incrementTimesChanged);

  function incrementTimesChanged() {
     timesChanged++
  }
</script>

I'm actually a fan of the $watch rune idea. In opposition to $effect, it would allow fine-grained reactivity. $effect can be messy because you have to read all the code inside the function to guess wha have triggered it.

Imagine a junior developer wrote 50 lines instead a $effect rune and it's your job to debug it...

ottomated commented 2 months ago

@Gin-Quin

Which you can do the same way in Svelte 5:

Actually, the point is you can't do it the same way. Try itβ€”that code will cause an infinite loop as it detects timesChanged being accessed even inside the function. That's why I still think that a separate rune for this is useful, for clarity even if it can be implemented in userspace.

Gin-Quin commented 2 months ago

Wow, you're right. I really need to change my vision of reactivity with Svelte 5.

This also means $effect is even more dangerous than I thought, and would re-run in many undesired cases.

"Side-effects" are bad design in programming, and $effect is promoting "reactivity side-effects" by its design.

The previous example is actually a very good minimal example of a reactivity side-effect:

<script>
  let num = $state(0);
  let otherNum = $state(0);

  $effect(() => logWithSideEffect(num));

  function logWithSideEffect(value) {
    console.log(value)
    console.log(otherNum) // side-effect, which triggers unwanted "side-effect reactivity"
  }
</script>

When reading the line with $effect, you expect it to run when num changes, but not when otherNum changes. This means you have to look at the code of every function called to check what triggers the whole effect function.

There is another caveat with the $effect rune. Let's say you want to log a value reactively, but only in some conditions:

<script>
  let num = $state(0);

  $effect(() => {
    if (shouldLog()) {  
      console.log({ num }) 
    } 
  });  

  function shouldLog() {
    return Math.random() > 0.3 // this is just an example
  }
</script>

You would expect the $effect callback to run every time numchanges, right?

But that's not what will happen. It will log at first, until it will randomly (once chance out of three) stop logging forever.

A $watch rune would solve it:

<script>
  let num = $state(0);

  $watch(num, () => {
    if (shouldLog()) {  
      console.log({ num }) 
    } 
  });  

  function shouldLog() {
    return Math.random() > 0.3
  }
</script>
Evertt commented 2 months ago

@Gin-Quin well I agree with almost everything you're saying. Especially the lack of transparency of when an $effect() would re-run could be a serious problem I think.

This also means $effect is even more dangerous than I thought, and would re-run in many undesired cases.

However, this is simply a bug that I believe could be fixed. I believe some other frameworks / libraries have fixed this already. Jotai for example, say this in their docs about their atomEffect:

  • Resistent To Infinite Loops: atomEffect does not rerun when it changes a value with set that it is watching.

But I do fully support adding the feature to run an effect based on an explicitly defined list of dependencies. Since an $effect() that watches an explicitly defined list of dependencies would need a significantly different implementation, I also think it makes sense to make it a different rune with a different name. And $watch makes a hell of a lot of sense to me.

Blackwidow-sudo commented 2 months ago

There is another caveat with the $effect rune. Let's say you want to log a value reactively, but only in some conditions:

<script>
  let num = $state(0);

  $effect(() => {
    if (shouldLog()) {  
      console.log({ num }) 
    } 
  });  

  function shouldLog() {
    return Math.random() > 0.3 // this is just an example
  }
</script>

You would expect the $effect callback to run every time numchanges, right?

But that's not what will happen. It will log at first, until it will randomly (once chance out of three) stop logging forever.

Oh wow, this is actually very weird behaviour:

let count = $state(0)

// Whole $effect callback wont run anymore
$effect(() => {
  console.log('Effect runs')

  if (false) {
    console.log(count)
  }
})
brunnerh commented 2 months ago

This is neither a bug, nor how $effect is supposed to be used.

$effect tracks only the dependencies it actually needs; they can change with every execution of the effect. Having randomness like this in an effect is also not very representative of actual use cases.

Blackwidow-sudo commented 2 months ago

This is neither a bug, nor how $effect is supposed to be used.

$effect tracks only the dependencies it actually needs; they can change with every execution of the effect. Having randomness like this in an effect is also not very representative of actual use cases.

I understand, but it is not intuitive imo. When i write:

let num = $state(0)

$effect(() => {
  console.log('Hello')

  if (false) {
    console.log(num)
  }
})

I would assume that the first console.log would always get executed when num changes.

brunnerh commented 2 months ago

Depends on your point of view, my expectation is that the dead-code elimination of the bundler leads to that bit of code never making it to production in the first place (example).

Blackwidow-sudo commented 2 months ago

Sure but with a condition that changes on runtime, we have no dead code and we would still have the issue that the effect stops running once the condition is false for one time.

Evertt commented 2 months ago

Normally the condition itself is based on another signal and therefor the effect will reevaluate the condition whenever that signal changes.

So in normal use-cases this should never lead to dead code.

aradalvand commented 2 months ago

For those claiming that this is "not useful/necessary" enough to be worth adding, there is literally an example in the Svelte 5 docs where something like this is precisely what's needed, and they're resorting to a dummy reference to work around it β€” see this $effect.pre example.

image

Goes to show that the need for this very much does arise in any sufficiently complex app.

7nik commented 2 months ago

It's easy to adjust the list of dependencies via referencing variables or untrack, which is a rare case. Cases, when you need strict control over the entire list of dependencies, seem to be exceptional and are not worth changing the API.

btw, that example won't work because neither messages nor div are $stateful.

FeldrinH commented 2 months ago

Cases, when you need strict control over the entire list of dependencies, seem to be exceptional

What are you basing this claim on? Since code in $effect usually has side effect and might not be idempotent, I have the exact opposite view -- I would use explicit dependencies most of the time and I would only fall back to automatic tracking if I know for sure that I want every variable the effect might read to trigger a re-execution.

Rich-Harris commented 2 months ago

For those claiming that this is "not useful/necessary" enough to be worth adding, there is literally an example in the Svelte 5 docs where something like this is precisely what's needed, and they're resorting to a dummy reference to work around it β€” see this $effect.pre example.

Fun fact: the only reason that example is in the docs is because I literally couldn't think of any other use case for $effect.pre. If someone can think of a better one, I'll replace it. The reality is that you'd be much better off implementing that autoscroll behaviour with event handlers... which is very often the case in my experience β€” most effects should be event handlers. (Also, as @7nik just pointed out, the example code is broken. Oops! I must not have actually tested it.)

I'd welcome non-contrived examples of cases where an effect should re-run when x changes but the function doesn't actually use x in any way, because that flawed autoscroll example is the only one I think I've ever found. (The 'mark as unsaved' example above is probably the most convincing alternative I've heard.) My hunch is that in the vast majority of cases, if you find yourself doing this it's a sign that there's an opportunity to refactor the code. Your effects should be few, and they should be small and simple. If you find yourself putting a lot of logic in there, consider it a red flag.

That's why I'm not persuaded by this argument:

it's not just whether it's easy or difficult to achieve such outcome, but providing an idiomatic and standard way to perform something that is common enough that it's worth having it included in the official API, instead of expecting everyone to develop it's own very-similar-but-not-quite-identical solution

I don't think it's common enough, and I don't think it's a pattern we should promote, which is what a dedicated rune would amount to. I think this thread is, more than anything, evidence that we haven't done a good job of articulating better patterns.

"Side-effects" are bad design in programming, and $effect is promoting "reactivity side-effects" by its design.

Let's dig into this a bit. I understand what it's getting at β€” pure functions are easier to understand than a web of side-effects β€” but what it misses is that side-effects are the whole point of programming. Changing the colour of a pixel is a side-effect! Svelte's job is to make side-effects happen (an event occurs, some state changes as a result of the event, and Svelte updates a DOM element to reflect the new state β€” that's a side-effect).

Unfortunately not all the side-effects that are necessary for your program to work can be modelled in that declarative way (rendering to a canvas, for example). So we have to have some way to allow arbitrary code to run in response to state changes. Far from promoting side-effects, the goal of $effect is to keep them contained, and we chose the word 'effect' over 'watch' (or similar) precisely to discourage people from overusing them.


Having said all that, let's consider this proposal:

$watch([dep1, dep2, dep3], () => {
    // do stuff
});

Suppose I do this:

$watch([todos], () => {
  console.log('todo list changed');
});

What happens when I do todos[i].done = !todos[i].done? Does the message get logged? If yes, it's because we read every member of todos, and every property therein, so that we can track all those reactive properties separately. That's potentially very expensive, and therefore a dealbreaker.

If no... well that's just extremely confusing, which is also a dealbreaker.

(Note that $inspect does track things deeply, because it's dev-only which makes the performance hit acceptable.)

And what about this β€” should this work?

const deps = [dep1, dep2, dep3];

$watch(deps, () => {
    // do stuff
});

The answer is it can't work (try transforming the code in your head and you'll understand). So we have to introduce a syntactical restriction β€” the first argument must be an array literal whose members are all state. And along with the syntactical restriction we've introduced a subtle difference in how variables behave β€” everywhere else, if you reference dep1 you're referencing its value at that moment, but here you're referencing... something else, which would be a difficult thing to explain to people. These sorts of subtle anomalies add up to a confusing and untrustworthy system, where you find yourself crossing in and out of the uncanny valley.

So in summary: I don't think we should do this, and even if I did, I don't think we've yet seen a plausible design.

aradalvand commented 2 months ago

To respond to some of those points:

I'd welcome non-contrived examples of cases where an effect should re-run when x changes but the function doesn't actually use x in any way

That's actually the far less common use case, the more common one is where the function accesses a, b, c, d, e, f, and g, but should only re-run in reaction to a and b changing. Yes, you could untrack c, d, e, f, and g, but it would be more idiomatic (and also less code) if you could just specify a and b and be done with it, instead of doing it in reverse (i.e. specifying the non-dependencies) and also having to remember to add anything else you might access in the future to that list, or having to worry about what that other functions invoked within the effect might access.

And what about this β€” should this work?

const deps = [dep1, dep2, dep3];

$watch(deps, () => {
    // do stuff
});

To me, $watch(deps is just sugar for $watch(() => deps where that function is executed once upon initialization, and the accessed stuff are recorded as dependencies.

eddiemcconkie commented 2 months ago

@Rich-Harris thanks for explaining some of the reasons against $watch, I think I'm convinced. Even in the example I gave, I was doing [id, name, description, location, ...tags, ...contents] and had to spread tags and contents so that each array item would be tracked. After reading your explanation, I think it probably makes more sense to just add an event listener to each input instead

Rich-Harris commented 2 months ago

That's actually the far less common use case, the more common one is where the function accesses a, b, c, d, e, f, and g, but should only re-run in reaction to a and b changing

Can you give an example of where you're doing that? Would love to better understand it.

I think it's instructive to look at the example of React. Firstly, a lot of React users strongly dislike the dependencies array and look at the corresponding APIs in reactive frameworks with jealousy, so I think people would be surprised at this discussion.

But secondly, even though there's an explicit dependency array β€” the thing people are asking for here β€” it's very much not a way to actually control the dependencies. It's just a way to tell the runtime about them. Your linter will yell at you if you omit c, d, e, f and g from the dependencies array, because the React team learned time and time again that omitting dependencies inevitably leads to bugs. And if the answer is 'but I wouldn't write buggy code', that's fine, but many people will and so the existence of the feature would increase the bugginess of the average Svelte app.

ottomated commented 2 months ago

@Rich-Harris Thanks for the input! I've been swayed into thinking this is a footgun after reading the responses, but just to address some points:

Rich-Harris commented 2 months ago

You often need to reset a dialog's state whenever it's closed. If its initial state depends on a $state, the reset function would be called multiple times. Example.

As far as I can tell this is a perfect example of an effect that should just be an event handler πŸ˜€

<script>
-   import {untrack} from 'svelte';
    let dialogOpen = $state(false);
    let initialText = $state('');

    let input = $state('');

-   $effect(() => {
-       untrack(() => console.log('effect triggered', dialogOpen, input, initialText));
-       dialogOpen;
-       untrack(() => {
-           input = initialText;
-       });
-   });

</script>

<label>
    Initial Text: <input bind:value={initialText} />
</label>
<br/>
-<button onclick={() => (dialogOpen = true)} disabled={dialogOpen}>Open dialog</button>
+<button onclick={() => (dialogOpen = true, input = initialText)} disabled={dialogOpen}>Open dialog</button>
<br/><br/><br/>

Agree that passing an array isn't possible. Would something like this work?

It's basically equivalent, because in normal JS you could still do $watch(fn, ...deps), and it still has the slight awkwardness of the first argument being treated literally while subsequent arguments are thunkified behind the scenes. It also gets butchered by Prettier:

image
ottomated commented 2 months ago

As far as I can tell this is a perfect example of an effect that should just be an event handler πŸ˜€

I disagree - often you can close a dialog in multiple ways (button press, click outside, pressing escape, native <dialog> element event listener, binding an open prop to a <Dialog> component). Duplicating the reset call in all of those different spots is flaky.

brunnerh commented 2 months ago

That sounds like an API or approach issue.

Regardless of reason, when a dialog closes, it should fire a close event. The native element specifically has a returnValue property on the element, so the source can be determined. If you have a dialog component which can be closed without firing a close event, then the problem is in how the component fires its events.

Personally, I would not reset on close anyway; instead either on open or not at all by just not reusing dialog instances (create/destroy instances via client-side API - snippets should make this even more convenient).

7nik commented 2 months ago
  • todos list example:

Example without any $effect. Using $effect for the mark-unsaved functionality even has a flaw in that the data is marked as unsaved right away due to the first run of $effect, which may be undersized behaviour.

Talking about the dialog example, it is weird to reuse it. A normal code looks like this:

{#if showDialog}
  <MyDialog prop="value" anotherProp={variable} onclosed={onDialogClosed}>
    some custom markup
  </MyDialog>
{/if}

But I prefer to wrap dialogs into a function so you can do

// data preparation
let result = await showDialog(params);
if (result) {
  // do this
} else {
  // do that
}

In both cases, you don't have the data resetting problem.


Cases, when you need strict control over the entire list of dependencies, seem to be exceptional

What are you basing this claim on? Since code in $effect usually has side effect and might not be idempotent, I have the exact opposite view -- I would use explicit dependencies most of the time and I would only fall back to automatic tracking if I know for sure that I want every variable the effect might read to trigger a re-execution.

My claim is based on observation - I have barely seen people adjusting the dependencies list and attempts to take full control over it only here. I think the dev team will say they observe nearly the same statistics. And, as was said multiple times, it's super easy to make your own $watch with the desired design and shallow/deep/configurable watching. Your words make me think you write $effects that you usually cannot completely understand due to using bad solutions, poor data-flow design, or maybe just strong untrusting of third-party code used in $effect. Or show us examples of such cases, which you seem to have a lot.

Rich-Harris commented 2 months ago

I disagree - often you can close a dialog in multiple ways (button press, click outside, pressing escape, native <dialog> element event listener, binding an open prop to a <Dialog> component). Duplicating the reset call in all of those different spots is flaky.

As noted above, a <dialog> emits a close event, and if you're implementing it some other way then in general there'll be one 'choke point' where you can put this logic.

But suppose that's not the case, and you really do have a situation where dialogOpen is being set to false in multiple locations. All you need to do is replace each dialogOpen = false occurrence with a closeDialog() call, where that function sets both pieces of state.

The central thing to understand is this: effects are about doing things in response to state being a certain way, not in response to state changing in a certain way. Take canvas rendering for example β€” I'm drawing a red circle because color is 'red', not because it changed to 'red'. But in the dialogOpen case, you're setting input not because dialogOpen is true or false, but because it changed. This might seem like a subtle and unimportant distinction but it's absolutely essential to understanding how to wield effects responsibly.

When you use the effect, you're introducing an insidious form of indirection, which is responsible for most incidental complexity in programming. The more indirection you can eliminate, the happier you'll be maintaining the code.

A good rule of thumb is that if you're setting state inside an effect, you're probably doing it wrong. There are enough exceptions to stop us from actually enforcing that (i.e. throwing an error if you set state in an effect) but it's a good rule nonetheless.

ottomated commented 2 months ago

The central thing to understand is this: effects are about doing things in response to state being a certain way, not in response to state changing in a certain way.

Great point! This clarifies things for me. I think in my case, the point of a $watch rune would be to do the second case. Looking through my larger svelte codebases, I see only a few instances of this:


@7nik , I don't think your todo example scales, no? It would require a context or additional prop drilling to be used in child components, and you would have to remember to mutate changed everywhere if you're doing anything more complex than a todo list.


One other thing I stumbled across while looking through my old code is a few instances where I would want to set two variables based on one $derived call. I can make another issue for this unless it's something basic I'm missing.


Anyway, I would be fine implementing these cases manually with untrack, given how rare they are. I'm happy to close this issue.

7nik commented 2 months ago

set two variables based on one $derived call

use destructuring

Why todo list won't scale? All inputs emit the change event. It doesn't matter how deep the element is unless the stopPropogation gets called. For custom inputs, e.g. button selector, the component can have onchange prop and emit change event on the DOM.

ottomated commented 2 months ago

Why todo list won't scale? All inputs emit the change event. It doesn't matter how deep the element is unless the stopPropogation gets called. For custom inputs, e.g. button selector, the component can have onchange prop and emit change event on the DOM.

Which is easier to scale, having one source of truth at the root of your form or making sure that every single way to mutate it emits a custom change event? Even in your todo list example there are two places (addTodo and deleteTodo) where you need to remember to manually trigger the change.

7nik commented 2 months ago

Two places are so many. It's possible to use this approach which is basically deep-equal with the saved data. Plus, this approach understands when all the changes are reverted. It's possible to face performance issues here, but only if you save a really huge object or too often, which is already bad.

opensas commented 2 months ago

The central thing to understand is this: effects are about doing things in response to state being a certain way, not in response to state changing in a certain way.

@Rich-Harris that's a quite subtle but I think absolutely necessary distinction. I have reached a rather similar conclusion in a couple situations that were starting to get out of control, but couldn't quite come out with a clear and decisive rule of thumb.

I hope you can find some time to explain this a bit better, with some real life example if possible, either in a blog post or, even better, some where in the docs

CrendKing commented 1 day ago

You can implement this in user land:

function explicitEffect(fn, depsFn) {
  $effect(() => {
    depsFn();
    untrack(fn);
  });
}

// usage
explicitEffect(
  () => {
     // do stuff 
  },
  () => [dep1, dep2, dep3]
);

@dummdidumm I tried to modify your example to

function explicitEffect(fn, ...deps) {
  $effect(() => {
    deps;
    untrack(fn);
  });
}

explicitEffect(
  () => {
     // do stuff 
  },
  dep1, dep2, dep3
);

but it doesn't work. Can you explain? Why does the deps part has to be a function?