Remove DelegateCommand.FromAsyncHanlder #785

Closed
brianlagunas opened this Issue Oct 3, 2016 · 83 comments

Projects

None yet

9 participants

@brianlagunas
Member

The current implementation of DelegateCommand is broken because of the newest FromAsyncHandler feature that was added by Microsoft prior to Prism being open sourced. Any exception thrown in a command is swallowed. For example, this test fails:

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

This is because of the way the DelegateCommand is implemented to support an async scenario. The problem is that ICommand is not called async by any of the XAML frameworks, and having an async command provides no value unless you are calling your command in code (which should be rare).

  • Step one is marking it Obsolete, and releasing an update to NuGet.
  • Step two is fixing the implementation by removing async command stuff, and to refactor DelegateCommand.
  • Step three is push new version 6.3 to nugget mid next year.

This will be a massive breaking change to anyone using FromAsyncHandler, but it is necessary.

@brianlagunas
Member

Please review this branch to make sure I didn't miss anything. I tried to simplify the DelegateCommand without breaking anything (except for the async stuff)

https://github.com/PrismLibrary/Prism/tree/DelegateCommand-Improvements/Source/Prism/Commands

@dvorn
Contributor
dvorn commented Oct 5, 2016

https://github.com/PrismLibrary/Prism/blob/DelegateCommand-Improvements/Source/Prism/Commands/DelegateCommandBase.cs#L27:

protected readonly Func<object, Task> _executeMethod;

Tasks are not needed anymore; _executeMethod should be Action<object>.

@dvorn
Contributor
dvorn commented Oct 5, 2016

This implementation does not provide good experience if someone wants to override the execute method (e.g., to add logging). I would expect to have public virtual void Execute(); in DelegateCommand and public virtual void Execute(T parameter); in DelegateCommand<T>. Those method should be called when a command is invoked by XAML framework via ICommand.

@brianlagunas
Member

@dvorn good catch :)

Just so you know, Execute(T parmater) is only used when you call the command in code against the DelegateCommand type. When XAML invokes the command, it will always use ICommand.Execute(object parameter). The framework will never directly call the Execute(T parameter) method. That's the reason I removed those methods. I wanted to create consistency in the API. I could add tem back it's no big deal. If I do that, if someone declares DelegateCommand MyCommand, and called MyCommand.Execute() somewhere in code, then changes the command type to ICommand (ICommand MyCommand), it will be broken.

@dvorn
Contributor
dvorn commented Oct 5, 2016 edited

Just so you know, Execute(T parmater) is only used when you call the command in code against the DelegateCommand type.

This is only in Prism 5 incorrect implementation.

One of possible implementations:

void ICommand.Execute(object parameter)
{
    this.Execute((T)parameter);
}

public virtual void Execute(T parameter)
{
    base.Execute(parameter);
}
@brianlagunas
Member

What would be the purpose of that? Regardless, the XAML framework is still only calling Execute(object param), while the developer would be calling DelegateCommand.Execute(T parameter). Then, base.Execute is still another ICommand implementation. I guess I'm not seeing the benefit.

@dvorn
Contributor
dvorn commented Oct 5, 2016

The benefit? It solves the problem. You override Execute(T) in the derived class, and both XAML and developer are executing the same method.

@dvorn
Contributor
dvorn commented Oct 5, 2016 edited

Suppose you have

class MyCommand<T> : DelegateCommand<T>
{
    public override Execute(T parameter)
    {
        base.Execute(parameter);
        // additional code
    }
}

XAML framework will call DelegateCommand.ICommand.Execute and then:
MyCommand.Execute
DelegateCommand.Execute(T)
DelegateCommandBase.Execute(object)
_executeMethod
// additional code

@brianlagunas
Member

Based on what is there now, there is only a single Execute method that is called by both XAML and code which is the same.

public virtual void Execute(object parameter = null)
{
    _executeMethod(parameter);
}

If you override this method in your custom command, it gives you the exact same behavior, minus the strongly type parameter. That is what I was meaning about the benefit of doing what you have vs. what's there. It is the strongly typed parameter that you want?

@dvorn
Contributor
dvorn commented Oct 5, 2016

Exactly. Or the lack of parameter. In the present form it is doable but not obvious. This is what I called "not a good experience".

@brianlagunas
Member

I don't see a problem with adding the strongly typed parameter as you have it. As long as it behaves the exact same when XAML or code calls it.

@brianlagunas
Member
brianlagunas commented Oct 5, 2016 edited

By doing this, we now have a slightly confusing API. We have two overloads of Execute and CanExecute.

In the case of DelegateCommand<string>

Execute(object parameter)
Execute(string parameter)

In the case of DelegateCommand

Execute()
Execute(object parameter)

Not really sure this helps a whole lot. Maybe I am missing something.

@brianlagunas
Member
brianlagunas commented Oct 5, 2016 edited

Execute(T parameter) is just a helper method for strongly typed parameters when calling in code so you don't pass an invalid type. XAML doesn't care about this and only uses Execute(object parameter). If you override Execute(object parameter) you are not blocked from adding any custom logic in your derived command. I pushed an update to the branch. Check it out. I cleaned the API up and not show the duplicate Execute/CanExecute methods (made them protected).

@dvorn
Contributor
dvorn commented Oct 5, 2016 edited

You are right. Moreover, there is a corner case of DelegateCommand<object>. Also, my sample assumes that DelegateCommand and DelegateCommand<T> both explicitly implement ICommand.Execute which I do not like.

Actually, this was "one of possible implementation" which was shorter to type. Real implementation could be

public class DelegateCommandBase : ICommand
{
    ICommand.Execute(object parameter)
    {
        OnExecute(parameter);
    }
    protected virtual void OnExecute(object parameter)
    {
        Doexecute(parameter);
    }
    protected void DoExecute(object parameter)
    {
        _executeMethod(parameter);
    }
}
public class DelegateCommand<T> : DelegateCommandBase
{
    protected override OnExecute(object parameter)
    {
        Execute((T) parameter);
    }
    public virtual Execute(T parameter)
    {
        DoExecute(parameter);
    }
}
public class MyCommand<T> : DelegateCommand<T>
{
    public override Execute(T parameter)
    {
        base.Execute(parameter);
        // extra stuff
    }
}
@dvorn
Contributor
dvorn commented Oct 5, 2016 edited

I pushed an update to the branch. Check it out.

You ended up in a variant they had in Prism 4. These commands are not overridable in a sense that you cannot override Execute(T parameter), and overriding Execute(object parameter) (especially for parameter-less command) in not obvious (poor experience).

@dvorn
Contributor
dvorn commented Oct 5, 2016

And I doubt that it will ever work: when you call base.Execute(parameter) from the DelegateCommand, will it execute the override of the Execute method in the derived class?

@brianlagunas
Member

Crap, you're right! I didn't think about that :)

@brianlagunas
Member

I didn't realize how ugly this DelegateCommand was :)

@brianlagunas
Member

You know... if we didn't care about that silly strongly typed parameter this would all be a non-issue. Is it really that important? I don't see any other command that implements it like that (MVVMLight, etc).

@brianlagunas
Member

Also, as a side effect, when trying to override the Execute(T parameter) method using intellisense, a Visual Studio exception is raised complaining of duplicate methods; because of the Execute(object parameter) in the base class. I would argue this is a worse experience.

@brianlagunas
Member

And, if a developer overrides OnExecute, it will never fire when calling DelegateCommand.Execute in code. That will only invoke with a XAML invocation, leading to another poor/confusing experience.

@dvorn
Contributor
dvorn commented Oct 6, 2016 edited

After some more thinking...

We already decided that async was a bad idea.

Now I think that the entire concept of overridable Execute method is conceptually wrong.

When you create a DelegateCommand you provide a delegate which contains the code that needs to be executed. Why didn't you put all you need there? The delegate command isn't something (a framework) that is executing its own code and providing you a hook for modifications. DelegateCommand does just that - executes your delegate.

In a class derived from the DelegateCommand you do not need two extension points - one in the base constructor parameters, and the other being overridable Execute method. If you have the two, a question arises how they should relate to each other.

Everything you would write in the overridden Execute method you could as well write in the constructor parameter delegate.

If you need some extra functionality common to all your commands, you can go something like this:

public class MyCommand<T> : DelegateCommand<T>
{
    private bool isRunning;
    public MyCommand(Action<T> action)
        : base((o) =>
            {
                 isRunning = true;
                 action(o);
                 isRunning = false;
            });
}

Thus, I suggest to remove "virtual" from the Execute method in DelegateCommandBase, essentially returning to Prism 4 variant.

@dvorn
Contributor
dvorn commented Oct 6, 2016

Note also that developers (@nickkazepis) ended up in exactly this approach: #746 (comment)

@dvorn
Contributor
dvorn commented Oct 6, 2016

Complete sample:

public class MyCommand<T> : DelegateCommand<T>
{
    private bool isRunning;
    public MyCommand(Func<T, Task> executeMethod, Func<T, bool> canExecuteMethod)
        : base(async (o) =>
            {
                 isRunning = true;
                 await executeMethod(o);
                 isRunning = false;
            },
            (o) => !isRunning && canExecuteMethod(o));
}
@brianlagunas
Member

I am actually leaning towards sealing the DelegateCommand and DelegateCommand<T> classes. Thus, any custom command should derive from DelegateCommandBase. See the latest update.

@dvorn
Contributor
dvorn commented Oct 19, 2016 edited

@brianlagunas But DelegateCommandBase is not overridable now. Having DelegateCommand sealed will make creating custom commands more difficult than it could be.

@brianlagunas
Member

All other commands are sealed, and probably for the same reason. Creating a custom command that derives from DelegateCommandBase is not difficult, and it would prevent all the issues we are trying to avoid. You get your strongly typed Execute/CanExecute :)

@dvorn
Contributor
dvorn commented Oct 20, 2016

But then if you want to create a custom command you'll need to repeat all the stuff that already is in DelegateCommand: casting of object parameters to T in constructors and, in particular, dealing with T which is a value type. It's doable but not a great experience.

@brianlagunas
Member

We can't make everyone happy. You can choose between loosing your Execute<T> which is a bad experience (and hasn't been fully tested yet), or losing the ability to derive from DelegateCommand<T> (that every other command does) which is a bad experience. Either way, someone is getting a bad experience.

@hardcodet

I'm not sure DelegateCommand is broken because the synchronous execution method doesn't catch a async event. Of course it doesn't (and it shouldn't!). In the end, ICommandis trival, and DelegateCommandis supposed to make stuff easy for the developer to handle all those common cases. Just as much as I want DelegateCommand<T>(and don't care about corner cases where a cast may not be working - it's never been an issue for me in reality), I just want it to simplify the execution of async code.

As such, I accept the responsibility of having to handle exceptions in my command handlers - so async voidwould do. However, in case of an unexpected exception (for other developers - I like never ever write buggy code of course ;), it would be nice if the framework would support me. That may go as easily as setting up something static like this:

DelegateCommand..BubbleAsyncErrors(failedTask => { myLogger.LogException(t.Exception));

You could also bake that into a command instance (with potential fallback to a global exception handler):

ICommand cmd = DelegateCommand.Create(OneAsync, HandleUnexpectedError);
ICommand cmd = DelegateCommand.Create<string>(TwoAsync, HandleUnexpectedError);

private async Task OneAsync() { ... }
private async Task<string> TwoAsync() { ... }
private void HandleUnexpectedError(Exception e) { /* display generic error on UI or smth /* }

Of course, that just helps me to avoid writing repetitive boilerplate code. But that's what DelegateCommandis IMO.

(OTOH, one could argue that baking your custom delegate command like this should be fairly easy, too, even on top of the existing simpler model.)

Just my 0.02$
Philipp

@brianlagunas
Member
brianlagunas commented Oct 27, 2016 edited

@hardcodet No, you're missing the point. There is no synchronous execution. Execute is always async and needs to be awaited, even when you are writing synchronous code. Synchronous exceptions are swallowed, and ICommand.Execute is not async, but DeleateCommand hides this implementation and makes it async. It's broken.

@pdinnissen
Contributor
pdinnissen commented Oct 29, 2016 edited

Looking at the original source code, the swallowing of exceptions is due to the 'async void Execute', which is always to be avoided but can't be in this case due to ICommand interface not being modifiable (I really wish WPF had been updated to be more async friendly). Technically exceptions are still thrown directly on the original SynchronizationContext. https://msdn.microsoft.com/en-us/magazine/jj991977.aspx

That being said, I agree that the current implementation is broken and needs to be addressed. But unless I'm mistaken, won't long running DelegateCommand.Execute functions still block the UI thread? Which is why they implemented this way in the first place. Would it make sense to clean up DelegateCommand, but also introduce a new DelegatCommandAsync? Based on:
https://msdn.microsoft.com/en-us/magazine/jj991977.aspx

I know I could extend DelegeCommandBase in the future for my own projects, but I believe others would appreciate it in the Prism library as well. I'm more than willing to put together a PR, but would like to get some feedback before wasting anyone's time reviewing it.

@brianlagunas
Member

Long running Execute functions should be placed on separate thread as to not block the UI. I will not be adding an async command to Prism at this time.

@pdinnissen
Contributor

Understood. But isn't that what async/await does without the mess of spawning threads? I don't want to sidetrack this discussion, so I will drop this and concede that there are significant pitfalls to C#'s async/await framework when mixing it with synchronous code. Regardless, I agree that DelegateCommand should be cleaned first and extended second.

Would you consider a DelegateCommandAsync in a 6.4 release?

@brianlagunas
Member

Honestly, I don't think an async command will ever make it into Prism. After DelegateCommand is fixed, I will have to get a pulse from the community about it. To me, it doesn't make sense.

@pdinnissen
Contributor

I just caught up on the other discussion threads. So I see what your wrestling with. That seems like a fair course of action. And hey, the great thing about this framework is that there's nothing stopping me from using my own DelegateCommandAsync in my apps in the meantime.

Thanks for all the work you do on this project. It's a lifesaver.

@Sockenfresser

I don't think that the Problem of the DelegateCommand is the existence of the FromAsyncHandler feature. The actual problem is that the non-async actions are wrapped in an async delegate which doesn't make much sense.
Instead the async and the non-async handlers should be handled individually. This way the non-async actions would work as expected with proper exception behaviour. The async handlers would still "swallow" the exception (actually they are throwing them in the original SynchronizationContext) which is the expected behaviour for a "async void" call.
We are strongly using this async features, as most of our code is using async/await. The async behaviour of the DelegateCommand makes it much easier to test our view models.
In the tests we are triggering the Task Execute() methods. This way it is very simple to await the execution of the async operation before we do the assert. So there actually is a scenario where you are calling the Execute method in you code. For us this is within test code.
For us it would be the best to split the DelegateCommand into a DelegateCommand and a DelegateCommandAsync. This way existing users of the async features could keep their stuff running. What do you think?

@dvorn
Contributor
dvorn commented Nov 2, 2016 edited

@Sockenfresser As I mentioned in another thread, the notion of async command is misleading. In fact it gives you a (false) impression that you did your code correctly, while actually it provided 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.

When you do an await MyCommand.Execute() in your test, generally speaking you are doing not the same thing as the real program does: the real program will not do an await. The GUI will run further immediately after Execute() was called. You may assert the correctness of the program state after the await and it may be OK, but in a real program the GUI or other threads in the program may access the program state before the async command completed. Thus, these tests gives you a false impression that you tested it correctly. Sometimes, yes, the tests are valid, but there may be a lot of situation where they are not or not cover all the possible cases of program behavior.

@pdinnissen
Contributor
pdinnissen commented Nov 2, 2016 edited

@Sockenfresser I agree, since our projects also primarily use the FromAsyncHandler feature for initializing DelegateCommands. @brianlagunas has outlined a good path. First have desired/expected behaviour for purely synchronous commands, then gauge community desire/requirement for an additional DelegateCommandAsync (especially since there's nothing stopping anyone from just using their own implementations of an async ICommand implementation). In other related threads, others (including @dvorn) have already suggested certain implementations. #746 #745 #724. Quite a few WPF related issues and pull requests are geared towards having Prism being more fully async/await compatible, I have a feeling that the community will keep requesting this feature, but hopefully we can come up with something that provides intended behaviour.

@dvorn could you give an example where it is bad that the GUI or other threads access the program state before the async command completes? If important can they not check the IActiveAware.IsActive property? At least from the Prism side of things?

async void is the recommended way to have Event Handlers be asynchronous and ICommand is often referred as being like an Event Handler.

@dvorn
Contributor
dvorn commented Nov 2, 2016 edited

@pdinnissen Suppose you have a button that is called "Update" which retrieves some data from a web service, and it is implemented as async method. In real program you may want to disable the Update button while the download is in progress; also there may be another "Save" button which also needs to be disabled while downloading.

Running await UpdateCommand.Execute(); in a test is not enough.

@Sockenfresser
Sockenfresser commented Nov 2, 2016 edited

@dvorn In fact the implementation of the DelegateCommand does exactly this, an await this.Execute(parameter); so what I'm testing is exactly what the UI triggers here.
The actual state of my application is altered by the command handlers or code in the layers below. So why is this different to calling it synchronously?
Your scenario is plain obvious for all asynchronous event or command handling applications. You do this anyway if you do everything async. And you would write an additional test for this behaviour I guess.

@pdinnissen I just wanted to raise my voice here and say "I'm using this and it would break my code if you remove it without a replacement!". @brianlagunas asked for feedback in his blog post. I couldn't find a post in this thread pointing out the test scenario as a use case. So I felt I had to say, that there are people using this and there is a use case for directly calling DelegateCommand.Execute.
My problem is, if you first remove the asnyc stuff, I will have to create my own DelegateCommandAsync implementation, because nearly all my view models and their tests are working this way.
If you later ask who needs this, I will probably not raise my hand, because I will have a working solution by then. And I guess anyone with the same usage of the DelegateCommand will have one too. So why not split it up into two a synchronous and an asynchronous implementation now?

After reading the whole blog post and the answers to my post again, I'm not even sure what is wrong with the old behaviour. The unit test from the blog post is simply not 100% correct because what a test has to do, is to set up the environment correctly. The normal non-test environment for a DelgateCommand has a SynchronizationContext set. The test has non.
Additionally the test calls the Task Execute() overload. This results in the Task to be thrown away, which is just wrong in this scenario.

If I write the test (using AsyncContext.Run from AsyncEx to set the SynchronizationContext) as follows, it does not fail.

using System;
using System.Windows.Input;
using Nito.AsyncEx;
using Prism.Commands;
using Xunit;

namespace DelegateCommandTests
{
    public class ViewModelTest
    {
        [Fact]
        public void DelegateCommand_ActionNotAsyncWithSyncContext_ExceptionThrown()
        {
            var exception = Record.Exception(() =>
            {
                // set up the 
                AsyncContext.Run(() =>
                {
                    ((ICommand)(new DelegateCommand(() => { throw new Exception(); }))).Execute(null);
                });
            });

            Assert.NotNull(exception);
        }
    }
}

So I understand that the behaviour is not really obvious. But if the people using it understand the idea of async/await in UI scenarios, the existing DelegateCommand works like a charm. In my opinion there is no exception swallowed in real world scenarios.

@dvorn
Contributor
dvorn commented Nov 2, 2016

@Sockenfresser

In fact the implementation of the DelegateCommand does exactly this, an await this.Execute(parameter); so what I'm testing is exactly what the UI triggers here.

No, not the same. await command.Execute(); in a test waits for the command to complete while ICommand.Execute in a real XAML application does not (although internally it does await Execute();, but in an async void method).

@Sockenfresser
Sockenfresser commented Nov 2, 2016 edited

@dvorn Just to summarize. I have a behaviour in my command handler. I want to test it's complete behaviour. It is an async call, so in the test I await this call to assert after the code has run.
In the application I obviously do not wait for the task to finish. But I want to trigger the same behaviour. Of course I build my UI in a way that reflects this asynchronism. I mean that is the whole idea behind asynchronously executing things triggered by the UI. But still the behaviour I want to test is in most cases the behaviour of the complete command handler.
There are some rare cases where I want to test for instance the state after a request is send but before the result is received. For obvious reasons I do not await under these circumstances. But I guess this is slightly off-topic here.

@dvorn
Contributor
dvorn commented Nov 2, 2016

@Sockenfresser I am not saying that you cannot use Task Execute() in tests for good... I am saying that there are better ways.

An async command starts some parallel process in your application, and the application does not actually wait for the async task to complete. If it ever waits, it waits for something different. In many cases this will be some other synchronization objects (and not the Task of the command) that is used to synchronize the process with the rest of the application.

In my example of Update command above this could be some IsUpdating property, with an event for changes of this property (INotifyPropertyChanged or other). It will be much more straightforward and clean in a test to execute a command and then wait for the IsUpdating property to become false as an indication that the command completed.

Having Task Execute() and using it in tests allows you to cut the corners of the real world synchronization of your asynchronous process and the rest of the program.

I personally do not think that Prism should encourage this. Thus I vote against Task Execute method for tests.

@Sockenfresser
Sockenfresser commented Nov 2, 2016 edited

@dvorn OK understood, but this makes most tests a lot more complicated and does not achieve much in our use cases.

@brianlagunas I still don't get what is so bad about the DelegateCommand it it's current state. It fits pretty well into the asynchronous pattern and the test that shows that it has a bug regarding exception handling is wrong in it's posted form, as you are lacking the SynchronizationContext and the returned Task is ignored.
Plus the exception behaviour in a running application is the same as in a synchronous version. So the DelegateCommand is working as it is.
And the async void isn't a code smell in this scenario. In my opinion the ICommand is just a bindable event replacement and async void calls are completely legal for UI event handlers.
So why really remove it? I understand @dvorn about having a problem with using Task Execute() for tests but I still think he is a bit to strict about this.

By the way, I really appreciate the work all of you are doing here. I don't want to get in the way of your work.
But a breaking change is a really tough decision and for me, as for many others this would just lead to implementing their own asynchronous DelegateCommand having exactly the interface the old one has.

@brianlagunas
Member

@Sockenfresser

test that shows that it has a bug regarding exception handling is wrong in it's posted form

No, it's not. DelegateCommand has been functioning as synchronous since it was first created. When the async stuff was added it broke every single dev using DelegateCommand and they didn't even know it. DelegateCommand is a primarily sync command, as is ICommand in general. Sync code is broken with the current implementation. In your modified test, you had to add an async context to make it work. This is wrong. This should not be needed for a default sync command. There should be no consideration of handling a Task, and the XAML framework doesn't even consider a Task. Right now, DelegateCommand behaves differently depending on if it was called from XAML or in code. Its behavior is inconsistent. Even if you call it in code, if your property is defined as ICommand and not DelegateCommand, it behaves differently. FromAsyncHandler is misleading, does not work as expected, breaks sync code, creates an inconsistent and confusing API as well as behavior.

ICommand is not async, and is never called as async. Trying to make it behave as async makes no sense just for the sake of supporting the async/await feature, and introduces nothing but problems. There is a reason that no other framework provides an async command. Commands are not meant to behave async. If you need to use async/await in a command delegate, you would just use async void as you would with am event handler.

@pdinnissen
Contributor

@Sockenfresser I agree since that's what I've been doing today. But merging it with: https://github.com/StephenCleary/Mvvm.Async/blob/master/src/Nito.Mvvm.Async/AsyncCommand.cs
(I would just use his, but I love the ObservesProperty function too much.)

@dvorn In that scenario, wouldn't it be easy to just have a boolean representing whether the command is active (I presume that's what the IActiveAware is for, although looking at the DelegateCommand code, I don't see where is ever changes). And then have the CanExecute of both those Commands reference that value. In fact, isn't that exactly what you kind of proposed earlier in this thread?

public class MyCommand<T> : DelegateCommand<T>
{
    private bool isRunning;
    public MyCommand(Func<T, Task> executeMethod, Func<T, bool> canExecuteMethod)
        : base(async (o) =>
            {
                 isRunning = true;
                 await executeMethod(o);
                 isRunning = false;
            },
            (o) => !isRunning && canExecuteMethod(o));
}

Except 'isRunning' would be a MVVM property and MyCommand would implement INotifyPropertyChanged.

And isn't the UI blocking on a long running command almost as bad? And launching a separate thread is essentially the same thing as async/await? It's not like you could catch the exception from that in the ICommand.Execute call. Although I will concede that crashing the app through an unhandled async void execution is worse.

@Sockenfresser

@brianlagunas So it seems we just started using PRISM in exactly the time the current behavior was introduced and build up everything on it. So I hope you can see why from my point of view your test is wrong. I understand your problems with the difficult interface. A better name for the async method would have been ExecuteAsync anyway. This would have also solved your test case problem. But I see that you don't intend to make the DelegateCommand work for async and non-async scenarios. That's your decision.
I guess we will switch over to Nito.AsyncEx's AsyncCommand because it nearly has the same features and a clearly async oriented interface.

@dvorn
Contributor
dvorn commented Nov 2, 2016

@pdinnissen @Sockenfresser If you think you really need async commands, one of the most sophisticated implementations is this one: https://msdn.microsoft.com/magazine/dn630647.aspx

@Sockenfresser
Sockenfresser commented Nov 2, 2016 edited

@dvorn Thank you. Seems that this is an early implementation from Stephen Cleary the author of AsyncEx. 😄

@pdinnissen
Contributor
pdinnissen commented Nov 2, 2016 edited

@dvorn Oh don't worry, I would not consider myself an async capable programmer without having read pretty much everything Stephen Cleary has published (his Concurrency in C# Cookbook is pound for pound the best book I have in my pile).

@Sockenfresser I recommend that you take a look at this link I have in my previous post, it is part of Stephen Cleary newer MVVM.Async library.

At the end of the day, swapping implementations of PRISM objects has always been one of the reasons I prefer this platform in the first place.

@dvorn
Contributor
dvorn commented Nov 2, 2016

One more thought on why async commands (such as Cleary's) are not ready for prime time.

Async commands just start some asynchronous process inside an application. What happens if the user attempts to execute the command second time while the previous invocation is still running? In the Cleary's implementation the command is disabled while the first invocation is running.

But there are other perfectly valid scenarios:

  • Allow multiple invocations; run them in parallel. E.g., a "Download" button while the user selects a file name in another control.
  • Allow multiple invocations; run them sequentially.
  • Cancel previous invocation and start the command a-new. E.g., a "Refresh" button.
  • Do not disable the command, but just do nothing.

You may even need all this patterns in a single program.

I do not see a single implementation of async command that will allow all this scenarios.

@Sockenfresser

@dvorn Cleary's AsyncCommand (from the Mvvm.Async library) just implements a simple default behavior. But you can always implement your own CanExecute, cancellation or whatever behavior. What's wrong with that? It's pretty much the same Prism does all the time. Provide a default implementation and an extension point to replace/alter the default behavior.

@dvorn
Contributor
dvorn commented Nov 2, 2016 edited

@Sockenfresser It's just more difficult to override the "default behavior" than to implement those patterns from scratch.

@pdinnissen
Contributor

@dvorn is the intent that DelegateCommand be able to handle all those scenarios? Won't it block the UI thread as it stands?

I don't disagree with your post, and plan on using as a goal for my implementation of DelegateCommandAsync and if I'm satisfied with it I could also propose it for a future release.

Can you be more specific by what you mean by 'Do not disable the command, but just do nothing.' though?

@dvorn
Contributor
dvorn commented Nov 2, 2016

@pdinnissen The plain old synchronous DelegateCommand does allow all this scenarios because the implementation of all this scenarios is up to you in the delegates you use to create the commands. The UI will not block because you will either start a task (and don't wait for completion) in the delegate, or use async void as a delegate.

Do not disable the command, but just do nothing: When the command is already running, the command (and hence the button) is not disabled and the user can click second time on it. In this case, though, nothing happens. (I agree this would be a rare scenario. I just mentioned it for completeness.)

@pdinnissen
Contributor

@dvorn Case closed. Perhaps this should be summarized in the docs?

@toadzky
toadzky commented Nov 2, 2016

I would like to point out that semantic versioning means if you are going to remove something, you should bump the minor version. Removing this should change the version to 7, not 6.3.

@brianlagunas
Member

@toadzky I'm not using semantic versioning at this time.

@dvorn
Contributor
dvorn commented Nov 3, 2016 edited

@Sockenfresser To ease the pain of porting your tests you can do the following: if you have

Task MyHandler() {...}
DelegateCommand myCommand = DelegateCommand.FromAsyncHandler(MyHandler);
/// in test:
await myCommand.Execute();

change it to

DelegateCommand myCommand = new DelegateCommand(async () => await MyHandler());
/// in test:
await MyHandler();
@spasas
spasas commented Nov 11, 2016

I think I have another approach, let me know if it makes sense...

Quick background, I am still using Prism 5, but I look from time to time what's up with the current version. I will probably migrate next year. In the meantime, I have to look into this shady DelegateCommand for our projects and doing some research I found the branch of @brianlagunas and this issue.

I understand the flaws around the DelegateCommand and the use of the TPL to make it asynchronous. However, it occurred to me that the PubSubEvents have another way of implementing the same behavior. You can subscribe on the current thread, the UI thread or a background thread. What if we can apply the same technique for the DelegateCommand ?

I've played around the DelegateCommand just to see if I can make it work with the PubSubEvents code, and it did. I've made a branch with the test code... It is far from being production code...It's only a proof of concept:

https://github.com/spasas/Prism/tree/DelegateCommand-OldNewApproach/Source/Prism/Commands

I'm curious to know what you think about it, and if it can be something to pursue.

Thanks,
Patrick

@brianlagunas
Member

@spasas Thank for the suggestion, but I am not looking to add to the API surface of DelegateCommand.

@feinstein
Contributor
feinstein commented Nov 13, 2016 edited

@dvorn all the issues you raised (multiple invocations, cancellation and etc), Stephen addressees them in his MVVM Commands blog post.

In this image you can see the AsyncCommand can bind Error Messages, Exceptions, Results, Loading Status, simplifying, encapsulating and reusing (all the magical words we devs love to hear and repeat, don't we? haha) a lot the work we should be doing in case we need async behavior in our commands:

ic716159

What's not on that image is the testability that wasn't comprimised at all since the AsyncCommand is just an Execute calling ExecuteAsync that returns a Taks and this Task can be used for testing, making your test cases target only ExecuteAsync and forget Execute.

I think many people in this thread have read all this already but I didn't see anyone pointing this out.

@brianlagunas My current project deals mainly with async services. AFAIK the Prism library is knows as the "Proven Patterns and Practices for predictable results" which means, as I understand it, proven patterns for software development in general and not a pure MVVM library....am I wrong?

So as I see it, if there are patterns that make our job dealing with async code more simple and secure, why should we not use it or share it?

I understand your points on DelegateCommand not being relevant as async...but should we just forget async entirely? Most new architectures, specially the ones dealing with HTTP services will be based on async, it provides the most responsive user experience and leverages the programmers ability to deal with concurrency.

Last time I checked async is being pushed by Microsoft mobile as the new way to do things to provide a better UX, and MVVM is the architecture of choice of all future Microsoft related applications, so having an async command is nothing but natural to me.

Yes there are several different use case scenarios, but I think we can customize most things to work nicely as the developer wants them, a lot of them can be just a change in behavior with a simple enum flag, and most use case scenarios are pretty well known. Even Stephen Cleary said his solution isn't going to be the best thing for everyone, bust still not even Prism can achieve this.

This is just my 2 cents on this matter. I intent to use Stephen's AsyncCommand pattern in my project because it will simplify it a lot, specially with all the bindings I need for showing loading, errors and results in the XAML UI, but I will be glad to find something similar on Prism on the coming future.

@dvorn
Contributor
dvorn commented Nov 14, 2016 edited

@feinstein It is very characteristic what Mr. Cleary ended up with in that 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.

@feinstein
Contributor
feinstein commented Nov 14, 2016 edited

@dvorn yes, precisely, and observable (bindable) Task properties is the main goal I think we could achieve here.

I think AsyncDelegateCommand would be a fitting name for it since it is a regular synchronous command that receives a delegate to an async operation and starts it. Hence Async-Delegate Command.

@brianlagunas
Member

Proven Patterns and Practices for predictable results

@feinstein this has yet to be proven. When the XAML platforms add native support for async commands, I will add it. Until then, you can simply add Cleary's approach to your application.

@feinstein
Contributor

@brianlagunas OK, I understand your reasons, thanks for listening though.

@StephenCleary StephenCleary referenced this issue in StephenCleary/Mvvm.Async Nov 15, 2016
Open

Examine Prism's AsyncCommand discussion #15

@brianlagunas
Member

I just checked in another approach. @dvorn check it out and let me know your thoughts.

@dvorn
Contributor
dvorn commented Nov 23, 2016 edited

@brianlagunas You removed a check for value types. This is going against the original design:

/// The constructor deliberately prevents the use of value types.
/// Because ICommand takes an object, having a value type for T would cause unexpected behavior when CanExecute(null) is called during XAML initialization for command bindings.
/// Using default(T) was considered and rejected as a solution because the implementor would not be able to distinguish between a valid and defaulted values.

My understanding is that will not work good in the following case: suppose you use something like ListBox.SelectedIndex as a parameter to the command. During initialization of XAML, CanExecute(null) will be issued and the view model will interpret that as CanExecute(0). Thus, the command will be enabled although no item was selected in the listbox. With Nullable the user would be able to distinguish between the absent value and the valid value of 0.

However, I may be wrong. I never used DelegateCommands with value types. ListBox.SelectedItem may be a bad example; it has a value of -1 to indicate absent selection. Maybe XAML calls CanExecute(null) anyway. Just do not know.

@dvorn
Contributor
dvorn commented Nov 23, 2016

@brianlagunas Oh, you removed the check for value type completely. This will crash

 : base((o) => executeMethod((T)o), (o) => canExecuteMethod((T)o))

when the parameter is null and T is e.g. int.

@pdinnissen
Contributor

@dvorn if T is int then the parameter should never be null and if it is then wouldn't a crash be an expected behaviour as opposed to "hiding" it behind a default value type?

Also, should DelegateCommandBase still have the following code?:

        void ICommand.Execute(object parameter)
        {
            Execute(parameter);
        }

        bool ICommand.CanExecute(object parameter)
        {
            return CanExecute(parameter);
        }

Since @brianlagunas and @dvorn mentioned that WPF will "ignore" any additional overrides function that add additional code (ie: logging like @dvor said).

@dvorn
Contributor
dvorn commented Nov 23, 2016 edited

@pdinnissen XAML knows nothing about CanExecute(T parameter) and works with ICommand.CanExecute(object parameter). Unfortunately, it calls CanExecute(null).

WPF does not "ignore" anything. All it does is calling ICommand.Execute. Everything else is our code which may be different and work differently.

@pdinnissen
Contributor
pdinnissen commented Nov 23, 2016 edited

Oh right. Too early in the morning to keep all this straight. Thanks for clarifying.

I have a small recommendation that can be ignored for the parameterless DelegateCommand's constructor:
: base((o) => executeMethod(), (o) => canExecuteMethod())
be changed to:
: base((_) => executeMethod(), (_) => canExecuteMethod())
It makes it more explicit that the lambda parameter is being ignored.

@brianlagunas
Member

@dvorn yeah, I am still playing around with the value type thing. I think I am going to remove the Action/Func's from the DelegateCommanBase, and have each command responsible for their own delegates instead of passing them around.

@dvorn
Contributor
dvorn commented Nov 23, 2016

@pdinnissen I am not opposed to (_)=> and used it myself but I noticed that this notation is less readable under harsh viewing conditions.

@brianlagunas
Member

@dvorn check it out now. What do you think of this direction? I will worry about the value type thing later.

@dvorn
Contributor
dvorn commented Nov 23, 2016 edited

@brianlagunas I thought about keeping delegates separately, too. Also, you could make implement ICommand in derived classes instead of base. A couple comments:

I did not tried to run this code. Could there be a problem with overload resolution and/or Intellisense for DelegateCommand<object> ?

Now DelegateCommandBase has nothing to do with delegates. Its responsibility is observing "can execute changed" event. Maybe a different name?

@brianlagunas
Member
brianlagunas commented Nov 23, 2016 edited

To handle the null Value Type issue, I think I am going to just throw a Debug.Assert instead or preventing the use of Value Types. No other command restricts the use of value types, and they don't seem to have any issues. I just need to figured out if the WPF framework passes null at anytime during it's initialization that is not controlled by the developer. I personal have never run into this.

        protected override void Execute(object parameter)
        {
            Debug.Assert(parameter == null && typeof(T).GetTypeInfo().IsValueType, "You have defined a ValueType for your paramater, but are tyring to pass null.  Fix your code!");

            Execute((T)parameter);
        }
@dvorn
Contributor
dvorn commented Nov 23, 2016

Google: "command parameter is null first time canexecute is called"

@brianlagunas
Member

WOW! Okay, we'll just have to leave the restriction of Value Types in there then. Other than that, does everything else look goo to you?

@dvorn
Contributor
dvorn commented Nov 23, 2016

Overall, I'm fine with it.

Yet another solution for value types might be to just return false from CanExecute(null) and do nothing in Execute(null).

@brianlagunas
Member
brianlagunas commented Nov 24, 2016 edited

Alright, this has been merged into the master and will be in the next major release. Since Prism has never been on semantic versioning, and since this is such as massive break for devs, I will move Prism to semantic versioning on the next release.... Prism 7.0.

@StanleyBroo
StanleyBroo commented Nov 25, 2016 edited
@nuitsjp nuitsjp referenced this issue in ProjectBlueMonkey/BlueMonkey Dec 13, 2016
Closed

[Vote]Do we select ReactiveProperty for ICommand? #39

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment