From cf7deaaea386d4c90239612ef8655c41b0e7fef4 Mon Sep 17 00:00:00 2001
From: Greg Johnston <greg.johnston@gmail.com>
Date: Sat, 11 Feb 2023 14:12:59 -0500
Subject: [PATCH] fix: proper disposal of nested route scopes (#499)

---
 examples/router/src/lib.rs      | 39 ++++++++++++++-
 leptos_reactive/src/scope.rs    |  1 -
 router/src/components/outlet.rs |  8 +--
 router/src/components/routes.rs | 88 +++++++++++++--------------------
 4 files changed, 77 insertions(+), 59 deletions(-)

diff --git a/examples/router/src/lib.rs b/examples/router/src/lib.rs
index 48bc101..54b73ca 100644
--- a/examples/router/src/lib.rs
+++ b/examples/router/src/lib.rs
@@ -1,14 +1,18 @@
 mod api;
-
+use crate::api::*;
 use leptos::*;
 use leptos_router::*;
 
-use crate::api::{get_contact, get_contacts};
+#[derive(Copy, Clone, Debug, PartialEq, Eq)]
+struct ExampleContext(i32);
 
 #[component]
 pub fn RouterExample(cx: Scope) -> impl IntoView {
     log::debug!("rendering <RouterExample/>");
 
+    // contexts are passed down through the route tree
+    provide_context(cx, ExampleContext(0));
+
     view! { cx,
         <Router>
             <nav>
@@ -59,6 +63,13 @@ pub fn RouterExample(cx: Scope) -> impl IntoView {
 pub fn ContactList(cx: Scope) -> impl IntoView {
     log::debug!("rendering <ContactList/>");
 
+    // contexts are passed down through the route tree
+    provide_context(cx, ExampleContext(42));
+
+    on_cleanup(cx, || {
+        log!("cleaning up <ContactList/>");
+    });
+
     let location = use_location(cx);
     let contacts = create_resource(cx, move || location.search.get(), get_contacts);
     let contacts = move || {
@@ -95,6 +106,15 @@ pub struct ContactParams {
 pub fn Contact(cx: Scope) -> impl IntoView {
     log::debug!("rendering <Contact/>");
 
+    log::debug!(
+        "ExampleContext should be Some(42). It is {:?}",
+        use_context::<ExampleContext>(cx)
+    );
+
+    on_cleanup(cx, || {
+        log!("cleaning up <Contact/>");
+    });
+
     let params = use_params::<ContactParams>(cx);
     let contact = create_resource(
         cx,
@@ -136,6 +156,16 @@ pub fn Contact(cx: Scope) -> impl IntoView {
 #[component]
 pub fn About(cx: Scope) -> impl IntoView {
     log::debug!("rendering <About/>");
+
+    on_cleanup(cx, || {
+        log!("cleaning up <About/>");
+    });
+
+    log::debug!(
+        "ExampleContext should be Some(0). It is {:?}",
+        use_context::<ExampleContext>(cx)
+    );
+
     // use_navigate allows you to navigate programmatically by calling a function
     let navigate = use_navigate(cx);
 
@@ -157,6 +187,11 @@ pub fn About(cx: Scope) -> impl IntoView {
 #[component]
 pub fn Settings(cx: Scope) -> impl IntoView {
     log::debug!("rendering <Settings/>");
+
+    on_cleanup(cx, || {
+        log!("cleaning up <Settings/>");
+    });
+
     view! { cx,
         <>
             <h1>"Settings"</h1>
diff --git a/leptos_reactive/src/scope.rs b/leptos_reactive/src/scope.rs
index f1ed456..f48ce49 100644
--- a/leptos_reactive/src/scope.rs
+++ b/leptos_reactive/src/scope.rs
@@ -191,7 +191,6 @@ impl Scope {
                     .dispose();
                 }
             }
-
             // run cleanups
             if let Some(cleanups) = runtime.scope_cleanups.borrow_mut().remove(self.id) {
                 for cleanup in cleanups {
diff --git a/router/src/components/outlet.rs b/router/src/components/outlet.rs
index d48e633..8ac93ad 100644
--- a/router/src/components/outlet.rs
+++ b/router/src/components/outlet.rs
@@ -26,9 +26,11 @@ pub fn Outlet(cx: Scope) -> impl IntoView {
                 if let Some(prev_scope) = prev.map(|(_, scope)| scope) {
                     prev_scope.dispose();
                 }
-                is_showing.set(Some((child.id(), child.cx())));
-                provide_context(cx, child.clone());
-                set_outlet.set(Some(child.outlet(cx).into_view(cx)))
+                _ = cx.child_scope(|child_cx| {
+                    provide_context(child_cx, child.clone());
+                    set_outlet.set(Some(child.outlet(child_cx).into_view(child_cx)));
+                    is_showing.set(Some((child.id(), child_cx)));
+                });
             }
         }
     });
diff --git a/router/src/components/routes.rs b/router/src/components/routes.rs
index 1198869..9641092 100644
--- a/router/src/components/routes.rs
+++ b/router/src/components/routes.rs
@@ -59,9 +59,6 @@ pub fn Routes(
         move |_| get_route_matches(branches.clone(), router.pathname().get())
     });
 
-    // Rebuild the list of nested routes conservatively, and show the root route here
-    let disposers = RefCell::new(Vec::<ScopeDisposer>::new());
-
     // iterate over the new matches, reusing old routes when they are the same
     // and replacing them with new routes when they differ
     let next: Rc<RefCell<Vec<RouteContext>>> = Default::default();
@@ -69,7 +66,7 @@ pub fn Routes(
     let root_equal = Rc::new(Cell::new(true));
 
     let route_states: Memo<RouterState> = create_memo(cx, {
-        let root_equal = root_equal.clone();
+        let root_equal = Rc::clone(&root_equal);
         move |prev: Option<&RouterState>| {
             root_equal.set(true);
             next.borrow_mut().clear();
@@ -83,8 +80,6 @@ pub fn Routes(
                 .map(|prev_matches| next_matches.len() == prev_matches.len())
                 .unwrap_or(false);
 
-            let prev_cx = Rc::new(Cell::new(cx));
-
             for i in 0..next_matches.len() {
                 let next = next.clone();
                 let prev_match = prev_matches.and_then(|p| p.get(i));
@@ -111,62 +106,41 @@ pub fn Routes(
                             root_equal.set(false);
                         }
 
-                        let disposer = prev_cx.get().child_scope({
-                            let next = next.clone();
-                            let router = Rc::clone(&router.inner);
-                            let prev_cx = Rc::clone(&prev_cx);
-                            move |cx| {
-                                prev_cx.set(cx);
-                                let next = next.clone();
-                                let next_ctx = RouteContext::new(
-                                    cx,
-                                    &RouterContext { inner: router },
-                                    {
-                                        let next = next.clone();
-                                        move |cx| {
-                                            if let Some(route_states) =
-                                                use_context::<Memo<RouterState>>(cx)
-                                            {
-                                                route_states.with(|route_states| {
-                                                    let routes = route_states.routes.borrow();
-                                                    routes.get(i + 1).cloned()
-                                                })
-                                            } else {
-                                                next.borrow().get(i + 1).cloned()
-                                            }
-                                        }
-                                    },
-                                    move || matches.with(|m| m.get(i).cloned()),
-                                );
+                        let next = next.clone();
+                        let router = Rc::clone(&router.inner);
 
-                                if let Some(next_ctx) = next_ctx {
-                                    if next.borrow().len() > i + 1 {
-                                        next.borrow_mut()[i] = next_ctx;
+                        let next = next.clone();
+                        let next_ctx = RouteContext::new(
+                            cx,
+                            &RouterContext { inner: router },
+                            {
+                                let next = next.clone();
+                                move |cx| {
+                                    if let Some(route_states) = use_context::<Memo<RouterState>>(cx)
+                                    {
+                                        route_states.with(|route_states| {
+                                            let routes = route_states.routes.borrow();
+                                            routes.get(i + 1).cloned()
+                                        })
                                     } else {
-                                        next.borrow_mut().push(next_ctx);
+                                        next.borrow().get(i + 1).cloned()
                                     }
                                 }
-                            }
-                        });
+                            },
+                            move || matches.with(|m| m.get(i).cloned()),
+                        );
 
-                        if disposers.borrow().len() > i {
-                            let mut disposers = disposers.borrow_mut();
-                            let old_route_disposer = std::mem::replace(&mut disposers[i], disposer);
-                            old_route_disposer.dispose();
-                        } else {
-                            disposers.borrow_mut().push(disposer);
+                        if let Some(next_ctx) = next_ctx {
+                            if next.borrow().len() > i + 1 {
+                                next.borrow_mut()[i] = next_ctx;
+                            } else {
+                                next.borrow_mut().push(next_ctx);
+                            }
                         }
                     }
                 }
             }
 
-            if disposers.borrow().len() > next_matches.len() {
-                let surplus_disposers = disposers.borrow_mut().split_off(next_matches.len() + 1);
-                for disposer in surplus_disposers {
-                    disposer.dispose();
-                }
-            }
-
             if let Some(prev) = &prev {
                 if equal {
                     RouterState {
@@ -195,6 +169,7 @@ pub fn Routes(
 
     // show the root route
     let id = HydrationCtx::id();
+    let root_cx = RefCell::new(None);
     let root = create_memo(cx, move |prev| {
         provide_context(cx, route_states);
         route_states.with(|state| {
@@ -208,7 +183,14 @@ pub fn Routes(
                 }
 
                 if prev.is_none() || !root_equal.get() {
-                    root.as_ref().map(|route| route.outlet(cx).into_view(cx))
+                    let (root_view, _) = cx.run_child_scope(|cx| {
+                        let prev_cx = std::mem::replace(&mut *root_cx.borrow_mut(), Some(cx));
+                        if let Some(prev_cx) = prev_cx {
+                            prev_cx.dispose();
+                        }
+                        root.as_ref().map(|route| route.outlet(cx).into_view(cx))
+                    });
+                    root_view
                 } else {
                     prev.cloned().unwrap()
                 }