Collaboration on Project #6
Comments
Many thanks.
Sure thing. Happy to see this grow. I will work on adding contribution guidelines. |
|
Super. I will pull the repo and play around. Let me know if there are any ideas you are looking at implementing. |
|
Hi @mishael-o, you expressed interest in collaborating. How is it going and what do you have planned? I ask as I am looking at adding more myself. |
|
Hey @gragra33, it has been a busy start to the year, I haven't had the time to dig deep but these are some improvements I have thought about Features
CI-CD improvements
Code Quality & Docs |
[..trimmed..]
[..trimmed..]
[..trimmed..] Yep, there are some there that I had on my private to-do list. I have several experiments on how to improve and expand. I will try and put some more time into trying to finalise them.
Also, received a request for supporting Any help would be greatly appreciated. |
|
Cool. I can look into the |
I offered the person who made the suggestion to create a PR however there is no reply. If I do not hear from him by the weekend, I am going to add it. |
|
Cool. I will look into |
|
@mishael-o I have pushed changes to the updates branch. How far away are you? If not too far, I will wait. |
I'm going to have a crack at this. I have thoughts on how. The key issue will be with how the It may be that we have an attribute on the ViewModel with a type reference to the Component or a set of Components. The MvvmSample project will be a good test case for building the Source Generator. In particular the SamplePageViewModel. Something like (freehand coding): [MvvmParameters (Component: typeof([HexEntry1), Pameters: ["Param1", "Param2"]]
public class HexEntryViewModel : RecipientViewModelBase<ConvertAsciiToHexMessage>
{
[Parameter]
public string Param1 { get; set; }
[CascadingParameter]
public string Param2 { get; set; }
// etc..
}These will then be added to the Component via the source generator in the file public partial class HexEntry1
{
[Parameter]
public string Param1
{
get => ViewModel!.Param1;
set => ViewModel!.Param1= value;
}
[CascadingParameter]
public string Param2
{
get => ViewModel!.Param2;
set => ViewModel!.Param2= value;
}
// etc..
}Will have to also recognise the Toolkit's observable properties... |
Hey, I have done some work on the auto registration of
How this can be an issue?
I see, but we should be able to get the Components and view model from
I haven't gone too far on this, I will dedicate some time over the weekend, I was playing around with some ideas for public class BaseComponent<T> : ComponentBase, IDisposable
where T : notnull, IViewModelBase
{
[Inject]
protected IParameterResolver ParameterResolver { get; set; } = default!;
[Inject]
public T ViewModel { get; set; } = default!;
public override async Task SetParametersAsync(ParameterView parameters)
{
ParameterResolver.ResolveParameters(parameters, this, ViewModel);
await base.SetParametersAsync(ParameterView.Empty);
await ViewModel.SetParametersAsync();
}
// shortened for brevity
}Parameter Resolver public class ParameterResolver : IParameterResolver
{
private readonly ConcurrentDictionary<Type, IReadOnlyDictionary<string, ParameterInfo>> cachedProperties = new();
private readonly Type typeOfParamaterAttribute = typeof(ParameterAttribute);
private readonly Type typeOfCascadingParameter = typeof(CascadingParameterAttribute);
public void ResolveParameter(KeyValuePair<string, object> parameter, object target)
{
ArgumentNullException.ThrowIfNull(target);
var type = target.GetType();
var objectProperties = GetObjectProperties(type);
ResolveKeyValuePair(parameter, objectProperties, target);
}
public void ResolveParameters(IReadOnlyDictionary<string, object> parameters, object target)
{
ArgumentNullException.ThrowIfNull(target);
ArgumentNullException.ThrowIfNull(parameters);
var type = target.GetType();
var objectProperties = GetObjectProperties(type);
foreach (var kvPair in parameters)
{
ResolveKeyValuePair(kvPair, objectProperties, target);
}
}
public void ResolveParameters(ParameterView parameters, IComponent component, IViewModelBase viewModel)
{
ArgumentNullException.ThrowIfNull(component);
ArgumentNullException.ThrowIfNull(viewModel);
var componentType = component.GetType();
var vmType = viewModel.GetType();
var vmProperties = GetObjectProperties(vmType);
var componentProperties = GetObjectProperties(componentType);
foreach (var item in parameters)
{
if (vmProperties.TryGetValue(item.Name, out var paramterInfo))
{
paramterInfo.PropertyInfo.SetValue(viewModel, item.Value);
continue;
}
if (componentProperties.TryGetValue(item.Name, out paramterInfo))
{
paramterInfo.PropertyInfo.SetValue(component, item.Value);
continue;
}
if (!item.Cascading)
{
throw new InvalidOperationException($"No match for parameter {item.Name} found in component {componentType.FullName} and viewmodel {vmType.FullName}.");
}
}
}
private static void ResolveKeyValuePair(KeyValuePair<string, object> parameter, IReadOnlyDictionary<string, ParameterInfo> objectProperties, object target)
{
if (!objectProperties.TryGetValue(parameter.Key, out var paramterInfo))
{
throw new InvalidOperationException($"No match for parameter {parameter.Key} found in {target.GetType().FullName}.");
}
paramterInfo.PropertyInfo.SetValue(target, parameter.Value);
}
private IReadOnlyDictionary<string, ParameterInfo> GetObjectProperties(Type objectType)
{
if (cachedProperties.TryGetValue(objectType, out var objectProperties))
{
return objectProperties;
}
objectProperties = objectType.GetProperties(BindingFlags.Public | BindingFlags.NonPublic | BindingFlags.Instance)
.Where(p => p.IsDefined(typeOfParamaterAttribute, true) || p.IsDefined(typeOfCascadingParameter, true))
.ToDictionary(p => p.Name, p => new ParameterInfo(p, IsCascadingParameter(p)), StringComparer.OrdinalIgnoreCase);
if (objectProperties.Count == 0)
{
return ImmutableDictionary<string, ParameterInfo>.Empty;
}
cachedProperties.TryAdd(objectType, objectProperties);
return objectProperties;
}
private bool IsCascadingParameter(PropertyInfo propertyInfo)
{
return propertyInfo.IsDefined(typeOfCascadingParameter, true);
}
private class ParameterInfo
{
public ParameterInfo(PropertyInfo propertyInfo)
{
PropertyInfo = propertyInfo;
}
public ParameterInfo(PropertyInfo propertyInfo, bool isCascadingParameter)
{
PropertyInfo = propertyInfo;
IsCascadingParameter = isCascadingParameter;
}
public PropertyInfo PropertyInfo { get; }
public bool IsCascadingParameter { get; }
}
}SampleViewModel public class SampleViewModel: ViewModelBase
{
[CascadingParameter]
public string Title { get; private set; } = default!;
[Parameter]
public int? Id { get; set; }
[Parameter]
[EditorRequired]
public EventCallback<bool> OnEditCompleted { get; set; }
}Sample View @code {
[CascadingParameter]
public string Title { get; private set; } = default!;
[Parameter]
public int? Id { get; set; }
[Parameter]
[EditorRequired]
public EventCallback<bool> OnEditCompleted { get; set; }
}We will need to also cater for attributes like |
Depends on the type of Blazor application and how and where all parts exist. AutoFac is also used for plug-in Frameworks. Then there is Lazy loading.
Interesting approach. My concern is that it is a little reflection-heavy. I have the same current concern with
Yes, I had thought of that with the approach that I was taking with |
@mishael-o The auto registration is nice and clean. Maybe add another option for full auto-discovery on top of selecting specific assemblies. Family commitments disrupted my weekend. I'll let you know when I have something to look at. Then we can compare and make a decision. |
I have added this feature as requested.
No stress. I understand. Still working on the TODOs on the PR. |
I have spotted an issue with .Net 7.0 support. Hosting services that do not support .Net 8.0, like Cloudflare "Pages", will fail to publish with The issue is with your var vmDefinitionAttributeGeneric = vmImplementationType.GetCustomAttribute(customAttributeGenericType);It is throwing the following exception: To replicate the issue, roll back to the following versions for the <ItemGroup>
<PackageReference Include="Microsoft.AspNetCore.Components.WebAssembly" Version="7.*" />
<PackageReference Include="Microsoft.AspNetCore.Components.WebAssembly.DevServer" Version="7.*" PrivateAssets="all" />
<PackageReference Include="Microsoft.AspNetCore.WebUtilities" Version="2.1.1" />
</ItemGroup>I have not looked at finding a solution. |
|
Interesting.....good catch. I will investigate. |
|
@gragra33 , I have implemented a fix/workaround to the branch in the PR. The exception only occurs .Net 7 Blazor wasm, however on Blazor server that code runs fine, which is very odd. I tried digging deep but couldn't find much info on this except these below. They may be related to the underlying issue.
Anywho, this got me thinking, I know we are looking at using source generators, but instead of doing this via reflection, maybe generating the code that adds each VM registration to the services might be a better approach. Idea:Manual registrationpublic partial class MainLayoutViewModel : ViewModelBase
{
}
// Register type
services.AddTransient<MainLayoutViewModel>();Auto registration[ViewModel]
public partial class MainLayoutViewModel : ViewModelBase
{
}
// Generated code for ServicesExtension.cs in ServicesExtension.g.cs
private static partial AddViewModels(IServiceCollection services)
{
services.TryAddTransient<MainLayoutViewModel>();
}Auto registration and ViewModel extensionExample 1: [ViewModel]
public partial class MainLayoutViewModel
{
}
// Generated class MainLayoutViewModel.g.cs
public partial class MainLayoutViewModel : ViewModelBase
{
}
// Generated code for ServicesExtension.cs in ServicesExtension.g.cs
private static partial AddViewModels(IServiceCollection services)
{
services.TryAddTransient<MainLayoutViewModel>();
}Example 2: [ViewModel<ITestNavigationViewModel>(Lifetime = ServiceLifetime.Scoped)]
public partial class TestNavigationViewModel : ITestNavigationViewModel
{
}
// Generated class TestNavigationViewModel .g.cs
public partial class TestNavigationViewModel : ViewModelBase
{
}
// Generated code for ServicesExtension.cs in ServicesExtension.g.cs
private static partial AddViewModels(IServiceCollection services)
{
services.TryAddScoped<ITestNavigationViewModel, TestNavigationViewModel>();
}Let me know what you think. |
Hmmm... Have not looked closely at this. AutoFac can do this easily, however, I am trying to stay with a clear MS DI solution.
A source generator? If you are game. They're not simple to build. However, The CommunityToolkit does have source generators and this looks like it would not be too difficult to modify for this requirement: ObservableObjectGenerator.cs |
Cool, I will have a go at it. If we need a release soon, you can let me know, so I clean up the existing branch in the PR. If not, then I can focus my time on implementing the source generator code. |
Keep going, I will wait. I need to finish working on the other source generator. |
|
@mishael-o It has been a few weeks since your last update. How is it going? |
|
@gragra33 Hey, I have made some progress, just have not had the time to flesh out the solution. Firstly, auto registration of view models via source generators may not be a good solution as it creates generated code in each library where a view model exists and we will need to find and consolidate each of those generated code files and make that callable under the hood when a consumer does Also, I found some interesting issues with source generators and Blazor. You can have a read below, but long story short from .NET 6, Blazor uses source generators to create the backing CS files and code for the Blazor markup, therefore we can't find the components via our source generator as source generators (or the code they generate) cannot interact with each other 😄.
There are two workaround for this:
public partial class Form : MvvmComponentBase<EditContactViewModel>
{
}
<PropertyGroup>
<UseRazorSourceGenerator>false</UseRazorSourceGenerator>
</PropertyGroup>I prefer the first workaround but we can let consumers choose whichever they like, source generator will work on both. Finally, see below a rough draft of the View generator that I am working on. I will have some capacity next week so I will continue then. using System.Diagnostics;
using System.Text;
using Microsoft.CodeAnalysis;
using Microsoft.CodeAnalysis.CSharp;
using Microsoft.CodeAnalysis.CSharp.Syntax;
namespace Blazing.Mvvm.SourceGenerators;
[Generator(LanguageNames.CSharp)]
public class ViewGenerator : IIncrementalGenerator
{
public ViewGenerator()
{
//System.Diagnostics.Debugger.Launch();
}
public void Initialize(IncrementalGeneratorInitializationContext context)
{
// Get views that implement IView<TViewModel> generic interface
// Get the generic type argument of the IView<TViewModel> interface
// Get properties of the generic type argument that have the ParameterAttribute
// Create a partial class of the view with the ViewModel properties
var views = context.SyntaxProvider.CreateSyntaxProvider(
IsSyntaxTargetForGeneration,
GetViewInfo)
.Where(static m => m is not null)!;
context.RegisterSourceOutput(views, GenerateViewCode!);
}
private static bool IsSyntaxTargetForGeneration(SyntaxNode node, CancellationToken cancellationToken)
{
cancellationToken.ThrowIfCancellationRequested();
if (node is not ClassDeclarationSyntax classDeclaration)
{
return false;
}
if (classDeclaration.Modifiers.Any(SyntaxKind.AbstractKeyword))
{
return false;
}
if (!classDeclaration.Modifiers.Any(SyntaxKind.PartialKeyword))
{
return false;
}
var inheritsMvvmBaseType = classDeclaration.BaseList?.Types
.Any(t => t.Type is GenericNameSyntax && t.ToString().StartsWith("MvvmComponentBase<"));
return inheritsMvvmBaseType.GetValueOrDefault();
}
private static ViewInfo? GetViewInfo(GeneratorSyntaxContext context, CancellationToken cancellationToken)
{
cancellationToken.ThrowIfCancellationRequested();
if (context.SemanticModel.GetDeclaredSymbol(context.Node) is not INamedTypeSymbol namedTypeSymbol)
{
return null;
}
var typeArguments = namedTypeSymbol.AllInterfaces
.FirstOrDefault(i => i.Name == "IView");
if (typeArguments is null)
{
return null;
}
var viewModelType = typeArguments.TypeArguments[0];
var prop = viewModelType.GetMembers().OfType<IPropertySymbol>();
var attributes = prop.SelectMany(p => p.GetAttributes());
var properties = viewModelType.GetMembers().OfType<IPropertySymbol>()
.Where(p => p.GetAttributes()
.Any(a => a.AttributeClass?.ToString() == "Microsoft.AspNetCore.Components.ParameterAttribute"))
.Select(p => new PropertyInfo(p.Name, p.Type.ToDisplayString(), p.Type.NullableAnnotation == NullableAnnotation.Annotated))
.ToArray();
if (properties.Length == 0)
{
return null;
}
return new(
namedTypeSymbol.Name,
namedTypeSymbol.ToString(),
namedTypeSymbol.ContainingNamespace?.ToString() ?? string.Empty,
properties);
}
private static void GenerateViewCode(SourceProductionContext context, ViewInfo view)
{
var stringBuilder = new StringBuilder();
stringBuilder.Append($$"""
// <auto-generated />
using Microsoft.AspNetCore.Components;
namespace {{view.Namespace}};
public partial class {{view.Name}}
{
""");
foreach (var property in view.Properties)
{
stringBuilder
.AppendLine()
.Append($$"""
[global::Microsoft.AspNetCore.Components.Parameter]
[global::System.Diagnostics.CodeAnalysis.ExcludeFromCodeCoverage]
[global::System.CodeDom.Compiler.GeneratedCode("Blazing.Mvvm.SourceGenerators.ViewGenerator", "1.0.0")]
public {{property.Type}} {{property.Name}} { get; set; }
""");
if (property.IsNullable)
{
stringBuilder.Append(" = default!;");
}
}
stringBuilder
.AppendLine()
.AppendLine()
.Append("""
[global::System.Diagnostics.CodeAnalysis.ExcludeFromCodeCoverage]
[global::System.CodeDom.Compiler.GeneratedCode("Blazing.Mvvm.SourceGenerators.ViewGenerator", "1.0.0")]
public override global::System.Threading.Tasks.Task SetParametersAsync(global::Microsoft.AspNetCore.Components.ParameterView parameters)
{
foreach (global::Microsoft.AspNetCore.Components.ParameterValue parameter in parameters)
{
var result = SetParameterValue(parameter.Name, parameter.Value);
if (!result && !parameter.Cascading)
{
throw new InvalidOperationException($"No match in ViewModel for parameter {parameter.Name}");
}
}
return base.SetParametersAsync(global::Microsoft.AspNetCore.Components.ParameterView.Empty);
}
[global::System.Diagnostics.CodeAnalysis.ExcludeFromCodeCoverage]
[global::System.CodeDom.Compiler.GeneratedCode("Blazing.Mvvm.SourceGenerators.ViewGenerator", "1.0.0")]
private bool SetParameterValue(string name, object value)
{
switch (name)
{
""");
foreach (var property in view.Properties)
{
stringBuilder
.AppendLine()
.Append($$"""
case "{{property.Name}}":
ViewModel.{{property.Name}} = ({{property.Type}})value;
return true;
""");
}
stringBuilder
.AppendLine()
.Append("""
}
return false;
}
""");
stringBuilder
.AppendLine()
.Append("}");
context.AddSource($"{view.Name}.razor.g.cs", stringBuilder.ToString());
}
}
internal record ViewInfo(
string Name,
string FullName,
string? Namespace,
PropertyInfo[] Properties);
internal record PropertyInfo(
string Name,
string Type,
bool IsNullable); |
|
@mishael-o sounds good. I'll take a look soon. |
|
Hey @gragra33, doing a check-in. Still working on the source generator, and I have made good progress. The core logic is in place, however, I still need to cater for these requirements (that I have picked up).
|
|
@mishael-o How are you going with it? |
Hey @gragra33, still here, I have been off sick for a while. Quick update: The source generator work is taking quite a bit of time to implement, apart from the requirements I mentioned in an earlier comment, I was also thinking that we are enforcing the parameters defined in the ViewModels in the View. If a ViewModel is used by multiple Views and a View would want to opt out of some parameters, how would we handle that? Do you think this is a use case we should cater for? I know you said you are not in a rush to get changes out, however, I think we could do the reflection implementation for the parameters in the meantime and then later on we can implement the changes in the same library or have an additional library called Blazing.Mvvm.SourceGenerators which is opt-in and can override the default reflection-based one, let me know what you think. Other Changes: I dropped the
Finally, I updated the test samples and also did some cleanup. Check out the latest changes in the PR, and let me know if there are areas you want reverted or further improvements. I would prefer this goes in first so we have a base before we start ramping up on the source generator work. |
|
@mishael-o going to have a look this weekend as work here has been crazy busy. |
|
@mishael-o let's close out this pull request and open another the the remainder. |
|
@mishael-o just a quick check to see where you are up to... |
|
Hey @gragra33, still on the tests. I have written most of the unit tests for the updated code and added code comments for improved documentation (I still need to update the readme). Now writing components tests using bunit to validate the behaviours of the updated ViewModel base classes. I will push this week or next. I will update once I have pushed. |
|
Hey @gragra33, I thought I would be done this weekend and push today but I found some interesting weirdness with form validation behaving differently on the server and wasm (caused by changes I made). I'm just trying to iron it out then I will aim to push this week. I have completed all the code changes, comments and tests, just sorting this out then I will push. |
|
@mishael-o Thank you for your contribution, much appreciated. I've merged your PR, and in the process of adding Keyed |
|
@gragra33 No stress. I enjoyed working on the repo. Thanks for inviting me to collaborate; I appreciate it. I'm looking forward to all the cool stuff we will build 😄. |
|
@mishael-o I've pushed my changes for Keyed support in Tests have been added, and the docs have been updated. Feedback is welcome. |
|
@gragra33 cool. I will check it out. |
Hey @gragra33, I went through the updates. However, I think we can implement the resolution of keyed view models in a better way. The implementation you added for @attribute [ViewModelDefinition(Key = nameof(TestKeyedNavigationViewModel))]
@inherits MvvmComponentBase<TestKeyedNavigationViewModel>I understand it would be preferable to resolve view models based on how they were defined but we need to ensure that we don't create unnecessary coupling of SuggestionsI prefer telling the consumers to override the [Inject(Key = "TestKeyedNavigationViewModel")]
protected override TestKeyedNavigationViewModel ViewModel
{
get => base.ViewModel;
set => base.ViewModel = value;
}I don't think Keyed dependencies are being used a lot in code bases so this shouldn't be a train smash. This can be a stop-gap so we take our time to see how we implement this without defining attributes in both the view and view models. Maybe source generators can play a role here for us. MvvmOwningComponentBaseAs for the
These are ideas I have. Tell me what you think. Oh, lastly, FYI some tests are failing also, I will need to check out why they are failing 😅. |
@mishael-o Yes, I tried this, and there is an issue where the Injection happens twice, and the second injection is missing data. It is not a viable option. The method implemented uses the least amount of plumbing required. Also, the sample code uses a concrete class however Interfaces works fine. Here is the update for the page: @attribute [ViewModelDefinition(Key = "TestKeyedNavigationViewModel")]
@inherits MvvmComponentBase<ITestKeyedNavigationViewModel>and the ViewModel: [ViewModelDefinition<ITestKeyedNavigationViewModel>(Key = nameof(TestKeyedNavigationViewModel))]
public sealed class TestKeyedNavigationViewModel(IMvvmNavigationManager mvvmNavigationManager, NavigationManager navigationManager)
: TestNavigationBaseViewModel(mvvmNavigationManager, navigationManager), ITestKeyedNavigationViewModelI've also updated the tests for this removing all concrete references: Services.AddKeyedSingleton<ITestKeyedNavigationViewModel, TestKeyedNavigationViewModel>("TestKeyedNavigationViewModel");and in the test itself: var cutViewModel = GetViewModel<ITestKeyedNavigationViewModel>("TestKeyedNavigationViewModel");
I hadn't touched the other code, so did [not] run those tests. But yes, they are failing for some odd reason. I have fixed all but |
@gragra33 this is intereseting, what scenario/view did this occurs. I am curious about this behaviour because the HexTranslate.razor does this also and I didn't notice any issues. Lastly....I will push a PR tomorrow or Saturday for the test fixes and improvements. |
@mishael-o When working with Keyed Injection. That is when I noticed the behaviour.
I'll update for .Net 9.0 this weekend and do a full review. |
|
Sorry, my busy work schedule and family commitments have slowed me down and last weekend couldn't allocate time to review. The review is now posted and it still requires more work. Please see my comments on the PR. |
|
Congrats ... changes are now live! |
That's great. Finally, V2 is out. |
Hey, great project you have here. I was looking at doing something similar with source generators and the community toolkit but I see you have already started the work here.
If you are open to collaboration, it would be great to add to and grow this project.
The text was updated successfully, but these errors were encountered: