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

Replace functions with Signal type #20

Merged
merged 4 commits into from
Mar 8, 2021
Merged

Conversation

Kestrer
Copy link
Contributor

@Kestrer Kestrer commented Mar 8, 2021

Essentially I swapped out using Rc<dyn Fn() -> Rc<T>> getters for the StateHandle<T> type, and I swapped out Rc<dyn Fn(T)> signal setters for the Signal<T> type. The functions have been replaced with .get/.set methods - although this is slightly longer, it also has other advantages such as being able to use .get_untracked instead of the untracked function, which is shorter and faster (since it doesn't access thread locals at all).

Fixes #14.

@lukechu10 lukechu10 self-requested a review March 8, 2021 15:43
@lukechu10
Copy link
Member

lukechu10 commented Mar 8, 2021

I don't really like combing .get() and .set() together into a single struct. Having two separate getter and setters makes it more explicit where the state is accessed and where it is mutated. This also allows passing readonly state to components. What do you think?

I like having .get_untracked directly on the state but I find having .get and .set a bit verbose. How about having StateHandle implement Deref<&dyn Fn() -> T> and SetStateHandle implement Deref<&dyn Fn(T)>? We could also have a .untracked method on StateHandle which is the same as .get_untracked as you have it now.

For consistency, there should also be a method on SetStateHandle that does not trigger effects. It could be called something like .silent(new_value: T).

Edit:
Oh wait, I didn't see that you implemented Deref<StateHandle> for Signal. Sorry :)
I actually like this approach, just that it would be nice if we could call StateHandle directly without calling .get

@lukechu10 lukechu10 added the C-enhancement Category: new feature or improvement to existing feature label Mar 8, 2021
@Kestrer
Copy link
Contributor Author

Kestrer commented Mar 8, 2021

Actually, I just realized that we can do one better than a function call - just have it implement Deref<Target = T> directly.

@lukechu10
Copy link
Member

Actually, I just realized that we can do one better than a function call - just have it implement Deref<Target = T> directly.

Would that really be better? That makes the dependency tracking a lot more implicit and people could get confused on what gets tracked and what does not. Making it a function call immediately signals to the user that something is happening other than simply accessing state.

@Kestrer
Copy link
Contributor Author

Kestrer commented Mar 8, 2021

Actually, that's true - and trying to implement it I realized that it actually wouldn't work :D. I'll try to implement a function call then - although I will point out that .get() is a much more common pattern in the Rust ecosystem for wrapper types (e.g. in the standard library Cell, OnceCell and NonZero* all have a .get() method).

@Kestrer
Copy link
Contributor Author

Kestrer commented Mar 8, 2021

The problem with making it callable is that I'm pretty sure it would basically require storing an Rc<dyn Fn() -> Rc<T>> in the StateHandle itself, which loses out on the performance benefit this change was supposed to bring about in the first place :(. I guess there's still the type-safety benefit and the advatange of avoiding two TLS accesses for untracked access, but that's comparatively small.

@lukechu10
Copy link
Member

.get() is a much more common pattern in the Rust ecosystem for wrapper types (e.g. in the standard library Cell, OnceCell and NonZero* all have a .get() method).

I guess you're right. We'll use .get() then.

Copy link
Member

@lukechu10 lukechu10 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The implementation looks really good. I like how some functionality is separated into the create_selector_with function.

examples/components/src/main.rs Outdated Show resolved Hide resolved
maple-core/src/reactive.rs Show resolved Hide resolved
maple-core/src/reactive.rs Outdated Show resolved Hide resolved
@Kestrer
Copy link
Contributor Author

Kestrer commented Mar 8, 2021

I've removed the unsafe now. The solution was to pass the handler (Rc<Computation>) as a parameter to the dependencies instead of each dependency accessing the handler from TLS - this way, the handler can actually be created after the first time the effect is run, which is necessary to implement memos without Options (as the memo's underlying signal can only be created after the first time the memo has been run).

@lukechu10 lukechu10 merged commit ed8e4a2 into sycamore-rs:master Mar 8, 2021
@Kestrer Kestrer deleted the signal branch March 8, 2021 21:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-enhancement Category: new feature or improvement to existing feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Use dedicated type for StateHandle instead of Rc<dyn Fn>
2 participants