Extend Command to handle Tasks

[jamesmontemagno]

I have updated my issue here to be a bit more verbose.

Description

Today, the Xamarin.Forms implementation of ICommand only takes in a Action as the method to execute:

public ICommand UpdateLocationCommand { get; }

 public UpdateLocationViewModel()
 {
     UpdateLocationCommand = new Command(ExecuteUpdateLocationCommand);
}

void ExecuteUpdateLocationCommand()
{
}

This works great when we are only working with simple methods that don't have to worry about async/await and Tasked based development. Once this comes into play as a developer we create some testable code and update the return value to a Task:

async Task  ExecuteUpdateLocationCommand()
{
   await Task.Delay(500);//any async thing
}

However, now that we have testable Task based code we can no longer use the constructor that exists and have to leverage hacky workarounds to execute our code:

 public UpdateLocationViewModel()
{
    UpdateLocationCommand = new Command(async () => await ExecuteUpdateLocationCommand());
}

The request here is to either enhance the existing Command that exists inside of Xamarin.Forms to handle this situation. Yes, I know that ICommands are events and we shouldn't do this stuff, but we have to in this day and age of development to be honest. This will help on-boarding when writing such code and seems to be what 80% of my Commands do today to be honest.

If we do not wish to modify the existing command we could introduce a new AsyncCommand, but it should still implement ICommand.

Discuss below.

[MKuckert]

The Mvx implementation is pretty good with support for Cancellation and locking against multiple calls of the same Command

[andreinitescu]

Just my 2 cents: it's easy to add an utility class class which creates an ICommand from a Task:
public static ICommand CreateCommand(Task t) { return new Command(async () => await t); }
The actual issue is if you want to have cancellable commands. But do you need it?

[Osmosis311]

@andreinitescu can you provide a more complete example of that? :-) . Thanks!

[andreinitescu]

@Osmosis311

        Task DoItAsync()
        {
            return Task.Delay(1);
        }
        static class CmdHelper
        {
            public static ICommand CreateCommand(Func<Task> t) { return new Command(async () => await t()); }
        }

       ICommand UpdateLocationCommand  => CmdHelper.CreateCommand(DoItAsync);

The implementation within t should make sure it is catching all exceptions.

[CZEMacLeod]

The NuGet package Nito.Mvvm.Async supports this too - Mvvm.Async
It is NetStandard based and works very well. It also supports binding to the command to make progress observable etc. @StephenCleary

using Nito.Mvvm;
Task DoItAsync(object arg)
{
    return Task.Delay(1);
}
ICommand UpdateLocationCommand  => new AsyncCommand(DoItAsync);

[brianlagunas]

I think this is something best left out of Xamarin.Forms. An "async command" is misleading at best. In fact it gives you a false impression that you did your code correctly, while actually it provides you no guarantees. With an async command you start some asynchronous process in your application which executes independently of the main (GUI) thread of the program. This is no different from starting a Task in a fully synchronous IComman.Execute method. How this asynchronous process interacts with the rest of the program is fully dependent on the particular details of the methods and data structures and the async command cannot add anything to it.

This will also create many confusing issues in unit tests depending on if you defined the command being tested as ICommand commandTotest or AsynCommand commandToTest. await commandToTest.ExecuteAsync(); in a test waits for the command to complete while ICommand.Execute in a real XAML application does not.

ICommand methods should be considered events (even MS docs say so). The elements that invoke a command will not invoke them in an async manner nor await their results. You will be writing much uglier code trying to implement an "async command' than either having an asyc void, or calling await in the delegate declaration itself.

It is very characteristic what @StephenCleary ended up with in his "Patterns for Asynchronous MVVM Applications: Commands" blog post:

The new UI will change the Go button to mean “start a new asynchronous operation and add it to the list of operations.” What this means is that the Go button is now actually synchronous.

Actually the positive value of Mr. Cleary's approach is observable (bindable) Task properties, and this is not the same as async command per se.

This is a much deeper conversation than just "add an async command". You're asking for a support nightmare if you add this to the Xamarin.Forms codebase.

My 2 cents

[andreinitescu]

@brianlagunas well said. My example above was just for the sake of it. I personally never quite understood the reason for async commands

[jassmith]

This doesn't really belong in core but in a community project. Forms does not ship an MVVM toolkit, but instead strives to work with MVVM toolkits in general.

[brianlagunas]

@jassmith agreed, and there are already a number of OSS projects that have this. Nito.Mvvm.Async being a great option.

[jamesmontemagno]

I have re-phrased my original proposal and idea here a bit more.

[brianlagunas]

@jamesmontemagno You definitely can't modify the existing Command to implement async calls. This is definitely a no go. If you did this, then the ICommand.Execute would be an async void in order to call your actual Task delegate. That means you would break every single Command implementation out there. To illustrate this, this test that should pass would actually fail:

        [Fact]
        public void Test_should_fail_because_of_thrown_exception()
        {
            Assert.Throws<Exception>(() =>
            {
                new Command(() => { throw new Exception(); }).Execute();
            });
        }

That test validates that trying to extend Command to execute a Task when calling ICommand.Execute is not going to be an option.

Essentially, what it looks lie you want to do, is make the COmmand API somewhat easier so that you can just supply a Task as a deegate and just have it work as if you had an async void method or by calling () => await delagate. Essentially you would have a command like this:

public class AsyncCommand<T> : Command<T>
{
    public AsyncCommand(Func<T, Task> executeMethod, Func<T, bool> canExecuteMethod)
        : base(async (o) =>  await executeMethod(o), (o) => canExecuteMethod(o));
}

Would that be a fair representation?

[jamesmontemagno]

I do believe that this is closer to what I am looking for, however if we do implement a form of this AsyncCommand is it possible to improve this type of experience. Does the AsyncCommand need to handle things better async type of stuff?

Or is what we are saying is the way to do it totally is:

UpdateLocationCommand = new Command(async () => await ExecuteUpdateLocationCommand());

But with this helper command we would just make the API prettier and not change the functionality at all?

[brianlagunas]

I think if you were to provide a class called AsyncCommand, you would inherently open it up for many more async type features such a cancelling, possible dispatching of data from one thread to the UI, determining task completion, exception handling, managing what happens if the button is clicked more than once, etc. At that point, your at some fully baked async command infrastructure like Mr. Cleary's.

From my point of view, having to write 17 more characters in order to have a Task based Execute method is not a show stopper considering the possible side-effects of having a separate AsyncCommand implementation. As I think about code sharing across platforms, being able to rely on ICommand.Execute behaving in a predictable and consistent way is extremely important.

For what it's worth, Prism's guidance has been your supplied code snippet, or to have an async void Execute method, ever since we dropped our AsyncCommand implementation. ICommand methods are viewed as events, and having async void on an event handler is a required practice.

[ghuntley]

Don't. No. Really bad idea. Especially in the core project.

[lightwaver]

We also use a AsyncCommand for correctly handling long taking actions with a common errorhandling.
And our Command implementation also blocks the concurrent invocation by using the CanExecute property and thats the most important feature for us.
It cleans up the ViewModel code and makes it more stable.
I personally like a common exception handling for not expected exceptions to prevent a app from dying totally...

[andreinitescu]

@lightwaver Can you give an example what you use the async command for? Do you bind a Button's Command to an async command for example? Because in this situation the async doesn't help, XF just calls Execute on the command, which is not async.
An async command makes sense when the caller knows about async and awaits on the ExecuteAync or CanExecuteAsync. Which means that's not just an ICommand anymore.

[StephenCleary]

I actually agree with keeping this out of Xamarin.Forms, too. One of the problems with AsyncCommand is that it can mean a lot of different things to different people projects, and there isn't a consensus yet around what the desired API looks like. I have (breaking) updates planned for mine in Nito.Mvvm.Async; it's very much a pattern in flux, even after several years.

[aleksandrsmyk]

Asynchronous Commands can not be implemented as a stable feature from Xamarin Forms team, until a lot of asynchronous support will be provided from the .Net.

[lightwaver]

@andreinitescu We a common implementation, that allowes to execute async calls (most of the times REST-calls to the backend) that are not directly transparent to the ViewModel, because it's wrapped through business logic classes. To have a easy and common binding and exeption handling we created a AsyncCommand that implements ICommand.

It simply sets the CanExecute to false (and calls the CanExecuteChanged event) before executing the Task and when the Task is Finished it re-enables CanExecuteAgain.

I attached an simple implementation of it.

Another more App specific Version also invokes a CommonBusyIndicator.

AsyncCommand.cs.txt

Its a very simple version of it but it works :) - for most of the cases.
I know you need to be careful with async things - but well "with great power comes great responsibility" ;)

And if you implement async/multithreaded things without care - you will end up in weird exceptions and locks...

[programmation]

The basic problem with async commands in current .NET is that you have to start them at some point with an async void function call. If I have understood things correctly, I think this means exceptions won't pass back to the caller across the async void invocation, but instead risk taking down the entire application.
Having read dotnet/roslyn#13897 I wonder if (for Xamarin.Forms purposes at least) it would be sufficient for an AsyncCommand to wrap the internal command in a try...catch block and have a callback lambda registered from the caller where it can post any exception - perhaps having a default global exception handler if nothing is registered.
If that description isn't clear I've modified @lightwaver 's example to show what I mean.
AsyncCommandWithExceptionCallback.cs.txt
This is one of those areas where I think it would benefit all of us for Xamarin.Forms to have more MVVM stuff in it. It already has a ICommand implementation - so there's at least some MVVM already there and a precedent to follow.

[lightwaver]

Yes that's what I meant...
the very often used lambda in the code when creating the Command nearly never implements a correct try catch block and that stays there even if the code behind gets more complex over time.
And the try...catch blocks including logging and all that stuff bloats up the code to get unreadable and copy/paste errors happen
-> no clean code, faulty exception handling and all the ugly things.
I still think it is a much prettier solution to create a async command to handle that.

[pictos]

@jamesmontemagno i don't know if this solve 100% of your problem... But you can extend the Command and do this:

https://github.com/pictos/AsyncCommand/blob/master/Asc/AsC/Helpers/AsyncCommand.cs

I took the implementation of the hotel as a reference. So I can call a task in Command like that:

MyCommand = new AsyncCommand(ExecuteMyCommand);

[StephaneDelcroix]

@brianlagunas points (
#1857 (comment) and
#1857 (comment)) are very well articulated and reflects the opinion of the team on that matter.

[ghuntley]

This is a good outcome.

[lightwaver]

Yippie... when will this feature be released ?

[brianlagunas]

@lightwaver It's not. It was closed becuase it will not be implemented.

[lightwaver]

Ok, sorry was a bit confused by the baclog and done state ;)
I'm also fine with it since we've now integrated a (for us) common way to deal with it.
I still think a more broad "start point" for new devs would be quite nice, because if you need a more specific Command-Implemetation - you can just do it...
Many devs i talked to (with a not very deep understanding of async) lost much time in searching for bugs, caused by a not clean async-handling in inline created Commands.