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

Variably mounted children stop responding to signals on second mount #2488

Closed
zakstucke opened this issue Apr 2, 2024 · 7 comments
Closed

Comments

@zakstucke
Copy link
Contributor

zakstucke commented Apr 2, 2024

Describe the bug
Continuation of #2482, but sort of different so opening new, turns out the solution provided is still buggy. If children are unmounted, any reactive blocks stop updating on remount.

Screen.Recording.2024-04-02.at.17.39.26.mp4

Leptos Dependencies

For example:

leptos = { version = "0.6.10" }
leptos_meta = { version = "0.6.10", features = [] }
leptos_router = { version = "0.6.10", features = [] }
leptos_axum = { version = "0.6.10", optional = true }
gloo-timers = { version = "0.3.0", features = ["futures"] }
once_cell = { version = "1.19" }

To Reproduce

  • Mount <Parent />
  • Click the toggle button to mount the children, you will see the counter increasing every 100ms
  • Unmount then remount (click click)
  • The counter will no longer update
use leptos::*;

#[component]
pub fn Parent() -> impl IntoView {
    let counter = create_rw_signal(0);
    create_effect(move |_| {
        spawn_local(async move {
            loop {
                gloo_timers::future::TimeoutFuture::new(100).await;
                counter.update(|v| *v += 1);
                tracing::info!("COUNTING");
            }
        })
    });

    view! {
        <Child>
            <div>CHILDREN</div>
            <div>{move || format!("Counter: {}", counter.get())}</div>
        </Child>
    }
}

#[component]
pub fn Child(children: Children) -> impl IntoView {
    let show = create_rw_signal(false);
    // Used your implementation, but my store_value_lazy() has same problem.
    let children = store_value(once_cell::unsync::Lazy::new(|| children()));
    view! {
        <div>
            <button on:click=move |_| {
                show.update(|v| *v = !*v);
            }>"Toggle children"</button>
            <div>{move || format!("Children shown: {}", show.get())}</div>
            {move || {
                show.get()
                    .then(move || {
                        view! { <div>{children.with_value(|value| (*value).clone())}</div> }
                    })
            }}

        </div>
    }
}

Expected behavior
Children shouldn't lose reactivity after the first mount/unmount.

Completely appreciate and excited for the work being done for 0.7, if this is only going to be properly fixed then, any ideas how to get something working for now?

@zakstucke zakstucke changed the title Variably mounted children stop responding to signals on remount. Variably mounted children stop responding to signals on second mount Apr 2, 2024
@zakstucke
Copy link
Contributor Author

zakstucke commented Apr 2, 2024

I should add, this example when using bog standard store_value(children()) without any laziness:

#[component]
pub fn Child(children: Children) -> impl IntoView {
    let show = create_rw_signal(false);
    let children = store_value(children());
    view! {
        <div>
            <button on:click=move |_| {
                show.update(|v| *v = !*v);
            }>"Toggle children"</button>
            <div>{move || format!("Children shown: {}", show.get())}</div>
            {move || {
                show.get()
                    .then(move || {
                        view! { <div>{children.get_value()}</div> }
                    })
            }}

        </div>
    }
}

leads to a leptos panic and toggle btn becoming unresponsive:

image

@gbj
Copy link
Collaborator

gbj commented Apr 2, 2024

Taking a step back for a minute: Is there a reason that you actually need Children (FnOnce) here, which can only be called once, and not ChildrenFn or ChildrenMut, which use Fn and FnMut? And likewise is there a reason not to just use the regular control-flow helpers like Show here?

It is fine if so, and I'm happy to walk you through how to implement it correctly, but it's probably a waste of time unless there is some specific reason not to use those.

#[component]
pub fn Child(children: ChildrenFn) -> impl IntoView {
    let show = create_rw_signal(false);

    view! {
        <div>
            <button on:click=move |_| {
                show.update(|v| *v = !*v);
            }>"Toggle children"</button>
            <div>{move || format!("Children shown: {}", show.get())}</div>
            <Show when=show children/>
        </div>
    }
}

@zakstucke
Copy link
Contributor Author

zakstucke commented Apr 2, 2024

Hmmm, I haven't been using <Show /> because of the Fn() requirement and it's less user-friendliness, but I'm not 100% it would be incompatible with my usage.

I'll do some refactoring (the place I found this bug has 4 chained FnOnce's I need to change) and see if it works for me, would a solution whilst maintaining FnOnce() be really complicated?

@gbj
Copy link
Collaborator

gbj commented Apr 2, 2024

It's not super complicated, but complicated enough.

Basically by lazily constructing the children inside a reactive move || closure, you have made the effects created to update the dynamic nodes inside the children belong to that reactive block. Whenever that parent block re-runs, whatever effects are its children will be canceled, and recreated. But your code is specifically written not to re-create them. (If this paragraph makes no sense but you'd like to understand it, here's the chapter on the reactive ownership model.)

I guess there's a simple enough solution: Just take your example Child code above and manually set the owner to the owner of Child, not the eventual lazy owner:

#[component]
pub fn Child(children: Children) -> impl IntoView {
    let show = create_rw_signal(false);
    let owner = Owner::current().unwrap();
    let children = store_value(once_cell::unsync::Lazy::new(move || {
        with_owner(owner, children)
    }));
    view! {

@zakstucke
Copy link
Contributor Author

zakstucke commented Apr 2, 2024

You're the best! Makes perfect sense and seems to work as described. Fyi, I checked and <Show /> would definitely have worked, but surrounding logic needs altering to conform to Fn(), which I'd rather not have to do!

My StoredValueLazy is modified with your with_owner as so:

/// Like [`leptos::store_value`], but will only call the getter if needed.
pub fn store_value_lazy<T>(value_getter: impl FnOnce() -> T + 'static) -> StoredValueLazy<T> {
    StoredValueLazy::new(value_getter)
}

#[derive(Debug)]
pub struct StoredValueLazy<T: 'static> {
    value: StoredValue<once_cell::unsync::Lazy<T, Box<dyn FnOnce() -> T>>>,
}

impl<T> Clone for StoredValueLazy<T> {
    fn clone(&self) -> Self {
        *self
    }
}

impl<T> Copy for StoredValueLazy<T> {}

impl<T> StoredValueLazy<T> {
    pub fn new(value_getter: impl FnOnce() -> T + 'static) -> Self {
        let owner = Owner::current().unwrap();
        Self {
            value: store_value(once_cell::unsync::Lazy::new(Box::new(move || {
                with_owner(owner, value_getter)
            }))),
        }
    }

    #[track_caller]
    pub fn with_value<R>(&self, cb: impl FnOnce(&T) -> R) -> R {
        self.value.with_value(|value| cb(value))
    }
}

impl<T: Clone> StoredValueLazy<T> {
    #[track_caller]
    pub fn get_value(&self) -> T {
        self.with_value(|v| v.clone())
    }
}

Would it be worth PR'ing in and modifying Show to only require FnOnce for convenience?

@gbj
Copy link
Collaborator

gbj commented Apr 3, 2024

No, Show intentionally runs its children in a child owner, because this means any on_cleanup functions will run and objects created will be dropped. Your version allows FnOnce but won't run cleanups or drop the view when the condition switches from true to false. Either one of these is fine, but applying these changes to Show to allow FnOnce would change the semantics of Show.

I'll close this one if you're happy with the solution!

@gbj gbj closed this as completed Apr 3, 2024
@zakstucke
Copy link
Contributor Author

Got it, thanks for your help!

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

2 participants