Skip to content

Commit

Permalink
Do not assume Signal is valid for entire duration of effect and make …
Browse files Browse the repository at this point in the history
…effect triggers deterministic (#145)

* Add create_nested_effect_from_outside test

* Add outer effects rerun first test

* Fix docs typo

* Deterministic effect trigger

* Make Dependency take a strong backlink

* Fix typo in template! proc-macro
lukechu10 authored Jul 6, 2021

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
1 parent 9b5ef37 commit 7ac9f1a
Showing 5 changed files with 84 additions and 17 deletions.
2 changes: 1 addition & 1 deletion packages/sycamore-macro/src/template/mod.rs
Original file line number Diff line number Diff line change
@@ -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)
)
},
},
2 changes: 1 addition & 1 deletion packages/sycamore/Cargo.toml
Original file line number Diff line number Diff line change
@@ -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"]}
83 changes: 74 additions & 9 deletions packages/sycamore/src/rx/effect.rs
Original file line number Diff line number Diff line change
@@ -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);
12 changes: 7 additions & 5 deletions packages/sycamore/src/rx/signal.rs
Original file line number Diff line number Diff line change
@@ -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(),
}
}

2 changes: 1 addition & 1 deletion packages/sycamore/src/template.rs
Original file line number Diff line number Diff line change
@@ -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))),

0 comments on commit 7ac9f1a

Please sign in to comment.