Skip to content

[WIP] Adding Elmish.Bridge as a communication option#265

Closed
Nhowka wants to merge 6 commits intoSAFE-Stack:masterfrom
Nhowka:bridge
Closed

[WIP] Adding Elmish.Bridge as a communication option#265
Nhowka wants to merge 6 commits intoSAFE-Stack:masterfrom
Nhowka:bridge

Conversation

@Nhowka
Copy link
Copy Markdown

@Nhowka Nhowka commented May 22, 2019

Implementing #107

I tried to modify as little as possible, only adding a simple clock that can be paused so the tick won't be sent to the client. Now probably need some more comments so the user can know better where to modify.

Sending the PR as the CI is very nice on checking if I broke anything.

cc @theimowski @isaacabraham @Zaid-Ajaj

@theimowski
Copy link
Copy Markdown
Member

First thing - big thanks for that, adding a feature like that is quite a challenge and I really appreciate your work on that since I realise what was involved.

Regarding the changes: can we just send the initial counter via Elmish.Bridge instead of introducing the clock?
I'd prefer we remain minimalistic with regards to the communication.

Also on a different note: those changes are quite big, and TBH I have never used Elmish.Bridge before.
Are you willing to help if any issues related to Elmish.Bridge arise as well as when we need to update dependencies to newer versions?
We just need to make sure we're able to maintain this option after we deliver it.

@Zaid-Ajaj
Copy link
Copy Markdown
Contributor

All in all it looks really good! One thing I noticed is executing side-effects inside event handlers, i.e. calling Bridge.Send Msg inside OnClick handlers which makes the view function impure/not unit-testable. It is not that of a big deal but it is better to put such code inside commands as they are responsible for all the side-effects

@Nhowka
Copy link
Copy Markdown
Author

Nhowka commented May 23, 2019

can we just send the initial counter via Elmish.Bridge instead of introducing the clock?

We could, but it's not really suitable for actions that are just one-time things as it creates a stateful connection between the server and client. Instead I could make the counter server-side and the client would just show the counter that would be incremented/decremented on the server, but that would make it harder to add a new endpoint to the api or be compatible with the remoting option as it would not have a starting point for them. Let me know if you are ok with the drawbacks.

Are you willing to help if any issues related to Elmish.Bridge arise as well as when we need to update dependencies to newer versions?

Yes, I'm willing to work on it and to smooth any rough edges that may appear.

it is better to put such code inside commands as they are responsible for all the side-effects

After making excuses for my laziness while replying I thought of a smarter way to fix that without much work. I'll test it and apply the changes if it works.

@Zaid-Ajaj
Copy link
Copy Markdown
Contributor

After making excuses for my laziness while replying I thought of a smarter way to fix that without much work. I'll test it and apply the changes if it works.

It shouldn't require a lot of work to create a command from the Send call, just this

module Cmd = 
  let bridgeSend (msg 't) : Cmd<'t> = [ fun _ -> Bridge.Send msg |> ignore ]

There might an existing Cmd helper that runs a function but I am not sure

@Nhowka
Copy link
Copy Markdown
Author

Nhowka commented May 23, 2019

I might've just changed one problem for another. My idea was creating a new case on the messages, something like Outgoing of ServerMsg and then calling Bridge.Send with those messages on update. 😅

I'll investigate if the Cmd helper works ok as Bridge.Send depends on the type of the message being sent. But I really like the idea, that would make it more alike the other libraries.

@Zaid-Ajaj
Copy link
Copy Markdown
Contributor

Zaid-Ajaj commented May 23, 2019

I'll investigate if the Cmd helper works ok as Bridge.Send depends on the type of the message being sent.

Yeah then you need to inline the function.

But I really like the idea, that would make it more alike the other libraries

Yeah definitely! This is a suggestion for the library (maybe not the best place to discuss it here but) the Bridge.Send call doesn't communicate back whether the call was successful or not. Maybe something like an onError : exn -> 'msg callback would be nice:

module Cmd = 
  let inline bridgeSend (msg: 'Msg) (onError:  exn -> 'Msg) = 
    [ fun dispatch -> Bridge.SendSafe msg (fun error -> dispatch (onError error) ]

where Bridge.SendSafe has the type: 'Msg -> (exn -> unit) -> unit

The example above only communicates the errors back to the dispatch loop but you could have other callbacks that tell the success status etc. Let me know what you think

@Nhowka
Copy link
Copy Markdown
Author

Nhowka commented May 23, 2019

Yeah then you need to inline the function.

I works! ❤

Bridge.Send call doesn't communicate back whether the call was successful or not

Maybe making a happy one that will gladly ignore the unsent messages when the socket is down and another that will try to add something to the loop. Adding an optional unit -> unit to Bridge.Send would support that. That would be needed as the dispatch reference inside bridge only points to the top-level message.

module Cmd =
    let inline bridgeSend (msg:'server) : Cmd<'client> = [ fun _ -> Bridge.Send msg ]
    let inline bridgeSendOr (msg:'server) (fallback:'client) : Cmd<'client> = [ fun dispatch -> Bridge.Send(msg, fun () -> dispatch fallback) ]

That is assuming that the message only fails to be sent when the socket closes. That would support different recoveries as different messages are tried to be sent when the socket is not up, but there is already an option to send a message when the socket closes.

@theimowski
Copy link
Copy Markdown
Member

@Nhowka how about sending the clock from server, but use it for Counter in the Client model? I.e. you could take the unix epoch seconds and use them as Counter on the client side.
The Increment and Decrement buttons could work as previously: +/- 1 and then on next clock tick you'd get a value from server which overwrites your clicks.
Or we could hide Increment and Decrement when Elmish.Bridge is used

@theimowski
Copy link
Copy Markdown
Member

Can we close this as #267 went in?

@Nhowka
Copy link
Copy Markdown
Author

Nhowka commented Jun 6, 2019

Yes, thanks!

@Nhowka Nhowka closed this Jun 6, 2019
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.

3 participants