Added design time support for ViewComponent TagHelpers #98

ajaybhargavb · 2016-09-21T17:30:41Z

ajaybhargavb · 2016-09-22T03:23:54Z

🆙 📅

pranavkm · 2016-09-22T16:34:57Z

src/Microsoft.AspNetCore.Razor.Design/Internal/AssemblyTagHelperDescriptorResolver.cs

+                serviceCollection.AddSingleton<ITagHelperDescriptorFactory>(_tagHelperDescriptorFactory);
+
+                // Populate the service collection
+                _populateMethod.Invoke(_serviceCollectionProviderClassInstance, new object[] { serviceCollection, assemblyName });


Does this happen more than once in the lifetime of the process?

This runs for every assembly that we resolve descriptors from.
Example, in the following command, it runs twice.
dotnet razor-tooling resolve-taghelpers .\project.json ViewComponentTagHelper.Web Microsoft.AspNetCore.Mvc.TagHelpers

pranavkm · 2016-09-22T16:42:39Z

src/Microsoft.AspNetCore.Razor.Design/Internal/AssemblyTagHelperDescriptorResolver.cs

+
+            try
+            {
+                var assembly = Assembly.Load(new AssemblyName("Microsoft.AspNetCore.Mvc"));


var type = Type.GetType("Microsoft.AspNetCore.Mvc.DesignTimeMvcServiceCollectionProvider, Microsoft.AspNetCore.Mvc"); if (type != null) { ... }

Also const strings

pranavkm · 2016-09-22T16:43:54Z

src/Microsoft.AspNetCore.Razor.Design/Internal/AssemblyTagHelperDescriptorResolver.cs

+                LoadMvcAssembly();
+            }
+
+            if (_hasMvcAssembly)


Can we check if _populateMethod is not null instead of having a separate variable?

We could do that but I feel the code looks more readable this way.

pranavkm · 2016-09-22T16:50:19Z

src/Microsoft.AspNetCore.Razor.Design/Internal/AssemblyTagHelperDescriptorResolver.cs

+                    throwOnError: true);
+
+                _serviceCollectionProviderClassInstance = Activator.CreateInstance(providerClass);
+                _populateMethod = providerClass.GetMethod(


Can we do something similar to this - https://github.com/aspnet/HttpAbstractions/blob/e3207557342f514deea4618f595a049bfbd7a024/src/Microsoft.AspNetCore.Http.Extensions/HeaderDictionaryTypeExtensions.cs#L223-L234 - to ensure we're getting the right overload of PopulateServiceCollection

Really sad that we can't just call into AddMvc 😢 .

NTaylorMullen · 2016-09-22T17:07:58Z

src/Microsoft.AspNetCore.Razor.Design/Internal/AssemblyTagHelperDescriptorResolver.cs


        public AssemblyTagHelperDescriptorResolver()
-            : this(new TagHelperTypeResolver())
+            : this(tagHelperTypeResolver: new TagHelperTypeResolver())


no need for named params

NTaylorMullen · 2016-09-22T17:09:07Z

src/Microsoft.AspNetCore.Razor.Design/Internal/AssemblyTagHelperDescriptorResolver.cs

+
+            if (_hasMvcAssembly)
+            {
+                var serviceCollection = new ServiceCollection();


We need to talk to @Eilon about having DI in RazorTooling.

I think we decided this is fine.

NTaylorMullen · 2016-09-22T17:10:22Z

src/Microsoft.AspNetCore.Razor.Design/Internal/AssemblyTagHelperDescriptorResolver.cs

+                var services = serviceCollection.BuildServiceProvider();
+                var tagHelperDescriptorResolver = services.GetRequiredService<ITagHelperDescriptorResolver>();
+
+                var directiveDescriptors = new TagHelperDirectiveDescriptor[]


NTaylorMullen · 2016-09-22T17:14:20Z

src/Microsoft.AspNetCore.Razor.Design/Internal/AssemblyTagHelperDescriptorResolver.cs

@@ -61,6 +52,47 @@ public IEnumerable<TagHelperDescriptor> Resolve(string assemblyName, ErrorSink e
                        DesignResources.InvalidProtocolValue,
                        typeof(TagHelperDescriptor).FullName, ProtocolVersion));
            }
+
+            if (!_loadedAssembly)


Does this ever get hit more than once?

Yes. The Resolve method is called every time you resolve an assembly. https://github.com/aspnet/RazorTooling/blob/dev/src/Microsoft.AspNetCore.Razor.Design/ResolveTagHelpersRunCommand.cs#L42

In which case, could we create a delegate from populateMethod (https://docs.microsoft.com/en-us/dotnet/core/api/system.reflection.methodinfo#System_Reflection_MethodInfo_CreateDelegate_System_Type_)

NTaylorMullen · 2016-09-22T17:16:59Z

src/Microsoft.AspNetCore.Razor.Design/Internal/AssemblyTagHelperDescriptorResolver.cs

+                var services = serviceCollection.BuildServiceProvider();
+                var tagHelperDescriptorResolver = services.GetRequiredService<ITagHelperDescriptorResolver>();
+
+                var directiveDescriptors = new TagHelperDirectiveDescriptor[]


You could share the logic from here on down with the falling back to manually resolving descriptors.

Aka, if MVC isn't in the project:

TagHelperDescriptorResolver tagHelperDescriptorResolver; if (_hasMvcAssembly) { // Do work to get tagHelperDescriptorResolver from MVC } else { tagHelperDescriptorResolver = new TagHelperDescriptorResolver(_tagHelperTypeResolver, _tagHelperDescriptorFactory); } // Build resolution context and resolve // Win

NTaylorMullen

⌚

NTaylorMullen · 2016-09-22T17:26:47Z

src/Microsoft.AspNetCore.Razor.Design/Internal/AssemblyTagHelperDescriptorResolver.cs

+                    throwOnError: true);
+
+                _serviceCollectionProviderClassInstance = Activator.CreateInstance(providerClass);
+                _populateMethod = providerClass.GetMethod(


Really sad that we can't just call into AddMvc 😢 .

NTaylorMullen

BUgz

ajaybhargavb · 2016-09-22T23:56:28Z

🆙 📅 Really sucks that Github doesn't hide outdated comments.

NTaylorMullen

Looks good.

NTaylorMullen · 2016-09-23T00:05:06Z

src/Microsoft.AspNetCore.Razor.Design/Internal/AssemblyTagHelperDescriptorResolver.cs

+        private void LoadMvcAssembly()
+        {
+            const string namespaceName = "Microsoft.AspNetCore.Mvc";
+            const string typeName = "DesignTimeMvcServiceCollectionProvider";


fullTypeName; can combine this with the above.

ajaybhargavb · 2016-09-23T18:55:29Z

🆙 📅

pranavkm · 2016-09-23T19:00:40Z

src/Microsoft.AspNetCore.Razor.Design/Internal/AssemblyTagHelperDescriptorResolver.cs

@@ -61,6 +52,47 @@ public IEnumerable<TagHelperDescriptor> Resolve(string assemblyName, ErrorSink e
                        DesignResources.InvalidProtocolValue,
                        typeof(TagHelperDescriptor).FullName, ProtocolVersion));
            }
+
+            if (!_loadedAssembly)


In which case, could we create a delegate from populateMethod (https://docs.microsoft.com/en-us/dotnet/core/api/system.reflection.methodinfo#System_Reflection_MethodInfo_CreateDelegate_System_Type_)

pranavkm · 2016-09-23T19:01:29Z

src/Microsoft.AspNetCore.Razor.Design/Internal/AssemblyTagHelperDescriptorResolver.cs

+            const string assemblyName = "Microsoft.AspNetCore.Mvc";
+            const string methodName = "PopulateServiceCollection";
+
+            _loadedAssembly = true;


Set this after you've loaded the assembly.

pranavkm · 2016-09-23T19:01:58Z

src/Microsoft.AspNetCore.Razor.Design/Internal/AssemblyTagHelperDescriptorResolver.cs

+
+            try
+            {
+                var providerClass = Type.GetType($"{typeFullName}, {assemblyName}", throwOnError: true);


Why throwOnError? It returns a null if it can't find it by default.

pranavkm · 2016-09-23T19:02:47Z

src/Microsoft.AspNetCore.Razor.Design/Internal/AssemblyTagHelperDescriptorResolver.cs

+                    .FirstOrDefault(methodInfo =>
+                    {
+                        if (string.Equals(methodName, methodInfo.Name, StringComparison.Ordinal)
+                            && methodInfo.ReturnParameter.ParameterType.Equals(typeof(void)))


Probably don't need to check for return type since overloads can only vary by parameters.

pranavkm · 2016-09-23T19:03:31Z

src/Microsoft.AspNetCore.Razor.Design/Internal/AssemblyTagHelperDescriptorResolver.cs

+                        return false;
+                    });
+
+                _hasMvcAssembly = true;


Couldn't we use the existence of the method to know if Mvc is present?

pranavkm · 2016-09-23T19:04:02Z

src/Microsoft.AspNetCore.Razor.Design/Internal/AssemblyTagHelperDescriptorResolver.cs

+
+        private void LoadMvcAssembly()
+        {
+            const string typeFullName = "Microsoft.AspNetCore.Mvc.DesignTimeMvcServiceCollectionProvider";


private const string ... at the top of the class.

pranavkm · 2016-09-23T19:05:40Z

src/Microsoft.AspNetCore.Razor.Design/Internal/AssemblyTagHelperDescriptorResolver.cs

+                        {
+                            var methodParams = methodInfo.GetParameters();
+                            return methodParams.Length == 2
+                                && methodParams[0].ParameterType.Equals(typeof(IServiceCollection))


&& on the previous line.

pranavkm · 2016-09-23T19:05:52Z

...crosoft.AspNetCore.Razor.Design/Internal/AssemblyViewComponentTagHelperDescriptorResolver.cs

+        private bool _hasAssembly;
+        private Type _providerClass;
+        private MethodInfo _populateMethod;
+        //private Type _serviceCollectionExtensionsClass;


pranavkm · 2016-09-23T19:06:47Z

...crosoft.AspNetCore.Razor.Design/Internal/AssemblyViewComponentTagHelperDescriptorResolver.cs

@@ -0,0 +1,160 @@
+// Copyright (c) .NET Foundation. All rights reserved.


Do you still need this type?

Accidental check-in. Removed.

pranavkm · 2016-09-23T22:07:21Z

src/Microsoft.AspNetCore.Razor.Design/Internal/AssemblyTagHelperDescriptorResolver.cs


            try
            {
-                var providerClass = Type.GetType($"{typeFullName}, {assemblyName}", throwOnError: true);
+                var providerClass = Type.GetType($"{TypeFullName}, {MvcAssemblyName}", throwOnError: true);


throwOnError: false

Sorry didn't fully update yet

pranavkm

Almost there 👍

pranavkm · 2016-09-23T22:14:34Z

src/Microsoft.AspNetCore.Razor.Design/Internal/AssemblyTagHelperDescriptorResolver.cs

+            _serviceCollectionProviderClassInstance = Activator.CreateInstance(providerClass);
+
+            // Get the method from the type
+            _populateMethod = providerClass.GetMethods(BindingFlags.Public | BindingFlags.Static)


var populateMethod = ...; _populateMethodDelegate = populateMethod.CreateDelegate(typeof(Action<IServiceCollection, string>));

pranavkm · 2016-09-23T22:16:17Z

src/Microsoft.AspNetCore.Razor.Design/Internal/AssemblyTagHelperDescriptorResolver.cs

@@ -56,6 +56,44 @@ public IEnumerable<TagHelperDescriptor> Resolve(string assemblyName, ErrorSink e
                        DesignResources.InvalidProtocolValue,
                        typeof(TagHelperDescriptor).FullName, ProtocolVersion));
            }
+
+            if (!_isInitialized)


We could move the if check in to the method. I really couldn't find a suitable example in AspNetCore so something from the old world (https://github.com/aspnet/AspNetWebStack/blob/1a987f82d648a95aabed7d35e4c2bee17185c44d/src/System.Web.Http/Routing/RouteCollectionRoute.cs#L41)

ajaybhargavb · 2016-09-23T23:26:39Z

🆙 📅

pranavkm

pranavkm · 2016-09-23T23:31:36Z

src/Microsoft.AspNetCore.Razor.Design/Internal/AssemblyTagHelperDescriptorResolver.cs

+                serviceCollection.AddSingleton<ITagHelperDescriptorFactory>(_tagHelperDescriptorFactory);
+
+                // Populate the service collection
+                _populateMethodDelegate.DynamicInvoke(serviceCollection, assemblyName);


_populateMethodDelegate(serviceCollection, assemblyName);

pranavkm

x2

pranavkm · 2016-09-23T23:32:30Z

src/Microsoft.AspNetCore.Razor.Design/Internal/AssemblyTagHelperDescriptorResolver.cs

        private readonly TagHelperTypeResolver _tagHelperTypeResolver;

+        private bool _isInitialized;
+        private Delegate _populateMethodDelegate;


private Action<IServiceCollection, string> _populateMethodDelegate;

pranavkm · 2016-09-23T23:32:47Z

src/Microsoft.AspNetCore.Razor.Design/Internal/AssemblyTagHelperDescriptorResolver.cs

+                    return false;
+                });
+
+            _populateMethodDelegate = populateMethod?.CreateDelegate(typeof(Action<IServiceCollection, string>));


You'd have to cast here.

dnfclas added the cla-already-signed label Sep 21, 2016

ajaybhargavb mentioned this pull request Sep 21, 2016

Adding design time support for ViewComponent TagHelpers aspnet/Mvc#5304

Merged

pranavkm reviewed Sep 22, 2016

View reviewed changes

pranavkm suggested changes Sep 22, 2016

View reviewed changes

NTaylorMullen reviewed Sep 22, 2016

View reviewed changes

ajaybhargavb force-pushed the ajbaaska/vcth-designtime branch from fd9875c to 69f01f1 Compare September 22, 2016 23:57

NTaylorMullen approved these changes Sep 23, 2016

View reviewed changes

pranavkm suggested changes Sep 23, 2016

View reviewed changes

pranavkm reviewed Sep 23, 2016

View reviewed changes

ajaybhargavb force-pushed the ajbaaska/vcth-designtime branch from c618ddf to fe3fcf2 Compare September 23, 2016 22:12

pranavkm suggested changes Sep 23, 2016

View reviewed changes

pranavkm approved these changes Sep 23, 2016

View reviewed changes

ajaybhargavb force-pushed the ajbaaska/vcth-designtime branch 2 times, most recently from 5b4fc0e to bf3a26f Compare September 24, 2016 00:14

Added design time support for ViewComponent TagHelpers

24a9c54

ajaybhargavb force-pushed the ajbaaska/vcth-designtime branch from bf3a26f to 24a9c54 Compare September 24, 2016 00:54

ajaybhargavb merged commit 24a9c54 into dev Sep 24, 2016

ajaybhargavb deleted the ajbaaska/vcth-designtime branch September 24, 2016 00:54

		@@ -0,0 +1,160 @@
		// Copyright (c) .NET Foundation. All rights reserved.

Added design time support for ViewComponent TagHelpers #98

Added design time support for ViewComponent TagHelpers #98

Conversation

ajaybhargavb commented Sep 21, 2016

ajaybhargavb commented Sep 22, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NTaylorMullen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NTaylorMullen left a comment

Choose a reason for hiding this comment

ajaybhargavb commented Sep 22, 2016

NTaylorMullen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ajaybhargavb commented Sep 23, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pranavkm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ajaybhargavb commented Sep 23, 2016

pranavkm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pranavkm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment