Skip to content
This repository has been archived by the owner on May 1, 2024. It is now read-only.

Scope management enhancement #109

Open
oleneveu opened this issue Jun 7, 2022 · 7 comments
Open

Scope management enhancement #109

oleneveu opened this issue Jun 7, 2022 · 7 comments
Labels
type: feature New feature or enhancement

Comments

@oleneveu
Copy link

oleneveu commented Jun 7, 2022

Feature description

Hello,

I am in the process of evaluating the technical solutions for a new and relatively large Blazor/WASM project.
We are going to use MVVM, and on this topic I think that MvvmBlazor could help us save a lot of time and energy.

However the fact that all services within a view are resolved to a scope limited to that view could be an issue for us.
I understand that this behavior might be the best one in the general case, and that there is a workaround using the RootServiceProvider.
But this is an option that we might have to rely on quite a lot, and that would likely make our code much less clear.

I would like to suggest a change that would introduce some kind of "hook" so that the services resolution could be customized:

In the MvvmComponentBase class, the ScopeFactory would be declared as IMvvmServiceScopeFactory instead of IServiceScopeFactory.

public interface IMvvmServiceScopeFactory : IServiceScopeFactory { }

The default implementation for that interface would be:

internal class MvvmServiceScopeFactory : IMvvmServiceScopeFactory
{
    private readonly IServiceScopeFactory serviceScopeFactory;

    public MvvmServiceScopeFactory(IServiceScopeFactory serviceScopeFactory)
    {
        this.serviceScopeFactory = serviceScopeFactory;
    }

    public IServiceScope CreateScope() => serviceScopeFactory.CreateScope();
}

It would be registered in AddMvvm()

serviceCollection.AddSingleton<IMvvmServiceScopeFactory, MvvmServiceScopeFactory>();

The current behavior would be unchanged.

I have actually already tested that solution, and implemented a custom IMvvmServiceScopeFactory in a test project.
That implementation relies on an attribute on the classes and constructors parameters (ViewScopeAttribute) to decide on which scope they should be resolved.

I can send a pull request if you think that this feature would benefit to the project.
There should be many alternative solutions to achieve a similar result, we can also discuss them if you are not convinced by this one.

Regards,

Code sample

No response

@oleneveu oleneveu added status: triage Needs to be triaged type: feature New feature or enhancement labels Jun 7, 2022
@klemmchr klemmchr removed the status: triage Needs to be triaged label Jun 7, 2022
@klemmchr
Copy link
Owner

klemmchr commented Jun 7, 2022

I definitely understand the need for alternatives since I'm not happy with the current situation either. The main reason for this pattern was proper support for IDisposable because otherwise the DI container wouldn't understand when to dispose a dependent service.

There should be many alternative solutions to achieve a similar result, we can also discuss them if you are not convinced by this one.

I'm open for every solution to tackle this issue. In my projects I work around this by declaring commonly used services as computed properties in my view model base class but I do understand that this is not a solution that fits every use case.

I can send a pull request if you think that this feature would benefit to the project.

It surely would however I would love to hear alternative solutions as well.

@oleneveu
Copy link
Author

oleneveu commented Jun 7, 2022

Here are a few thought that I have regarding alternative options to handle the scope management.
I define the scopes as:

  • "Application scope": The HttpContext's scope for a Blazor Server application, the same scope as the HttpClient for a Blazor Client application.
  • "View scope": The scope created by the MvvmComponentBase.

New ServiceProvider implementation

My first thought would be to replace the implementation of ServiceProvider for the whole application. This would allow to handle nested scopes in a different way, were a child scope would get a service instance from its parent if it already exists (I hope that my explanation makes sense...).

I have not dug very deep but that seems possible, for instance using WebAssemblyHostBuilder.ConfigureContainer<TBuilder>(IServiceProviderFactory<TBuilder> factory, Action<TBuilder>? configure = null). That would not require any change to the current implementation of MvvmBlazor. The IServiceProviderFactory implementation could be provided as a separate nuget package.

This might however be a source of unexpected side effects, for instance if another library expects the original behavior from the ServiceProvider.

Register the view-modes within MvvmBlazor

Another option might be to register the view-models within MvvmBlazor, with a parameter to control the scope.
Something like:

services.AddMvvm(options =>
{
	options.MvvmServices
		.AddViewModel<MyViewModel1>(ViewModelScope.Application)
		.AddViewModel<MyViewModel2>(ViewModelScope.View);
});

(MvvmServices would not be the standard ServiceCollection implementation, but a specific one).

The MvvmComponentBase could then choose from which service provider it would create the ViewModel.
This solution would be less versatile than using the IMvvmServiceProviderFactory I have suggested first as we would have no control over the service provider used to instantiate the services injected to the view-models.

Thinking a bit more about it, MvvmBlazor could actually use its own IServiceProvider implementation without the need to create a scope. The implementation would search first the application's service provider, then it's own list...

Using a factory to control the scope:

The services could be registered to the ServiceCollection using a factory that will ensure that the service is always resolved at application scope.
Nothing was tested, please consider that as pseudo-code:

public static class DIExtensions
{
	public static IServiceCollection AddMvvmServices(this IServiceCollection services)
	{
		services.AddScoped<ParentScopeAccessor>();
	}
	
	// Register a service that will always be resolved at application scope
	public static IServiceCollection AddApplicationScoped<TService>(this IServiceCollection services)
	{
		return services.AddScoped<TService>(serviceProvider =>
		{
			ParentScopeAccessor accessor = serviceProvider.GetRequiredService<ParentScopeAccessor>();
			IServiceProvider = accessor.ServiceProvider ?? serviceProvider;
			return serviceProvider.GetRequiredService<TService>();
		});
	}
	
	// That method should be used from MvvmComponentBase to create the scope
	internal static IServiceScope CreateScope(this IServiceProvider serviceProvider)
	{
		var scope = serviceProvider.CreateScope();
		// Give an access to the ServiceProvider from the parent scope from within that child scope
		scope.GetRequiredService<ParentScopeAccessor>().ServiceProvider = serviceProvider;
		return scope;
	}
}

public class ParentScopeAccessor
{
	public ServiceProvider? { get; set; }
}

This is however not a viable solution until I have found a way to make sure that the service is disposed from the correct scope (provided that this can be done...).

@oleneveu
Copy link
Author

oleneveu commented Jun 7, 2022

I have pushed the IMvvmServiceFactory implementation on my repo:
https://github.com/oleneveu/MvvmBlazor/commit/4b1af2c02d13a2a28eaac828701385cdb83073fa

@klemmchr
Copy link
Owner

klemmchr commented Jun 8, 2022

Looks good to me, I'm gonna make a deep dive into this topic on the weekend. However, we need to ensure a solution that is non-breaking. I don't want to introduce any breaking changes outside of major upgrades and they should be aligned together with .NET releases.

@klemmchr
Copy link
Owner

I made my thoughts about your solutions:

  • New ServiceProvider implementation: this would be highly intrusive and also could be a problem when people want to use a different IoC container (e.g. Autofac)
  • Registering via AddMvvm: this breaks with the usual flow of using a DI container
  • Factory to control the scope: people would need to register their services in a project that references MvvmBlazor. While this may be the common case it could produce a mess as soon as you have different layers in your application.

I would propose a different solution: decorated constructor parameters.

Consider this to be pseudo code

public class MyViewModel : ViewModelBase
{
    public MyViewModel([InjectFromScope] MyScopedService scoped, [InjectFromRoot] MyRootService root) {}
}

To prevent breaking changes or a cluttered constructor a default injection strategy would be specified

public void ConfigureServices(IServiceCollection services)
{
    services.AddMvvm(o => o.DefaultInjectionStrategy = InjectionStrategy.Scoped);
}

This would allow to toggle the default behavior and would only require to decorate parameters that fall out of the default strategy.

public class MyViewModel : ViewModelBase
{
    public MyViewModel(MyScopedService scoped, [InjectFromRoot] MyRootService root) {}
}

You could specify InjectionStrategy.Root to be used in the whole application. This should resolve the current issue you have.

@oleneveu
Copy link
Author

This is actually the same solution I came to!

I have regenerated the nuget packages from the code from my fork, and injected a custom IMvvmServiceScopeFactory implementation that is using a modified version of the ActivatorUtilities class to instanciate the view-models.
This MvvmActivatorUtilities takes two IServiceProvider as parameters instead of one, and select the one to use based on the presence of a FromViewScopeAttribute, both to instantiate the view-models (is it useful?) and to provide the parameters to the constructors.

For sure, having a default strategy is even better.

My implementation need some rework and might have some flaws: I still haven't done any unit testing as our project is still in a POC stage.
Mainly, I am not sure of the behavior that my implementation have (or even the one that it should have!) when services are injected in cascade (should the attribute be taken in consideration at any level?).

I am really busy with several other aspects of my work at the moment, but if you think that this could be of any help I will extract that part of the code in a distinct project and share it.

@klemmchr
Copy link
Owner

Mainly, I am not sure of the behavior that my implementation have (or even the one that it should have!) when services are injected in cascade (should the attribute be taken in consideration at any level?).

I would argue against that. Deeper down the injection tree the services should be resolved as usual depending on the scope of the root service.

I am really busy with several other aspects of my work at the moment, but if you think that this could be of any help I will extract that part of the code in a distinct project and share it.

Same for me but this shouldn't be too hard to implement. I will try to make a first implementation on the weekend.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
type: feature New feature or enhancement
Projects
None yet
Development

No branches or pull requests

2 participants