sycamore-rs / sycamore

A library for creating reactive web apps in Rust and WebAssembly
https://sycamore-rs.netlify.app
MIT License
2.79k stars 148 forks source link

Remove Rc and RefCell from Signal? #600

Closed lyphyser closed 1 year ago

lyphyser commented 1 year ago

Currently signals store the value in an Rc, requiring an extra memory allocation, and getting the value requires to clone the signal rather than getting a reference.

This seems a poor design since the Rc is unnecessary a lot of times (e.g. if the value is an integer or the value is never cloned) and the user can always create a Signal<Rc> if they want it.

Also it's obviously much more efficient to just get a reference to a value rather than cloning it with get() (although the user must be careful to not do anything that could set the signal while the reference is held) so either get() could return a reference or a get_ref() function could be provided; again, the user can clone the value themselves if they so desire.

So the options could be to either get rid of Rc on Signal or create a parallel set of Signal types without Rc.

RefCell is also not ideal in case the value is an integer, so it might be nice to have the user specify the cell type and support both RefCell and Cell.

There's also the problem that Signal doesn't support multithreading, which requires to solve this problem and also provide a way to specify an Arc-based signal emitter instead of the Rc-based one.

lukechu10 commented 1 year ago

Yeah, I definitely agree that making Signals internally use a RefCell<Rc<_>> was a mistake on my part when designing the reactivity system. Getting rid of the extra Rc wrapper will definitely make everything cleaner.

I think the best solution here would just be to get rid of the Rc all together, since one can easily get the old behavior back by just doing create_signal(Rc::new(...)).

As for .get(), I think the most convenient would be to clone the value (another option may be to make .get() only available for Copy types). We can then introduce .read() and .write() (final names subject to discussion) which would just forward to the internal RefCell's borrow and borrow_mut methods.

For RefCell/Cell, I personally think we should just stick with RefCell, even for Copyable types since the performance penalty is just an extra integer equality check which would dwarf all the other machinery that is run such as automatic dependency tracking inside effects etc...

Finally, I'm not sure if it's worth it to make Signals multi-threaded because that would slow down the single-threaded case and also make it harder to debug because we would get dead-locks instead of panics. However, this should also be up for debate and I'm open to changing my mind on this.

anwarhahjjeffersongeorge commented 1 year ago

Finally, I'm not sure if it's worth it to make Signals multi-threaded because that would slow down the single-threaded case and also make it harder to debug because we would get dead-locks instead of panics. However, this should also be up for debate and I'm open to changing my mind on this.

Even if the main Signal remains single-threaded, it would be nice to have an option for a multi-threaded alternative for use in async contexts. Or to support a generic alternative over some subset of smart pointer types.

lukechu10 commented 1 year ago

Although #612 and #626 don't make the reactivity system thread-safe, the Rcs and RefCells have been removed so I think this issue should be considered fixed.