Do not assume Signal is valid for entire duration of effect and make effect triggers deterministic

packages/sycamore-macro/src/template/mod.rs

@@ -86,7 +86,7 @@ impl ToTokens for HtmlTree {
                 },
                 Text::Splice(_, _) => quote! {
                     ::sycamore::template::Template::new_lazy(move ||
-                        ::sycamore::render::IntoTemplate::create(&#text)
+                        ::sycamore::template::IntoTemplate::create(&#text)
                     )
                 },
             },

packages/sycamore/Cargo.toml

@@ -16,8 +16,8 @@ version = "0.5.0-beta.1"
 [dependencies]
 chrono = {version = "0.4", features = ["wasmbind"]}
 html-escape = {version = "0.2.7", optional = true}
+indexmap = "1.7"
 js-sys = {version = "0.3", optional = true}
-ref-cast = "1.0"
 serde = {version = "1.0", optional = true}
 sycamore-macro = {path = "../sycamore-macro", version = "=0.5.0-beta.1"}
 wasm-bindgen = {version = "0.2", optional = true, features = ["enable-interning"]}

packages/sycamore/src/rx/effect.rs

@@ -122,29 +122,25 @@ impl PartialEq for Callback {
 }
 impl Eq for Callback {}
 
-/// A [`Weak`] backlink to a [`Signal`] for any type `T`.
+/// A strong backlink to a [`Signal`] for any type `T`.
 #[derive(Clone)]
-pub(super) struct Dependency(pub(super) Weak<dyn AnySignalInner>);
+pub(super) struct Dependency(pub(super) Rc<dyn AnySignalInner>);
 
 impl Dependency {
-    #[track_caller]
     fn signal(&self) -> Rc<dyn AnySignalInner> {
-        self.0.upgrade().expect("backlink should always be valid")
+        Rc::clone(&self.0)
     }
 }
 
 impl Hash for Dependency {
     fn hash<H: Hasher>(&self, state: &mut H) {
-        Rc::as_ptr(&self.signal()).hash(state);
+        Rc::as_ptr(&self.0).hash(state);
     }
 }
 
 impl PartialEq for Dependency {
     fn eq(&self, other: &Self) -> bool {
-        ptr::eq::<()>(
-            Rc::as_ptr(&self.signal()).cast(),
-            Rc::as_ptr(&other.signal()).cast(),
-        )
+        ptr::eq::<()>(Rc::as_ptr(&self.0).cast(), Rc::as_ptr(&other.0).cast())
     }
 }
 impl Eq for Dependency {}
@@ -615,6 +611,75 @@ mod tests {
         assert_eq!(*inner_cleanup_counter.get(), 1);
     }
 
+    #[test]
+    fn create_nested_effect_from_outside() {
+        let trigger = Signal::new(());
+        let outer_counter = Signal::new(0);
+        let inner_counter = Signal::new(0);
+
+        let inner_effect: Signal<Option<Box<dyn Fn()>>> = Signal::new(None);
+
+        create_effect(
+            cloned!((trigger, outer_counter, inner_counter, inner_effect) => move || {
+                trigger.get(); // subscribe to trigger
+                outer_counter.set(*outer_counter.get_untracked() + 1);
+
+                if inner_effect.get_untracked().is_none() {
+                    inner_effect.set(Some(Box::new(cloned!((inner_counter) => move || {
+                        inner_counter.set(*inner_counter.get_untracked() + 1);
+                    }))));
+                }
+            }),
+        );
+
+        create_effect(move || (*inner_effect.get()).as_ref().unwrap()());
+
+        assert_eq!(*outer_counter.get(), 1);
+        assert_eq!(*inner_counter.get(), 1);
+
+        trigger.set(());
+        assert_eq!(*outer_counter.get(), 2);
+        assert_eq!(*inner_counter.get(), 1);
+    }
+
+    #[test]
+    fn outer_effects_rerun_first() {
+        let trigger = Signal::new(());
+
+        let outer_counter = Signal::new(0);
+        let inner_counter = Signal::new(0);
+
+        create_effect(cloned!((trigger, outer_counter, inner_counter) => move || {
+            trigger.get(); // subscribe to trigger
+            outer_counter.set(*outer_counter.get_untracked() + 1);
+
+            create_effect(cloned!((trigger, inner_counter) => move || {
+                trigger.get(); // subscribe to trigger
+                inner_counter.set(*inner_counter.get_untracked() + 1);
+            }));
+        }));
+
+        assert_eq!(*outer_counter.get(), 1);
+        assert_eq!(*inner_counter.get(), 1);
+
+        trigger.set(());
+
+        assert_eq!(*outer_counter.get(), 2);
+        assert_eq!(*inner_counter.get(), 2);
+    }
+
+    #[test]
+    fn drop_signal_inside_effect() {
+        let state = RefCell::new(Some(Signal::new(0)));
+
+        create_effect(move || {
+            if let Some(state) = state.take() {
+                state.get(); // subscribe to state
+                drop(state);
+            }
+        });
+    }
+
     #[test]
     fn destroy_effects_on_scope_drop() {
         let counter = Signal::new(0);

packages/sycamore/src/rx/signal.rs

@@ -1,10 +1,11 @@
 use std::cell::RefCell;
-use std::collections::HashSet;
 use std::fmt;
 use std::hash::{Hash, Hasher};
 use std::ops::Deref;
 use std::rc::Rc;
 
+use indexmap::IndexSet;
+
 use super::*;
 
 /// A readonly [`Signal`].
@@ -21,7 +22,7 @@ impl<T: 'static> StateHandle<T> {
         // If running inside a destructor, do nothing.
         let _ = CONTEXTS.try_with(|contexts| {
             if let Some(last_context) = contexts.borrow().last() {
-                let signal = Rc::downgrade(&self.0);
+                let signal = Rc::clone(&self.0);
 
                 last_context
                     .upgrade()
@@ -168,7 +169,8 @@ impl<T: 'static> Signal<T> {
         // Clone subscribers to prevent modifying list when calling callbacks.
         let subscribers = self.handle.0.borrow().subscribers.clone();
 
-        for subscriber in subscribers {
+        // Reverse order of subscribers to trigger outer effects before inner effects.
+        for subscriber in subscribers.iter().rev() {
             // subscriber might have already been destroyed in the case of nested effects
             if let Some(callback) = subscriber.try_callback() {
                 callback();
@@ -237,14 +239,14 @@ impl<'de, T: serde::Deserialize<'de>> serde::Deserialize<'de> for Signal<T> {
 
 pub(super) struct SignalInner<T> {
     inner: Rc<T>,
-    subscribers: HashSet<Callback>,
+    subscribers: IndexSet<Callback>,
 }
 
 impl<T> SignalInner<T> {
     fn new(value: T) -> Self {
         Self {
             inner: Rc::new(value),
-            subscribers: HashSet::new(),
+            subscribers: IndexSet::new(),
         }
     }
 

packages/sycamore/src/template.rs

@@ -31,7 +31,7 @@ impl<G: GenericNode> Template<G> {
         }
     }
 
-    /// Create a new [`Template`] from a [`FnOnce`].
+    /// Create a new [`Template`] from a [`FnMut`].
     pub fn new_lazy(f: impl FnMut() -> Template<G> + 'static) -> Self {
         Self {
             inner: TemplateType::Lazy(Rc::new(RefCell::new(f))),