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

adding `set_fn_mut` and `set_fn_mut_silent` #560

Closed blainehansen closed 1 year ago

blainehansen commented 1 year ago

More convenience functions :) These set_fn_mut variants use RefCell::borrow_mut and Rc::get_mut to directly mutably borrow the underlying value without a clone.

I thought you would consider the possible panics acceptable since the Modify implementation does the same thing. It seems this is safe to do, since the borrow only lasts during the function's synchronous call and won't conflict with other borrows since Rc can't be sent across thread boundaries.

Am I crazy? There are tests covering it even with dependent signals, so it seems safe!

codecov[bot] commented 1 year ago

Codecov Report

Base: 60.52% // Head: 60.60% // Increases project coverage by +0.08% :tada:

Coverage data is based on head (4a246f2) compared to base (6cecd1a). Patch coverage: 100.00% of modified lines in pull request are covered.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #560 +/- ## ========================================== + Coverage 60.52% 60.60% +0.08% ========================================== Files 55 55 Lines 9299 9319 +20 ========================================== + Hits 5628 5648 +20 Misses 3671 3671 ``` | [Impacted Files](https://codecov.io/gh/sycamore-rs/sycamore/pull/560?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=sycamore-rs) | Coverage Δ | | |---|---|---| | [packages/sycamore-reactive/src/signal.rs](https://codecov.io/gh/sycamore-rs/sycamore/pull/560?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=sycamore-rs#diff-cGFja2FnZXMvc3ljYW1vcmUtcmVhY3RpdmUvc3JjL3NpZ25hbC5ycw==) | `87.93% <100.00%> (+0.63%)` | :arrow_up: | Help us with your feedback. Take ten seconds to tell us [how you rate us](https://about.codecov.io/nps?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=sycamore-rs). Have a feature suggestion? [Share it here.](https://app.codecov.io/gh/feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=sycamore-rs)

:umbrella: View full report at Codecov.
:loudspeaker: Do you have feedback about the report comment? Let us know in this issue.

lukechu10 commented 1 year ago

I thought you would consider the possible panics acceptable since the Modify implementation does the same thing. It seems this is safe to do, since the borrow only lasts during the function's synchronous call and won't conflict with other borrows since Rc can't be sent across thread boundaries.

Just for reference, the current Modify implementation can't panic because it creates a clone implicitly behind the hood. However, that being said, I would probably still consider this possible panic acceptable.

blainehansen commented 1 year ago

This doesn't actually reliably work, I just tried to use it in a fairly simple case and it panicked. I'll close and open another pr if I can get this to work ha.

lukechu10 commented 1 year ago

This doesn't actually reliably work, I just tried to use it in a fairly simple case and it panicked. I'll close and open another pr if I can get this to work ha.

I think to make this work, we would need to either get rid of the inner Rc in RefCell<Rc> or add another nested RefCell so RefCell<Rc<RefCell>>. I am personally leaning towards the former. I've been thinking maybe we can just make .get() only work for Copy types and add a .read() which wraps around RefCell::borrow and a similar .write(). We could also add a .get_clone() which would be an alias for .read().as_ref().clone().

So to get the original behavior for Signal back, you'll need to add a Rc::new(value) to all the create_signals and use .get_clone() instead of .get().

Sadly, this would be a really massive breaking change again but I think it could be worth it.