[Enhancement] Update AsyncCommand constructor parameters to match Command ones

[maxkoshevoi]

Summary

Related to #709

This repository has AsyncCommand class that accompanies Xamarin's Command class, but for async methods.
It's great, but has two issues:

1. Sometimes it accepts object as parameter. This is not ideal. We should use static typing whenever we can (that's why generics were created). And we can do this here with little API change.
2. AsyncCommand and Command should accept the same parameters (that are common for both classes), but right not this is not always the case:

• canExecute parameter for AsyncCommand<T> accept Func<object, bool> instead of Func<T, bool>? It's Func<T, bool> for regular Command<T>.
• canExecute parameter for AsyncCommand (without T) accept Func<object, bool> instead of Func<bool>? It's Func<bool> for regular Command.

They should the same parameters so user can seamlessly switch between them (methods become async and sync again all the time).

Common place to create all the commands

Also it would be great to create some CommandHelper.Create() that would automatically resolve what command (sync or async) you're trying to create and return an instance of it. This is helpful because id some method will become async or sync, Create() would automatically return command of needed type. And in general, when creating a command user wouldn't need to think what type it would be, they just need to call Create on whatever method they have.

Intended Use Case

User would be able to change new Command(a, b) to new AsyncCommand(a, b) (and new Command<T>(a, b) to new AsyncCommand<T>(a, b)) and a and b would still be valid arguments.

Who Will Do The Work?

• I am willing to take this on myself
• Just putting this out there for someone to pick up

[pictos]

@maxkoshevoi. I friendly suggest you wait we take a look at the issue before submitting a PR, this can avoid you wast your time and effort on an issue that can be refused.

[maxkoshevoi]

@pictos Just wanted to take this one off my chest while I had some time. I already use CommandHelper in my projects, and AsyncCommand changes were minimal, so this wasn't a big effort.
I feel like description and implementation complement each this one. You need to see both to make up your mind.

For example, thanks to type validation introduced by it, I already caught as error in our tests (command was used instead of command2):

Anyway, I really hope this one makes it, cause passing object be default is rarely a good idea. And having single method to create all most commonly used types of commands is also nice (I especially like how it automatically switches between them).

[TheCodeTraveler]

Maksym,

Ten days ago, in the Discussions tab of this repo, we discussed these changes and why they are not recommended.

tl;dr This is a breaking change that removes & duplicates existing functionality and violates the ICommand spec.

Below are the answers from our previous discussion, copy/pasted here for visibility:

Question 1

Why does canExecute parameter for AsyncCommand<T> accept Func<object, bool> instead of Func<T, bool>? It's Func<T, bool> for regular Command<T>.

Answer

In AsyncCommand<T>, T is the type for the parameter for AsyncCommand.Execute(T parameter).

We purposely did not use T for canExecute<object, bool> to allow for scenarios where CanExecute and and Execute use different parameters. For example, here is a case where we are using an int for the Execute parameter and a bool for the CanExecute parameter:

public ICommand PullToRefreshCommand { get; } = new AsyncCommand((int) count => FetchNewData(count), (bool) isBusy => !isBusy)

This implementation best honors the paradigm of ICommand which doesn't require Execute to use the same parameter type as CanExecute.

I strongly disagree with Xamarin.Forms' original implementation of Command<T> that assigns T to both Execute and CanExecute, because the ICommand Specification does not require Execute and CanExecute to use the same parameter type.

Recommendation

For a type-safe canExecute, I recommend using AsyncCommand<TExecute, TCanExecute> like so:

public ICommand PullToRefreshCommand { get; } = new AsyncCommand<int,bool>(count => FetchNewData(count), isBusy => !isBusy)

Question 2

Why does canExecute parameter for AsyncCommand (without T) accept Func<object, bool> instead of Func<bool>? It's Func<bool> for regular Command.

Answer

Similar to the Answer for Question #1, Execute and CanExecute are separate and are not required by the ICommand Specification to use the same parameter type.

I.e. Foregoing an input parameter for Execute does not predicate that you must forego an input parameter for CanExecute.

Here is an example where Execute does not use an input parameter and CanExecute does use an input parameter:

public ICommand PullToRefreshCommand { get; } = new AsyncCommand(() => FetchNewData(count), (bool) isBusy => !isBusy)

Again, I strongly disagree with Xamarin.Forms' implementation of Command as it breaks the ICommand paradigm.

[maxkoshevoi]

@brminnick Could you please take a closer look at AsyncCommad implementation in this PR.

1. It still allows to pass parameters with different types to Execute and CanExecute, but now user need to explicitly specify those types, so it doesn't break ICommand Specification.
2. No AsyncCommad constructor parameters default to object. If user needs to pass object in Execute or CanExecute it needs to be explicitly specified.
3. As I described in PR description: This is breaking change that will only affect users that pass canExecute as Func<object, bool> instead of creating with two type arguments to specify type of canExecute. So this change will push those users to a better (more statically checked) code.

Could you please explain why do you disagree with all or any of those points?

Demo

Take a look at simplified version of CommandHelper class. AsyncCommand now supports all variations of execute and canExecute type combitations:

// Without parameters for both
public static IAsyncCommand Create(
	Func<Task> action,
	Func<bool> canExecute = null) =>
	new AsyncCommand(action, canExecute);

// With staticky typed parameter for execute, without parameters for canExecute
public static IAsyncCommand<TExecute> Create<TExecute>(
	Func<TExecute, Task> action,
	Func<bool> canExecute) =>
	Create<TExecute>(action, ConvertCanExecute<TExecute>(canExecute));

// Without parameters execute, with staticky typed parameter for canExecute
public static IAsyncCommand<TCanExecute> Create<TCanExecute>(
	Func<Task> action,
	Func<TCanExecute, bool> canExecute) =>
	Create<TCanExecute>(ConvertExecute<TCanExecute>(action), canExecute);

// With staticky typed parameter for both. Parameter has the same type (same as in Command)
public static IAsyncCommand<T> Create<T>(
	Func<T, Task> action,
	Func<T, bool> canExecute = null) =>
	Create<T, T>(action, canExecute);

// With staticky typed parameter for both. Parameter has different types
public static IAsyncCommand<TExecute, TCanExecute> Create<TExecute, TCanExecute>(
	Func<TExecute, Task> action,
	Func<TCanExecute, bool> canExecute = null,
	Action<Exception> onException = null,
	bool continueOnCapturedContext = false,
	bool allowsMultipleExecutions = true) =>
	new AsyncCommand<TExecute, TCanExecute>(action, canExecute, onException);