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

remove Channels from the reduxStore implementation #143

Merged
merged 3 commits into from
Jun 20, 2021

Conversation

gabrielittner
Copy link
Member

@gabrielittner gabrielittner commented Jun 19, 2021

This replaces the internal usages of channels by just merging the upstream actions and the actions emitted by side effects into one flow that than is collected. I'm not sure about all the issues that we had in the past before going to select and onReceive but back then we also used launch internally which we don't need here. Also collecting a Flow inside the flow {} builder is fine, so I think this implementation should work.

There is one behavior change. Previously we had essentially 2+ action queues, one for actions coming from upstream and one each for actions emitted by each side effect. The side effect action queues were always handled first and only if they were empty we would look at the next upstream action. Also within the side effects there was strict ordering by side effects (essentially the first side effect could starve all others by handling each action, including its own and always emitting a new action in response).
Now there is just one queue were actions from everywhere are put it in. Since by default there is no concurrency in the store these are still ordered. So if there is an upstream action and each side effect immediately emits another action in response to that the side effect action will still arrive in the order of the side effects like before. So this does not introduce randomness.

The behavior change can be seen in the tests. With the now uncommented delay(10) in store with 2 simple side effects the test behaves like before. Without the delay (the added store with 2 simple side effects no delay test) the upstream 2 action reaches the reducer before the side effect actions because it was first in the shared queue of actions.

}
}
}

Copy link
Collaborator

Choose a reason for hiding this comment

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

Great stuff 👍

@sockeqwe
Copy link
Collaborator

sockeqwe commented Jun 20, 2021

Thanks so much Gabriel! This is really great simplification.

Previously we had essentially 2+ action queues, one for actions coming from upstream and one each for actions emitted by each side effect.

That was never an explicit requirement or need ... it just evolved "naturally" into this without explicitly aiming for it. If a users statemachine is designed properly (i.e. via DSL) then order should not matter as the reducer takes care of computing the right state independent of order.

Also within the side effects there was strict ordering by side effects (essentially the first side effect could starve all others by handling each action, including its own and always emitting a new action in response).

Same as above. This was also not explicitly desired, I think we just didn't know how to do better back then (nor saw an issue with that approach). So all good by changing that behavior.

So this does not introduce randomness.

Great 👍 This helps debugging and makes it for us as maintainers more deterministic. Good job!

Shouold we create a new release 0.6.0 after this has been merged to get "early" feedback if something doesn't work as expected after this foundational changes by real FlowRedux users out there (there are probably not too many yet, but you never know 😄 )?

@gabrielittner
Copy link
Member Author

I think a release makes sense. Do you want to do it? I've been following these steps with the actions setup https://github.com/freeletics/mad/blob/main/RELEASING.md

@gabrielittner gabrielittner merged commit 7d8f687 into main Jun 20, 2021
@gabrielittner gabrielittner deleted the remove-channels branch June 20, 2021 16:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants