Fixing scope handling on dynamic children

[gbj]

This is basically a "yeah, I hadn't quite thought that all the way through..." issue.

The Problem

Currently, you can render a dynamic child node like this:

{move || count()}

or this

{move || view! { cx, <Child/> }}

This is all well and good. However, consider the following example

#[component]
pub fn App(cx: Scope) -> impl IntoView {
    let (count, set_count) = create_signal(cx, 0);
    view! { cx,
        <button on:click=move |_| set_count.update(|n| *n += 1)>"+1"</button>
        {move || (count() % 2 == 1).then(|| view! { cx, <Child/> })}
    }
}

#[component]
pub fn Child(cx: Scope) -> impl IntoView {
    on_cleanup(cx, || leptos::log!("disappearing"));
    view! { cx,
        <p>"Child"</p>
    }
}

In this example, disappearing never logs, even though the <Child/> is added and removed.

What's Happening?

So what's happening here?

DynChild (the internal representation for this kind of thing) does create a child scope, which it uses to call .into_view() when building the child content

leptos/leptos_dom/src/components/dyn_child.rs

Lines 191 to 192 in 64e056f

	let (new_child, disposer) =
	cx.run_child_scope(|cx| child_fn().into_view(cx));

And it correctly disposes of that scope

leptos/leptos_dom/src/components/dyn_child.rs

Lines 200 to 201 in 64e056f

	// Dispose of the scope
	prev_disposer.dispose();

However, this Scope is actually not the Scope that the <Child/> is rendered with.

In particular, this line is a problem.

{move || (count() % 2 == 1).then(|| view! { cx, <Child/> })}

cx here is moved in from <App/>.

• Passing the move || into the view creates a DynChild. When it renders its content, it creates and disposes child scopes descended from <App/>.
• Creating the <Child/> component creates a child scope, and renders <Child/> in it. But note that in view! { cx, <Child/>, the cx is the <App/> scope.
  This means that these two are siblings, not parent and child.

App
|_ DynChild
  |_ (created child scope)
|_ Child

The Solution

The correct design for this is fairly simple: rendering a dynamic child should require a function that takes a Scope as its first argument, like this

{move |_| count()}

or this

{move |cx| view! { cx, <Child/> }}

In this case, because the cx is not moved into the closure from <App/> but lazily provided by DynChild, the chain is correctly ordered

App
|_ DynChild
  |_ (created child scope)
    |_ Child

This means that when DynChild disposes of the created child scope, it will correctly clean up.

Discussion

This has the potential to reduce the complexity of scope disposal code in the router and possibly the <For/> component. It will also fix this bug, and probably some unknown others.

On the other hand, it's a breaking change to pretty much every example, piece of documentation, and user application.

[pikhosh]

Seems like a small price to pay for the potential to fix the problems you mentioned? 🤔
Even for writing new code with Leptos, I think it makes more sense to pass the Scope as an argument to the move closure than to pass nothing and use the Scope inside {move || (count() % 2 == 1).then(|| view! { cx, <Child/> })}...

[benwis]

I said it elsewhere, but if that's what we need to do to make things work right, then break away!

[gbj]

So, it turns out sometimes there’s a good reason to sit and let things stew. Thinking about the discussion in #880 and well as a comment from @novacrazy (I think) that we should be able to detect the current scope programmatically led me to realize that the much better solution here is (perhaps unsurprisingly) to do what Solid does: associate cleanups with effects (or memo), rather than with a Scope, and run them every time the effect runs. Because conditional/dynamic logic will always end up running within an effect (or memo), this means automatic, correct cleanup without manual scope manipulation.

Apparently it took me nine months and a couple bug reports to understand why Solid is designed this way but now it makes perfect sense.

We could probably do the same thing with contexts and ownership for signals/etc., which may lead to the eventual abolition of explicit cx itself, but we can… leave that for a future release.

For now, since this will still be a change in the framework’s behavior it will be part of 0.3.0, but I do not expect user code to have to change to accommodate it.

Edited to add: If contexts were associated with effects instead of scopes we could have avoided #542 but I think we might need context provider components to wrap children in an effect instead of a simple provide_context(cx, /* */) so I’m not sure it’s worth changing that in fact.

[gbj]

Closed by #918.