Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Reactivity v3! (Part 1) 🎉 #612

Merged
merged 43 commits into from
Sep 14, 2023
Merged

Conversation

lukechu10
Copy link
Member

@lukechu10 lukechu10 commented May 16, 2023

A brand new implementation of Reactivity!

This gets rid of all the lifetimes introduced in #337 while maintaining the ergonomics of not having to clone everything into every closure.

Most of the API surface still has the same form, except without the lifetime annotations.
A few APIs, however, need to be changed or removed all together, including create_ref and provide_context_ref. This is because these APIs internally relied on the arena allocator attached to each scope which has been removed completely.
Instead, you can just use create_signal and create_context instead.

Another API that has been removed completely is RcSignal. Since Signals are 'static now, we can just include them directly in whichever state we want without any lifetime woes. However, this comes at the risk of accessing a signal beyond its "runtime-lifetime", i.e., it can still be accessed even after the scope is dropped, causing the app to immediately panic. In practice, however, these situations are rare enough that they justify the added ergonomics of removing lifetimes.

One tricky part is when collections are involved with signals. The current design of Sycamore encourages using nested signals for fine-grained updates. This, however, is no longer a good solution with the new reactive primitives. This is because if we create_signal and insert the result into, say, a Vec, the Signal will not be dropped until the enclosing Scope is dropped. Suppose we now want to remove a row from the Vec. The Signal, however, will keep on holding onto its data which essentially causes a memory leak.

Leptos solves this issue by adding a .dispose() method on Signals so that you can manually clean up after yourself when you remove Signals from a Vec. I believe, however, that this approach is error-prone and boilerplate-y. Instead, I propose introducing a new "Store-API" modeled on SolidJS's createStore primitive. This would get rid of the need all together to have nested signals. Instead, everything can be kept inside a normal Rust data-structure wrapped inside a Store which would keep track of reactive gets and sets with fine-grained updating.

Edit:

Upon further consideration, I've decided to remove the explicit reactive scope tracking with cx: Scope in favor of implicitly tracking it. This brings the function signature back to the pre-0.8 style of:

fn create_signal<T>(value: T) -> Signal<T> { ... }

instead of:

fn create_signal<T>(cx: Scope, value: T) -> &Signal<T> { ... }

which I believe is much nicer.

Remaining tasks:

@lukechu10 lukechu10 added C-enhancement Category: new feature or improvement to existing feature A-reactivity Area: reactivity and state handling BREAKING CHANGE Breaking changes introduced in this PR labels May 16, 2023
@lukechu10 lukechu10 added this to the v0.9 milestone May 16, 2023
@codecov
Copy link

codecov bot commented May 16, 2023

Codecov Report

Patch coverage: 91.98% and project coverage change: +4.69% 🎉

Comparison is base (ddf95d8) 62.21% compared to head (8ebbeab) 66.90%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #612      +/-   ##
==========================================
+ Coverage   62.21%   66.90%   +4.69%     
==========================================
  Files          54       63       +9     
  Lines        9408    11168    +1760     
==========================================
+ Hits         5853     7472    +1619     
- Misses       3555     3696     +141     
Files Changed Coverage Δ
packages/sycamore-reactive3/src/context.rs 0.00% <0.00%> (ø)
packages/sycamore/src/web/html.rs 69.36% <ø> (ø)
packages/sycamore-reactive3/src/utils.rs 34.88% <34.88%> (ø)
packages/sycamore-reactive3/src/signals.rs 93.96% <93.96%> (ø)
packages/sycamore-reactive-macro/src/lib.rs 94.57% <94.57%> (ø)
packages/sycamore-reactive3/src/scope.rs 94.96% <94.96%> (ø)
packages/sycamore-reactive3/src/memos.rs 97.58% <97.58%> (ø)
packages/sycamore-reactive3/src/iter.rs 98.17% <98.17%> (ø)
packages/sycamore-reactive3/src/effects.rs 100.00% <100.00%> (ø)
packages/sycamore-reactive3/src/store.rs 100.00% <100.00%> (ø)

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

lukechu10 and others added 2 commits May 16, 2023 11:12
Co-authored-by: Genna Wingert <wingertge@gmail.com>
@lukechu10
Copy link
Member Author

Hey @wingertge, I included the docs you wrote from #601 in this PR and I added you as a co-author. Hopefully you don't mind!

@lukechu10 lukechu10 changed the title Reactivity v3! 🎉 Reactivity v3! (Part 1) 🎉 Sep 14, 2023
@lukechu10 lukechu10 marked this pull request as ready for review September 14, 2023 16:03
@lukechu10
Copy link
Member Author

I will be merging this now since there is already quite a significant amount of changes. The remaining work of migrating the rest of the Sycamore code base to use the new reactivity system will be done in a different PR so as to not make this one bigger than necessary.

@lukechu10 lukechu10 merged commit 3ce37fb into sycamore-rs:master Sep 14, 2023
12 checks passed
@lukechu10 lukechu10 deleted the reactivity-v3 branch September 14, 2023 20:48
@lukechu10 lukechu10 mentioned this pull request Sep 15, 2023
11 tasks
@lukechu10 lukechu10 mentioned this pull request Apr 3, 2024
8 tasks
@lukechu10 lukechu10 mentioned this pull request Sep 4, 2024
7 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-reactivity Area: reactivity and state handling BREAKING CHANGE Breaking changes introduced in this PR C-enhancement Category: new feature or improvement to existing feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant