[WIP] Future Concept #437

pakrym · 2016-07-27T22:54:01Z

davidfowl · 2016-07-28T03:12:45Z

...ensions.DependencyInjection.Abstractions/Extensions/ServiceCollectionDescriptorExtensions.cs

@@ -152,7 +150,7 @@ public static class ServiceCollectionDescriptorExtensions
                throw new ArgumentNullException(nameof(implementationType));
            }

-            var descriptor = ServiceDescriptor.Transient(service, implementationType);
+            var descriptor = TypeServiceDescriptor.Transient(service, implementationType);


I think you should leave these factory methods on the ServiceDescriptor

khellang · 2016-07-28T07:59:32Z

src/Microsoft.Extensions.DependencyInjection.Abstractions/EnumerableServiceDescriptor.cs

+
+namespace Microsoft.Extensions.DependencyInjection
+{
+    public class EnumerableServiceDescriptor: ServiceDescriptor


👏

From #416 (comment):

BTW, why are these properties on same type instead of different types?

How do we get to these types from the collection? Just enumerate and switch on type? Since this is a breaking change, does this mean 2,0 is coming pretty soon after 1.1? 😄

Yes you have to downcast. We're willing a limited set of APIs for 1.1. This is one of them.

cwe1ss · 2016-07-28T19:01:27Z

....Extensions.DependencyInjection.Specification.Tests/DependencyInjectionSpecificationTests.cs

@@ -123,7 +124,7 @@ public void TransientServiceCanBeResolvedFromScope()
        }

        [Fact]
-        public void SingleServiceCanBeIEnumerableResolved()
+        public void NonEnumerablServiceCanNotBeIEnumerableResolved()


Typo: "Enumerabl"

cwe1ss · 2016-07-28T19:29:26Z

I'm wondering if it's really required to differentiate between AddOrdered and AddEnumerable?! Since AddOrdered can also be resolved as IEnumerable, why not exclusively use this implementation for all enumerables?

It wouldn't have any disadvantage if all enumerables were ordered and always resolved in order, would it?

Will AddOrdered also support ways to add items somewhere else than at the end? (e.g. at the beginning or somewhere in the middle by specifying an index, ...)? Wouldn't this be required for IStartupFilter etc? Especially being able to add something at the beginning seems really important to me.

cwe1ss · 2016-07-28T19:30:38Z

If there were only one ordered version, the name should still be AddEnumerable though IMO

pakrym · 2016-07-28T21:17:45Z

@cwe1ss we are trying to decrease framework reliance ordered enumerable. Also most dependency injection frameworks support IEnumerable natively which should give better performance and feature set then current Ordered implementation does (uses ActivatorUtilities to create instances of type).

halter73 · 2016-07-28T21:26:29Z

src/Microsoft.Extensions.DependencyInjection.Abstractions/EnumerableServiceDescriptor.cs

@@ -0,0 +1,19 @@
+using System;


nit: missing (c)

halter73 · 2016-07-29T06:28:17Z

src/Microsoft.Extensions.DependencyInjection/Resources.resx

@@ -150,4 +150,8 @@
  <data name="DirectScopedResolvedFromRootException" xml:space="preserve">
    <value>Cannot resolve {1} service '{0}' from root provider.</value>
  </data>
+  <data name="UnsupportedServiceDescriptorType" xml:space="preserve">


Remove from this resx

halter73 · 2016-07-29T18:12:23Z

...xtensions.DependencyInjection.Ordered.Tests/ServiceCollectionOrderedServiceExtensionsTest.cs

+
+namespace Microsoft.Extensions.DependencyInjection
+{
+    public class ServiceCollectionOrderedServiceExtensionsTest


Shouldn't these tests be added to the SpecificationTests to replace RegistrationOrderIsPreservedWhenServicesAreIEnumerableResolved? In theory, containers should get this for "free" once they Populate methods are updated to deal with the new factoring of the existing service descriptors while ignoring the ordered descriptors. Still it would be good to verify this.

Uhhhh, that shouldn't be in the spec tests.

It's free tests for container authors in case they decides to re-implement IOrdered.

pranavkm · 2016-07-30T18:00:03Z

src/Microsoft.Extensions.DependencyInjection.Abstractions/ServiceDescriptor.cs

@@ -173,7 +68,7 @@ public static ServiceDescriptor Transient<TService>(Func<IServiceProvider, TServ
            return Describe(typeof(TService), implementationFactory, ServiceLifetime.Transient);
        }

-        public static ServiceDescriptor Transient(Type service,
+        public static FactoryServiceDescriptor Transient(Type service,


Indentation.

davidfowl · 2016-08-02T02:13:05Z

src/Microsoft.Extensions.DependencyInjection.Abstractions/FactoryServiceDescriptor.cs

+    public class FactoryServiceDescriptor : ServiceDescriptor
+    {
+        /// <summary>
+        /// Initializes a new instance of <see cref="ServiceDescriptor"/> with the specified <paramref name="factory"/>.


Initializes a new instance of FactoryServiceDescriptor

khellang · 2016-08-02T21:33:24Z

src/Microsoft.Extensions.DependencyInjection.Abstractions/FactoryServiceDescriptor.cs

+            ServiceLifetime lifetime)
+            : base(serviceType, lifetime)
+        {
+            if (serviceType == null)


Is there a reason you're checking this in some descriptors, but not in EnumerableServiceDescriptor and OrderedEnumerableServiceDescriptor? Shouldn't thisnull check just be moved up to the base class? I don't believe you ever want ServiceType to be null....

…ered and Enumerable

pakrym added 4 commits July 26, 2016 09:46

EnumerableServiceDescriptor

d1ea6fe

Ordered

6932574

Move files around

116ad6f

Seperate them all

ab2db0b

dnfclas added the cla-already-signed label Jul 27, 2016

davidfowl reviewed Jul 28, 2016
View reviewed changes

davidfowl mentioned this pull request Jul 28, 2016

Changes to DependencyInjection requirements #433

Closed

khellang reviewed Jul 28, 2016
View reviewed changes

Move factory methods back, more tests

05c8e95

cwe1ss reviewed Jul 28, 2016
View reviewed changes

halter73 reviewed Jul 28, 2016
View reviewed changes

pakrym force-pushed the pakrym/di-future branch from d19f6c0 to 11543ff Compare July 28, 2016 22:25

Split ordered into seperate package

cdc4479

pakrym force-pushed the pakrym/di-future branch from 11543ff to cdc4479 Compare July 28, 2016 22:27

halter73 reviewed Jul 29, 2016
View reviewed changes

Remove error message from resources

d437a80

halter73 reviewed Jul 29, 2016
View reviewed changes

pranavkm reviewed Jul 30, 2016
View reviewed changes

pakrym added 2 commits August 1, 2016 09:48

PR comments and make AddOrdered not resolve as IEnumerable

79a4c06

Use OfType<> to search for descriptor

43d44f8

davidfowl reviewed Aug 2, 2016
View reviewed changes

More PR feedback

b46a2f0

khellang reviewed Aug 2, 2016
View reviewed changes

Add support for scoped ordered services

ea30315

pakrym force-pushed the pakrym/di-future branch from 6435bf6 to ea30315 Compare August 18, 2016 17:10

pakrym added 5 commits August 22, 2016 08:31

Add IServiceEditor move extension methods to use it and implement Ord…

1d46155

…ered and Enumerable

Fix overdisposing of singletons

7ec4551

Move extension methods around

0ed8e05

Move some tests out of specs project

8b15017

Unity and Ninject

6f230a6

pakrym closed this Sep 29, 2016

aspnet-hello mentioned this pull request Jan 1, 2018

Changes to DependencyInjection requirements dotnet/aspnetcore#2346

Closed

natemcmaster deleted the pakrym/di-future branch November 2, 2018 16:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[WIP] Future Concept #437

[WIP] Future Concept #437

Uh oh!

pakrym commented Jul 27, 2016

Uh oh!

davidfowl Jul 28, 2016

Uh oh!

khellang Jul 28, 2016

Uh oh!

davidfowl Jul 28, 2016

Uh oh!

cwe1ss Jul 28, 2016

Uh oh!

cwe1ss commented Jul 28, 2016

Uh oh!

cwe1ss commented Jul 28, 2016

Uh oh!

pakrym commented Jul 28, 2016

Uh oh!

halter73 Jul 28, 2016

Uh oh!

halter73 Jul 29, 2016

Uh oh!

halter73 Jul 29, 2016

Uh oh!

davidfowl Jul 29, 2016

Uh oh!

pakrym Jul 29, 2016

Uh oh!

pranavkm Jul 30, 2016

Uh oh!

davidfowl Aug 2, 2016

Uh oh!

khellang Aug 2, 2016

Uh oh!

Uh oh!

[WIP] Future Concept #437

[WIP] Future Concept #437

Uh oh!

Conversation

pakrym commented Jul 27, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cwe1ss commented Jul 28, 2016

Uh oh!

cwe1ss commented Jul 28, 2016

Uh oh!

pakrym commented Jul 28, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!