[Feature] Basic MVVM primitives (.NET Standard) #3230
Comments
Sergio0694
added
the
feature request 📬
|
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! |
|
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. |
|
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 Looking forward to seeing where this goes, glad I could at least spark a discussion on the topic! 😊 |
|
@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 |
|
And are these numbers for .Net Native? |
|
@azchohfi Those are numbers for .NET Core 3.1, since Will update the post as soon as I have more results. EDIT: updated the post, the ASP.NET solution is faster than EDIT 2: done, it's all in the first post! 👍 |
|
@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 😄
The situation with singleton services is like before, whereas when retrieving transient ones, this |
|
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? |
|
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 The draft PR that is linked instead was just me experimenting with some ideas in the meantime 😄 |
|
@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). |
|
Hi @sibille - thank you for chiming in! 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! 😄 |
|
I've added the new
You can see that not only is the new 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. |
|
Adding "basic" mvvm tooling has been discussed numerous times, both publicly and privately.
There are a lot of frameworks already out there.
MVVM Light is just one implementation. If it seems that MVVM Light needs a maintainer or work to make it better or "more light" wouldn't it be best to put work into that project? It's already established, it's well known, and it's it's heavily used (as a MVVM framework. Not saying this toolkit isn't used). 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. |
|
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 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. |
|
Hey @hansmbakker - glad you liked the performance improvements in the 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 Having a built-in set of MVVMs types boils down to a few key points:
Regarding what @skendrot said here:
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. |
|
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
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? |
|
@hansmbakker Glad to hear that! 😄 As for that quote, it actually refers to an old version of the PR - both the 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 🚀 |
|
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.Default.ConfigureServices(services =>
{
services.AddSingleton<ILogger, Logger>();
services.AddSingleton<IPrinter, PrinterService>();
// ...
});The This solution has the advantage of being both easy to use, highly customizable, and using the same APIs from 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
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
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 |
|
Really happy to see the direction you took! |
|
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.) |
|
@hansmbakker Sergio wants to explicitly not use constructor injection and instead uses the anti-pattern where you access the 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 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 |
@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.
@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. |
|
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 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
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:
|
|
Regarding my MVVM helpers:
|
|
Hey @StephenCleary - thank you for chiming in!
|
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? |
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 This will all be properly documented in the docs of course! |
|
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?) |
|
@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 👍 |
|
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. |
|
is this feature ready to use? I tried install |
This is currently only available in preview (from the MyGet source) Documentation on migrating from MvvmLight is currently in progress. |
|
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 <?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 You can refer to the preview docs that are in the first post for this issue. EDIT: ah, @mrlacey beat me to it by 7 seconds 🤣 |
|
thanks a lot! your response is the fastest i've ever seen in github. i'll try myget. |
|
Hello, |
|
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 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 Pinging @XamlBrewer @jamesmcroft @mrlacey >>> Further discussions in #3428 <<<This is the issue tracking all new changes to the MVVM Toolkit for the next preview of the library. |
## Follow up for #3230, part of #3428 <!-- Add the relevant issue number after the "#" mentioned above (for ex: Fixes #1234) which will automatically close the issue once the PR is merged. --> <!-- Add a brief overview here of the feature/bug & fix. --> ## PR Type What kind of change does this PR introduce? <!-- Please uncomment one or more that apply to this PR. --> - Optimization <!-- - Bugfix --> <!-- - Feature --> <!-- - Code style update (formatting) --> <!-- - Refactoring (no functional changes, no api changes) --> <!-- - Build or CI related changes --> <!-- - Documentation content changes --> <!-- - Sample app changes --> <!-- - Other... Please describe: --> ## Overview This PR includes two main parts: an improvement for the memory pooling buffers, and a refactor of how delegates are registered for message handlers. This last part allows to completely remove local closures for message handlers 🚀 ### Memory pooling improvements We're using memory pooling (by relying on the `ArrayPool<T>` APIs) in the `Messenger` class to achieve an amortized 0-allocation execution of the `Send` method in particular (`UnregisterAll` uses this too). We have code like this: https://github.com/windows-toolkit/WindowsCommunityToolkit/blob/5bf426523cf456fc13db7b6505c56cad380d5f5f/Microsoft.Toolkit.Mvvm/Messaging/Messenger.cs#L250 https://github.com/windows-toolkit/WindowsCommunityToolkit/blob/5bf426523cf456fc13db7b6505c56cad380d5f5f/Microsoft.Toolkit.Mvvm/Messaging/Messenger.cs#L401 This works just fine, and we do get the 0-allocation after the initial first invocation where we rent the buffer. There are two downsides here though: - In the `Send` method, we rent an `Action<TMessage>` buffer, so we'll rent a different buffer for every message type - Given that these types are either internal or not likely to ever by used by consumers of the library, all these rented buffers would only ever by used by the `Messenger` class itself, and the consumers would not be able to reuse them on their own. Neither of these points is a big deal and the current implementation is fine, but I think we can do better 😄 **Idea:** let's leverage the fact that arrays are covariant, and only use a single type to solve both problems, like so: ```csharp object[] maps = ArrayPool<object>.Shared.Rent(count); // Do stuff, example: foreach (object obj in maps) { var myActualItem = (SomeFancyTypePossiblyInternal)obj; // Do stuff with the item... } ``` This both allows us to just use `object[]` arrays, which both reduces the total number of rented arrays (as we can reuse them for different message types), and also makes it so that these rented arrays might potentially also be reused by consumers of the library (should they ever need to pool `object[]` arrays), further reducing allocations 🚀 ## Benchmarks Here are some benchmarks comparing the `Messenger` from this PR with the one in the `Preview1`. ### Sending messages | Method | Categories | Mean | Error | StdDev | Ratio | Gen 0 | Gen 1 | Allocated | |--------- |----------------- |-------------:|----------:|----------:|------:|------:|------:|----------:| | Old_Send | DefaultChannel | 161.7 us | 1.52 us | 1.43 us | 1.00 | - | - | - | | **New_Send** | DefaultChannel | **153.9 us** | 1.62 us | 1.43 us | 0.95 | - | - | - | | | | | | | | | | | | Old_Send | MultipleChannels | 148,668.7 us | 886.65 us | 785.99 us | 1.00 | - | - | 336 B | | **New_Send** | MultipleChannels | **138,825.0 us** | 797.61 us | 746.08 us | 0.93 | - | - | 336 B | Performance when sending message is slightly faster than before. Worst case scenario, it's not any slower. ### Registering messages | Method | Mean | Error | StdDev | Ratio | Gen 0 | Gen 1 | Allocated | |------------- |---------:|--------:|--------:|------:|---------:|--------:|----------:| | Old_Register | 443.6 us | 1.73 us | 1.53 us | 1.00 | 113.7695 | 25.3906 | 581.53 KB | | **New_Register** | **386.1 us** | 2.47 us | 2.31 us | 0.87 | 82.5195 | 4.3945 | **381.53 KB** | The new version is **13%** faster when registering messages, and uses **34%** less memory 🚀 ### Enabled recipient type-specific handlers One major annoyance for users working with manually registered handlers was the fact that type information was lost. As in, recipients were registered as just `object` instances, as it was necessary to cast them in the handler every time. This PR also changes this by adding support for a type parameter to specify the recipient type. This enables the following change (which is totally optional anyway, you can still just use `object` if you want): ```csharp // Before Messenger.Register<MyMessage>(this, (s, m) => ((MyViewModel)s).SomeMethod(m)); // After Messenger.Register<MyViewModel, MyMessage>(this, (s, m) => s.SomeMethod(m)); ``` ### Removed local closures The original implementation used `Action<T>` for handlers, which caused closures to be constantly created whenever the users wanted to access any local member on the registered recipient. This was because the recipient itself needed to be captured too to be accessed from the handlers. This detail also made it more difficult for other devs to implement `IMessenger` if they wanted to use a weak reference system. I've replaced that handler type with a custom delegate, called `MessageHandler`: https://github.com/windows-toolkit/WindowsCommunityToolkit/blob/32656db1cdd4ddc25e3a88c297ce4062fe64d2ad/Microsoft.Toolkit.Mvvm/Messaging/IMessenger.cs#L10-L22 This delegate also receives the target recipient as input, which allows developers to just use that to access local members, without the need to create closures. The handlers are now essentially static, so the C# compiler can cache the whole delegate too. This is especially useful for the `IRecipient<TMessage>` pattern, as that was previously creating unnecessary closures when registering handlers - that's completely gone now and delegates are cached there as well 🎉 For instance, you can see that here: https://github.com/windows-toolkit/WindowsCommunityToolkit/blob/32656db1cdd4ddc25e3a88c297ce4062fe64d2ad/Microsoft.Toolkit.Mvvm/Messaging/IMessengerExtensions.cs#L175-L179 We're now using that cached static delegate (will be able to also explicitly mark that as static when C# 9 lands, but it already is) instead of the method group syntax on `recipient.Receive`, which allocated a closure for each invocation. ## PR Checklist Please check if your PR fulfills the following requirements: - [X] Tested code with current [supported SDKs](../readme.md#supported) - [ ] ~~Pull Request has been submitted to the documentation repository [instructions](..\contributing.md#docs). Link: <!-- docs PR link -->~~ - [ ] ~~Sample in sample app has been added / updated (for bug fixes / features)~~ - [ ] ~~Icon has been created (if new sample) following the [Thumbnail Style Guide and templates](https://github.com/windows-toolkit/WindowsCommunityToolkit-design-assets)~~ - [X] Tests for the changes have been added (for bug fixes / features) (if applicable) - [X] Header has been added to all new source files (run *build/UpdateHeaders.bat*) - [X] Contains **NO** breaking changes
ghost
locked as resolved and limited conversation to collaborators
Describe the problem this feature would solve
Follow up from the conversation with @michael-hawker on the UWP Community Discord server.
With the
MvvmLightlibrary 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 newMicrosoft.Toolkit.Mvvmpackage so that users not in need of those wouldn't have to reference those too. We're also using this occasion to integrate with theMicrosoft.Extensions.DependencyInjectionpackage 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.Mvvmpackage.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:
ObservableObjectViewModelBaseIocRelayCommandRelayCommand<T>AsyncRelayCommandAsyncRelayCommand<T>IRelayCommandIRelayCommand<in T>IAsyncRelayCommandIAsyncRelayCommand<in T>MessengerIMessengerPropertyChangedMessage<T>RequestMessage<T>AsyncRequestMessage<T>CollectionRequestMessage<T>AsyncCollectionRequestMessage<T>ValueChangedMessage<T>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
autofacorPrism, 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
Messengerclass from bothMvvmLightandCalcium, and the one from the related PR (WCTstands for this newMvvmpackage). Note thatCalciumonly supports a single channel mode:Some summarized results for the
Messengerclass from #3229:MvvmLight, and ~11% faster thanCalcium. At the same time, it uses about 38 million times less memory thanMvvmLight, specifically just 20 bytes vs. almost 700MB (!), and about 387,000x less memory thanCalciumas well, specifically again just 20 bytes vs. about 7 MB.MvvmLight, and using almost 7 million times less memory, specifically just 416 bytes vs almost 3 GB (!).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.Mvvmpackage, integrating some features fromMvvmLight, refactoring them when needed, in order to offer a self-contained alternative. The package would not need a reference to any otherMicrosoft.Toolkitpackages, 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.DependencyInjectionpackage, 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 actualMvvmpackage, 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
INotifyPropertyChangedorICommand).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,FodyorPrismare much bigger in scope than what many devs typically need and introduce more overhead.MvvmLighton the other hand is also not supported anymore, other than some forks.The text was updated successfully, but these errors were encountered: