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

Experimental/store redesign #125

Merged
merged 6 commits into from
May 8, 2023
Merged

Experimental/store redesign #125

merged 6 commits into from
May 8, 2023

Conversation

buntec
Copy link
Owner

@buntec buntec commented May 6, 2023

No description provided.

)(
makeDispatcher: SignallingRef[F, State] => Action => F[Unit]
def apply[F[_]: Concurrent, State, Action](init: State)(
update: Action => State => (State, F[Option[Action]])
Copy link
Contributor

Choose a reason for hiding this comment

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

could it be List[Action]?

Copy link
Owner Author

Choose a reason for hiding this comment

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

Good point! And would you make them atomic, i.e., have a Queue[F, List[Action]]?

Copy link
Contributor

Choose a reason for hiding this comment

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

hm, what do you mean by atomic in this context? e.g. what's an example of how Queue[F, List[Action]] would behave differently to Queue[F, Action]?

Copy link
Owner Author

Choose a reason for hiding this comment

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

Meaning that no other actions can be interspersed. Say my long-running effect returns List(Inc, Inc) and there is button dispatching Reset, then I could get the sequence Inc, Reset, Inc because I cannot guarantee atomic insertion of two elements into a queue.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah indeed, you are right! Hmm 🤔 I think in that case the user should define a new type of action? Not sure, don't have a strong opinion. But it feels a bit inconsistent if some actions are atomic with each other depending on how they were submitted and other actions are atomic only to themselves.

In theory I suppose a user could even define a compound action that is just a list of actions and achieve the same semantic.

Copy link
Owner Author

Choose a reason for hiding this comment

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

Agree! I guess in that case keeping F[Option[Action]] is simpler and doesn't open up the question of atomicity. Or do you see any other benefits of List over Option that cannot be achieved with compound actions?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes you're right, that could also be implemented in userland :)

Copy link
Owner Author

Choose a reason for hiding this comment

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

Perhaps it should be Action => State => (State, Option[F[Option[Action]]]) for otherwise there is a call supervisor.supervise(none.pure[F]) on every state update? 🤔

Copy link
Owner Author

Choose a reason for hiding this comment

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

I think I've convinced myself that this change is needed - for the sake of performance and to avoid having to write none.pure[F] or similar all over the place.

Comment on lines 41 to 43
actionQ <- Queue.unbounded[F, Action].toResource

state0 <- SignallingRef.of[F, State](initialState).toResource
effectQ <- Queue.unbounded[F, F[Option[Action]]].toResource
Copy link
Contributor

Choose a reason for hiding this comment

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

You could also consider using an fs2.concurrent.Channel. I can't think of any meaningful differences between the two for your usecase.

dispatcher = makeDispatcher(state0, router)
_ <- Stream
.fromQueueUnterminated(actionQ)
.evalMap(action => stateSR.modify(update(action)).flatMap(effectQ.offer))
Copy link
Contributor

Choose a reason for hiding this comment

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

actually I think instead of an effectQ you could create an internal Supervisor and directly run the effects on that. It should be more-or-less equivalent to parEvalMapUnorderedUnbounded.

@buntec buntec marked this pull request as ready for review May 6, 2023 14:40
@buntec buntec merged commit 94a865f into main May 8, 2023
@buntec buntec deleted the experimental/store-redesign branch May 8, 2023 09:28
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.

2 participants