Skip to content
This repository was archived by the owner on Feb 25, 2021. It is now read-only.

Add Service Provider Factory support #1623

Closed
wants to merge 2 commits into from
Closed

Add Service Provider Factory support #1623

wants to merge 2 commits into from

Conversation

csnewman
Copy link

Copied implementation from aspnet/Hosting. Allows overriding of service provider implementation.

@danroth27
Copy link
Member

Thanks for the contribution! Could you please also add some tests to verify the feature works? Also, out of curiosity, have you been successful using any 3rd party IoC containers using this change?

@csnewman
Copy link
Author

csnewman commented Nov 12, 2018

@danroth27 I've added a unit test using autofac, I know its not ideal to add that as a test dependency, however its hard to properly test it without using a different DI framework. I can however revert this change, and implement a unit test that instead just checks whether the GetService method is called on a custom service provider (wraps the default ServiceProvider) atleast once, however due to how ServiceProvider is implemented, it means the host.Services type won't be the custom provider type, and the only easy way to achieve this was through using autofac.

Yes, I have Autofac working. It has the same linking issue as json.net, so it just requires the "System.Linq.Expressions*" to be kept in the linker config and to be registered via the Service Provider Factory added by this PR.

@csnewman
Copy link
Author

@danroth27 I assume the AppVeyor build failing can be safely ignored, seeing as travis and blazor-ci both build and the tests pass?

@DaRosenberg
Copy link

@csnewman Nice job on this PR, very surgical and neat. Are you able to use the Startup.ConfigureContainer(ContainerBuilder builder) method after your changes? Or must you pass a configureAction lamda to the AddAutofac() method to be able to do Autofac-specific container configuration?

I wonder if it wouldn't be a good idea to also add support for the Startup.ConfigureServices(IServiceCollection services) returning an IServiceProvider directly, to maintain similarity with aspnet/Hosting? I'm guessing many devs coming from ASP.NET Core would expect that to work.

@csnewman
Copy link
Author

Currently neither are supported. This is the simpler IServiceProviderFactory<IServiceCollection> support as seen in aspnet/Hosting. To use autofac, you have to create a custom IServiceProviderFactory<IServiceCollection>, in which you can do any customisations you wish, including creating the autofac service provider.

I have a planned followup PR that will implement the ConfigureContainer and ConfigureServices support, however those will be much larger pull requests. I think its worth getting this more manual IServiceProviderFactory<IServiceCollection> added first so people can atleast start to use custom DI frameworks and then adding the shortcut methods later in the followup PR. Especially seeing as any code implemented with IServiceProviderFactory<IServiceCollection>, will still work post my followup PR.

@DaRosenberg
Copy link

@csnewman Makes sense to separate these two things, indeed.

Just to clarify though - you say we need to create a custom IServiceProviderFactory<IServiceCollection>, but already with this current PR, isn't it as simple as this?

var host = BlazorWebAssemblyHost
    .CreateDefaultBuilder()
    .ConfigureServices(services => 
    {
        services.AddAutofac(builder =>
        {
            // Interact directly with ContainerBuilder here...
            builder.RegisterModule<CoreModule>();
        });
    })
    .UseBlazorStartup<Startup>()
    .Build();

host.Run();

As far as I understand, the AddAutofac() extension method adds the service provider factory.

@csnewman
Copy link
Author

@DaRosenberg Unfortunately not. As I am following the same principals as aspnet/Hosting, it doesn't work this way.

AddAutofac registers a IServiceProviderFactory<ContainerBuilder>, with ContainerBuilder being a custom Autofac type. In aspnet/Hosting, they check the parameter type of ConfigureContainer(T sometype), and use this to instantiate the correct IServiceProviderFactory<T>. If there is no ConfigureContainer method, they then fallback to IServiceProviderFactory<IServiceCollection>. For AddAutofac to work, it will need the ConfigureContainer method support, to allow us to instantiate the correct type using DI.

Example ServiceProviderFactory for Autofac

private class MyServiceProviderFactory : IServiceProviderFactory<IServiceCollection>
{
    public IServiceCollection CreateBuilder(IServiceCollection services)
    {
        return services;
    }

    public IServiceProvider CreateServiceProvider(IServiceCollection serviceCollection)
    {
        var containerBuilder = new ContainerBuilder();
        containerBuilder.Populate(serviceCollection);
        // TODO: Call your configure action here
        var container = containerBuilder.Build();
        return new AutofacServiceProvider(container);
    }
}

@DaRosenberg
Copy link

@csnewman Ah, got it. Thanks for explaining that. @danroth27 this PR would unblock our development too, so having it merged would be super helpful.

@csnewman csnewman closed this Nov 28, 2018
@csnewman csnewman reopened this Nov 28, 2018
@csnewman
Copy link
Author

@danroth27 I have fixed the merge conflicts, if you wish to review the commit.

@SteveSandersonMS
Copy link
Member

Thanks very much for this, @csnewman! It's now moved to https://github.com/aspnet/AspNetCore/pull/4785/files, where it will be merged as soon as it passes automated verification.

I did tweak the test to eliminate the Autofac dependency (though I do understand why it was a bit awkward and why you chose to use Autofac in the test originally). Mainly I wanted to avoid the external dependency so as to make the test even more explicit about what interactions we're having with other units of code. Hope that's OK!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants