Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feature: Add Microsoft.Extensions.DependencyInjection adapter #362

Closed
RLittlesII opened this issue Jul 20, 2019 · 23 comments · Fixed by #370
Closed

feature: Add Microsoft.Extensions.DependencyInjection adapter #362

RLittlesII opened this issue Jul 20, 2019 · 23 comments · Fixed by #370

Comments

@RLittlesII
Copy link
Member

Add a Microsoft.Extensions.DependencyInjection package

Relates To: #233

@weitzhandler
Copy link
Contributor

weitzhandler commented Jul 21, 2019

There are few things different in Msft DI, in my own project I implemented it the way I found as right but if I'm to PR, I better consult with you guys.

A resolver isn't mutable

In Msft DI, there is no mutable-resolver. There is either configurator or resolver, once the provider is built, it's locked up and you can no longer inject new services.

What I've done is added an intermediate interface IMicrosoftDependencyResolver that provides an UpdateServiceProvider method, that allows plugging in the container once it's ready by the service host etc. if not plugged in, the resolver builds the provider itself if requested.

No registration with string contracts

Msft doesn't provide contract or keying of services.

Should we completely ignore the parameter, raise an exception if it's provided?
Otherwise maybe inherit from ModernDependencyResolver etc. registering and resolving internally using MS DI?

@glennawatson
Copy link
Contributor

David has got around point 1 by rebuilding the locator instance with other resolvers.

Second point you have scoped operations in ms di extensions which are attempting to do the same thing as contracts.

@weitzhandler
Copy link
Contributor

rebuilding the locator instance with other resolvers.

What do you mean? In Splat, during the configure container stage there are calls to the provider (for logging). MS doesn't support this. How do you notify the resolver that its provider is different?

@glennawatson
Copy link
Contributor

See

var builder = new ContainerBuilder();

Essentially unregister on autofac will scan through all items registered. Create a fresh container. Add back all services but the one removed.

@glennawatson
Copy link
Contributor

If that's not feasible then exception is fine

@weitzhandler
Copy link
Contributor

I'm not familiar with AutoFac, but is it also that there is a 'build' point that the service collection gets locked up?

@glennawatson
Copy link
Contributor

Yep most do that. Our general idea it should stop being mutable when you run the extension method to register it with splat

@weitzhandler
Copy link
Contributor

You can't do that, because while splat registers its services, it also tries to consume some of them (for example loggers).

@glennawatson
Copy link
Contributor

Every di we offer btw has a extension method to set it as the di engine inside splat.

@glennawatson
Copy link
Contributor

So until that point it doesn't attempt to consume it.

@weitzhandler
Copy link
Contributor

Wouldn't it be better to dedicate a special interface that provides this 'override'?

master...weitzhandler:master

@glennawatson
Copy link
Contributor

Nope. This keeps it consistent

@weitzhandler
Copy link
Contributor

Oh, I see now. It's the same ex. method over the container. Looks good and less verbose than an interface indeed.

@glennawatson
Copy link
Contributor

So yeah. The idea is you do all your registration either in the splat override or your native di, use the extension method and after that point it's immutable

@weitzhandler
Copy link
Contributor

weitzhandler commented Jul 21, 2019

builder.Update(_componentContext.ComponentRegistry);

How to implement Register since MS DI doesn't offer any Update method?
I thought I'd create a constructor that accepts either an IServiceCollection or a built IServiceProvider does that sound right?

@glennawatson
Copy link
Contributor

Add () is a similar mechanism

@dpvreony
Copy link
Member

autofac's builder.update method is actually obsolete, so the "unregister" rebuild logic will probably have to to be used by add at some point

@RLittlesII
Copy link
Member Author

That was the problem I ran into with Simple Injector. The container is only built once. I thought I found away around it, but I let the branch go stale for other priorities.

I thought you had a working implementation?

This should be no more difficult than hooking your working solution into the same structure as the other DI Adapters already implemented. All the code is generally the same.

There is also some work going on that may change how some of the adapters are implemented. So you might want to be aware to reduce additional efforts.

I'll dig up that branch and see if it can provide a baseline.

@glennawatson
Copy link
Contributor

I think all these containers will be way easier if we just make it immutable with the extension method.

@weitzhandler
Copy link
Contributor

Does it look any better now?
master...weitzhandler:master

@RLittlesII
Copy link
Member Author

One minor observation, it's usually better to work on a branch off master for pull requests.

Looks closer to what I was imagining.

@weitzhandler
Copy link
Contributor

One minor observation, it's usually better to work on a branch off master for pull requests.

Will remember that in the future.

Looks closer to what I was imagining.

So can I focus on making some tests?

@dpvreony
Copy link
Member

just a heads up. I was looking at your branch. your unit test project csproj can be simplified. the directoy.build.props detects test projects and includes xunit etc. also the main unit test package uses msbuild.extras - I would just compare one of the others and tweak :)

also I've included a base unit test class for some of the repeatable tests.

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

Successfully merging a pull request may close this issue.

4 participants