Pauan / rust-signals

Zero-cost functional reactive Signals for Rust
MIT License
675 stars 37 forks source link

Can Signals be Cloned? #42

Closed SohumB closed 3 years ago

SohumB commented 3 years ago

I've been wondering how you would use this library to solve a particular design.

struct Icon {
  icon: Mutable<String>
}

impl Icon {
  pub fn css(&self): Signal<Item=String> {
    self.icon.signal().map(|icon| format!("mdi-{}", icon))
  }
}

This seems like the natural way to design data and derived values, with the current API, and it makes sense to me.

However, this does mean that you can't really do certain classes of computation on the Signal objects independently of their origin. If I hand you a Signal, you can only consume it once and in one way, and you can't make two separate Signals — two separate pieces of derived data — out of it. You need to be handed the Fn() -> Signal at minimum to do that.

Furthermore, this also seems to mean that if you have A → B → {C, D}, where B is derived data, but the computation to calculate it is expensive, then the computation isn't shared in any way between the two pathways.

Obviously not all Signals can be cloned, and I'll be the first to admit that I do not understand the nuances behind the design of this crate, but it seems to me that a lot of these issues could be bypassed by just adding #[derive(Clone)] to the various Signal structs. Is that correct?

Or maybe I'm missing something more obvious, and this is what ReadonlyMutable is for, and all I'm really asking for is a to_readonly method, something like

trait SignalExt {
  fn to_readonly(self) -> ReadOnlyMutable<Item> {
    let mut cell = Mutable::new();
    spawn_task(|| self.for_each(|v| cell.set(v)));
    cell.read_only()
  }
}

But that seems somewhat heavyweight, right?

I would greatly appreciate your thoughts. Thank you for your time and this excellent crate!

SohumB commented 3 years ago

...and I had the thought to look through the API again, and this time I actually clicked on Broadcaster, and that's it, that's exactly the answer to my question. My apologies!

Pauan commented 3 years ago

This seems like the natural way to design data and derived values, with the current API, and it makes sense to me.

I assume you meant to write impl Signal, but yes it is a good design that I myself use frequently.

You need to be handed the Fn() -> Signal at minimum to do that.

Yes, accepting an Fn() -> Signal or FnMut() -> Signal is a perfectly good design pattern.

Furthermore, this also seems to mean that if you have A → B → {C, D}, where B is derived data, but the computation to calculate it is expensive, then the computation isn't shared in any way between the two pathways.

That is true, though in practice the computations are not expensive. In the rare case where the computation is expensive, Mutable or Broadcaster is the correct approach (as you found).

it seems to me that a lot of these issues could be bypassed by just adding #[derive(Clone)] to the various Signal structs. Is that correct?

Unfortunately that's not the case (otherwise I would have already made all the Signals impl Clone).

When you clone a Signal, it does a deep clone, which means that the new Signal is completely independent from the old Signal. It is exactly the same as using Fn() -> Signal, you are creating an entirely new Signal, which means the computation is calculated twice.

It also means that any captured variables in closures will also be cloned. This is very unintuitive behavior, which is why I chose to not impl Clone and instead created Broadcaster (which does share the computation, as you would intuitively expect).

Or maybe I'm missing something more obvious, and this is what ReadonlyMutable is for, and all I'm really asking for is a to_readonly method, something like

Yes, using Mutable is my preferred approach when I want to share computation. In the Dominator TodoMVC example, I need to recalculate the route whenever the URL changes, and this is an expensive operation.

  1. First I create a Mutable to store the route.
  2. Then I use for_each to run a closure whenever the URL changes.
  3. Inside of the closure I set the Mutable.

Internally the .future method uses spawn_local, so this is very similar to your to_readonly method.

There are three reasons why I prefer doing this instead of using Broadcaster:

  1. Using Mutable is slightly faster than Broadcaster.
  2. I can access the current value using the get() or lock_ref() methods.
  3. Because it uses set_neq it will only update the Mutable if the value actually changed. This can also be done using Broadcaster, but you have to use the dedupe method, which is slightly slower.

However, if your library is accepting a Signal as input, then using Broadcaster is likely the correct choice, despite those three downsides.

But that seems somewhat heavyweight, right?

Yes, it is heavyweight, however... Mutable is about the same performance as Broadcaster.

Sharing computation is an inherently expensive thing to do, which is why Signals requires you to opt-in to it (using Broadcaster).

Other FRP libraries share everything by default, but this makes them a lot slower than my Signals library.

SohumB commented 3 years ago

Thank you for your insight, and the example, that helps a lot!

Let me see if I can summarise:

That's a reasonable set of tradeoffs to be aware of, thank you!

Pauan commented 3 years ago

Yes, that sounds correct.

Also note that Broadcaster naturally supports cancellation (just like all Signals). However, for_each will keep running until the input Signal is ended. So you might want to use abortable in order to support cancellation:

let (future, aborter) = abortable(signal.for_each(move |value| {
    // ...
    async {}
}));

spawn_task(future);

// You can now use `aborter.abort()` to cancel the `for_each` Future

The Dominator .future method uses this technique in order to automatically cleanup Futures when the DOM node is removed.