Skip to content

Commit

Permalink
Fix memo dirty flag after unrelated writes (#2992)
Browse files Browse the repository at this point in the history
* Fix memo dirty flag after unrelated writes

* improve regression test

* Update packages/signals/tests/memo.rs

Co-authored-by: Matt Hunzinger <[email protected]>

* Update packages/signals/tests/memo.rs

Co-authored-by: Matt Hunzinger <[email protected]>

* Update packages/signals/tests/memo.rs

Co-authored-by: Matt Hunzinger <[email protected]>

---------

Co-authored-by: Matt Hunzinger <[email protected]>
  • Loading branch information
ealmloff and matthunz authored Oct 1, 2024
1 parent 5925f3c commit d1c84d9
Show file tree
Hide file tree
Showing 4 changed files with 186 additions and 153 deletions.
5 changes: 3 additions & 2 deletions packages/hooks/src/use_memo.rs
Original file line number Diff line number Diff line change
@@ -1,13 +1,14 @@
use crate::use_callback;
use dioxus_core::prelude::*;
use dioxus_signals::{Memo, Signal};
use dioxus_signals::Memo;

#[doc = include_str!("../docs/derived_state.md")]
#[doc = include_str!("../docs/rules_of_hooks.md")]
#[doc = include_str!("../docs/moving_state_around.md")]
#[track_caller]
pub fn use_memo<R: PartialEq>(mut f: impl FnMut() -> R + 'static) -> Memo<R> {
let callback = use_callback(move |_| f());
let caller = std::panic::Location::caller();
#[allow(clippy::redundant_closure)]
use_hook(|| Signal::memo(move || callback(())))
use_hook(|| Memo::new_with_location(move || callback(()), caller))
}
7 changes: 4 additions & 3 deletions packages/signals/src/memo.rs
Original file line number Diff line number Diff line change
Expand Up @@ -139,10 +139,11 @@ impl<T: 'static> Memo<T> {
drop(peak);
let mut copy = self.inner;
copy.set(new_value);
update_write
.dirty
.store(false, std::sync::atomic::Ordering::Relaxed);
}
// Always mark the memo as no longer dirty even if the value didn't change
update_write
.dirty
.store(false, std::sync::atomic::Ordering::Relaxed);
}

/// Get the scope that the signal was created in.
Expand Down
1 change: 1 addition & 0 deletions packages/signals/src/signal.rs
Original file line number Diff line number Diff line change
Expand Up @@ -557,6 +557,7 @@ struct SignalSubscriberDrop<T: 'static, S: Storage<SignalData<T>>> {
#[allow(clippy::no_effect)]
impl<T: 'static, S: Storage<SignalData<T>>> Drop for SignalSubscriberDrop<T, S> {
fn drop(&mut self) {
println!("dropping signal subscriber");
#[cfg(debug_assertions)]
{
tracing::trace!(
Expand Down
326 changes: 178 additions & 148 deletions packages/signals/tests/memo.rs
Original file line number Diff line number Diff line change
@@ -1,148 +1,178 @@
// TODO: fix #1935

// #![allow(unused, non_upper_case_globals, non_snake_case)]
// use dioxus_core::NoOpMutations;
// use std::collections::HashMap;
// use std::rc::Rc;

// use dioxus::html::p;
// use dioxus::prelude::*;
// use dioxus_core::ElementId;
// use dioxus_signals::*;
// use std::cell::RefCell;

// #[test]
// fn memos_rerun() {
// let _ = simple_logger::SimpleLogger::new().init();

// #[derive(Default)]
// struct RunCounter {
// component: usize,
// effect: usize,
// }

// let counter = Rc::new(RefCell::new(RunCounter::default()));
// let mut dom = VirtualDom::new_with_props(
// |counter: Rc<RefCell<RunCounter>>| {
// counter.borrow_mut().component += 1;

// let mut signal = use_signal(|| 0);
// let memo = use_memo({
// to_owned![counter];
// move || {
// counter.borrow_mut().effect += 1;
// println!("Signal: {:?}", signal);
// signal()
// }
// });
// assert_eq!(memo(), 0);
// signal += 1;
// assert_eq!(memo(), 1);

// rsx! {
// div {}
// }
// },
// counter.clone(),
// );

// dom.rebuild_in_place();

// let current_counter = counter.borrow();
// assert_eq!(current_counter.component, 1);
// assert_eq!(current_counter.effect, 2);
// }

// #[test]
// fn memos_prevents_component_rerun() {
// let _ = simple_logger::SimpleLogger::new().init();

// #[derive(Default)]
// struct RunCounter {
// component: usize,
// memo: usize,
// }

// let counter = Rc::new(RefCell::new(RunCounter::default()));
// let mut dom = VirtualDom::new_with_props(
// |props: Rc<RefCell<RunCounter>>| {
// let mut signal = use_signal(|| 0);

// if generation() == 1 {
// *signal.write() = 0;
// }
// if generation() == 2 {
// println!("Writing to signal");
// *signal.write() = 1;
// }

// rsx! {
// Child {
// signal: signal,
// counter: props.clone(),
// }
// }
// },
// counter.clone(),
// );

// #[derive(Default, Props, Clone)]
// struct ChildProps {
// signal: Signal<usize>,
// counter: Rc<RefCell<RunCounter>>,
// }

// impl PartialEq for ChildProps {
// fn eq(&self, other: &Self) -> bool {
// self.signal == other.signal
// }
// }

// fn Child(props: ChildProps) -> Element {
// let counter = &props.counter;
// let signal = props.signal;
// counter.borrow_mut().component += 1;

// let memo = use_memo({
// to_owned![counter];
// move || {
// counter.borrow_mut().memo += 1;
// println!("Signal: {:?}", signal);
// signal()
// }
// });
// match generation() {
// 0 => {
// assert_eq!(memo(), 0);
// }
// 1 => {
// assert_eq!(memo(), 1);
// }
// _ => panic!("Unexpected generation"),
// }

// rsx! {
// div {}
// }
// }

// dom.rebuild_in_place();
// dom.mark_dirty(ScopeId::ROOT);
// dom.render_immediate(&mut NoOpMutations);

// {
// let current_counter = counter.borrow();
// assert_eq!(current_counter.component, 1);
// assert_eq!(current_counter.memo, 2);
// }

// dom.mark_dirty(ScopeId::ROOT);
// dom.render_immediate(&mut NoOpMutations);
// dom.render_immediate(&mut NoOpMutations);

// {
// let current_counter = counter.borrow();
// assert_eq!(current_counter.component, 2);
// assert_eq!(current_counter.memo, 3);
// }
// }
#![allow(unused, non_upper_case_globals, non_snake_case)]
use dioxus::html::p;
use dioxus::prelude::*;
use dioxus_core::ElementId;
use dioxus_core::NoOpMutations;
use dioxus_signals::*;
use std::cell::RefCell;
use std::collections::HashMap;
use std::rc::Rc;
use std::sync::atomic::{AtomicBool, Ordering};

#[test]
fn memos_rerun() {
let _ = simple_logger::SimpleLogger::new().init();

#[derive(Default)]
struct RunCounter {
component: usize,
effect: usize,
}

let counter = Rc::new(RefCell::new(RunCounter::default()));
let mut dom = VirtualDom::new_with_props(
|counter: Rc<RefCell<RunCounter>>| {
counter.borrow_mut().component += 1;

let mut signal = use_signal(|| 0);
let memo = use_memo({
to_owned![counter];
move || {
counter.borrow_mut().effect += 1;
println!("Signal: {:?}", signal);
signal()
}
});
assert_eq!(memo(), 0);
signal += 1;
assert_eq!(memo(), 1);

rsx! {
div {}
}
},
counter.clone(),
);

dom.rebuild_in_place();

let current_counter = counter.borrow();
assert_eq!(current_counter.component, 1);
assert_eq!(current_counter.effect, 2);
}

#[test]
fn memos_prevents_component_rerun() {
let _ = simple_logger::SimpleLogger::new().init();

#[derive(Default)]
struct RunCounter {
component: usize,
memo: usize,
}

let counter = Rc::new(RefCell::new(RunCounter::default()));
let mut dom = VirtualDom::new_with_props(
|props: Rc<RefCell<RunCounter>>| {
let mut signal = use_signal(|| 0);

if generation() == 1 {
*signal.write() = 0;
}
if generation() == 2 {
println!("Writing to signal");
*signal.write() = 1;
}

rsx! {
Child {
signal: signal,
counter: props.clone(),
}
}
},
counter.clone(),
);

#[derive(Default, Props, Clone)]
struct ChildProps {
signal: Signal<usize>,
counter: Rc<RefCell<RunCounter>>,
}

impl PartialEq for ChildProps {
fn eq(&self, other: &Self) -> bool {
self.signal == other.signal
}
}

fn Child(props: ChildProps) -> Element {
let counter = &props.counter;
let signal = props.signal;
counter.borrow_mut().component += 1;

let memo = use_memo({
to_owned![counter];
move || {
counter.borrow_mut().memo += 1;
println!("Signal: {:?}", signal);
signal()
}
});
match generation() {
0 => {
assert_eq!(memo(), 0);
}
1 => {
assert_eq!(memo(), 1);
}
_ => panic!("Unexpected generation"),
}

rsx! {
div {}
}
}

dom.rebuild_in_place();
dom.mark_dirty(ScopeId::APP);
dom.render_immediate(&mut NoOpMutations);

{
let current_counter = counter.borrow();
assert_eq!(current_counter.component, 1);
assert_eq!(current_counter.memo, 2);
}

dom.mark_dirty(ScopeId::APP);
dom.render_immediate(&mut NoOpMutations);
dom.render_immediate(&mut NoOpMutations);

{
let current_counter = counter.borrow();
assert_eq!(current_counter.component, 2);
assert_eq!(current_counter.memo, 3);
}
}

// Regression test for https://github.com/DioxusLabs/dioxus/issues/2990
#[test]
fn memos_sync_rerun_after_unrelated_write() {
static PASSED: AtomicBool = AtomicBool::new(false);
let mut dom = VirtualDom::new(|| {
let mut signal = use_signal(|| 0);
let memo = use_memo(move || dbg!(signal() < 2));
if generation() == 0 {
assert!(memo());
signal += 1;
} else {
// It should be fine to hold the write and read the memo at the same time
let mut write = signal.write();
println!("Memo: {:?}", memo());
assert!(memo());
*write = 2;
drop(write);
assert!(!memo());
PASSED.store(true, std::sync::atomic::Ordering::SeqCst);
}

rsx! {
div {}
}
});

dom.rebuild_in_place();
dom.mark_dirty(ScopeId::APP);
dom.render_immediate(&mut NoOpMutations);
assert!(PASSED.load(Ordering::SeqCst));
}

0 comments on commit d1c84d9

Please sign in to comment.