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

feat(state): New state system #1025

Closed
wants to merge 59 commits into from
Closed

feat(state): New state system #1025

wants to merge 59 commits into from

Conversation

Flemmli97
Copy link
Collaborator

@Flemmli97 Flemmli97 commented Jul 21, 2023

What this PR does 📖

  • This is the PR for a new state system utilising a local subscription dioxus signals.
  • For now it implements this system for the message struct in chats.
  • Goal is to have a system that allows one to update only a selected set of components without updating the whole state.
  • Something to note is that since this only implements it for messages the actual impact will probably not be noticed much since in a lot of places components still subscribe to the generic State in uplink. We will likely only notice it if the whole State uses this system.
    - TODO:
    - [x] Local Subscription system
    - [ ] Wrap needed uplink state fields in LocalSubscription
    - [ ] Change methods in state so they properly subscribe to their correct local subs

Signals are used here just as a wrapper around the actual value. Writing to/Reading from it is simply done via the simple read and write functions. Dioxus will handle updating the correct components

@Flemmli97 Flemmli97 added the Draft PR is still a draft and needs more work label Jul 21, 2023
@github-actions
Copy link
Contributor

⚠️ The title of this PR is invalid.
Please make the title match the regex (?:add|update|task|chore|feat|fix)\([a-z-A-Z]+\):\s.+.
e.g.) add(cli): enable --verbose flag, fix(api): avoid unexpected error in handler

@github-actions github-actions bot added Don't merge yet DO NOT MERGE Missing dev review Still needs to be reviewed by a dev labels Jul 21, 2023
@github-actions
Copy link
Contributor

⚠️ The title of this PR is invalid.
Please make the title match the regex (?:add|update|task|chore|feat|fix)\([a-z-A-Z]+\):\s.+.
e.g.) add(cli): enable --verbose flag, fix(api): avoid unexpected error in handler

2 similar comments
@github-actions
Copy link
Contributor

⚠️ The title of this PR is invalid.
Please make the title match the regex (?:add|update|task|chore|feat|fix)\([a-z-A-Z]+\):\s.+.
e.g.) add(cli): enable --verbose flag, fix(api): avoid unexpected error in handler

@github-actions
Copy link
Contributor

⚠️ The title of this PR is invalid.
Please make the title match the regex (?:add|update|task|chore|feat|fix)\([a-z-A-Z]+\):\s.+.
e.g.) add(cli): enable --verbose flag, fix(api): avoid unexpected error in handler

@github-actions
Copy link
Contributor

github-actions bot commented Jul 21, 2023

UI Automated Test Results Summary for MacOS/Windows

466 tests   242 ✔️  1h 14m 47s ⏱️
  39 suites  224 💤
    3 files        0

Results for commit f14a3d9.

♻️ This comment has been updated with latest results.

@github-actions
Copy link
Contributor

github-actions bot commented Jul 21, 2023

UI Automated Tests execution is complete! You can find the test results report here

@dariusc93 dariusc93 changed the title feat(state: New state system feat(state): New state system Jul 21, 2023
@sdwoodbury
Copy link
Contributor

this looks almost the same as the implementation of UseSharedState - calling cx.scope_id() and adding that to a list of consumers. How is the new state system different?
image

@github-actions github-actions bot added the Failed Automated Test This PR is failing Luis's Appium test and needs revised label Jul 25, 2023
@github-actions github-actions bot added the missing fixing conflict A conflict exists in current PR and needs resolution label Jul 28, 2023
@Flemmli97 Flemmli97 removed the Draft PR is still a draft and needs more work label Jul 31, 2023
@Flemmli97
Copy link
Collaborator Author

this looks almost the same as the implementation of UseSharedState - calling cx.scope_id() and adding that to a list of consumers. How is the new state system different?
image

The thing with use_stared_state is that it does not support sub components.
If you use use_stared_state you will subscribe to the whole State and any changes in State will cause a rerender for the subscribed component regardless of what was updated.

We can circumvent it by simply splitting State into multiple independent structs and register each of them with use_stared_state but that is hard to manage.

With this we can still mostly keep our current structure.

At the end we would have instead of

State {
  chat: Chats,
  some_other_field: String
}
State {
  chat: LocalSubscription<Chats>,
  some_other_field: String
}

and accessing the local subscription will automatically listen to changes in chat only. If now some_other_field is updated it will not cause a rerender for the component listening to chat

@github-actions github-actions bot removed the Failed Automated Test This PR is failing Luis's Appium test and needs revised label Jul 31, 2023
@github-actions github-actions bot added the missing fixing conflict A conflict exists in current PR and needs resolution label Oct 12, 2023
@Flemmli97 Flemmli97 removed the missing fixing conflict A conflict exists in current PR and needs resolution label Oct 12, 2023
@github-actions github-actions bot added Failed Automated Test This PR is failing Luis's Appium test and needs revised and removed Failed Automated Test This PR is failing Luis's Appium test and needs revised labels Oct 12, 2023
@github-actions github-actions bot added the Failed Automated Test This PR is failing Luis's Appium test and needs revised label Oct 17, 2023
@sdwoodbury
Copy link
Contributor

This looks cool but I'm not sure how much it will help at the moment.

If a message is updated, the messages still need to be grouped and I believe that would still cause a re-render. Fortunately the chats page has been refactored to not store so many messages in memory.

I wonder if this could help in other parts of the codebase. For example - if a friend comes online, it might be nice to not re-render stuff that doesn't include that friend. Still, for stuff like that I have a theory that it may be easier to have a top level element that pulls everything it needs from State and passes it to a child Element via props. That way if State is updated, in theory the child elements may have the same props and if props don't change, the element doesn't get re-rendered. (this is easier said than done because State gets used everywhere).

@github-actions github-actions bot removed the Failed Automated Test This PR is failing Luis's Appium test and needs revised label Oct 17, 2023
@Flemmli97
Copy link
Collaborator Author

unless we do it across the whole app (and not just messages) the gain atm isn't really big.
but e.g. editing messages should only update that one message component and not the whole chat

@github-actions github-actions bot added the missing fixing conflict A conflict exists in current PR and needs resolution label Oct 20, 2023
@Flemmli97 Flemmli97 removed the missing fixing conflict A conflict exists in current PR and needs resolution label Oct 23, 2023
@github-actions github-actions bot added Failed Automated Test This PR is failing Luis's Appium test and needs revised missing fixing conflict A conflict exists in current PR and needs resolution and removed Failed Automated Test This PR is failing Luis's Appium test and needs revised missing fixing conflict A conflict exists in current PR and needs resolution labels Oct 23, 2023
@github-actions github-actions bot added Failed Automated Test This PR is failing Luis's Appium test and needs revised and removed Failed Automated Test This PR is failing Luis's Appium test and needs revised labels Oct 25, 2023
@tooshel
Copy link
Contributor

tooshel commented Oct 26, 2023

Closing this for now and we will revisit after the next release of Dioxus.

@tooshel tooshel closed this Oct 26, 2023
@github-actions github-actions bot removed QA Tested QA has tested and approved Missing dev review Still needs to be reviewed by a dev Don't merge yet DO NOT MERGE labels Oct 26, 2023
@stavares843 stavares843 deleted the new_state branch November 1, 2023 19:20
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.

10 participants