sveltejs / svelte

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

svelte 5: suggestion to add an optional argument to the $effect cleanup callback to know if the effect is being destroyed or re-ran #11477

Open petermakeswebsites opened 6 months ago

petermakeswebsites commented 6 months ago

Describe the problem

$effects can take a callback function that's called every time they are destroyed. They offer a convenient "cleanup" return function as follows:

$effect(() => {
  console.log("I am an effect of some " + someStatefulVariable)
  return () => {
    console.log("I'm being either cleaned up for a re-run or destroyed!")
  }
})

This is nice but it would be even better if the cleanup function could know whether it's being destroyed or re-ran. This would allow different kinds of processing depending, and could possibly clean up some otherwise verbose and confusing workarounds if the use for it does arise.

Describe the proposed solution

The return function of an $effect could take a argument that notifies the function whether it's being cleaned for a re-run or destroyed completely.

$effect(() => {
  console.log("I am an effect of some " + someStatefulVariable)
  return (destroyed) => {
    console.log("I am being " + destroyed ? 'destroyed' : 're-ran')
  }
})

Drawbacks

Might encourage the use of $effect more than necessary, and might also have a very small performance hit for the vast majority of the times it's not being used.

Importance

nice to have

karimfromjordan commented 6 months ago

Do you have a specific use case in mind?

petermakeswebsites commented 6 months ago

I haven't encountered one. I came up with the idea while writing the internals of Svelte-5-like nano reactivity system that I plan to use for educational purposes. I just thought it might be useful for someone at some point (hello Googler from 3 years down the line 👋).

I also had another idea which would be to pass the return value of the cleanup as an $effect argument. It might just keep things a little bit more "compartmentalised", tied into the lifecycle of the $effect itself rather than needing to work with external state. I won't cry if it's not implemented, but it might make things just a tad bit neater. Who knows it might be useful for something like debouncing api requests or something.

$effect((count) => {
  if (count === undefined) count = 1
  console.log("The " + statefulVar + " has changed " + count + " times")
  return () => {
    console.log("Cleaning up and incrementing count")
    count + 1
  }
})
petermakeswebsites commented 6 months ago

Here's a use case actually, say I need to debounce and update some state. I can do something like this. Might be nice to have it neat and all tucked into an $effect rather than have state spilling out all over the place. Neat and tidy.

$effect((timeoutID) => {
  clearTimeout(timeoutID)

  const someApiCall = runOnce(() => /* fetch(statefulStuff) */)
  timeoutID = setTimeout(timeoutID, 1000)

  return (destroyed) {
    if (destroyed) {
      clearTimeout(timeoutID)
      someApiCall()
    } else {
      return timeoutID
    }
  }
})
Conduitry commented 6 months ago

There has been at least a little bit of internal conversation about something like this. I had floated it as an idea for a way to let $effect functions know whether they should completely clean up after themselves (when they're destroyed) or whether they should just make some adjustments to what happens when various listeners/etc. are run. I ended up deciding that having nested/sibling $effect blocks was probably the way to go here - one of which wouldn't be torn down until the whole component was being destroyed, and the other of which would be torn down when there were other kinds of changes - without requiring any additional functionality. I haven't come up with a concrete case where knowing whether an $effect was being destroyed or being re-created is actually helpful.

7nik commented 6 months ago

What about moving the destroying logic to the onDestroy and hoisting all the necessary variables to the script level so that they are available in the onDestroy?

cdcarson commented 6 months ago

I don't understand why the callback is called before each run of $effect, rather than just when the component is destroyed. Not saying it's wrong to do so -- I just can't think of a pattern where it'd be useful. The $effect body already runs each time the state it references changes, so presumably all the "update" logic can go there.

let open = $state(false);
let focusTrap: FocusTrap|undefined = undefined;
$effect(() => {
  if (!focusTrap) {
     focusTrap = createFocusTrap(/*some init*/);
  }
  if (open) {
     focusTrap.activate();
  } else {
    focusTrap.deactivate();
  }
  return () => {
     // seems like this would be redundant on update, given the code above
    focusTrap.deactivate()
  }
})
MotionlessTrain commented 6 months ago

I don't understand why the callback is called before each run of $effect, rather than just when the component is destroyed.

It requires you to have those resources on the global scope, though. Otherwise you cannot refer to the previous version of the resource in order to clean it up. Cleaning it up in the cleanup function which is ran each time the effect refires can just keep the variable in the effect function, as closure

especially with things like event listeners (if there is ever a use case for that), the above code wouldn't precisely work, and you get something like

let listener: ((evt: Event) => void)|undefined = undefined
$effect(() => {
  if(listener) {
    //Hoping that el is not a state which changed, otherwise this is even uglier with a previousEl which needs to be updated by the effect in order to clean the listener up
    el.removeEventListener('event', listener)
  }
  // listener depends on the state this effect listens to
  listener = evt => { /*...*/ }
  el.addEventListener(listener)
  return () => el.removeEventListener(listener) // Needed anyway, when component is destroyed. If it wouldn't run each time, the mess at the top of the function is needed
})

which is less clean than just keeping the previous element and listener within the cleanup closure which needs to exist anyway (Granted, I know this is not the best example, an action is much better for this, but there are probably resources which work the same as event listeners in this regard)

cdcarson commented 6 months ago

@MotionlessTrain Thanks, I get it. If in your example el was a property of the component (not a bound html element in the component's own markup) we'd have remove the listener each time $effect ran.