Upgrade to beta5

[isaacabraham]

No description provided.

[mattgallagher92]

I guess that, because Async<'a> is cold, the API isn't actually called until Elmish handles the command. Have we tested that?

I would feel weird about accidentally triggering an API call in the update loop as it's meant to be a pure function. Maybe it doesn't matter? Not sure what the implication is if there's a network exception or something.

[isaacabraham]

I spoke with @MangelMaxime about exactly this, and as you say, since they are cold workflow computations, this is safe to do. From a "pure" perspective it might be a little weird but from a "learner" perspective I think that this is easier to grok than the separate "method" + "arg" approach (plus pipelining seems more natural here).

[MangelMaxime]

Personally, I think sticking to the native Cmd module is "better".

Consistent API

This is because, like that people learn to work with Cmd in general and when they need to execute a function inside of Cmd they use the same API.

Cmd.OfAsync.attempt
Cmd.OfPromise.attempt
Cmd.OfFunc.attempt
// etc.

You always do:

1. I want a command : Cmd

2. I want to work with X (async for example): Cmd.OfAsync

3. I only case care about the success path: Cmd.OfAsync.attempt

4. Now I need to provide the "action", its arguments and the way to get back the result:

   Cmd.OfAsync.attempt todosApi.getTodo () LoadTodos
   
   // ------------
   Cmd.OfAsync.attempt
   	todosApi.getTodos
   	()
   	LoadTodos
   
   // ------------
   let args = { ... } // A really complex/lengthy arguments to build
   Cmd.OfAsync.attempt todosApi.getTodo args LoadTodos

And these questions / logics can be applied to any type of commands.

More focused intellisense

The intellisense also give them only the Cmd API not the whole Async API in the intellisense. Compare the result of Cmd.OfAsync.* vs Async.* where * is the place where you ask for intellisense.

[isaacabraham]

These are good points. Perhaps we can piggy back on the Cmd module instead e.g.

Cmd.ofAsync.startApiCall or similar?

The concerns I have with the Cmd.OfAsync module are:

1. attempt and the other names are sub-optimal - they do not easily reflect the behaviours that distinguish each of them.
2. The separate "method" and "argument" approach is confusing. People's initial inclination is to simply want "here's an Async, please can you map it to a command".

I've seen beginners all fail to grasp the Cmd.OfAsync module in this way - and the same way that the different modules e.g. OfAsync rather than OfAsyncWith (or whatever it's called) is also not immediately obvious.

I guess I'm trying to provide a simple, constrained model that fits for the beginner and (ideally) 80-90% of the most common use cases people have, especially with Fable Remoting.

[MangelMaxime]

Personally, I don't find startApiCall to be a better name because startApiCall feels limited to API call when it works for any async operations.

Edit: It is possible that your startApiCall is doing more that I am seeing in this PR and so perhaps my comment above is perhaps outdated. I saying that because I just read in another thread the following:

Regarding the ApiCall behaviour - it will actually work with any Async workflow, although you're right, it only works with the ApiCall type as it automatically wraps it in Finished. I also considered:

Async.toCmdUnsafe was created to kind of escape the Cmd knowledge but it back into it because people need to find the *Cmd* API in the whole intellisense proposition.

I used Fable.Remoting in one of my project with a beginner client and using Cmd.OfAsync never caused issues, once you know the pattern you always apply the same code.

IHMO in this situation it is best to teach people how Elmish API works and make them understand it than adding a layer of abstraction on top of it. My reason for that is I often see people who used abstraction and are lost once they need to think a little bit out of the box / standard canvas that was teaches to them.

The way I teach it to people is by either looking at the signature of the function:

1. Do you need to be notified on success and don't care about error? Cmd.perform
2. Do you need to be notified only for errors? Cmd.attempt
3. Do you want to be notified in both cases? Cmd.either
4. Do you just want to "execute" it and never be notified then use Cmd.exec => This is one doesn't exist in Elimish standard API, I always add it in the project manually.

Coming back to them being lost, this is also why I teach/mention early that init, update and view are just normal functions. They requires in general 2 arguments but if needed please use 3,4,... arguments for theses functions.

I don't think the complexity of mastering Elmish is coming from the Cmd API, but more in finding a way to teach people to create re-usable components or patterns in how to structure their code / modules in a way that the code is easily discoverable.

[isaacabraham]

I like the idea of adding links to recipes to the xmldocs - this could really help beginners. Maybe we actually need a recipe / explanation post on the different Cmd modules e.g. Async etc..

And yes, the abstraction is very shallow, it doesn't do much except wrap the Finished thing. I have an alternate version that was using a class rather than functions - the benefit being that there was only one member (either overloaded or with an optional argument for the onError message.) which meant that there was nothing to learn in terms of member names e.g.

Cmd.ofAsync.Execute(api.getTodos (), GetTodos) // don't handle server exceptions
Cmd.ofAsync.Execute(api.getTodos (), GetTodos, HandleServerException) // also valid

[isaacabraham]

I actually like the function and arguments being separate passed separately - it feels like I'm telling Elmish "here are the ingredients you need to make an async when you're ready" and I know that the async won't be started immediately. If the async is awaiting a task then it could otherwise be triggered immediately. IMO, that's a subtlety that will be much harder for newcomers to grok than passing functions and arguments separately.

I know that is a subjective thing - my opinion isn't the "one true way" :) I'm not convinced though that the current API can't be improved but perhaps I'm in the minority here :) My main concern is that outside of Elmish, this pattern of "pass a function and then pass its arguments separately" is very, very unusual - the only ways I've really seen this done in the past is with a thunk e.g. (fun () -> api.GetTodos()) but in the case of Async this isn't even needed since it's a cold computation. I'm not sure about the Task stuff either - there are no Tasks in JS land, and even if it did, wouldn't that only apply if you started the task outside of the Async block?

I'm really just focusing of the use case for 80-90% of the time: "I have an API call that I want to make and I want the result to be pushed into this Elmish message when it returns" and optimising that as much as possible. Perhaps that's not the right way to go about this though...

[mattgallagher92]

Yeah, you'd need the task to be started (hence created) separately for the hot start to be a problem, for example someTask |> Async.AwaitTask. As you say, given we're working in the browser, maybe that's not actually something that we need to worry about - it's presumably all promises that cold start? My JS knowledge is a bit hazy here tbh. Let's set that aside for the moment and assume that it's fine.

I get what you're saying with it being unusual. And I was thinking that people might need to use the old API a lot, which would significantly weaken the case for the new API (and provide an argument against it), but on reflection maybe that's not true. I think that there probably is a good case for something like the change that you're proposing.

I think most of my remaining reservation comes from the fact that the Finished part of the ApiCall is hidden in the function called from the Start handler. There's a weird asymmetry there that makes it feel like we're not working completely at either the Async level of abstraction or the ApiCall level. How about we just use asyncs and don't do any ApiCall stuff at all in the background?

Suggested change

	let loadTodosCmd = todosApi.getTodos () |> Async.toCmdUnsafe LoadTodos
	let loadTodosCmd = todosApi.getTodos () |> Async.toCmdUnsafe (Finished >> LoadTodos)

I'm okay with that and it doesn't separate the argument and function, which seems to be the main motivator. Then I think the last thing we'd need to do would be to settle on an appropriate name and module.

[MangelMaxime]

To highlight that the names could be improved, these are the wrong way round.

1. Do you need to be notified on success and don't care about error? Cmd.attempt
2. Do you need to be notified only for errors? Cmd.perform

I knew I was going to mess them up and had the feeling I did invert them and didn't check. TBH fair, you would have an IDE to write the code. 😝

Maxime, you can see in the implementation code that this is wrapping a successful response in the Finished case of the ApiCall type.

I see, personally I am not convinced that having a nested structure in the Msg make it better than 4 flat Msg:

type Msg =
	| LoadTodo of string
	| TodosReceived of Todo list
	| SaveTodo of string
	| TodoSaved of Todo

I never used this kind of Msg structure, so it is also possible that I am also in the resisting to changement phase.

In general, I teach people that one action = one message.

However, having a similar structure in the model allows for some easy way to handle ressources that needs to be fetched.

[<RequireQualifiedAccess>]
type Deferred<'T> =
    | Resolved of 'T
    | InProgress

type Model =
	{
		Todos: Deferred<Todo list>
	}

type Msg =
	| TodosReceived of Todo list

let init userId =
	{
		Todos = Deferred.InProgress
	}, 
	Cmd.OfPromise.perform fetchTodos userId TodosReceived

let update msg model =
	match msg with
	| TodosReceived todos ->
		{ model with
			Todos = Deferrend.Resolved todos
		}, 
		Cmd.none

let view model dispatch =
	match model.Todos with
	| Deferred.InProgress ->
		Html.div "Loading..."
	| Deferred.Resolved todos ->
		renderTodoList todos

Warning

Typos possible in the code, I wrote it from memory and Github directly 🙈

[mattgallagher92]

Thanks Maxime 🙂 The point about separate messages is an interesting one - I like the clarity you get that the two messages are linked, and it potentially halves the number of cases in Msg, which can be significant, but the flat Msg structure is a little bit simpler. I don't see a strong argument either way, so happy to leave it as is for now.

Isaac, as another data point, I discussed your proposed change with @Larocceau who said that he didn't have any trouble with Cmd.OfAsync.perform when first encountering it as an F# novice - checking the type signature was enough to quickly work out what was going on. On that basis, I don't really see how the slightly unconventional nature is a problem - new F# devs don't have expectations and experienced F# devs can check the signature and easily understand what's necessary.

[mattgallagher92]

The test change is a must and I think that the others are worth discussion.