Design review: Change AddNodeServices to take an Action<NodeServicesOptions> like other aspects of MVC DI config

Author: SteveSandersonMS

samples/misc/LatencyTest/Program.cs

@@ -15,10 +15,10 @@ public class Program
         public static void Main(string[] args) {
             // Set up the DI system
             var services = new ServiceCollection();
-            services.AddNodeServices(new NodeServicesOptions {
-                HostingModel = NodeServicesOptions.DefaultNodeHostingModel,
-                ProjectPath = Directory.GetCurrentDirectory(),
-                WatchFileExtensions = new string[] {} // Don't watch anything
+            services.AddNodeServices(options => {
+                options.HostingModel = NodeServicesOptions.DefaultNodeHostingModel;
+                options.ProjectPath = Directory.GetCurrentDirectory();
+                options.WatchFileExtensions = new string[] {}; // Don't watch anything
             });
             var serviceProvider = services.BuildServiceProvider();
 

src/Microsoft.AspNetCore.NodeServices/Configuration/Configuration.cs

@@ -0,0 +1,43 @@
+using System;
+using Microsoft.AspNetCore.NodeServices.HostingModels;
+
+namespace Microsoft.AspNetCore.NodeServices
+{
+    public static class NodeServicesFactory
+    {
+        public static INodeServices CreateNodeServices(NodeServicesOptions options)
+        {
+            if (options == null)
+            {
+                throw new ArgumentNullException(nameof (options));
+            }
+            
+            return new NodeServicesImpl(() => CreateNodeInstance(options));
+        }
+
+        private static INodeInstance CreateNodeInstance(NodeServicesOptions options)
+        {
+            if (options.NodeInstanceFactory != null)
+            {
+                // If you've explicitly supplied an INodeInstance factory, we'll use that. This is useful for
+                // custom INodeInstance implementations.
+                return options.NodeInstanceFactory();
+            }
+            else
+            {
+                switch (options.HostingModel)
+                {
+                    case NodeHostingModel.Http:
+                        return new HttpNodeInstance(options.ProjectPath, options.WatchFileExtensions, options.NodeInstanceOutputLogger, 
+                            options.EnvironmentVariables, options.LaunchWithDebugging, options.DebuggingPort, /* port */ 0);
+                    case NodeHostingModel.Socket:
+                        var pipeName = "pni-" + Guid.NewGuid().ToString("D"); // Arbitrary non-clashing string
+                        return new SocketNodeInstance(options.ProjectPath, options.WatchFileExtensions, pipeName, options.NodeInstanceOutputLogger,
+                            options.EnvironmentVariables, options.LaunchWithDebugging, options.DebuggingPort);
+                    default:
+                        throw new ArgumentException("Unknown hosting model: " + options.HostingModel);
+                }
+            }
+        }
+    }
+}

src/Microsoft.AspNetCore.NodeServices/Configuration/NodeServicesFactory.cs

@@ -2,28 +2,52 @@
 using System.Collections.Generic;
 using Microsoft.AspNetCore.NodeServices.HostingModels;
 using Microsoft.Extensions.Logging;
+using Microsoft.Extensions.DependencyInjection;
+using Microsoft.AspNetCore.Hosting;
+using Microsoft.Extensions.Logging.Console;
 
 namespace Microsoft.AspNetCore.NodeServices
 {
     public class NodeServicesOptions
     {
         public const NodeHostingModel DefaultNodeHostingModel = NodeHostingModel.Http;
-
+        private const string LogCategoryName = "Microsoft.AspNetCore.NodeServices";
         private static readonly string[] DefaultWatchFileExtensions = { ".js", ".jsx", ".ts", ".tsx", ".json", ".html" };
 
-        public NodeServicesOptions()
+        public NodeServicesOptions(IServiceProvider serviceProvider)
         {
+            if (serviceProvider == null)
+            {
+                throw new ArgumentNullException(nameof (serviceProvider));
+            }
+
+            EnvironmentVariables = new Dictionary<string, string>();
             HostingModel = DefaultNodeHostingModel;
             WatchFileExtensions = (string[])DefaultWatchFileExtensions.Clone();
+
+            // In an ASP.NET environment, we can use the IHostingEnvironment data to auto-populate a few
+            // things that you'd otherwise have to specify manually
+            var hostEnv = serviceProvider.GetService<IHostingEnvironment>();
+            if (hostEnv != null)
+            {
+                ProjectPath = hostEnv.ContentRootPath;
+                EnvironmentVariables["NODE_ENV"] = hostEnv.IsDevelopment() ? "development" : "production"; // De-facto standard values for Node
+            }
+
+            // If the DI system gives us a logger, use it. Otherwise, set up a default one.
+            var loggerFactory = serviceProvider.GetService<ILoggerFactory>();
+            NodeInstanceOutputLogger = loggerFactory != null
+                ? loggerFactory.CreateLogger(LogCategoryName)
+                : new ConsoleLogger(LogCategoryName, null, false);
         }
-        public Action<System.Diagnostics.ProcessStartInfo> OnBeforeStartExternalProcess { get; set; }
+
         public NodeHostingModel HostingModel { get; set; }
         public Func<INodeInstance> NodeInstanceFactory { get; set; }
         public string ProjectPath { get; set; }
         public string[] WatchFileExtensions { get; set; }
         public ILogger NodeInstanceOutputLogger { get; set; }
         public bool LaunchWithDebugging { get; set; }
         public IDictionary<string, string> EnvironmentVariables { get; set; }
-        public int? DebuggingPort { get; set; }
+        public int DebuggingPort { get; set; }
     }
 }

src/Microsoft.AspNetCore.NodeServices/Configuration/NodeServicesOptions.cs

@@ -0,0 +1,41 @@
+using System;
+using Microsoft.AspNetCore.NodeServices;
+
+namespace Microsoft.Extensions.DependencyInjection
+{
+    /// <summary>
+    /// Extension methods for setting up NodeServices in an <see cref="IServiceCollection" />.
+    /// </summary>
+    public static class NodeServicesServiceCollectionExtensions
+    {
+        public static void AddNodeServices(this IServiceCollection serviceCollection)
+            => AddNodeServices(serviceCollection, _ => {});
+
+        [Obsolete("Use the AddNodeServices(Action<NodeServicesOptions> setupAction) overload instead.")]
+        public static void AddNodeServices(this IServiceCollection serviceCollection, NodeServicesOptions options)
+        {
+            serviceCollection.AddSingleton(typeof (INodeServices), _ =>
+            {
+                return NodeServicesFactory.CreateNodeServices(options);
+            });
+        }
+
+        public static void AddNodeServices(this IServiceCollection serviceCollection, Action<NodeServicesOptions> setupAction)
+        {
+            if (setupAction == null)
+            {
+                throw new ArgumentNullException(nameof (setupAction));
+            }
+
+            serviceCollection.AddSingleton(typeof(INodeServices), serviceProvider =>
+            {
+                // First we let NodeServicesOptions take its defaults from the IServiceProvider,
+                // then we let the developer override those options
+                var options = new NodeServicesOptions(serviceProvider);
+                setupAction(options);
+
+                return NodeServicesFactory.CreateNodeServices(options);
+            });
+        }
+    }
+}

src/Microsoft.AspNetCore.NodeServices/Configuration/NodeServicesServiceCollectionExtensions.cs

@@ -35,7 +35,7 @@ internal class HttpNodeInstance : OutOfProcessNodeInstance
         private int _portNumber;
 
         public HttpNodeInstance(string projectPath, string[] watchFileExtensions, ILogger nodeInstanceOutputLogger,
-            IDictionary<string, string> environmentVars, bool launchWithDebugging, int? debuggingPort, int port = 0)
+            IDictionary<string, string> environmentVars, bool launchWithDebugging, int debuggingPort, int port = 0)
         : base(
                 EmbeddedResourceReader.Read(
                     typeof(HttpNodeInstance),

src/Microsoft.AspNetCore.NodeServices/HostingModels/HttpNodeInstance.cs

@@ -49,7 +49,7 @@ public OutOfProcessNodeInstance(
             ILogger nodeOutputLogger,
             IDictionary<string, string> environmentVars,
             bool launchWithDebugging,
-            int? debuggingPort)
+            int debuggingPort)
         {
             if (nodeOutputLogger == null)
             {
@@ -101,12 +101,12 @@ public void Dispose()
         // This method is virtual, as it provides a way to override the NODE_PATH or the path to node.exe
         protected virtual ProcessStartInfo PrepareNodeProcessStartInfo(
             string entryPointFilename, string projectPath, string commandLineArguments,
-            IDictionary<string, string> environmentVars, bool launchWithDebugging, int? debuggingPort)
+            IDictionary<string, string> environmentVars, bool launchWithDebugging, int debuggingPort)
         {
             string debuggingArgs;
             if (launchWithDebugging)
             {
-                debuggingArgs = debuggingPort.HasValue ? $"--debug={debuggingPort.Value} " : "--debug ";
+                debuggingArgs = debuggingPort != default(int) ? $"--debug={debuggingPort} " : "--debug ";
                 _nodeDebuggingPort = debuggingPort;
             }
             else

src/Microsoft.AspNetCore.NodeServices/HostingModels/OutOfProcessNodeInstance.cs

@@ -40,7 +40,7 @@ internal class SocketNodeInstance : OutOfProcessNodeInstance
 
         public SocketNodeInstance(string projectPath, string[] watchFileExtensions, string socketAddress,
             ILogger nodeInstanceOutputLogger, IDictionary<string, string> environmentVars,
-            bool launchWithDebugging, int? debuggingPort)
+            bool launchWithDebugging, int debuggingPort)
         : base(
                 EmbeddedResourceReader.Read(
                     typeof(SocketNodeInstance),