search

unitycontainer / microsoft-dependency-injection

Unity.Microsoft.DependencyInjection package
Apache License 2.0
63 stars 36 forks source link

Scoped provider now used for factory methods #64

Closed jamesdoran closed 4 years ago

jamesdoran commented 4 years ago

New PR based off release/5.11 as requested in #63

Added ScopedDependencyFromTransientFactoryNotSharedAcrossScopes test which fails when checking that different instances of scoped are checked to be different in different scopes.

When calling the factory method, the scoped provider is now always passed to the delegate. Previously, the root container was being passed for transients, so if a transient had a scoped dependency it would be resolved from the root too and would be used across scopes.

jamesdoran commented 4 years ago

Thanks for the quick merge.

I noticed those 2 failing unit tests have been fixed too - nice work :) On that note, does the build fail if any tests fail? As it said it was passing previously with the failures. Tried to follow the appveyor link but got Project not found or access denied. so couldn't investigate further.

ENikS commented 4 years ago

Tests are disabled at the moment at build. https://ci.appveyor.com/project/unitycontainer/microsoft-dependency-injection-02uof

I'll work on the fix tomorrow.

jamesdoran commented 4 years ago

Thanks for the link. Do you plan to enable them now you've got those other tests passing too?

ENikS commented 4 years ago

I am planning to fix it up and release 2.2 compatible version this week.

jamesdoran commented 4 years ago

Awesome, good luck with that.

jamesdoran commented 4 years ago

@ENikS, is there any chance of an updated nuget package with this fix in please? I tried adding a submodule to this project to keep me moving, but it caused a heap of nuget errors.

jamesdoran commented 4 years ago

Thanks for pushing a new nuget package @ENikS . Unfortunately 5.11 seems to have lost my commit (it's no longer in the repo), so the bug is still present.

Here's the commit in my fork for reference: https://github.com/jamesdoran/microsoft-dependency-injection/commit/228d98d94d2c0e0edac0dc931b436fe443901937

I've tried pulling down the latest 5.11 branch and running unit tests, but am getting the same test aborted error too.

jamesdoran commented 4 years ago

Here's the test added against 5.11 to show the bug is still present: https://github.com/jamesdoran/microsoft-dependency-injection/tree/show-bug-in-5-11

ENikS commented 4 years ago

You are right, missed that one. Released 5.11.1