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

Receive state #122

Merged
merged 4 commits into from
Jul 12, 2022
Merged

Receive state #122

merged 4 commits into from
Jul 12, 2022

Conversation

rowrowrowrow
Copy link
Contributor

I think sticking to the previous -> next state convention makes reconciling the incoming easy to use.

@rowrowrowrow
Copy link
Contributor Author

#108

@rowrowrowrow
Copy link
Contributor Author

@aohua Hoping you can provide some feedback.

@aohua aohua closed this Feb 9, 2022
@aohua aohua reopened this Feb 9, 2022
@rowrowrowrow
Copy link
Contributor Author

@aohua Anything I can do to help get this change included? If we want to avoid breaking changes for people with the current order of arguments for example.

@aohua
Copy link
Owner

aohua commented Feb 14, 2022

Hi @rowrowrowrow Thanks for your PR, but I think the init function is only supposed to be triggered once, storing the initial state may not be very useful in most of the use cases. I'm currently converting the entire project to TS and trying to improve the init process, I will consider this if there is a valid use case for it, and this is a breaking change I might include it together with other major updates.

@rowrowrowrow
Copy link
Contributor Author

rowrowrowrow commented Feb 14, 2022

Hey @aohua, thanks for taking the time to consider the PR.

I think that adding a receiveState function is necessary as the prepareState function is currently used both for sending AND receiving state. The issue is when you receive state you may have existing state that shouldn't be overwritten by the incoming state for whatever reason. So without a receiveState function, which receives both the existing and incoming states as arguments, I don't think there is a way to reconcile the two.

e.g. this should also resolve #108

@rowrowrowrow
Copy link
Contributor Author

I rebased the commit on master.

@rowrowrowrow
Copy link
Contributor Author

@aohua Is there anything I can do to help with this?

@rowrowrowrow
Copy link
Contributor Author

@aohua bump! Thanks

@aohua aohua merged commit c3a6f5d into aohua:master Jul 12, 2022
@aohua
Copy link
Owner

aohua commented Jul 12, 2022

Thanks @rowrowrowrow merged and released with version 3.1.4

@aohua
Copy link
Owner

aohua commented Jul 12, 2022

@rowrowrowrow Could you help to update the Readme file as well? Thanks

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.

None yet

2 participants