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

Add use_state View to xilem-core #130

Closed
wants to merge 1 commit into from
Closed

Add use_state View to xilem-core #130

wants to merge 1 commit into from

Conversation

matthunz
Copy link
Contributor

Add the generic use_state view to xilem-core.

fn use_state<F, S, G, V>(
    make_state: F,
    make_view: G
) -> UseState<F, S, G, V>

This would generalize and replace the current UseState view in the main xilem crate. The main benefit here is no Arc to hold the current state. This is still a WIP and is being ported from the remember view in my UI crate.

I think this has huge benefits as it would allow us to compose state at no runtime cost. For example, global app state could be replaced by small sections of state created with this view.

Copy link
Contributor

@raphlinus raphlinus left a comment

Choose a reason for hiding this comment

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

I've taken a quick look. I'm not 100% sure I understand what's going on here, partly because it's incomplete.

The original use_state implementation uses Arc for a reason: it does smart change propagation using the same techniques as Druid. In particular, in event (message) propagation, it offers the child view exclusive mutable access to a copy of the app state in T, then only updates the parent state if the child has changed it. In other words, it preserves pointer equality of the state, which is a primary mechanism to drive incremental computation.

It's not clear whether this approach would support that. It's also not 100% clear that this approach to use_state is what we want to do. It was added to the Xilem prototype speculatively, largely because people were asking after it, and because it's very important in the React way of doing things. Yet, in the Elm way of doing things, people seem to get by just fine without it.

type Element = V::Element;

fn build(&self, cx: &mut $cx) -> (Id, Self::State, Self::Element) {
todo!()
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems rather important.

Copy link
Contributor Author

@matthunz matthunz Aug 17, 2023

Choose a reason for hiding this comment

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

Lol agree with ya there... Just wanted to make sure I was heading in the right direction. Definitely looks like there are a few ways to go here.

If you're curious this is basically the method implementation I had in mind: remember
Here I use the second function argument to create a view with the state from Remember. The first function would create the state only once

@matthunz
Copy link
Contributor Author

Huh sorry for the confusion! I wasn't sure why the other implementation required Arc but that makes sense for the pointer eq.

The remember view in my crate just served the purpose of adding state in views so I didn't need global state. Xilem's use_state seems really interesting for change propagation though so I can see why we'd want it.

For this PR then I feel like I could either try to bring the old version of use_state into core or rename this remember and port the version from my crate. I do still think having remembering state, without the added change propagation, would be useful

@Philipp-M
Copy link
Contributor

Just that it's not overlooked, I have fixed the original UseState some time ago: #98

It's not yet ported to core in that PR, but should be easy to do so, either in that PR, or IMHO preferably (because of the currently smaller diff) in a new PR. The PR is basically mergeable I think, just needs a rubber stamp review I guess.

@matthunz
Copy link
Contributor Author

matthunz commented Mar 9, 2024

Closing in favor of #98

@matthunz matthunz closed this Mar 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants