Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Reactive statement cleanup function, like useEffect return value in React #5283

Closed
DylanVann opened this issue Aug 18, 2020 · 14 comments
Closed

Comments

@DylanVann
Copy link

DylanVann commented Aug 18, 2020

Is your feature request related to a problem? Please describe.

I'm trying to port some code that uses React hooks to Svelte. React has useEffect, which can be used to run some code when its dependencies change. I think the Svelte equivalent would be reactive statements ($: { doStuff() }).

When using useEffect you can return a function, and that function will be called when dependencies change, or when the component using the useEffect is unmounted. I do not see an equivalent in Svelte.

Here are the relevant docs for useEffect: https://reactjs.org/docs/hooks-reference.html#cleaning-up-an-effect

Describe the solution you'd like

I think it might be beneficial for Svelte to allow for returning cleanup functions in reactive statements.
In order to allow for returning though I think it would need to be possible to give a function as a reactive statement. Not sure, I'm hoping something like this could be possible though.

// A prop. Some kind of track object that can have listeners.
export let track

// We want to stop all tracks when our stop event listener is called.
// When the track prop is changed to a different track, or when the component is unmounted
// we want to remove the listener.
// This must be a function so we can use a return statement. The function should be called as if it was a statement in the same place.
$: () => {
  let onStop = () => track.stopAllTracks()
  track.on('stop', onStop)
  return () => {
    track.off('stop', onStop)
  }
}

For reference in React this would look like:

useEffect(() => {
  let onStop = () => track.stopAllTracks()
  track.on('stop', onStop)
  return () => {
    track.off('stop', onStop)
  }
}, [track])

Describe alternatives you've considered

The closest I could come up with is this:

import { onDestroy } from 'svelte'
    
export let track
    
let cleanup
    
$: {
  if (cleanup) {
    cleanup()
  }
  const onStop = () => track.stopAllTracks()
  track.on('stop', onStop)
  cleanup = () => track.off('stop', onStop)
}
    
onDestroy(() => {
  if (cleanup) {
    cleanup()
  }
})

This is verbose compared to how useEffect works. As a React user this seems like a step backwards, feeling more like class components, lifecycle methods, instance variables, instead of clean like the hooks version.

This could be cleaned up a bit by initializing cleanup:

import { onDestroy } from 'svelte'
    
export let track
    
let cleanup = () => {}
$: {
  cleanup()
  const onStop = () => track.stopAllTracks()
  track.on('stop', onStop)
  cleanup = () => track.off('stop', onStop)
}
onDestroy(cleanup)

How important is this feature to you?

It's important to me because I'm trying to convert React code to Svelte code and there doesn't seem to be a clean translation of this common React feature (useEffect + cleanup function).

I believe many other users may come from the React ecosystem and encounter this issue.

@dummdidumm
Copy link
Member

dummdidumm commented Aug 18, 2020

There is a library which attempts to shim react hooks, maybe you can just use that
https://github.com/devongovett/svelte-hooks

The fact this library exists means it's possible to do something similar without having to add new features to the core IMO.

@DylanVann
Copy link
Author

DylanVann commented Aug 18, 2020

@dummdidumm Thanks for adding the link. I did take a look at that repo, but it doesn't look like it implements calling cleanup functions. I tried out the code with the REPL and it seems they are not supported.

@kevmodrome
Copy link
Contributor

kevmodrome commented Aug 18, 2020

I suspect that there is an easier way to do this in Svelte that might not be immediately obvious if you're coming from React. A custom store or an action might work well here.

@DylanVann
Copy link
Author

DylanVann commented Aug 18, 2020

@kevmodrome I actually had a similar hook that I was trying to convert to Svelte and using a store creating function in a reactive declaration worked perfectly.

React:

import { useState, useEffect } from 'react';
import { LocalAudioTrack, LocalVideoTrack, RemoteAudioTrack, RemoteVideoTrack } from 'twilio-video';

type TrackType = LocalAudioTrack | undefined;

export default function useIsTrackEnabled(track: TrackType) {
  const [isEnabled, setIsEnabled] = useState(track ? track.isEnabled : false);

  useEffect(() => {
    setIsEnabled(track ? track.isEnabled : false);

    if (track) {
      const setEnabled = () => setIsEnabled(true);
      const setDisabled = () => setIsEnabled(false);
      track.on('enabled', setEnabled);
      track.on('disabled', setDisabled);
      return () => {
        track.off('enabled', setEnabled);
        track.off('disabled', setDisabled);
      };
    }
  }, [track]);

  return isEnabled;
}

Svelte:

import type { LocalAudioTrack } from 'twilio-video'

type TrackType = LocalAudioTrack | undefined

export const useIsTrackEnabled = (track: TrackType) => ({
  subscribe: (onChange: (v: boolean) => void) => {
    onChange(track ? track.isEnabled : false)
    if (track) {
      const onEnabled = () => onChange(true)
      const onDisabled = () => onChange(false)
      track.on('enabled', onEnabled)
      track.on('disabled', onDisabled)
      return () => {
        track.off('enabled', onEnabled)
        track.off('disabled', onDisabled)
      }
    }
  },
})

Usage in Svelte:

  export let track

  // We get a new store when track is changed, cleanup is handled by store auto subscription when the isTrackEnabled store is used.
  $: isTrackEnabled = useIsTrackEnabled(track)

For cases where data is not being derived though, and we're using a reactive statement for side effects, this doesn't seem to work well. I would have to render something invisible to get auto subscription / auto unsubscription working.

@dummdidumm
Copy link
Member

dummdidumm commented Aug 18, 2020

@dummdidumm Thanks for adding the link. I did take a look at that repo, but it doesn't look like it implements calling cleanup functions. I tried out the code with the REPL and it seems they are not supported.

Maybe this is just not implemented yet and can be added.

@DylanVann
Copy link
Author

DylanVann commented Aug 20, 2020

One thing I considered was abusing a custom store for this kind of thing. I figured I would have to do something like render an invisible element, but just using a reactive statement with $ is enough. That actually does seem to work alright:

// Store is reactive because it should re-evaluate when track is changed.
let store
$: store = {
    // We're not trying to get a value out of this, so onChange is never called.
    subscribe(onChange) {
        const onStop = () => track.stopAll()
        track.on('stop', onStop)
        return () => {
            track.off('stop', onStop)
        }
    }
}

// This reactive statement is just used to have the store automatically subscribed and unsubscribed.
$: $store

Alternatively:

const useEffect = (subscribe) => ({ subscribe })

let effect
$: effect = useEffect(() => {
  const onStop = () => track.stopAll()
  track.on('stop', onStop)
  return () => {
    track.off('stop', onStop)
  }
})
$: $effect

@DylanVann
Copy link
Author

DylanVann commented Aug 23, 2020

I've been thinking more about this. I think Svelte has some things to learn from React hooks.

Svelte right now has a lot of opportunities to have component state become out of sync with props. The most basic example I can think of is a label like this:

<script>
	import { onMount } from 'svelte'
	
	export let text
	
	let element
	
	onMount(() => {
		console.log(text)
		element.innerText = text
	})
</script>

<h1>
	{text}
</h1>

<h1 bind:this={element} > </h1>

When the text prop is changed the onMount function will not be run again, so the state of the DOM will not be synchronized with props, which is probably not what a user of this component would expect. The new text will also not be logged.

Solving this sort of issue is an advantage of React hooks. This is discussed in React as a UI Runtime, you can search for "For example, this code is buggy:". It's explaining that if the dependency array is not correct and the effect is not re-run then it becomes out of sync.

If Svelte came up with some kind of hooks like API maybe it could solve both these issues at once.

@pngwn
Copy link
Member

pngwn commented Aug 23, 2020

Just use onDestroy to cleanup when a component dismounts? Your most recent example is expected to cause this issue, it isn't a bug; it is how it is designed to work.

I'm not sure I understand the problem, everything you are describing is already possible. If you want to perform some cleanup everytime a reactive declaration runs then add that cleanup before your side effect. If you want to cleanup when a component dismounts, use onDestroy. If you want to abstract all of this into a reusable thing, use a combination of stores and onDestroy. Svelte doesn't re-render, so you need to respond to component mount/dismount and prop changes separately as they are distinct concepts and never tied together, unlike in React.

While react hooks were one of the catalysts for v3 we don't agree with with the APIs or the model and won't be emulating it.

@pngwn pngwn closed this as completed Aug 23, 2020
@DylanVann
Copy link
Author

DylanVann commented Aug 23, 2020

Cleanup is also required when dependencies (props in the example) change. As things are I think there will be many cases where components do not reflect their props in Svelte code if people are just using the lifecycle methods for these kinds of things.

Svelte doesn't re-render, so you need to respond to component mount/dismount and prop changes separately as they are distinct concepts and never tied together, unlike in React.

I'm suggesting this is a problem generally. Users will not think of being out of sync with props when writing onMount. Often this might not be a big deal, but I don't think it's optimal.

I'm not suggesting that Svelte should re-render like React does, or have dependency arrays, I'm suggesting there should be a way to write lifecycle related code that also responds to changing props, like how useEffect works. I think how React handles this could be a good source of inspiration.

I think Svelte's automatic/compiled reactivity is great. I think it just needs a few changes, possibly non-breaking additions, to be as powerful as hooks, when it comes to abstracting lifecycle related logic, and making it easy to keep effects in sync with props.

This actually does almost exactly what I want, and could almost be used to replace onMount:

let effect
$: effect = useEffect(() => {
  const onStop = () => track.stopAll()
  track.on('stop', onStop)
  return () => {
    track.off('stop', onStop)
  }
})
$: $effect

If there was a version of that that waited for actual mounting, behaved exactly the same, but was slightly less verbose, that would be perfect, while working the same way as the rest of Svelte (no needless re-rendering, no dependency arrays).

React version of the last REPL, for comparison.

@raythurnevoid
Copy link

raythurnevoid commented Sep 14, 2020

I agree with @DylanVann. Svelte should implement an official way to take advantage of hooks.
There are some cases where having a react style hooks is actually pretty useful and allow more code reusability cross components

@TylerRick
Copy link

TylerRick commented Oct 13, 2020

I came across this (via https://dylanvann.com/svelte-version-of-useeffect) while looking for a way to do cleanup (unsubscribe and resubscribe) in reaction to a changes to props in Svelte.


Maybe this is just not implemented yet and can be added.

@dummdidumm, @DylanVann: Indeed, it looks like svelte-hooks did add support for clean-up functions to their useEffect in devongovett/svelte-hooks@1d39d95! ... which is great, though @DylanVann's much simpler and zero-dependency version is even better in some ways.

@DylanVann, I updated your Svelte Hooks Testing Cleanup REPL to use that latest version and the clean-up seems to work now (let me know if not; I don't completely understand your example).

I also created a more minimal example REPL based on your React sandbox.

I guess the workarounds kinda work, but I would rather see first-class support for this. The workarounds so far are too verbose and ugly, and (like @DylanVann says) don't feel like idiomatic Svelte.

Things that users of a framework should be doing (like reacting to changes in props) should be encouraged by the framework by providing first-class support for them and making them super easy.

(BTW, being able to subscribe to an expression like one of the ideas in #4079 could make this a little more concise (eliminate 2 lines of boilerplate): $: $(useEffect(...)) ... though I kind of doubt that will get added.)


Personally, I think the solution @DylanVann suggested (allowing reactive statements to optionally providing a cleanup function) seems pretty reasonable:

$: () => {
  const onStop = () => track.stopAll()
  track.on('stop', onStop)
  return () => {
    track.off('stop', onStop)
  }
}

It would be pretty backwards compatible since most/all existing reactive statements don't currently look (to the parser) like a function, since a function by itself (without invoking it or assigning it to a variable) would have no effect (no pun intended).

@rgossiaux
Copy link
Contributor

rgossiaux commented Jan 27, 2022

While I don't agree with everything OP said here, I do agree that this is something that's generally valuable. Sure, you can simulate this yourself with a bunch of boilerplate, but it's pretty annoying.

Here's the basic way of doing this:

  $: _cleanup = (() => {
    if (_cleanup) {
      _cleanup();
    }
    // Do something
    return () => { /* Do cleanup */ };
  })();

  onDestroy(() => {
    if (_cleanup) {
      _cleanup();
    }
  });

That's a lot of boilerplate.

Ideally you could reduce this to something like:

$: runWithCleanup(() => {
  // Do something
 return () => { /* Do cleanup */ };
});

but if you actually try implementing this you will run into an issue keeping track of which call corresponds to which cleanup function when runWithCleanup is used multiple times within one component. React hooks solve this problem by requiring hooks to always be called in the same order, so they can just use the execution order. But obviously that doesn't work with Svelte since Svelte's reactivity model is more fine-grained & only selectively updates $ statements on invalidations.

If you want to keep this same API, the best I can see is using toString on the function to identify it, but of course this isn't great (not totally correct, for one). The alternative is you require the user to provider some kind of identifier explicitly. This might be the best you can do, but I think it's still a pain since now you have to come up with identifiers each time you invoke this (pretty annoying).

(You also need to do something similar to createEventDispatcher in order to set up the onDestroy properly, I think, but while that's another line of boilerplate I think it's not so bad).

As far as I can see, you need a framework change to actually get really good ergonomics for this use case. If this were done at the compiler level it could be done without much cost to users. To me the only question would be whether it's felt that this is useful enough to add in. Personally I feel like there's a reason React supports this--it's pretty common for something with side effects to need to be cleaned up on changes. Lots of subscription type APIs, like IntersectionObserver, or perhaps something that mutates some other part of the DOM (not the component's DOM where an action would make more sense).

Am I missing anything here?

Edit: thinking about this more, I think an API using a string identifier is really not the end of the world. Though it would be nice if there were a cleaner version built-in, I think I'm going to just go with this and maybe publish it somewhere for other people to use too.

@ivands
Copy link

ivands commented Feb 6, 2022

I agree with @DylanVann.
We need first-class support.
+1

@Swoorup
Copy link

Swoorup commented Jul 20, 2022

Any reason this is closed? Searched around wanting the same thing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

9 participants