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

Messages based on Commands are not fed into stream #25

Open
rommsen opened this issue Jun 24, 2019 · 11 comments
Open

Messages based on Commands are not fed into stream #25

rommsen opened this issue Jun 24, 2019 · 11 comments

Comments

@rommsen
Copy link
Contributor

rommsen commented Jun 24, 2019

Hey Dag,

I am trying to use Elmish Stream in the Safe Confplanner and I am banging my head for the last couple of hours because it is not working as (I) expected.

In the Confplanner I need to work with commands (so no mksimple) and I have a couple of situations where I need to create messages from commands within the update function. For this I am using the elmish Cmd.OfFunc.result MyMsg et al functions. It seems that those message are never fed into the message stream (it is somehow circumvented and the messages go directly into update). Is this expected behaviour? I can create a minimal repro but I wanted check first if this is the way it is supposed to be.

@rommsen rommsen changed the title Based on Commands are not fed into stream Messages based on Commands are not fed into stream Jun 24, 2019
@rommsen
Copy link
Contributor Author

rommsen commented Jun 25, 2019

I think the reason is that the new dispatch function is only put into the view (https://github.com/dbrattli/Elmish.Streams/blob/c8341378cd4d72ba04172a5fbecac2382e3d1dc2/Fable.Elmish/src/Program.fs#L84). That means that only messages generated by the view are fed into the stream.

Maybe it would be better to use the withSyncDispatch function to get a dispatcher that works everywhere (https://github.com/elmish/elmish/blob/d3770c3d94aa4542e2debab06f03756ee8d050f5/src/program.fs#L111)

In the current state it is (imho) not possible to add elmish.streams incrementally or partially to an application that already uses commands.

@rommsen
Copy link
Contributor Author

rommsen commented Jun 26, 2019

I spend almost two days figuring it all out. I think at the moment this is not possible because syncDispatch is overwritten when Program.map is called. I will file a bug over there

@rommsen
Copy link
Contributor Author

rommsen commented Jun 26, 2019

@dbrattli ok now that Eugene has closed the issue I am pretty clueless how to go on from here. Maybe I am missing something obvious and there is a good solution. I am still convinced that it is totally worthwile to be able to react to messages created by commands. What do you think?

@dbrattli
Copy link
Owner

dbrattli commented Jun 26, 2019

Hi @rommsen. I will look into this, but I might not have any real amount time until the weekend. Why do you need commands? I'm not sure you really need them. Can I look at the code?

The way I do it is to e.g flatMap/map the messages before they go into update. Thus a login message that needs to navigate could be flatMap'ped into 2 messages (1 login, 1 navigate) etc.

@rommsen
Copy link
Contributor Author

rommsen commented Jun 26, 2019

Hi @dbrattli . I think most of the stuff can in principle be done as you said. My point is that the best way to raise adoption and usage of Elmish.Streams is by allowing it to be introduced gradually. I love the library but people are pretty resistent. Another thing to learn. Pretty much all real elmish apps use commands. I think that it is not very good to tell people that only messages that are generated in a view can actually be fed into the msg stream. Imagine people already doing their http requests etc with commands and now they want to use the messages that are created when such a request returns within the event stream. It is just not possible and imho this is a very severe limitation. People needed to actually change the way they write elmish apps completely and I think this is not going to happen.

Does this make sense?

@dbrattli
Copy link
Owner

Yes, I get the point. I'll look into it.

@johannesegger
Copy link
Contributor

johannesegger commented Jun 28, 2019

@rommsen Ohh yeah, it happend in my case. It took me some days to get used to it, but I'm really happy that Cmds are now gone (e.g. composition of updates got much better, because it's just a simple fold when you only have the model). I am not completely happy with how the stream function works (can't remember exactly, but it had to do with the fact that the function is called every time the model changes), so I rewrote it a bit. But the overall idea is brilliant IMO.

@johannesegger
Copy link
Contributor

And when you feed messages coming from update back into the stream (where they came from) then you can very very easily get endless loops, which are also hard to understand in the beginning and hard to avoid.

@TysonMN
Copy link

TysonMN commented Aug 31, 2019

Hello @johannesegger. I would like to better understand what you meant when you said

...I'm really happy that Cmds are now gone (e.g. composition of updates got much better, because it's just a simple fold when you only have the model).

Are you saying that instead of having update return the new model and a list of commands, you passed into update a function that accepts a command (and in production would put it into some stream), so then update now directly returns just the new model and indirectly returns the commands by invoking the given function on them?

@johannesegger
Copy link
Contributor

@bender2k14 not directly. With streams I don't use Cmds at all. update has the signature 'msg -> 'model -> 'model instead of 'msg -> 'model -> 'model * Cmd<'msg>. The side effects that are usually performed by Cmds are now performed as part of the stream. Mocking those side effects - e.g. in tests - might become harder but I consider those tests to be too trivial to create anyway (probably an unpopular opinion).

I found that the coupling between a state update and deciding what side-effect to perform sometimes makes things unnecessarily complicated (e.g. throttling/debouncing is not trivial with Cmds but built into streams).

@dbrattli
Copy link
Owner

Fable.Reaction (new name) does not depend on Elmish anymore and will use it's own message loop. Thus, components built on Fable.Reaction will not directly be able to handle stuff made from Commands. The way to communicate with Fable.Reaction components is currently to use properties (Props). You can pass your (pre-composed) dispatch function to have a component communicate back.

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

No branches or pull requests

4 participants