Update remoting to latest, docs for client #118

Zaid-Ajaj · 2018-07-15T22:08:58Z

This PR updates remoting to latest version and api, I also took the liberty and updated a couple of things along the way, PLEASE REVIEW (I haven't tested every possible combination):

Client

Added docs to model, msg, init and update

type Model = Counter option became type Model = { Counter: Counter option }

The messages now became:

type Msg =
| Increment
| Decrement
| LoadInitialCount 
| InitialCountLoaded of Result<Counter, exn>

where LoadInitialCount is a message the initiates the request and InitialCountLoaded is the one that results from the (http) request, and init() becomes:

// defines the initial state and initial command (= side-effect) of the application
let init () : Model * Cmd<Msg> =
    let initialModel = { Counter = None }
    let initialCmd = Cmd.ofMsg LoadInitialCount
    initialModel, initialCmd

Saturn

use_router instead of router as per the depreciation message
disable_diagnostics doesn't seem necessary anymore with remoting enabled (probably because the routing with remoting doesn't rely on existing combinators any more)

Suave

opening Suave.Filters and Suave.Successful instead of fully qualifying them with the OK and path web parts (it is common that they are fully qualified?)

build.fsx

DotNet.Release_2_1_300 => DotNet.Versions.Release_2_1_300 as per the depreciation message
Run target is Clean-ed before running

Zaid-Ajaj · 2018-07-16T00:27:43Z

Tested working combinations:

dotnet new SAFE
dotnet new SAFE --remoting
dotnet new SAFE --server suave --remoting
dotnet new SAFE --server suave

Testing thse combinations is very cumbersome and time-consuming: found a bug? then

Update template
Rebuild
Uninstall => install from built nuget package
Delete old project
scaffold a new project
build created project (& resolve deps from scratch, etc.)
try again 😓

Any effort to automate this would be highly appreciated

isaacabraham · 2018-07-16T07:10:35Z

@Zaid-Ajaj indeed, I had the same thoughts. I'm not sure how easy it would be to automate, but yes. Could you create an issue for it?

isaacabraham · 2018-07-16T07:11:48Z

@Zaid-Ajaj there are a few shortcuts around this, however. For example, you can install a template "over the top" of an existing folder, and it usually works fine. This removes the need to do the yarn download etc. etc. Also, did you test out giraffe?

Zaid-Ajaj · 2018-07-16T08:31:54Z

Could you create an issue for it?

Yeah, sure

there are a few shortcuts around this, however. For example, you can install a template "over the top" of an existing folder, and it usually works fine

I will try it out

Also, did you test out giraffe?

No, not yet, will do sometime today :)

Krzysztof-Cieslak · 2018-07-16T08:55:17Z

disable_diagnostics doesn't seem necessary anymore with remoting enabled

I've fixed that in latest - site.map generation doesn't crash anymore if it hasn't detected any routes.

theimowski · 2018-07-16T11:32:29Z

@Zaid-Ajaj see ad8bcbe - I've added a FAKE target to help with the test workflow.
Not very much, but now you can fake build -t install and have the updated template already available in dotnet

Also, I agree that testing the template is kinda cumbersome.
I'm open to any suggestions to improve the experience.
Automated tests would be very welcome

Zaid-Ajaj · 2018-07-16T12:50:44Z

@theimowski Thank you! I will test it out next time 😄

I have now tested giraffe with and without remoting and it seems to work (On My Machine™), I am working on windows 10 with dotnet 2.1.300

I came across another issue: I am noticing global.json is magically coming into existence and disappearing on it's own after I execute the Run for the second time in a different directory, having the effect of the Run target not running the server and I get:

(The process cannot access the file 'C:\projects\safe-giraffe\no-remoting\global.json' because it is being used by another process.)

only after I exit the process. This seems due to this block:

let install = lazy DotNet.install DotNet.Versions.Release_2_1_300

let inline withWorkDir wd =
    DotNet.Options.lift install.Value
    >> DotNet.Options.withWorkingDirectory wd

my workaround is to replace runDotNet with runTool "dotnet" cmd workingDir

theimowski · 2018-07-16T13:02:17Z

I'm considering removing the DotNet.install alltogether - we have .NET SDK 2.1 in the prerequisites anyway

Relates to #79

Zaid-Ajaj · 2018-07-17T19:46:46Z

This can be merged, if I missed something, just let know :)

theimowski · 2018-07-18T11:00:16Z

Actually there are 2 things (posted comments in code diff):

How about float pinning the Fable.Remoting dependencies?
e.g. nuget Fable.Remoting.Suave ~> 3 to prevent future breaking changes?
I'm not convinced with adding one more Msg case - IMO it makes the update function more complex than it needs to be

I'm fine with rest of changes

Zaid-Ajaj · 2018-07-18T11:24:16Z

How about float pinning the Fable.Remoting dependencies?

That shouldn't be needed anymore when pulling lastest packages (also, I will try to publish one library with it's dependencies in one package)

I'm not convinced with adding one more Msg case - IMO it makes the update function more complex than it needs to be

In the context of this simple counter app, I agree with you.

The reasoning behind this is for larger apps: usually a component doesn't ask for it's data when initialized at application startup, but when loaded or navigated to from other components where you will explicitly need to dispatch a message to initiate the request (hence the LoadInitialCounter)

I can remove the message for now, but since we are on the subject, I was actually thinking of using a more representative app in the template like a to-do list instead of a counter: it showcases the used frameworks much better with a CRUD scenario while still being very simple to follow and understand

What do you think? @isaacabraham @Krzysztof-Cieslak

theimowski · 2018-07-18T12:07:48Z

I'm happy to discuss the idea or more representative app, but let's make it a separate issue

isaacabraham · 2018-07-18T13:58:03Z

I'm definitely of the view that the template should be as simple as possible in terms of code. I really don't want to see it include CRUD stuff that is opinionated in any way or might not be applicable - we want the template to be useful for any SAFE app really.

I'd say there's a good case to be made for creating such an example app to include in the SAFE github area like the bookstore etc

Zaid-Ajaj · 2018-07-20T00:52:06Z

I see, then we will keep it simple 😄

theimowski · 2018-07-16T11:33:17Z

Content/paket.dependencies

@@ -15,11 +15,9 @@ group Server
 //#if (!remoting && server != "suave")
    nuget Fable.JsonConverter
 //#elseif (remoting && server == "suave")
-    nuget Fable.Remoting.Server ~> 3.6
-    nuget Fable.Remoting.Suave ~> 2.7
+    nuget Fable.Remoting.Suave 


Shall we float pin to a major version?

We could, but it is not necessary afaik

theimowski · 2018-07-16T11:36:02Z

Content/src/Client/Client.fs

 type Msg =
 | Increment
 | Decrement
-| Init of Result<Counter, exn>
+| LoadInitialCount 


Not sure about this change, it's just additional step and IMO makes things more complex

theimowski · 2018-07-20T11:05:28Z

Content/src/Client/Client.fs

+// The update function computes the next state of the application based on the current state and the incoming events/messages 
+// It can also run side-effects (encoded as commands) like calling the server via Http. 
+// these commands in turn, can dispatch messages to which the update function will react.  
+let update (msg : Msg) (currentModel : Model) : Model * Cmd<Msg> =


I think I prefer the shorter 6-line version
I'll merge it but revert this change afterrwards

Sure, it was just my personal preference to do it this way 👌

theimowski · 2018-07-20T11:11:58Z

Also for future, please install https://editorconfig.org/ - we use it in SAFE template to handle white spaces in a consistent shape

Zaid-Ajaj · 2018-07-20T11:23:16Z

Also for future, please install https://editorconfig.org/ - we use it in SAFE template to handle white spaces in a consistent shape

Good idea, that's why I always seemed to get the space diffs on my commits

This reverts commit 82f4d98, reversing changes made to 21cf12b.

theimowski · 2018-07-20T12:13:52Z

Unfortunately I had to revert this PR - sorry, should have tested it before merging.
The default template dotnet new SAFE nor azure dotnet new SAFE --deploy azure don't work - keep in mind for those two we keep the paket.lock files (paket-default.lock and paket-azure.lock)

Zaid-Ajaj · 2018-07-20T12:28:02Z

Hmm before the last change (removing the client msg) the template was working fine...

theimowski · 2018-07-20T12:39:54Z

It failed on:

build script with change DotNet.Versions (locked Fake.DotNet.Cli didn't have that module
saturn application CE - I got compiler error on url with Saturn pinned to 0.5

theimowski · 2018-07-23T12:37:36Z

I've rebased the commits now on new master, the changes are released in 0.26 - thanks again for contribution!

Zaid-Ajaj added 6 commits July 15, 2018 23:32

Update remoting to latest, docs for client

2e821fc

Correct result of command when remoting isn't used

8668eb6

fix remoting conditional with suave

16def73

Open Suave.Files missing

6038cd0

missing webApi definition without remoting

fc432b2

Include counterApi only if remoting is enabled

a7f1af9

Zaid-Ajaj mentioned this pull request Jul 16, 2018

Testing template after making changes is not contributor-friendly #119

Closed

Remove unnecessary client msg

e9d2b14

Zaid-Ajaj mentioned this pull request Jul 20, 2018

Use a more representative app in the template to showcase the used frameworks better #121

Closed

theimowski approved these changes Jul 20, 2018

View reviewed changes

theimowski reviewed Jul 20, 2018

View reviewed changes

theimowski merged commit 82f4d98 into SAFE-Stack:master Jul 20, 2018

Zaid-Ajaj deleted the upgrade-remoting branch July 20, 2018 11:23

theimowski added a commit that referenced this pull request Jul 20, 2018

Revert "Merge pull request #118 from Zaid-Ajaj/upgrade-remoting"

beef8bc

This reverts commit 82f4d98, reversing changes made to 21cf12b.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update remoting to latest, docs for client #118

Update remoting to latest, docs for client #118

Zaid-Ajaj commented Jul 15, 2018

Zaid-Ajaj commented Jul 16, 2018

isaacabraham commented Jul 16, 2018

isaacabraham commented Jul 16, 2018

Zaid-Ajaj commented Jul 16, 2018

Krzysztof-Cieslak commented Jul 16, 2018 •

edited

Loading

theimowski commented Jul 16, 2018 •

edited

Loading

Zaid-Ajaj commented Jul 16, 2018 •

edited

Loading

theimowski commented Jul 16, 2018

Zaid-Ajaj commented Jul 17, 2018

theimowski commented Jul 18, 2018

Zaid-Ajaj commented Jul 18, 2018

theimowski commented Jul 18, 2018

isaacabraham commented Jul 18, 2018 •

edited

Loading

Zaid-Ajaj commented Jul 20, 2018

theimowski Jul 16, 2018

Zaid-Ajaj Jul 20, 2018

theimowski Jul 16, 2018

theimowski Jul 20, 2018

Zaid-Ajaj Jul 20, 2018

theimowski commented Jul 20, 2018

Zaid-Ajaj commented Jul 20, 2018

theimowski commented Jul 20, 2018

Zaid-Ajaj commented Jul 20, 2018

theimowski commented Jul 20, 2018

theimowski commented Jul 23, 2018

Update remoting to latest, docs for client #118

Update remoting to latest, docs for client #118

Conversation

Zaid-Ajaj commented Jul 15, 2018

Client

Saturn

Suave

build.fsx

Zaid-Ajaj commented Jul 16, 2018

isaacabraham commented Jul 16, 2018

isaacabraham commented Jul 16, 2018

Zaid-Ajaj commented Jul 16, 2018

Krzysztof-Cieslak commented Jul 16, 2018 • edited Loading

theimowski commented Jul 16, 2018 • edited Loading

Zaid-Ajaj commented Jul 16, 2018 • edited Loading

theimowski commented Jul 16, 2018

Zaid-Ajaj commented Jul 17, 2018

theimowski commented Jul 18, 2018

Zaid-Ajaj commented Jul 18, 2018

theimowski commented Jul 18, 2018

isaacabraham commented Jul 18, 2018 • edited Loading

Zaid-Ajaj commented Jul 20, 2018

theimowski Jul 16, 2018

Choose a reason for hiding this comment

Zaid-Ajaj Jul 20, 2018

Choose a reason for hiding this comment

theimowski Jul 16, 2018

Choose a reason for hiding this comment

theimowski Jul 20, 2018

Choose a reason for hiding this comment

Zaid-Ajaj Jul 20, 2018

Choose a reason for hiding this comment

theimowski commented Jul 20, 2018

Zaid-Ajaj commented Jul 20, 2018

theimowski commented Jul 20, 2018

Zaid-Ajaj commented Jul 20, 2018

theimowski commented Jul 20, 2018

theimowski commented Jul 23, 2018

Krzysztof-Cieslak commented Jul 16, 2018 •

edited

Loading

theimowski commented Jul 16, 2018 •

edited

Loading

Zaid-Ajaj commented Jul 16, 2018 •

edited

Loading

isaacabraham commented Jul 18, 2018 •

edited

Loading