cushy icon indicating copy to clipboard operation
cushy copied to clipboard

Why does `for_each` not run on initial value?

Open bluenote10 opened this issue 1 year ago • 4 comments

I was experimenting with reactivity a little bit and noticed something that feels a bit inconsistent: Apparently, with_for_each does not to run on the first/initial value, only on subsequent/updated values. My expectation from other reactive frameworks was that an "effect" would run on all values, including the initial value. This also creates an awkward inconsistency with map, which does run on the initial value as well (well, it has to :wink:).

I'm also a bit confused about with_for_each vs for_each: I vaguely remember seeing plain usages of for_each in older examples, i.e., usages that didn't explicitly hold on to the returned handle. But it looks like it is no longer possible to use just for_each without doing something (what exactly?) with the handle.

The following example summarizes my observations:

use cushy::figures::units::Lp;
use cushy::value::{Dynamic, IntoReader, Source};
use cushy::widget::MakeWidget;
use cushy::widgets::slider::Slidable;
use cushy::Run;

fn main() -> cushy::Result {
    let value = Dynamic::new(50_i32);

    // This doesn't run on the initial value, but subsequent values.
    let value = value.with_for_each(|value| {
        println!("value: {value} [with_for_each]");
    });

    // This doesn't run at all, probably due to dropped handle.
    let _ = value.for_each(|value| {
        println!("value: {value} [for_each, dropped handle]");
    });

    // Interestingly, a "dropped" map runs on the initial value, but not
    // on subsequent values.
    value.map_each(|value| {
        println!("value: {value} [map_each, unused]");
    });

    // This runs on all (initial and subsequent) values.
    let dummy = value.map_each(|value| {
        println!("value: {value} [map_each, used]");
        *value
    });

    "value: "
        .and(value.clone().into_label())
        .and(dummy.clone().into_label())
        .into_columns()
        .centered()
        .and(
            value
                .slider_between(0, 100)
                .width(Lp::points(400)..Lp::points(800)),
        )
        .into_rows()
        .contain()
        .centered()
        .run()
}

My expectation would have been:

  • a dropped for_each and dropped map would both run never.
  • a used for_each and used map would both run on all values, including the first.

Is there any deeper reason why that is not the case?

bluenote10 avatar Jun 05 '24 19:06 bluenote10

Thank you for trying out Cushy! In short, I've tried to make Cushy as predictable as possible, but clearly the fact that you've been confused by its behavior is an indication I may not be there yet and/or I need better documentation.

To answer the question that is the title of this issue, the reason it doesn't do it is that it doesn't need to. With map_each, the function returns a Dynamic<T> which must already contain the result of invoking the mapping function once. for_each has no such requirement, so I never wrote it to invoke with the initial value.

The reason I like this behavior is that it allows you to make your callback stateful and be able to observe all future values without having to write special logic to ignore the first value. If for_each called its callback with the initial value, such a callback would need to look something like this:

dynamic.for_each({
  let mut first_value = false;
  move |value| {
    if !first_value {
      first_value = true;
      return;
    }

    ....
  }
});

With the current setup, to have your closure invoked with the first value, the code can be written to first store the closure in a variable, use map_ref, then map_each:

    let d = Dynamic::new(0_u8);
    let mut all_values = Vec::new();
    let mut callback = move |value: &u8| {
        all_values.push(*value);
    };
    d.map_ref(&mut callback);
    d.for_each(callback);

I'm also a bit confused about with_for_each vs for_each

The difference is who owns the callback handle. with_for_each is meant to be used in a builder-style pattern like:

let my_dynamic = Dynamic::new(0_u8).with_for_each(...);

The for_each function is designed to be standalone and gives full control over the lifecycle of the attached callback by returning the callback handle.

Regarding your expectations of the example, mappings do not give access to the underlying CallbackHandle. If you wish to have full control over breaking a callback from executing, you must create the linkage directly with for_each as you've seen. The callback created with map_each uses a weak reference to the dynamic being observed. The callback will continue being invoked as long as a strong reference to the underlying dynamic still exists.

All of what I've written are the reasons why or explanations how it's currently designed, but I fully understand how my decisions can feel arbitrary. I'll definitely take your feedback into account and try to improve things in the future -- I also welcome any further feedback or questions! Thank you again!

Update with additional thought: Maybe renaming for_each to for_each_subsequent and adding a new for_each that invokes with every value would be a good solution.

ecton avatar Jun 06 '24 14:06 ecton

Update with additional thought: Maybe renaming for_each to for_each_subsequent and adding a new for_each that invokes with every value would be a good solution.

Yes, that would be the simplest solution.

Out of curiosity:

The reason I like this behavior is that it allows you to make your callback stateful and be able to observe all future values without having to write special logic to ignore the first value.

Interesting, so it comes down to what's is the more common use: Observe all values, or all but the first?

From my experience with React/Solid/Leptos, the corresponding mechanisms (useEffect, createEffect, ...) typically always run on the initial value as well. I'm not really familiar with the theoretical foundations in functional reactive programming, but the similarity suggested to me that this is general concept.

My use cases typically always matched these semantics. The standard use case in webapps is probably fetching data (HTTP requests), which should normally be done also for the initial state. If skipping a side effect is needed (be it the initial one, or any subsequent), it is often rather expressed in the value/type, e.g. by using null. Admittedly, making a slider Option<u8> for that purpose would be awkward.

What would be examples where one doesn't want to run the initial side effect? My first use case where I needed the opposite was e.g. this.

The for_each function is designed to be standalone and gives full control over the lifecycle of the attached callback by returning the callback handle.

I forgot that there is actually a difference between let _ = value.for_each(...) and let _handle = value.for_each(...).

And looking at the examples again, I've noticed that the reason why you never have to use a dummy handle on the left-hand-side is because you're using value.for_each(...).persist(). That's the pattern I was missing :+1:

bluenote10 avatar Jun 06 '24 22:06 bluenote10

Interesting, so it comes down to what's is the more common use: Observe all values, or all but the first?

Probably not very common. In fact, the lack of emitting the first value can lead to confusion as you point out. I think that's what made me like the idea I came up with after writing my comment initially.

What would be examples where one doesn't want to run the initial side effect?

I personally don't have good examples because I learned a long time ago that I prefer to write idempotent chunks of code as often as possible. In the past I would usually cite examples where you already have a current state and the observing callback should only ever observe updates from the initial state, not the initial state because it was already initialized using the initial state. This could be a callback that is designed to track changes that must be initialized with an initial state first. A properly idempotent callback it doesn't matter if it fires an extra time, but if it's not designed that way, the difference matters.

To me, Cushy shouldn't assume people only write idempotent code, so I want to offer both options.

That's the pattern I was missing

This is good to know! Maybe the must_use hint for CallbackHandle can be improved to hint at persisting it.

ecton avatar Jun 07 '24 14:06 ecton

Turns out the DebugWindow relies on this in a subtle way :laughing: I did the suggested change and even noted I didn't add a for_each_subesquent to the ForEach. The problem arises not because of needing to track first state, it's due to local thread lock management. In this particular case, the callback needed to lock another dynamic to perform actions on it, but that dynamic was already locked as part of the setup code being executed. By causing for_each to be invoked with the initial value, it caused a deadlock error to be raised.

To work around it without having for_each_subsequent, I would have to do extra lock management which would increase the code complexity. So I'm just going to add ForEach::for_each_subsequentand use it in the DebugWindow implementation.

ecton avatar Jun 07 '24 16:06 ecton

I guess after 7bd13e2a0a5bff238c45200530db5abbb4ecc5a5 this issue should be fully resolved, right? Thanks for the fix!

bluenote10 avatar Sep 22 '24 09:09 bluenote10

There's still a little more macro work to make ForEachCloned::for_each_subsequent_cloned, there was something tricky about the macro that implements the trait that I needed to revisit.

ecton avatar Sep 22 '24 13:09 ecton

Nevermind, apparently this was completely implemented at some point. While the previously mentioned method does not exist, it doesn't exist on Source either, so it's not missing functionality from what I was doing at the time.

Thanks for noticing and closing!

ecton avatar Sep 22 '24 15:09 ecton