search

CommunityToolkit / WindowsCommunityToolkit

The Windows Community Toolkit is a collection of helpers, extensions, and custom controls. It simplifies and demonstrates common developer tasks building .NET apps with UWP and the Windows App SDK / WinUI 3 for Windows 10 and Windows 11. The toolkit is part of the .NET Foundation.
https://docs.microsoft.com/windows/communitytoolkit/
Other
5.89k stars 1.38k forks source link

[Feature] Basic MVVM primitives (.NET Standard) #3230

Closed Sergio0694 closed 4 years ago

Sergio0694 commented 4 years ago

Describe the problem this feature would solve

Follow up from the conversation with @michael-hawker on the UWP Community Discord server.

With the MvvmLight library not being supported anymore, and other existing libraries being much bigger in scope that what is often needed for simple apps, I was wondering whether we should add some basics MVVM primitives to the toolkit, possibly in a new Microsoft.Toolkit.Mvvm package so that users not in need of those wouldn't have to reference those too. We're also using this occasion to integrate with the Microsoft.Extensions.DependencyInjection package to provide dependency injection features, so that it'll be both easier for devs to interoperate between this package and existing code, as well as allowing us to offload that part of the code to an official library from Microsoft.

All this would come together in a new Microsoft.Toolkit.Mvvm package.

Preview package

The CI is now running for #3229 , so you should be able to just get the latest package from the CI build artifacts.

Key Tenants

Draft docs here 📖

Feature set 🧰

The idea for this package would be to have a series of common, self-contained, lightweight types:

These types alone are usually enough for many users building apps using the MVVM toolkit. Having these built right into the toolkit would allow them to just reference the new package and bet all set, without having to go dig through other documentation and referencing a series of external packages just to gain access to these basic types. Furthermore, we could keep these smaller in scopes than other big libraries like autofac or Prism, which include a lot of features that are not necessarily useful for many (particularly non-enterprise) devs, while still taking a toll on performance and memory usage. This would also make the learning curve much less steep for developers.

As an example of the benefit of a more modern codebase, which is both more optimized and minimalistic, here's a benchmark showing the performance of the Messenger class from both MvvmLight and Calcium, and the one from the related PR (WCT stands for this new Mvvm package). Note that Calcium only supports a single channel mode:

image

Some summarized results for the Messenger class from #3229:

The full benchmark code can be found here.

Describe the solution

As mentioned above, and as can be seen in the related PR #3229, this proposal is to add a new Microsoft.Toolkit.Mvvm package, integrating some features from MvvmLight, refactoring them when needed, in order to offer a self-contained alternative. The package would not need a reference to any other Microsoft.Toolkit packages, it would be lightweight and focused on ease of use, performance and memory efficiency.

Since it targets .NET Standard 2.0, this means that it can be used anywhere from UWP apps, to Uno, Xamarin, Unity, ASP.NET, etc. Literally any framework supporting the .NET Standard 2.0 feature set. There are no platform specific dependencies at all. The whole package is entirely platform, runtime, and UI stack agnostic.

Additionally, it would integrate with the existing Microsoft.Extensions.DependencyInjection package, to offer a well known and tested DI platform, with the addition of an easy to use entry point in the toolkit, especially for new users. This would also remove a lot of code from the actual Mvvm package, as there would not be the need to write and maintain yet another DI library or set of APIs.

As mentioned above, the package would target .NET Standard 2.0 to have as much reach as possible, and it wouldn't have any platform-specific dependencies.

Having a set of MVVM-enabled features built-in into the toolkit would align with the general philosophy of the toolkit to help developers getting started in common scenarios (such as with the MVVM pattern), and it would complement all the available APIs from the BCL that just lack a "reference" implementation (such as INotifyPropertyChanged or ICommand).

Furthermore, having a package like this being published by a Microsoft supported project directly would be a big selling points for users not wanting to add "3rd party" references to their projects (this point has been brought up before in the comments below as well by other users).

Describe alternatives you've considered

As mentioned above, common alternatives like autofac, Fody or Prism are much bigger in scope than what many devs typically need and introduce more overhead. MvvmLight on the other hand is also not supported anymore, other than some forks.

ghost commented 4 years ago

Thanks for submitting a new feature request! I've automatically added a vote 👍 reaction to help get things started. Other community members can vote to help us prioritize this feature in the future!

michael-hawker commented 4 years ago

To add to this, I had ported @StephenCleary's MVVM Async helpers from his articles to UWP here. Though I didn't realize he had them on GitHub already here (though not sure if UWP enabled).

Also, Windows Template Studio has an MVVM Basic pattern with some basic helpers, this could be an opportunity to align those in the toolkit instead of part of their template engine. FYI @sibille.

@Sergio0694 I think the best place to start before we dive deeper in code, is to talk more to @lbugnion and @sibille and figure out the best path forward (even if community driven) for longer-term support of a light-weight MVVM framework.

Sergio0694 commented 4 years ago

That sounds like a great idea! Looking forward to hearing what they think too! 😄

As for the PR, I've mostly just opened it as a draft to have a reference, and because I wanted to experiment with that Ioc idea I had (which I'll definitely use at least in my apps either way given those benchmarks 🚀). Wasn't planning to work more on it for the time being, as I completely agree with you on gathering more feedback first, and besides I think the fork that @jamesmcroft made of the MvvmLight already contains most of the basic building blocks we'd need, at least for a prototype.

Looking forward to seeing where this goes, glad I could at least spark a discussion on the topic! 😊

azchohfi commented 4 years ago

@Sergio0694 how does your implementation compares to the ASP.Net default one? https://www.nuget.org/packages/Microsoft.Extensions.DependencyInjection/5.0.0-preview.2.20160.3 I'm not even sure it will work on UWP, but it should, as there is a .Net Standard 2.0 version of it. This might help as well: https://github.com/danielpalme/IocPerformance

azchohfi commented 4 years ago

And are these numbers for .Net Native?

Sergio0694 commented 4 years ago

@azchohfi Those are numbers for .NET Core 3.1, since BenchmarkDotNet doesn't support UWP. I did setup a benchmark class to test things on UWP too a while back though, it's not perfectly accurate as BenchmarkDotNet (and it lacks the memory diagnoser) but it's reliable enough, I'll give it a go to see how it runs there! As for the ASP.NET one, not sure if it works on UWP either, but I can definitely at least add that to the .NET Core benchmarks and see how it goes, thank you for the suggestion! 😄

Will update the post as soon as I have more results.

EDIT: updated the post, the ASP.NET solution is faster than MvvmLight but it's still about 18x slower than the Ioc class in this PR. Of course, the ASP.NET version much like the other 2 libraries has more features, though I'll say that the Ioc class in this PR has the advantage of being much easier to setup, of offering static APIs so you don't necessarily need to pass instances of your container around, plus it's all self contained in a single file and with no dependencies. Different use cases, sure 😊 I'll go ahead and test on UWP too!

EDIT 2: done, it's all in the first post! 👍

Sergio0694 commented 4 years ago

@azchohfi Not sure whether GitHub sends notifications when mentioning through an edit to an existing comment, posting this just in case and also to add some more details 😄 Since you seemed interested in performance metrics, I run some more benchmarks across all the various implementations, both when retrieving singleton services as well as transient ones. Here are the updated results (.NET Core 3.1):

image

The situation with singleton services is like before, whereas when retrieving transient ones, this Ioc implementation is about 60x faster than MvvmLight (thanks to compiled expressions) and using less than 1/11th the memory, and it remains over twice as fast as the ASP.NET solution. 🚀

shaggygi commented 4 years ago

I recall seeing a Visual Studio survey for MVVM feedback on Twitter a few months back. It appeared there was possible work being done to add MVVM basics to the framework to help in this area. Just curious, is this the same work or is it possible there is duplicate work being investigated?

Sergio0694 commented 4 years ago

Hey @shaggygi - this is not the same work, it's more of an exploratory issue to put some ideas together and see how best to proceed. We're talking with @jamesmcroft who is maintaining a .NET Standard fork of MvvmLight, and @michael-hawker has contacted both Laurent, the original author of the MvvmLight library, and the folks working on the Windows Template Studio, to see if it could make sense to integrate some of the existing work into the toolkit, or to just link an external repository and update that one, etc.

The draft PR that is linked instead was just me experimenting with some ideas in the meantime 😄

sibille commented 4 years ago

@sergio0694 and @michael-hawker I think adding basic MVVM classes to the Toolkit makes a lot of sense.

Among the goals for the MVVMBasic implementation in Windows Template Studio was to provide a solution for people who:

https://github.com/microsoft/WindowsTemplateStudio/blob/dev/docs/UWP/frameworks/mvvmbasic.md

Not sure if this is important to a lot of people, if it is, in WTS we could still offer people both options MVVMBasic (using the WCT) or MVVMBasic (generating the classes in the code).

Sergio0694 commented 4 years ago

Hi @sibille - thank you for chiming in! I'm glad to hear this idea makes sense to you, that's great! 😊

And yes, if the WTS then offered both the option to either reference the MVVM classes from the WCT directly, or having them autogenerated into a project, that'd be ideal, good idea! 😄 Maximum flexibility for users, and offering those who don't mind using an external library the ability to use a compact set of classes that are more up to date, and right from the WCT ecosystem.

Sergio0694 commented 4 years ago

I've added the new Messenger implementation (code here), which completely avoids the use of reflection and is able to have no memory allocations at all when sending messages (and it's also faster than the messenger from MvvmLight). Here's a benchmark of the two Messenger class (gist here), when sending a number of messages between multiple receivers, either through the main channel (no token), or through various separate channels (using a token to distinguish them):

image

You can see that not only is the new Messenger class much faster than the one from MvvmLight (especially when using the default channel), but it also literally does no memory allocations at all, whereas the MvvmLight type ends up allocating almost 500MB (!) in this benchmark 🚀

P.S. the PR should be more or less be feature complete at this point, since the idea was just to have the basic types available. Will go ahead and add more unit tests, as they are just a few for now.

skendrot commented 4 years ago

Adding "basic" mvvm tooling has been discussed numerous times, both publicly and privately. Here are a few I found easily:

There are a lot of frameworks already out there. Just to name a few:

MVVM Frameworks always start out as something small. Then more and more features are added making them larger and larger. Creating a framework within the toolkit would just explode to being more and more.

My suggestion is that we should maintain the stance that there are already frameworks out there and that's not what this toolkit is meant for.

hansmbakker commented 4 years ago

About the IoC part: I would really like it if there would be a bit more standardization on dependency injection for UWP, like Asp.Net Core has. Benefits are a clearer approach for everybody (helps to find unified documentation) and aligned efforts rather than scattered efforts. In that sense, personally, I would rather see standardization on Microsoft.Extensions.DependencyInjection. I find the performance you achieved impressive though!

On the MVVM part: I would really like to see a MVVM approach for UWP supported by Microsoft and/or the WCT. The issue with MVVM for UWP currently is that as a developer I do not feel confident at all taking a dependency on any of the established 3rd party frameworks. That is because e.g. they seem to have dropped support for UWP or seem to have stopped being maintained. That is why I am really looking for a clear, reliable, supported way of doing things for UWP. If moving MVVM into the WCT is the way to do that, please go forward.

I know about this XKCD comic about standards. However, I think we got into this situation because, for UWP projects, there was never a clear drive from Microsoft on these two points (MVVM & IoC/DI). Supporting MVVM frameworks for a longer time takes a large effort which I understand, but developers taking a dependency on a MVVM framework do that because they develop a more complex app that needs usually needs support for a longer time. Then it feels scary to take a dependency on a 3rd party MVVM framework that might lose support.

Sergio0694 commented 4 years ago

Hey @hansmbakker - glad you liked the performance improvements in the Ioc class! I'll try to go through all the various points that you and @skendrot raised one at a time.

Regarding the various closed PRs you linked, I feel like some context is important, specifically, the timing. Most of those issues are pretty old (some from 2017), and a lot of things have changed. If you mention the overlap in different frameworks, I'd argue that the Windows Community Toolkit and MvvmLight actually already overlap in quite a few places - for instance, MvvvmLight has a whole helper class that's UWP-specific that handles dispatching to the UI thread, which is exactly the same functionality that the toolkit offers through the DispatcherHelper class.

Having a built-in set of MVVMs types boils down to a few key points:

Regarding what @skendrot said here:

MVVM Frameworks always start out as something small. Then more and more features are added making them larger and larger. Creating a framework within the toolkit would just explode to being more and more.

This is the textbook "slippery slope argument", and I don't think we should base our decision on this. We can define the scope of the package and leave it at that, but not doing anything at all just because it could potentially grow too big in the future is not what I'd suggest. I think all the numerous advantages of this package should also be weighted in, along with the fact that as I mentioned, we could just clearly state the main points of the package and reject possible future proposal that would not go in the same direction. That wouldn't be any different from the toolkit as a whole.

All in all, the main idea here would be to have a minimalistic, easy to use, well maintained, modern, high-performance set of MVVM primitives from a 1st party package, an all-in-one toolkit for MVVM scenarios that would suit a large number of developers needing these basic building blocks.

Many of these are things that literally every developer basically needs (eg. ObservableObject, RelayCommand, etc.), and I think the package as a whole, for how it's structured right now, would be a good fit for what the toolkit represents. Of course I might be biased since I did write the thing, but I hope I raised some valid points here as well 😄

hansmbakker commented 4 years ago

I agree with a lot of things you say. And 👍 for creating this PR! There are a lot of useful classes in it and, if they are maintained in a nuget package, that allows for much easier updating than one-time code generation.

One thing I am wondering about: are there many people who do MVVM without dependency injection? If not, would there be a case for standardization towards DI? Or would too many people complain? What would that change to the ViewModelBase class?

offering static APIs so you don't necessarily need to pass instances of your container around

In what cases would it be needed to pass that container around? I thought that, since both the ViewModels and their dependencies are registered and have their dependencies injected, there is no need to pass your container around?

Sergio0694 commented 4 years ago

@hansmbakker Glad to hear that! 😄

As for that quote, it actually refers to an old version of the PR - both the Ioc and the Messenger class fully support DI now. In fact, I've also added some constructors to ViewModelBase to make it easier for people that use DI to pass those instances. At the same time, both classes also expose a static, thread-safe Default instance, so you're free to use whatever approach you prefer, either DI or static 👍

As for why people would need multiple instances, an example is when running multiple app instances in the same process (eg. with multiple app windows), people would want to have an instance per window. Or, having instances to inject could be useful when writing unit tests.

As for your other question, for instance I use the static approach in some of my apps, as it simplifies the codebase when you don't need the additional flexibility of using DI.

In general, multiple people in the Discord server said they'd want to use the DI approach, so since the required code changes were minimal I added both options 🚀

Sergio0694 commented 4 years ago

Relaying some updates here from the UWP Discord server to keep everyone up to date.

Following an extensive conversation with @Aminator and some prototyping, I went ahead and did a number of changes in #3229, specifically related to the dependency injection area:

Sample usage of how to configure services with the new Ioc class:

Ioc.Default.ConfigureServices(services =>
{
    services.AddSingleton<ILogger, Logger>();
    services.AddSingleton<IPrinter, PrinterService>();
    // ...
});

The Ioc class then exposes an IServiceProvider ServiceProvider property which is automatically used by ViewModel base if no other provider is specified, which means that users can just use that code to initialize services, and then every single ViewModel in their app will automatically have the resulting IServiceProvider instance being injected. They can then just use all the extensions available from the Microsoft.Extensions.DependencyInjection package to get service instances from that provider.

This solution has the advantage of being both easy to use, highly customizable, and using the same APIs from Microsoft.Extensions.DependencyInjection while still having the same behavior from the original Ioc class from MvvmLight. Plus we don't have any code to maintain at all in that area.

Other general changes from the PR (and a few more points for @skendrot I forgot to mention the other day). Regarding having basic MVVM building blocks in the toolkit, I realized the toolkit actually already does have a few of them. In particular Singleton<T> and NotifyTaskCompletion<TResult>:

private Task<string> myTask = Task.FromResult<string>(null);

public Task<string> MyTask
{
    get => myTask;
    private set => SetAndNotifyOnCompletion(ref myTask, () => myTask, value);
}
<TextBlock>
    <Run Text="Task status:"/>
    <Run Text="{x:Bind ViewModel.MyTask.Status, Mode=OneWay}"/>
    <LineBreak/>
    <Run Text="Result:"/>
    <Run
        xmlns:ex="using:Microsoft.Toolkit.Extensions"
        Text="{x:Bind ex:TaskExtensions.ResultOrDefault(ViewModel.MyTask), Mode=OneWay}"/>
</TextBlock>

Which results in this with a function that waits two seconds and then returns "Hello world!":

NVXa3N8Wom

Same feature as before (but in a dedicated, specialized package now), and without the additional "proxy" object, so that the property you have in your models is actually of type Task<T> 🎉

hansmbakker commented 4 years ago

Really happy to see the direction you took!

hansmbakker commented 4 years ago

By the way, don’t the view models and views need to be registered with DI, too?

(I thought that is a common practice, but I didn’t pick that up from your description.)

Aminator commented 4 years ago

@hansmbakker Sergio wants to explicitly not use constructor injection and instead uses the anti-pattern where you access the IServiceProvider directly in your view model. The ViewModelBase accesses a static instance of the IServiceProvider by default, that's why it doesn't need to be registered as well.

This is the opposite to what I do in my apps. I always use constructor injection in my view models and services, which is also why I don't need the Ioc or ViewModelBase classes at all. Singleton and sometimes transient view models are also part of the service provider if you use this pattern. This has the benefit that the view models and services don't know where their dependencies come from and can also be instantiated normally if needed. This is also the recommended pattern in ASP.NET.

Lastly, views are never part of the service provider because those should generally have an empty constructor to be able to instantiate them in XAML, which would interfere with constructor injection. The services a view needs should come from the associated view model instead, which is typically the DataContext property and exposed as a strongly typed ViewModel property to the view for proper support with x:Bind.

hansmbakker commented 4 years ago

explicitly not use constructor injection and instead uses the anti-pattern where you access the IServiceProvider directly in your view model

@Aminator thank you for explaining that, I had not seen that intent / consequence!

@Sergio0694 I agree with @Aminator here, constructor injection with registered view models has the advantages that @Aminator describes, and furthermore it is a common practice (so it makes people "feel at home" quickly), why deviate from that? Is there a reason why you are doing it this way?

If this is going to be the approach that the toolkit will provide, and WTS might use this for the basic MVVM code in the future, please ensure that it promotes best practices.

views are never part of the service provider

@Aminator probably it is less common, I was not used to it too, but I saw it being done in this ReactiveUI/Uno sample (code) last week.

Sergio0694 commented 4 years ago

We had an extensive conversation about this too with @Aminator while drafting the new APIs structure, and I'm actually a bit surprised to see my approach be referred to as an "anti-pattern" now, because it really itsn't. Or at least, it might technically be if you're trying to exactly replicate the architecture of an ASP.NET app, but that's not the point of this package. This is not Microsoft.Toolkit.AspDotNet, it's .Mvvm. The MVVM pattern per se doesn't have any single "official" way of handling services, mostly because it doesn't really concern itself with how they are handled. Many people mix MVVM with other patterns like DI, but that is indeed a mix of two separate patterns. MVVM on its own is only about logically separating the view with the business logic of the application and the models, and in particular is meant to facilitate the use of features like data binding. That's really all that MVVM is in a nutshell.

That said, let me clarify a couple points on this and also answer @hansmbakker's question:

<UserControl
    x:Class="MySampleApp.Views.ConsoleView"
    xmlns="http://schemas.microsoft.com/winfx/2006/xaml/presentation"
    xmlns:x="http://schemas.microsoft.com/winfx/2006/xaml">
    <UserControl.DataContext>
        <views:ConsoleViewModel x:Name="ViewModel"/>
    </UserControl.DataContext>

    <!--Other stuff in the view here-->
</UserControl>

You can see that I'm spawning the viewmodel right from XAML - I can do this since it doesn't need any kind of DI in its constructors. This also lets me have the ViewModel field directly from there to use x:Bind, without the need of manually adding the strongly typed ViewModel property in code behind at all. In fact, with this approach I often don't need to write any code behind at all. Now, that viewmodel will also automatically receive the default IMessenger and IServiceProvider instance from the ViewModelBase, which means that:

You could say that this is not 100% "proper ASP.NET-like DI patter", and you might be correct, but it's 100% valid MVVM pattern, which is the main point of this package. Furthermore, I should add:

StephenCleary commented 4 years ago

Regarding my MVVM helpers:

  1. I would recommend taking the NotifyTask from my GitHub; its API has gotten a bit better over the years.
  2. I would not recommend using AsyncCommand. Possibly IAsyncCommand and an AsyncCommandBase, but that would be about it. There's just too much variation in what people expect for a single AsyncCommand to be a useful type. I think it makes more sense for the "async command" concept to be composed of several smaller pieces, and this is the approach I've started down in my GitHub repo, but it's still quite a way from being finished (hence the prerelease on the NuGet package), and I'm not prioritizing it at this time.
Sergio0694 commented 4 years ago

Hey @StephenCleary - thank you for chiming in!

  1. I'm not really a fan of the NotifyTask class due to the way it basically duplicates and proxies every single property, whereas we can just bind directly from XAML to all of these, without even needing the second object at all. Have you seen my code example in the second part of this message? That's the solution I came up with so far in the PR, let me know what you think!
  2. Not really sure I understand the argument against not having AsyncCommand 🤔 We do have the interface, it's just a matter of also providing a reference implementation ready to use - devs who are fine can just use that directly, and devs in need of more control can just reimplement their commands anyway. There's no loss of functionality either way, it's just about also having a concrete type for each interface available out of the box. Additionally, not providing a concrete type would also break the "symmetry" of the available types in the package, where right now we offer a concrete class for each interface we expose, so that devs that are ok with the base functionality don't have to reinvent the wheel on their own every time (eg. Messenger for IMessenger, ObservableObject for INotifyPropertyChanged, RelayCommand for ICommand, etc.).
StephenCleary commented 4 years ago

I like how SetAndNotifyOnCompletion simplifies the implementation, and I love how it allows the VM property to be of type Task<T>, but I'm not wild about how it adds complexity to the end user:

  1. The indirect binding syntax is more awkward.
  2. This approach is more error-prone because the necessity of indirect binding for Result is not obvious. Your docs would all have to be very clear right from the beginning that it is not correct to bind to ViewModel.MyTask.Result and that users need to bind to ex:TaskExtensions.ResultOrDefault(ViewModel.MyTask) instead. And even then, you'll get a number of bug reports about how binding to ViewModel.MyTask.Result doesn't work.

That said, building NotifyTask<T> on SetAndNotifyOnCompletion would be straightforward, if a friendlier API was desired in the future.

Regarding AsyncCommand, it's just my general sense (from helping many, many async MVVM projects) that most people need something different, and that there is no general-purpose AsyncCommand that satisfies everyone or even a majority of people. E.g., let's say it's something like a 4-way tie between incompatible desired implementations. So whatever AsyncCommand you have will only satisfy about 25% of users, and 75% will need to implement IAsyncCommand themselves. I don't have firm numbers but I'm pretty sure there isn't an implementation that satisfies even a majority (50%) of users.

That said, your arguments for including an AsyncCommand of some kind are very valid, and I have no objection to that.

Sergio0694 commented 4 years ago

I'm really glad to hear you like how SetAndNotifyOnCompletion is structured 😊 As for your points:

  1. It is more awkward, I agree, but thankfully that's only needed for the Result property, all the others can be bound to directly. Als I feel like as time goes by, developers should already be quite familiar with the x:Bind to function syntax at this point. And as you said, docs would also have to be very clear on that, absolutely agree.
  2. About this and the point above on the docs, I feel like this would actually be a perfect opportunity to solve multiple problems in one. Just having the Task property is better compared to having a specific type like NotifyTaskCompletion (after all, this is similar to why the #1056 proposal was rejected too, it was essentially the same just for generic T values), it's more efficient (less allocations) and makes the syntax simpler when you're not binding to Result. As for Result, I think that that's such an important topic that every developer working with async code in C# should really understand why they should never use the Task<T>.Result property in their code unless they really know what they're doing, and this would be the perfect chance to make that extremely clear from the docs, so that devs would learn both how to use the new SetAndNotifyOnCompletion method properly and how to bind to Result, and possibly also learn the basics of that, so they could also avoid incurring in the same issue in other parts of their code. After all, the reason why they shouldn't bind to Result is exactly the same why they shouldn't access Result in code behind too. So I really feel like this could both simplify code, be more efficient and be a good learning opportunity for everyone, especially less experienced developers.

And of course, as you mentioned I agree it would be easy to eventually add an actual NotifyTask<T> by using that method, I just feel like for the reasons above, we might as well take this chance to avoid having to do that and improve the general API surface in that area 😄

Regarding AsyncCommand, I see what you're saying. I did try to make the basic implementations as general purpose as possible, but I definitely understand what you mean when you say there will always be devs needing to write their own commands no matter what, that's fair. I'm glad to hear you're not actually against the idea of simply providing some basic implementation too though, that's great 😊

Sergio0694 commented 4 years ago

Alright, so after another conversation with @Aminator I proceeded to make another small refactoring (https://github.com/windows-toolkit/WindowsCommunityToolkit/pull/3229/commits/86d3b49f5b3175f1c27d64efc8036abe434aea2f). The Ioc.ServiceProvider property is now faster as it doesn't lock anymore, and I've removed the IServiceProvider property from ViewModelBase, so that the resulting implementation should now better follow the suggested DI pattern from ASP.NET, and also be closer to the implementation from MvvmLight. For devs relying on the service locator pattern instead of DI (including me), the same exact functionality is still available, just through the default service provider instead of a local property:

Ioc.Default.GetInstance<IFoo>().DoSomethingCool();

This works because Ioc now directly implements IServiceProvider, and relays calls from the extensions from Microsoft.Extension.DependencyInjection to the internal ServiceProvider instance. This means that users can directly use ASP.NET extension methods on the Ioc instance, without even having to go through the ServiceProvider property at all anymore.

Hopefully this makes everyone happy, as the new solution is more adherent to the guidelines for DI, while still being similar to MvvmLight and offering the flexibility of using both approaches (service locator or proper DI) to developers either migrating from MvvmLight or building a new app 😊

michael-hawker commented 4 years ago

Really appreciate everyone's thoughts and support on this topic! 🦙❤ 🎉🎉🎉

I've been heads down with our upcoming 6.1 release, but I'll plan to circle-back on this in June. The biggest open concerns are how this plays in the ecosystem at large and to make sure we can maintain the code and the ideology of leanness here and not just have this continually grow in scope.

I've been thinking it may make the most sense to at least have this as part of a 7.0 preview package to broaden the scope of folks being able to try this out on their own and gather more feedback.

This could also help with conversations with other groups about how this came to be and why having a 'reference' implementation in the toolkit would be important, lower the barrier to entry, but also give them freedom to continue the advanced topics they handle best.

mrlacey commented 4 years ago

Is there an official statement about an end of support for MVVMLight anywhere? I've not seen anything. Seeing something from Laurent would really help understand the justification/motivation here. I was of the understanding that MVVMLight was "Done, not dead." It was meant to be a minimal set of building blocks to follow the MVVM pattern. It was never meant to be a fully-fledged, all-singing-all-dancing framework. It's had this for years and so doesn't need active development. The impression I get from this issue is that due to wanting more than what it offers and because it's not being actively developed a replacement should be created.

If this is intended as a replacement for MVVMLight how/should the lightweight principle of the original be preserved? Will anyone wishing to switch from MVVMLight be guaranteed that they can start with just a namespace change?--Requiring more than this might be hard to justify.

Sergio0694 commented 4 years ago

Hey @mrlacey - thank you for chiming in! I'll try to go through the various points you made as best I can.

"Is there an official statement about an end of support for MVVMLight anywhere?"

I haven't seen an official statement per se, but you can see that the latest commit is from late 2018, and Laurent himelf has commented on this MVVM package on Twitter (here), saying he liked the idea and also mentioning how he doesn't have time to work on MvvmLight anymore.

" It was meant to be a minimal set of building blocks to follow the MVVM pattern. It was never meant to be a fully-fledged, all-singing-all-dancing framework."

The same goes for this MVVM package. You can see that it isn't an all around, complex framework like eg. Prism, it's basically a "reference" implementation of the basic MVVM primitives, in a package that's up to date, modern, lightweight, minimal and efficient. You can see the Key Tenants that @michael-hawker added to the first post in this issue for the main points about this.

"The impression I get from this issue is that due to wanting more than what it offers and because it's not being actively developed a replacement should be created."

It's not really about "wanting to do more" - there are some key differences between MvvmLight and this package. I'll try to briefly go through them to hopefully help clear things up on this point:

"If this is intended as a replacement for MVVMLight how/should the lightweight principle of the original be preserved?"

The lightweight principle is still present. Actually, I'd argue this is even more lightweight in principle, considering the pretty big performance gains and memory efficiency improvements compared to MvvmLight. As far as being simple, as mentioned above this package isn't supposed to be a full "middleware" like Prism or similar libraries, but rather a set of basic APIs to get started with MVVM and related patterns. You can see the full list of available APIs in the first post, which is not at all different than the ones provided in MvvmLight. Actually, here there are slightly less APIs, as we removed some platform-specific ones (eg. a navigation service), to make the package truly platform agnostic.

"Will anyone wishing to switch from MVVMLight be guaranteed that they can start with just a namespace change?--Requiring more than this might be hard to justify."

Switching from MvvmLight will not be just a namespace change, due to some API differences, but I've already done this in one of my apps as a test bench (and @Aminator did the same in his DX12GameEngine project as well), and moving to this package was extremely straightforwards in general. In many cases it really was just a matter of changing namespaces and possibly tweaking a thing or two. Of course, I'll provide a detailed get started guide with an introduction to all the available APIs, as well as code samples that will show how to use every bit of this new package 🚀

Additionally, I don't agree that the hypothetical constriction of offering the same exact API surface just to make the transition slightly easier would be worth it. As I said, this would be the perfect time to make some breaking changes that will be hugely beneficial in the long run, such as switching from yet another custom DI container, to something like Microsoft.Extensions.DependencyInjection, which also comes with advanced features that many developers might want to use, as well as improving the interoperability in general with other libraries, as it uses common BCL interfaces. Or, even just for a performance perspective, I think it's worth it to have some slight changes if you get a major performance/memory efficient boost in return.

Hope this helps! 😊

Sergio0694 commented 4 years ago

I've uploaded a preview package that can be used locally to try out the new APIs for anyone interested, as suggested by @michael-hawker. Updated the first post, reposting here to notify subscribers:

Preview package (built out of https://github.com/windows-toolkit/WindowsCommunityToolkit/pull/3229/commits/c6b4cb652de8a563c0a727d43ff11f344655624e): download here.

cirrusone commented 4 years ago

@Sergio0694 - thanks for this, simple cases on messaging are working really well. One question, does the messenger allow calling async methods on the receiver?

For example, the code below gives the following error on line string response = Messenger.Default.Send<MsMVVMRequestMessageEvent>(); 

System.InvalidOperationException
  HResult=0x80131509
  Message=No response was received for the given request message
  Source=Microsoft.Toolkit.Mvvm

Event:

public class MsMVVMRequestMessageEvent : RequestMessage<string> { }

ViewModel1:

Messenger.Default.Register<MsMVVMRequestMessageEvent>(this, async m =>
{
    string s = await TestMsMVVMMessaging();
    m.ReportResult(s); // Assume "CurrentUser" is some local var/field/property
});

public async Task<string> TestMsMVVMMessaging()
{
    await Task.Delay(5000);
    return DateTime.UtcNow.ToString("yyyy MMM dd HH:mm:ss.fff");
}

ViewModel2:

private void GetData()
{
    string response = Messenger.Default.Send<MsMVVMRequestMessageEvent>();
    Data = response;
}

Is there a way to make this work currently?

It would also be really nice to see some documentation and examples in the future 👍

Sergio0694 commented 4 years ago

Hi @cirrusone - glad to hear you're already trying this out! 😄

The reason why you're seeing that error is that as the callback is asynchronous, the sender continues before that async method completes and thinks no response has been returned (which is true, no response has been returned yet at that point). The whole messenger class is synchronous, but you should be able to work around this by simply making the actual return type of your request message a Task<T>. That is, consider the following example:

// "Async" request message, returns a Task<T>
public sealed class MyAsyncRequestMessage : RequestMessage<Task<string>> { }

// Subscribe to the request
Messenger.Default.Register<MyAsyncRequestMessage>(this, m =>
{
    m.ReportResult(TestMsMVVMMessaging());
});

// Request and wait for the response
string response = await Messenger.Default.Send<MyAsyncRequestMessage>().Result;

The key here is that since the Messenger APIs are all synchronous, you need to synchronously set the resulting Task<T> in the request message (even if that hasn't completed yet, that's totally fine). This way the messenger will see that a response has been returned, and will just relay the Task<T> to await to the original sender, like in the example above.

Also note that in this case you do need to explicitly use .Result on the returned message - the C# compiler isn't able to use the implicit converter automatically when you're using await in front of that, so you need to give it a little hint there 😊

As for the docs, they're currently work in progress as the package hasn't been officially approved yet, but we'll probably start looking into this a bit more starting from next week. I'll publish them as soon as I have a draft I can share, absolutely!

Thanks again for your interest! 🚀

cirrusone commented 4 years ago

Thanks for the example it's working.

I thought awaiting on .Result was blocking?

Perhaps it's related but using the async example with the receiver in the MainWindow.xaml / MainWindowViewModel.cs, if I open a child window whilst awaiting the result using something like _dialogService.Show(nameof(UserControlMVVMChildWindow), new DialogParameters(), null); the next time I call string response = await Messenger.Default.Send<MsMVVMRequestMessageEvent>().Result; results in the following error

System.InvalidOperationException: 'A result has already been reported for the current message'

System.InvalidOperationException
  HResult=0x80131509
  Message=A result has already been reported for the current message
  Source=Microsoft.Toolkit.Mvvm

Sample project

Steps to recreate: 1) Click Launch 2) Click Launch Child from Main VM 3) Click Get Data From MainViewModel Best way using new MS MVVM library 4) Before Result from 4) is returned in 5s, click Launch Window from MainViewModel injecting variables 5) Close new window 6) Repeat 3) gives error

Sergio0694 commented 4 years ago

"I thought awaiting on .Result was blocking?"

It is, but that .Result there is not the Task<T>.Result property, it's the RequestMessage<T>.Result property! 😄 As in, you're getting the resulting response value, which is a Task<T>, and then awaiting that one.

As for the error, as I said that happens if the request message doesn't synchronously receive a response. Be careful not to confuse the usage of sync/async code here: the fact that you're invoking an async method is completely irrelevant here, all that matters for the messenger is that the Task<T> result is set synchronously within the message handler. Also, make sure to register handlers before sending request messages, otherwise they just won't be able to send a reply at all.

I'll see if I can take a look at that repro in a bit 👍

michael-hawker commented 4 years ago

"I thought awaiting on .Result was blocking?"

It is, but that .Result there is not the Task<T>.Result property, it's the RequestMessage<T>.Result property! 😄 As in, you're getting the resulting response value, which is a Task<T>, and then awaiting that one.

Should we think about a different property name or something?

@Sergio0694 let's start working on docs and samples, anyone who uses tech other than UWP that wants to help us is more than welcome to contribute as well! I'm thinking we can create a separate repo in the org here specific to MVVM samples and where we can host some of the initial documentation before release, thoughts?

Sergio0694 commented 4 years ago

"I thought awaiting on .Result was blocking?"

It is, but that .Result there is not the Task<T>.Result property, it's the RequestMessage<T>.Result property! 😄 As in, you're getting the resulting response value, which is a Task<T>, and then awaiting that one.

Should we think about a different property name or something?

Whoops, forgot to update my reply here! I've refactored those APIs quite a bit in a previous PR, they're way less verbose and more user friendly now when specifically dealing with async request messages, in fact there's a dedicated message type now.

Here's how it works:

// "Async" request message, returns a Task<T>
public sealed class MyAsyncRequestMessage : AsyncRequestMessage<string> { }

// Subscribe to the request
Messenger.Default.Register<MyAsyncRequestMessage>(this, m =>
{
    m.Reply(GetSomeStringAsync());
});

// Request and wait for the response
string response = await Messenger.Default.Send<MyAsyncRequestMessage>();

The trick here is that AsyncRequestMessage<T> exposes a GetAwaiter method, so you can now just use await on it directly, and it'll just relay that to the actual Task<T> response contained within the message for you 😊

This will all be properly documented in the docs of course!

Sergio0694 commented 4 years ago

Hey everyone, as @michael-hawker suggested I've linked a (very) draft preview of the docs in the first post 😊

Also including the link here for extra visibility: https://gist.github.com/Sergio0694/987209d0855837ee6835f6e758f5cf40.

mrlacey commented 4 years ago

Hey everyone, as @michael-hawker suggested I've linked a (very) draft preview of the docs in the first post 😊

Also including the link here for extra visibility: https://gist.github.com/Sergio0694/987209d0855837ee6835f6e758f5cf40.

Is there a version that makes it easy to give inline feedback on? (say as part of a PR?)

Sergio0694 commented 4 years ago

@mrlacey I'm working on a draft in a branch on the sample repo, I'll open a PR there as soon as it's finished so you'll be able to just add review comments directly on each line or section you think needs some work or changes 👍

mrlacey commented 4 years ago

I've been looking at how the Calcium framework includes a built-in exception handler for exceptions thrown within messages. Has anything similar been considered/discussed for inclusion here too? I couldn't find anything but this strikes me as a possible nice addition.

N.B. I haven't considered how this might work but just thinking about possibilities.

LeiYangGH commented 4 years ago

is this feature ready to use? I tried install Microsoft.Toolkit.mvvm to my .net framework 4.7.2 application but nuget cannot search such package. Which package in Microsoft.Toolkit is equivalent of mvvmlight?

mrlacey commented 4 years ago

is this feature ready to use? I tried install Microsoft.Toolkit.mvvm to my .net framework 4.7.2 application but nuget cannot search such package. Which package in Microsoft.Toolkit is equivalent of mvvmlight?

This is currently only available in preview (from the MyGet source) It will be released as part of version 7.0

Documentation on migrating from MvvmLight is currently in progress.

Sergio0694 commented 4 years ago

Hey @LeiYangGH - the feature is ready but it's still in early preview for now, so the package is only available on MyGet. If you'de like to try it out, you need to create a nuget.config file next to your .sln project, with this content:

<?xml version="1.0" encoding="utf-8"?>
<configuration>
  <packageSources>
    <add key="ToolkitMyGet" value="https://dotnet.myget.org/F/uwpcommunitytoolkit/api/v3/index.json"/>
  </packageSources>
</configuration>

Then you need to go to the NuGet package manager page for your project and ensure that the ToolkitMyGet feed is selected. From there, you should be able to find the Microsoft.Toolkit.Mvvm package and install it 👍

You can refer to the preview docs that are in the first post for this issue. We'll also be providing more detailed docs and samples shortly 😄

EDIT: ah, @mrlacey beat me to it by 7 seconds 🤣

LeiYangGH commented 4 years ago

thanks a lot! your response is the fastest i've ever seen in github. i'll try myget.

nanney54 commented 4 years ago

Hello, have you plan to add support of cancellation to async command (New class CancelAsyncCommand or add to existing implementation) ? Adding property like IsBusy/progress or whatever would be great too.

Sergio0694 commented 4 years ago

We've considered it, but it seemed like too much functionality for a base implementation to add to the library at least for now. It should be relatively easy to write a custom command extending the async command interface, to also add cancellation support. Of course, it's certainly doable to also just add support for this to the IAsyncRelayCommand interface, though we'd need more feedbacks on this to decide to go forward with this change.

As a reference, with this change the interface would be something like this:

namespace Microsoft.Toolkit.Mvvm.Input
{
    public interface IAsyncRelayCommand : IRelayCommand, INotifyPropertyChanged
    {
        Task? ExecutionTask { get; }
        bool IsRunning { get; }
        bool IsCanceled { get; }

        void Cancel();
        Task ExecuteAsync(object? parameter);
    }
}

And I'd probably make the constructor take a Func<CancellationToken, Task> instead of just Func<Task>, so you'd be able to either just do _ => { ... } if you don't care about cancellation, or just use it within your code as needed. Might be a bit less handy to use for folks using the C# method group syntax though if they were closing over parameterless async methods. But then again this in turn would enable support for doing the same with ones accepting a token. I guess we can have an entire conversation on this suggestion 😄

Pinging @XamlBrewer @jamesmcroft @mrlacey

>>> Further discussions in https://github.com/windows-toolkit/WindowsCommunityToolkit/issues/3428 <<<

This is the issue tracking all new changes to the MVVM Toolkit for the next preview of the library. This issue only referred to the initial preview version and is not tracking the current state of the library.