Pauan / rust-dominator

Zero-cost ultra-high-performance declarative DOM library using FRP signals for Rust!
MIT License
967 stars 62 forks source link

Multi-Consumer Signals #3

Closed davidhewitt closed 6 years ago

davidhewitt commented 6 years ago

While I was playing around with creating a store struct for state I noticed that only one signal reciever gets the update (I hadn't really grokked this before). E.g.

let (sender_text, receiver_text) = signal::unsync::mutable(String::from("Hello  "));

html!("div", {
    children(&mut [
        text("First: "),
        text(receiver_text.clone().dynamic()),
        text(", Second: "),
        text(receiver_text.clone().dynamic()),
        html!("button", {
            children(&mut [text("Click Me!")]);
            event(move |_: ClickEvent| { sender_text.set(String::from("World")); });
        })
    ]);
})

The rendered Dom populates the text after First: with "Hello", but then when the button is clicked it's the second text that recieves "World". Ouch 😨

I guess that either:

I have a concept on how I would build that last one, would it be interesting if I made a PR with a signal::mpmc module to discuss?

Pauan commented 6 years ago

Yes, Signals are fundamentally designed around a 1 producer 1 consumer model, just like Futures and Streams. This is very important to ensure that they are zero-cost. Requiring full broadcast ability for all Signals would incur extra expense.

Most of the time having a 1:1 model works perfectly fine, but as you said sometimes you need a 1:N model, with multiple consumers.

My plan is to create a .shared() / .broadcast() method which can be used on all Signals. It returns a new Signal, and that new Signal can be cloned, and the clones receive the same messages. I just haven't had the time to make it yet.

You're right that it is confusing to have Receiver implement Clone, since this behavior is surprising.

A pull request is welcome, especially if it's an initial sketch that we can discuss, since I have some ideas on how to implement it as well.

davidhewitt commented 6 years ago

Agree that keeping to zero-cost is desirable. I think what I have in mind doesn't have too much overhead but it's probably enough that you wouldn't want it for all signals.

I'll try to cook up a PR (probably tomorrow)!

Pauan commented 6 years ago

I just now made a breaking change: Receiver, Sender, and mutable no longer exist.

Instead, they are replaced with Mutable and MutableSignal:

Old way:

let (sender, receiver) = mutable(5);

sender.set(10);

receiver.for_each(...);

New way:

let x = Mutable::new(5);

x.set(10);

x.signal().for_each(...);

x.signal().for_each(...);

As demonstrated above, it is now possible to call the .signal() method multiple times, each time it is called it returns a fresh MutableSignal. When x.set(...) is called it will broadcast the value to all of the MutableSignals.

It will .clone() the value once per MutableSignal, so you should use Mutable::new(Rc::new(...)) if you want to avoid the expense of a .clone()

Pauan commented 6 years ago

And now I added back in Sender and Receiver (and renamed mutable to channel).

Now there's two ways to create Signal sources:

let (sender, receiver) = channel(5);

sender.send(10);

receiver.for_each(...);
let x = Mutable::new(5);

x.set(10);

x.signal().for_each(...);

Mutable is the preferred way of creating Signal sources, since it is much more convenient, it supports retrieving the value (with x.get()), and it supports broadcasting.

But in the situations where you don't need those extra features, using channel is faster.

Pauan commented 6 years ago

P.S. I still would like to have a generic .broadcast() method that works on all signals, but I figured that it's more efficient and convenient to have Mutable support broadcasting by default.

Pauan commented 6 years ago

Similarly I've now changed signal_vec::unsync::Sender and Receiver to be MutableVec:

Old way:

let (sender, receiver) = mutable();

sender.push(5);
sender.push(10);

receiver.for_each(...);

New way:

let x = MutableVec::new();

x.push(5);
x.push(10);

x.signal_vec().for_each(...);

After all the various changes I've made in the past couple days, now there is a full implementation of TodoMVC in the examples/todomvc folder!

This is a big milestone, since it means that rust-dominator now has enough combinators implemented that it's possible to start making apps!

davidhewitt commented 6 years ago

Thanks! I've had a busy couple of weeks but finally had a chance today to play around with the released dominator on crates.io with this new Mutable. It's really really nice, thanks!

I hope to get to the .broadcast() impl this week, sorry about the delay. Ping me if it's blocking you and I'll step on it asap.

Pauan commented 6 years ago

No worries, it's not blocking anything.

I've given some thought to this, and I think we should have two methods: broadcast_sync_cloned and broadcast_unsync_cloned.

Actually... now that I think of it, rather than using methods, I think it would be even better if they were structs, like this:

use futures_signals::signals::unsync::Broadcaster;

let bar = Broadcaster::new(foo);

bar.signal_cloned()  // for Clone
bar.signal()         // for Copy

And similarly for signals::sync::Broadcaster (but we don't need to implement sync right now. I'm prioritizing wasm, which is single-threaded).

But it still needs to solve the problem of only polling once per change. Maybe the Broadcaster can somehow set up its own internal Task which it uses to keep track of whether the parent Signal has changed or not. I remember seeing some Futures APIs that can do that.

Pauan commented 6 years ago

P.S. A reminder that you'll need to make the changes to the rust-signal repo.

davidhewitt commented 6 years ago

See my broadcaster branch for a new unsync::Broadcaster attempt! (Built on top of the futures-0.2 branch, didn't think it wise to open a PR until that one's merged).

Pauan commented 6 years ago

Closing since this is resolved.