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

$derived.by destructor support (Svelte 5) #13249

Open
timephy opened this issue Sep 15, 2024 · 18 comments
Open

$derived.by destructor support (Svelte 5) #13249

timephy opened this issue Sep 15, 2024 · 18 comments
Assignees

Comments

@timephy
Copy link

timephy commented Sep 15, 2024

Describe the problem

I use $derived or $derived.by to "couple" state widely in my app. For example like so:

    const baseObj = $state(...)
    const derivedObj = $derived.by(() => {
        // Not possible here, because it cannot reference itself
        // derivedObj.destroy()

        const _obj = new DerivedObj(baseObj)
        _obj.setup()
        return _obj
    })

The baseObj might not be a primitive type, but rather a class with "complex behaviour", for example in many cases a rxjs.Observable inside. When I create a new DerivedObj(baseObj) from that baseObj, then I might want to setup listeners/subscriptions on baseObj, which should be unsubscribed/destroyed on the old derivedObj, when it is recreated by $derived.

This raises the need for a destructor function.

Describe the proposed solution

$effect currently has something like "destuctor support": (see Preview Docs)

You can return a function from $effect, which will run immediately before the effect re-runs, and before it is destroyed (demo)."

Therefore it might make sense to implement it similarly for $derived.

There should be a functionality that does this (sentence from $effect docs reformulated):

You can specify a "destuctor" function, which will run immediately before the derived.by re-runs, and before it is destroyed.

I could imagine using it like so:

    const baseObj = $state(...)
    const derivedObj = $derived.by(
        // "constructor"
        () => {
            const _obj = new DerivedObj(call, modalVC)
            _obj.setup()
            return _obj
        },
        // "destructor"
        () => {
            derivedObj.destroy()
        }
    )

The destructor should be run immediately before $derived reruns and creates a new object from the source state. And before $derived is destroyed, so that the destructor is also called when a component is destroyed, otherwise we have to repeat the destructor inside onDestroy(() => derivedObj.destroy()), which would be repeating every time.

Importance

i cannot use svelte without it

@dummdidumm
Copy link
Member

I think the better (and more broadly applicable) solution would be to provide the current value to the callback function.

$derived.by((curr) => {
  curr.destroy();
  return ...
})

@timephy
Copy link
Author

timephy commented Sep 15, 2024

What should in your opinion happen on the first initialization? Then curr == null?

@paoloricciuti
Copy link
Member

What should in your opinion happen on the first initialization? Then curr == null?

Yeah either null or undefined

@paoloricciuti paoloricciuti self-assigned this Sep 15, 2024
@timephy
Copy link
Author

timephy commented Sep 15, 2024

What should in your opinion happen on the first initialization? Then curr == null?

Yeah either null or undefined

If that is the solution, then most usages of this would look like this:

const foo = $derived.by((curr) => {
  if (curr) curr.destroy() // curr?.destroy()

  return ...
})

It would mean that you have to use if (foo) foo.destroy() or foo?.destroy(), every time.
Therefore destroy() not being called, or being called, only depending on if it is the first "iteration", or not. It think this logic should be abstracted away by giving a seperate function for destroying.

As far as I am aware $effect does not use a parameter for the returned destructor function either, and one is able to access all variables freely. I think this should apply to $derived.by's destructor as well.

const foo = $derived.by(
  () => new Foo(),
  () => foo.destroy() // or even just `foo.destroy`
)

@paoloricciuti
Copy link
Member

The point is that having the current value can be useful for more things than just cleanup. So while it's annoying to have to check for the first instance it's not much more code than writing a new function...and since we've come this far away without even having the initial value I don't think cleaning up stuff in derived will be as common (especially because in theory they should be side effect free)

@timephy
Copy link
Author

timephy commented Sep 15, 2024

I can see some advantages to doing it your way too! This could also solve something that I have struggled with before:

I wanted to "propagate" state as if I was subscribing to an Observable, like so:

const x = $state(0)
const foo = new Foo()
$effect(() => {
    foo.update(x + 1)
}) // basically `x.subscribe((x) => foo.update(x + 1))`
// sadly not possible in `.svelte.ts` files

With this implementation, something like this would be possible:

const x = $state(0)
let foo = $derived.by((curr) => {
    // init if undefined
    if (!curr) {
        curr = new Foo()
    }
    
    // update
    curr.update(x + 1)
    return curr
})

I like that, because it allows me to do things that I used to do when using rxjs.Observables, but better.

(One more thing: if undefined | null is used for when the value is still uninitialized, then we can not differ between "uninitialized" (first run) or that the value actually IS undefined | null – might not be a problem, but worth thinking about)

@paoloricciuti
Copy link
Member

Yeah good point on that...also @dummdidumm i have an implementation of this locally but there's a problem. We access props like this prop_name() and set the value like this prop_name(34). This doesn't create much of a problem per se but there's an optimization in b.thunk that eliminate the thunk if the argument is a call with 0 arguments (like the prop get)...however if derived now passes the value as the first argument something like this

<script>
    let { my_prop } = $props();
    const derived_prop = $derived(my_prop);
</script>

receive the first argument everytime and instead of reading sets the value. I've tried adding a force parameter to the b.thunk function to avoid removing the optimization all together but it's very brittle this way and if in the future we have a similar problem with other functions that could lead to a very bad experience for whoever has to work on that part of the codebase. Wdyt?

@Rich-Harris
Copy link
Member

think the better (and more broadly applicable) solution would be to provide the current value to the callback function

I'm 👎 on this, it's an invitation to add complexity and it comes with all the aforementioned wrinkles. There are two reasons you might want the previous value:

  1. So that you can do cleanup work (i.e. the case at hand) or other side-effects
  2. So that the previous value can be an input to the current value

These are very different cases but they're both things that should generally be discouraged. In the first case, if you have stuff that needs to happen on cleanup, you can do this (i.e. stash the previous value yourself, rather than having it passed to you, which is the same except that a) Svelte is looking the other way rather than actively promoting it, and b) it doesn't take account of any future ambitions to add forking to the framework) but there's a bug — the cleanup doesn't happen when the component is destroyed, which might be acceptable in some scenarios but is absolutely not acceptable as a general pattern. If you have cleanup work to do, use an effect. It's fine. That's what they're for.

In the second case, while there might be some very rare cases where it's valid to treat the previous value, it's likely a sign that things need to be rethought a bit. What if you need the previous-but-one value? And so on. Providing it directly to the derivation might be convenient in a handful of cases but it makes it impossible to infer the correct type, and — again — it actively promotes something we should likely discourage.

It's possible that as the framework evolves I'll need to update this stance and we should consider passing in the previous value, but that's a bridge to cross then — for now it feels reckless.

@ryanatkn
Copy link
Contributor

If you have cleanup work to do, use an effect. It's fine. That's what they're for.

Any thoughts on SSR? Related: #9278

@paoloricciuti
Copy link
Member

Considering the amount of catches the implementation still has and that objectively even if a bit out of the ordinary (deriveds with side effects and assigning state in effects) a solution is possible in user land while also avoid constraining the framework I'm 100% on Rich side here.

@paoloricciuti
Copy link
Member

If you have cleanup work to do, use an effect. It's fine. That's what they're for.

Any thoughts on SSR? Related: #9278

You can just initialize thing with new Thing right?

@ryanatkn
Copy link
Contributor

ryanatkn commented Sep 15, 2024

You can just initialize thing with new Thing right?

How would you do cleanup during SSR? I can see using onDestroy, but it complicates things with duplicated cleanup, and on the client you'd need to guard against calling it both there and in the effect because calling it twice may cause problems depending on the implementation. You'd also need additional code to avoid churn on mount on the client, otherwise you'd create and immediately destroy and recreate the instance.

So if SSR was a concern I think you'd want to use the $derived.by version in combination with onDestroy like this. The main downside with my adjustment is that we're calling destroy in two places. The effect version doesn't seem compatible with these needs and SSR with the current behaviors.

@timephy
Copy link
Author

timephy commented Sep 16, 2024

These are very different cases but they're both things that should generally be discouraged. In the first case, if you have stuff that needs to happen on cleanup, you can do this (i.e. stash the previous value yourself, rather than having it passed to you

While I appreciate the want to keep such things away from the framework, sadly more complex things than primitive mappings are common in many projects... To illustrate how this issue would/could provide a simpler solution:

// A: currently possible
const x = $state(0)
const thing: Thing | null = null
const y: Thing = $derived.by(() => {
    thing?.destoy()
    thing = new Thing(x)
    return thing
})
onDestroy(() => {
    thing?.destroy()
})

// B: with solution in PR
const x = $state(0)
const y = $derived.by((curr) => {
    curr?.destroy()
    return new Thing(x)
})
onDestroy(() => {
    y.destroy()
})

// C: other possible solution
const x = $state(0)
const y = $derived.by(
    () => new Thing(x),
    (curr) => curr.destroy() // or `() => y.destroy()` or even `y.destoy`
)

I believe a need for this does occur, because it bridges a gap between derived-rune-land and imperative-land, and many would benefit from being able to do this more easily.

When looking at the code above I feel like A is the anti-pattern, not B or C.

the cleanup doesn't happen when the component is destroyed, which might be acceptable in some scenarios but is absolutely not acceptable as a general pattern.

The solution C would be more in line with how the $effect cleanup works and also keep the first argument as simple as it currently is.

And would also solve this bug/behaviour.

If you have cleanup work to do, use an effect. It's fine. That's what they're for.

This does not work in .svelte.ts files, which is a limitation that I ran into before, using $derived works and would create a unified solution for .svelte and .svelte.ts files together.

@paoloricciuti
Copy link
Member

How would you do cleanup during SSR? I can see using onDestroy, but it complicates things with duplicated cleanup, and on the client you'd need to guard against calling it both there and in the effect because calling it twice may cause problems depending on the implementation. You'd also need additional code to avoid churn on mount on the client, otherwise you'd create and immediately destroy and recreate the instance.

You shouldn't subscribe to anything on the server since it's kinda "one-shot"...the page execute to render the html and once it's returned it doesn't interact with that page anymore. If you are registering a subscription during SSR you will affect other requests which is bad. This is true regardless if you do it in a derived or in state. So this case should be handled regardless.

@paoloricciuti
Copy link
Member

When looking at the code above I feel like A is the anti-pattern, not B or C.

The reason why it feels like an antipattern is because it doesn't come from the framework. The point Rich tried to make is that this is generally a bit of antipattern, hiding this behind the framework curtain doesn't make it more good.

This does not work in .svelte.ts files, which is a limitation that I ran into before, using $derived works and would create a unified solution for .svelte and .svelte.ts files together.

You just need to structure your code differently and make use of $effect.root or in most case $effect.tracking() to register an effect only when it's actually useful (when inside another effect).

@7nik
Copy link

7nik commented Sep 16, 2024

then I might want to setup listeners/subscriptions on baseObj, which should be unsubscribed/destroyed

Just use the StartStopNotifier which subs and unsubs (or whatever you need) depending on the actual data usage but not object creation and destruction.

@trueadm
Copy link
Contributor

trueadm commented Sep 16, 2024

Deriveds are meant to be pure functions – so the notion of a "cleanup" doesn't sound right. Furthermore, you definitely don't want to be subscribing on the server – as the cleanup will never fire and you'll end up with memory leaks? I think it would be good to know what you're subscribing to here – does the thing that you subscribe to have an intermediate access point rather than needing a subscription? i.e. getLatest()

@ryanatkn
Copy link
Contributor

ryanatkn commented Sep 16, 2024

Another possibility using the proposed $state.from from #12956 adding a hypothetical destroy and not needing set -

const x = $state(0);
const y = $state.from({
    get: () => new Thing(x),
    destroy: (value) => value.destroy(), // the argument makes it compose better but could be omitted
});

This avoids sneaking side effects into $derived.by like the other ways, and it composes better using objects than multiple arguments. But maybe this is too elegant/powerful of an escape hatch that could lead people to worse designs.

Also re: destroy during SSR, I looked at all of my usecases and I don't have one that shouldn't follow the advice of "don't subscribe during SSR". The cases where I call destroy are wasted work on the server, but I'll admit it doesn't sit well with me because potential mistakes aren't a problem if you can rely on destroy instead of expecting the whole tree to get garbage collected without it. In addition to subscriptions there's a related "registration" pattern that may need cleanup - I use context instead of module-scoped globals so cleanup is less error-prone, but a lot of people don't do this - but with code I consider well-structured this seems to be just wasted work with the cases I have. (although the v4 docs mention destroy as running in SSR where other lifecycle functions don't specifically for cleanup)

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

Successfully merging a pull request may close this issue.

7 participants