[WIP] Adding Elmish.Bridge as a communication option (counter version)#267
[WIP] Adding Elmish.Bridge as a communication option (counter version)#267theimowski merged 7 commits intoSAFE-Stack:masterfrom
Conversation
|
Yeah I think I prefer this lighter option rather than #265 |
| "description": "add Fable.Remoting (https://zaid-ajaj.github.io/Fable.Remoting/) to server and client" | ||
| }, | ||
| { | ||
| "choice": "bridge", |
There was a problem hiding this comment.
lets add it to Tests as in previous PR
There was a problem hiding this comment.
Sorry, sent it on a limited internet connection and has kinda of hurried.
| |> Remoting.buildProxy<ICounterApi> | ||
| #endif | ||
| let initialCounter = Server.api.initialCounter | ||
| #elseif (bridge) |
There was a problem hiding this comment.
This one is preventing the else branch
There was a problem hiding this comment.
I see, let's change the else branch to elseif (!bridge) instead - those empty lines are misleading
| let loadCountCmd = | ||
| #endif | ||
| #if (!reaction && remoting) | ||
| #if (!reaction && remoting && !bridge) |
There was a problem hiding this comment.
those 3 conditions got bit complex, can we refactor those to make it cleaner? Maybe use nested #if statements?
| let nextModel = { Counter = Some initialCount } | ||
| nextModel, Cmd.none | ||
|
|
||
| #endif |
There was a problem hiding this comment.
don't forget to add your credits to safeComponents !
| |> Remoting.buildHttpHandler | ||
|
|
||
| #else | ||
| let getInitCounter () : Task<Counter> = task { return { Value = 42 } } |
There was a problem hiding this comment.
let's inline getInitCounter both for remoting and the default option - we no longer need it to be a separate function
| |> Remoting.buildHttpHandler | ||
|
|
||
| #else | ||
| let getInitCounter() : Task<Counter> = task { return { Value = 42 } } |
| type Communication = | ||
| | Remoting | ||
| | Bridge | ||
| | NoCommunication |
There was a problem hiding this comment.
let's use option type and None for default
There was a problem hiding this comment.
Huh, another example of how taking less time thinking creates overly complex things. That will even help on the other steps, good catch
|
Added couple comments |
|
I was taken by surprise and got moved from workplaces where I don't have internet access yet. But I'll try to send commits using my phone as soon as possible. |
|
Take your time, no need to hurry :) |
Zaid-Ajaj
left a comment
Zaid-Ajaj
left a comment
There was a problem hiding this comment.
Bridge.Send should use proper Cmds when using reaction && bridge
| match currentModel.Counter, msg with | ||
| | Some counter, Increment -> | ||
| #if (bridge) | ||
| Bridge.Send (ServerMsg.Increment) |
There was a problem hiding this comment.
Combination of reaction && bridge are using the side-effects directly instead of proper commands using Cmd.bridgeSendOr
|
Feels ready... Any pointers on what can be better? |
|
I'll get back to this next week |
| #endif | ||
|
|
||
| #if (reaction) | ||
|
|
There was a problem hiding this comment.
those will result in many empty lines in generated project, let's try to minimise them
| /// Elmish init function with a channel for sending client messages | ||
| /// Returns a new state and commands | ||
| let init clientDispatch () = | ||
| let value = {Value = 42} |
There was a problem hiding this comment.
Formatting: let's keep a single space at beginning and end of record construction { Value = 42 } - this comment applies to other places as well
|
|
||
| type CombinedPaketDependencies = | ||
| { Azure : bool | ||
| Remoting : bool | ||
| Communication : Communication option |
There was a problem hiding this comment.
Have you tried running UpdatePaketLockFiles after this change? We need to make sure it still works
There was a problem hiding this comment.
I believe I did, I had to change some lines so the files that weren't created wouldn't fail the build target, then I reverted after creating everything and updated the files.
|
This is looking good now 👍 Added couple minor comments |
|
Sure! I'll fix the points raised. I hope I can write a full guide soon using the template as starting point. |
|
Thank you very much for this! Released in v1.8 On a side note, as per this issue from now on we will strive to simplify the template, so we'll need to discuss every new feature before adding it |
3 participants
Alternative to #265. Makes the counter server-side with a fallback function so the client can work even when the server disconnects.
Changes made mainly to the elmish part of it.