Hydration fix

[zakstucke]

This might fix #3283 by superseding #3278

Give it a good look, i don't really know what I'm doing. 😅

This seems to solve my hydration bug (which I still can't simply repro), and still solves the issue you were fixing in #3278 .

The issue doesn't seem to apply to Suspend(async {..}) anyway, so this solution is targeted just to the sync versions.

[gbj]

Nice! Yes, I had tried this path initially for #3278, but was not notifying until after loading had finished anyway. This looks reasonable to me (and glad it solves both your hydration problem and the under-notifications). If the CI passes that's a good sign -- I will do some manual testing as well. My intuition here is that it's possible that this will over-notify inside Suspense, because it notifies the subscribers an extra time. But perhaps that doesn't manifest in a real issue.

I will probably look into it a little more tomorrow.

Thank so much for the PR, you rock.

[zakstucke]

Awesome thanks, just managed to make it even simpler too 😁

[gbj]

Here's an example test case that passes on current main and fails on this PR. It tests how many times an effect that depends on an AsyncDerived runs. In the case of the PR, every effect runs 2x per update due to the added set of notifications.

This is what causes the suspense_tests integration test to fail in CI: That test is a little subtle, but is designed to keep track of issues related to over-notifying subscribers of async derived tasks. This is especially important because async derived things are often relatively expensive (e.g., hitting a server API twice rather than once is more expensive than updating a DOM node twice rather than once)

(You can add this to reactive_graph/tests/async_derived.rs and run cargo test --features=effects --test=async_derived)

(Edit to add: So I think my goal here would be to get a good MRE for the hydration issue so that we can figure out how to fix it without the over-notification happening, if possible. But I'm glad you were at least able to unblock yourself and possibly others with the PR in the meantime.)

#[tokio::test]
async fn only_notifies_once_per_run() {
    _ = Executor::init_tokio();
    let owner = Owner::new();
    owner.set();

    let signal = RwSignal::new(0);
    let derived1 = AsyncDerived::new(move || async move {
        Executor::tick().await;
        signal.get() + 1
    });
    let derived2 = AsyncDerived::new(move || async move {
        let value = derived1.await;
        value * 2
    });

    let effect_runs = StoredValue::new(0);
    Effect::new_isomorphic(move |_| {
        *effect_runs.write_value() += 1;
        println!("{:?}", derived2.get());
    });

    Executor::tick().await;
    assert_eq!(derived2.await, 2);
    assert_eq!(effect_runs.get_value(), 1);

    signal.set(2);
    Executor::tick().await;
    assert_eq!(derived2.await, 6);
    assert_eq!(effect_runs.get_value(), 2);

    signal.set(3);
    Executor::tick().await;
    assert_eq!(derived2.await, 8);
    assert_eq!(effect_runs.get_value(), 3);
}

[zakstucke]

Closing, see repro in the issue, I messed around with the notifications in here a bit today but I think all throwaway.