Add AsyncCommand from JT #43
Conversation
|
Hey James! I've been doing a lot of work lately with I've been iterating & improving this implementation for about a year and it's all available on NuGet: https://www.nuget.org/packages/AsyncAwaitBestPractices.MVVM/ Here's a few tips/gotchyas I learned when implementing
|
MvvmHelpers/Commands/Command.cs
Outdated
| readonly Action<object> execute; | ||
| readonly WeakEventManager weakEventManager = new WeakEventManager(); | ||
| /// <summary> | ||
| /// Implementaiton of ICommand |
MvvmHelpers/Utils.cs
Outdated
| { | ||
| // The parameter isn't null, so we don't have to worry whether null is a valid option | ||
| valid = o is T; | ||
| internal static bool IsValidParameter<T>(object o) |
There was a problem hiding this comment.
Maybe we could find a better name that actually indicates for which class we are checking if the parameter is a valid one.
| [TestClass] | ||
| public class WeakEventManagerTests | ||
| { | ||
| static int count; |
There was a problem hiding this comment.
I usually use Xunit which runs tests randomly and in parallel therefore seeing static in a unit test class always triggers a warning in my head. The comment is here just in case the behavior is similar and could cause breaking tests.
| /// <summary> | ||
| /// Initializes a new instance of the <see cref="T:MvvmHelpersInvalidCommandParameterException"/> class. | ||
| /// </summary> | ||
| /// <param name="excpectedType">Excpected parameter type for AsyncCommand.Execute.</param> |
There was a problem hiding this comment.
Typo => excpectedType and Excpected
| /// <summary> | ||
| /// Initializes a new instance of the <see cref="T:MvvmHelpers.InvalidCommandParameterException"/> class. | ||
| /// </summary> | ||
| /// <param name="excpectedType">Excpected parameter type for AsyncCommand.Execute.</param> |
There was a problem hiding this comment.
Typo => excpectedType and Excpected
There was a problem hiding this comment.
- Breaking changes
- Update minimum .NET Standard dependency to .NET Standard 2.0 because of
IsValueType
- Update minimum .NET Standard dependency to .NET Standard 2.0 because of
- Performance recommendations
- Using catch filters
- Using
is nullinstead of== null
- Philosophical recommendations
- Remove the
Asyncsuffix fromSafeFireAndForgetAsync - Don't catch an Exception in
SafeFireAndForgetwhenonExceptionis null - Make
WeakEventManagerinternal
- Remove the
| } | ||
|
|
||
| // Not a Nullable, if it's a value type then null is not valid | ||
| valid = !t.GetTypeInfo().IsValueType; |
There was a problem hiding this comment.
IsValueType is only available in .NET Standard 2.0: https://docs.microsoft.com/dotnet/api/system.type.isvaluetype.
It looks like MvvmHelpers' current minimum dependency is .NET Standard 1.0, so make sure to bump it up to .NET Standard 2.0, or add the .NET Standard 2.0 NuGet as a dependency.
Here's the support for IsValueType compared to the current dependencies for MvvmHelpers:
- .NET Core
- 3.0 Preview8, 2.2, 2.1, 2.0
- .NET Framework
- 4.8, 4.7.2, 4.7.1, 4.7, 4.6.2, 4.6.1, 4.6, 4.5.2, 4.5.1, 4.5, 4.0, 3.5, 3.0, 2.0, 1.1
- .NET Standard
- 2.1-Preview, 2.0
- Xamarin.Android
- 7.1
- Xamarin.iOS
- 10.8
Xamarin.Mac - 3.0
- 10.8
| { | ||
| await task.ConfigureAwait(continueOnCapturedContext); | ||
| } | ||
| catch (Exception ex) |
There was a problem hiding this comment.
Careful - this is dangerous because it catches every exception thrown by the Task.
I recommend not catching the Exception if the user has not implemented onException because it means that they don't want to gracefully handle the exception. I like to think of onException as the way the user implements a try/catch for FireAndForgetAsync; if they haven't implemented onException, don't catch any exceptions; if they have, catch 'em (all 😜).
try
{
await task.ConfigureAwait(continueOnCapturedContext);
}
catch (Exception ex)
{
if(onException is null)
throw;
onException.Invoke(ex);
}Same code, but optimized using Catch filters as recommended by the great Bill Wagner: brminnick/AsyncAwaitBestPractices#8
try
{
await task.ConfigureAwait(continueOnCapturedContext);
}
catch (Exception ex) when (ex != null)
{
onException.Invoke(ex);
}|
|
||
| namespace MvvmHelpers | ||
| { | ||
| public class WeakEventManager |
There was a problem hiding this comment.
I recommend making this class internal, because otherwise devs will start using it and you'll beholden to maintaining a WeakEventManager despite this library focusing on MVVM. I implemented it in AsyncAwaitBestPractices, and I wish I had kept it internal so that only AsyncCommand would use it. Here's why:
Story Time
I released AsyncAwaitBestPracitces, with WeakEventManager public, and shortly after its release someone asked me for WeakEventManager<T> for EventHandler<T>- sure, no problem!
But then someone pointed out that not every event is an EventHandler and that events can also be a Delegate (like PropertyChangedEventHandler which is used by NotifyPropertyChanged), so I added support for Delegate.
But THEN someone pointed out that Action can also be used for events, and so I added support for event Action. But this was easier said than done, because now I needed to add an overloaded method for WeakEventManager.HandleEvent because Action and Action<T> doesn't consume an object, so I also ended up creating a custom Exception to throw when a dev uses the incorrect overloaded method for HandleEvent: https://github.com/brminnick/AsyncAwaitBestPractices/blob/master/Src/AsyncAwaitBestPractices/WeakEventManager/EventManagerService.cs#L58
tl;dr - save yourself the headaches and make this internal
| if (IsNullOrEmpty(eventName)) | ||
| throw new ArgumentNullException(nameof(eventName)); | ||
|
|
||
| if (handler == null) |
There was a problem hiding this comment.
Change handler == null to handler is null to squeeze out some more performance benefits: https://www.gullberg.tk/blog/is-null-versus-null-in-c/
tl;dr Pattern Matching is faster than Equality comparison in the .NET IL
| if (IsNullOrEmpty(eventName)) | ||
| throw new ArgumentNullException(nameof(eventName)); | ||
|
|
||
| if (handler == null) |
There was a problem hiding this comment.
Change handler == null to handler is null to squeeze out some more performance benefits: https://www.gullberg.tk/blog/is-null-versus-null-in-c/
tl;dr Pattern Matching is faster than Equality comparison in the .NET IL
|
|
||
| var subscriber = subscription.Subscriber.Target; | ||
|
|
||
| if (subscriber == null) |
There was a problem hiding this comment.
Change subscriber == null to subscriber is null to squeeze out some more performance benefits: https://www.gullberg.tk/blog/is-null-versus-null-in-c/
tl;dr Pattern Matching is faster than Equality comparison in the .NET IL
| if (IsNullOrEmpty(eventName)) | ||
| throw new ArgumentNullException(nameof(eventName)); | ||
|
|
||
| if (handler == null) |
There was a problem hiding this comment.
Change handler == null to handler is null to squeeze out some more performance benefits: https://www.gullberg.tk/blog/is-null-versus-null-in-c/
tl;dr Pattern Matching is faster than Equality comparison in the .NET IL
| if (IsNullOrEmpty(eventName)) | ||
| throw new ArgumentNullException(nameof(eventName)); | ||
|
|
||
| if (handler == null) |
There was a problem hiding this comment.
Change handler == null to handler is null to squeeze out some more performance benefits: https://www.gullberg.tk/blog/is-null-versus-null-in-c/
tl;dr Pattern Matching is faster than Equality comparison in the .NET IL
| eventHandlers.Add(eventName, targets); | ||
| } | ||
|
|
||
| if (handlerTarget == null) |
There was a problem hiding this comment.
Change handlerTarget == null to handlerTarget is null to squeeze out some more performance benefits: https://www.gullberg.tk/blog/is-null-versus-null-in-c/
tl;dr Pattern Matching is faster than Equality comparison in the .NET IL
| WithTimeout(task, (int)timeout.TotalMilliseconds); | ||
|
|
||
| #pragma warning disable RECS0165 // Asynchronous methods should return a Task instead of void | ||
| public static async void SafeFireAndForgetAsync(this Task task, Action<Exception> onException = null, bool continueOnCapturedContext = false) |
There was a problem hiding this comment.
I recommend removing the Async suffix because it may confuse devs who then try to await SafeFireAndForgetAsync which cannot be done.
Instead, change it to SafeFireAndForget.
| valid = o is T; | ||
|
|
||
| if (!valid) | ||
| throw new InvalidCommandParameterException(typeof(T), null); |
There was a problem hiding this comment.
Since we know o !=null, we can pass in its Type to InvalidCommandParameterException.
throw new InvalidCommandParameterException(typeof(T), o.GetType());In the exception message, this will tell the user both the Expected Type and the Actual Type.
Example Error Message:
Unhandled Exception: MvvmHelpers.Exceptions.InvalidCommandParameterException: Invalid type for parameter. Expected Type: System.Int32, but received Type: System.String
| valid = !t.GetTypeInfo().IsValueType; | ||
|
|
||
| if (!valid) | ||
| throw new InvalidCommandParameterException(typeof(T), null); |
There was a problem hiding this comment.
Here, o is null, but passing in null to new InvalidCommandParameterException(Type expectedType, Type actualType) creates a confusing error message:
Unhandled Exception: MvvmHelpers.Exceptions.InvalidCommandParameterException: Invalid type for parameter. Expected Type: System.Int32, but received Type:
at MvvmHelpers.Commands.CommandUtils.IsValidCommandParameter[T](Object o)
I recommend adding additional constructors for InvalidCommandParameterException that only take in one Type parameter:
/// <summary>
/// Initializes a new instance of the <see cref="T:TaskExtensions.MVVM.InvalidCommandParameterException"/> class.
/// </summary>
/// <param name="excpectedType">Excpected parameter type for AsyncCommand.Execute.</param>
/// <param name="innerException">Inner Exception</param>
public InvalidCommandParameterException(Type excpectedType, Exception innerException) : base(CreateErrorMessage(excpectedType), innerException)
{
}
/// <summary>
/// Initializes a new instance of the <see cref="T:TaskExtensions.MVVM.InvalidCommandParameterException"/> class.
/// </summary>
/// <param name="excpectedType">Excpected parameter type for AsyncCommand.Execute.</param>
public InvalidCommandParameterException(Type excpectedType) : base(CreateErrorMessage(excpectedType))
{
}
static string CreateErrorMessage(Type excpectedType) => $"Invalid type for parameter. Expected Type {excpectedType}";
|
|
||
| } | ||
|
|
||
| static string CreateErrorMessage(Type expectedType, Type actualType) => |
There was a problem hiding this comment.
Add two more constructors and an additional method to align with this feedback: #43 (comment)
/// <summary>
/// Initializes a new instance of the <see cref="T:TaskExtensions.MVVM.InvalidCommandParameterException"/> class.
/// </summary>
/// <param name="excpectedType">Excpected parameter type for AsyncCommand.Execute.</param>
/// <param name="innerException">Inner Exception</param>
public InvalidCommandParameterException(Type excpectedType, Exception innerException) : base(CreateErrorMessage(excpectedType), innerException)
{
}
/// <summary>
/// Initializes a new instance of the <see cref="T:TaskExtensions.MVVM.InvalidCommandParameterException"/> class.
/// </summary>
/// <param name="excpectedType">Excpected parameter type for AsyncCommand.Execute.</param>
public InvalidCommandParameterException(Type excpectedType) : base(CreateErrorMessage(excpectedType))
{
}
static string CreateErrorMessage(Type excpectedType) => $"Invalid type for parameter. Expected Type {excpectedType}";
Code from: https://johnthiriet.com/removing-async-void/#<3