Skip to content

Commit

Permalink
Fix do not track nested reactivity in map_keyed/map_indexed (#742)
Browse files Browse the repository at this point in the history
* Add unit tests

* Implement Trackable for MaybeDyn

* Make sure that we only track list in map_keyed/map_indexed
  • Loading branch information
lukechu10 authored Oct 29, 2024
1 parent 7e58231 commit 217ee7c
Show file tree
Hide file tree
Showing 2 changed files with 36 additions and 4 deletions.
30 changes: 26 additions & 4 deletions packages/sycamore-reactive/src/iter.rs
Original file line number Diff line number Diff line change
Expand Up @@ -42,8 +42,9 @@ where
let mut disposers_tmp: Vec<Option<NodeHandle>> = Vec::new();

// Diff and update signal each time list is updated.
let _list = list.clone();
let mut update = move || {
let new_items = list.get_clone();
let new_items = _list.get_clone();
if new_items.is_empty() {
// Fast path for removing all items.
for dis in mem::take(&mut disposers) {
Expand Down Expand Up @@ -173,7 +174,7 @@ where
mapped.clone()
};
let scope = use_current_scope();
create_memo(move || scope.run_in(&mut update))
create_memo(on(list, move || scope.run_in(&mut update)))
}

/// Function that maps a `Vec` to another `Vec` via a map function.
Expand Down Expand Up @@ -206,8 +207,9 @@ where
let mut disposers: Vec<NodeHandle> = Vec::new();

// Diff and update signal each time list is updated.
let _list = list.clone();
let mut update = move || {
let new_items = list.get_clone();
let new_items = _list.get_clone();

if new_items.is_empty() {
// Fast path for removing all items.
Expand Down Expand Up @@ -263,7 +265,7 @@ where
mapped.clone()
};
let scope = use_current_scope();
create_memo(move || scope.run_in(&mut update))
create_memo(on(list, move || scope.run_in(&mut update)))
}

#[cfg(test)]
Expand Down Expand Up @@ -517,4 +519,24 @@ mod tests {
assert_eq!(counter.get(), 3);
});
}

/// Regression test for <https://github.com/sycamore-rs/sycamore/issues/739>
#[test]
fn issue_739_keyed_should_not_track_nested_signals() {
let _ = create_root(|| {
let items = create_signal(vec![create_signal(1), create_signal(2)]);
map_keyed(items, |x| on_cleanup(move || x.dispose()), |x| x.get());
items.set(items.get_clone()[1..].to_vec());
});
}

/// Regression test for <https://github.com/sycamore-rs/sycamore/issues/739>
#[test]
fn issue_739_indexed_should_not_track_nested_signals() {
let _ = create_root(|| {
let items = create_signal(vec![create_signal(()), create_signal(())]);
map_indexed(items, |x| on_cleanup(move || x.dispose()));
items.set(items.get_clone()[1..].to_vec());
});
}
}
10 changes: 10 additions & 0 deletions packages/sycamore-reactive/src/utils.rs
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,16 @@ impl<T> Trackable for ReadSignal<T> {
}
}

impl<T: Into<Self>> Trackable for MaybeDyn<T> {
fn _track(&self) {
match self {
MaybeDyn::Static(_) => {}
MaybeDyn::Signal(signal) => signal.track(),
MaybeDyn::Derived(f) => f()._track(),
}
}
}

macro_rules! impl_trackable_deps_for_tuple {
($($T:tt),*) => {
paste::paste! {
Expand Down

0 comments on commit 217ee7c

Please sign in to comment.