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

samples/misc/LatencyTest/Program.cs

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

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

@@ -1,91 +0,0 @@
-using System;
-using Microsoft.Extensions.DependencyInjection;
-using Microsoft.AspNetCore.Hosting;
-using Microsoft.AspNetCore.NodeServices.HostingModels;
-using Microsoft.Extensions.Logging;
-using Microsoft.Extensions.Logging.Console;
-using System.Collections.Generic;
-
-namespace Microsoft.AspNetCore.NodeServices
-{
-    public static class Configuration
-    {
-        const string LogCategoryName = "Microsoft.AspNetCore.NodeServices";
-
-        public static void AddNodeServices(this IServiceCollection serviceCollection)
-            => AddNodeServices(serviceCollection, new NodeServicesOptions());
-
-        public static void AddNodeServices(this IServiceCollection serviceCollection, NodeServicesOptions options)
-        {
-            serviceCollection.AddSingleton(
-                typeof(INodeServices),
-                serviceProvider => CreateNodeServices(serviceProvider, options));
-        }
-
-        public static INodeServices CreateNodeServices(IServiceProvider serviceProvider, NodeServicesOptions options)
-        {
-            return new NodeServicesImpl(() => CreateNodeInstance(serviceProvider, options));
-        }
-
-        private static INodeInstance CreateNodeInstance(IServiceProvider serviceProvider, 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
-            {
-                // Otherwise we'll construct the type of INodeInstance specified by the HostingModel property
-                // (which itself has a useful default value), plus obtain config information from the DI system.
-                var projectPath = options.ProjectPath;
-                var envVars = options.EnvironmentVariables == null
-                    ? new Dictionary<string, string>()
-                    : new Dictionary<string, string>(options.EnvironmentVariables);
-
-                var hostEnv = serviceProvider.GetService<IHostingEnvironment>();
-                if (hostEnv != null)
-                {
-                    // 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
-                    if (string.IsNullOrEmpty(projectPath))
-                    {
-                        projectPath = hostEnv.ContentRootPath;
-                    }
-
-                    // Similarly, we can determine the 'is development' value from the hosting environment
-                    if (!envVars.ContainsKey("NODE_ENV"))
-                    {
-                        // These strings are a de-facto standard in Node
-                        envVars["NODE_ENV"] = hostEnv.IsDevelopment() ? "development" : "production";
-                    }
-                }
-
-                // If no logger was specified explicitly, we should use the one from DI.
-                // If it doesn't provide one, we'll set up a default one.
-                var logger = options.NodeInstanceOutputLogger;
-                if (logger == null)
-                {
-                    var loggerFactory = serviceProvider.GetService<ILoggerFactory>();
-                    logger = loggerFactory != null
-                        ? loggerFactory.CreateLogger(LogCategoryName)
-                        : new ConsoleLogger(LogCategoryName, null, false);
-                }
-
-                switch (options.HostingModel)
-                {
-                    case NodeHostingModel.Http:
-                        return new HttpNodeInstance(projectPath, options.WatchFileExtensions, logger, 
-                            envVars, options.LaunchWithDebugging, options.DebuggingPort, /* port */ 0);
-                    case NodeHostingModel.Socket:
-                        var pipeName = "pni-" + Guid.NewGuid().ToString("D"); // Arbitrary non-clashing string
-                        return new SocketNodeInstance(projectPath, options.WatchFileExtensions, pipeName, logger,
-                            envVars, options.LaunchWithDebugging, options.DebuggingPort);
-                    default:
-                        throw new ArgumentException("Unknown hosting model: " + options.HostingModel);
-                }
-            }
-        }
-    }
-}

src/Microsoft.AspNetCore.NodeServices/Configuration/NodeServicesFactory.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/NodeServicesOptions.cs

@@ -2,21 +2,45 @@ using System;
 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
             }
-        public Action<System.Diagnostics.ProcessStartInfo> OnBeforeStartExternalProcess { get; set; }
+
+            // 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 NodeHostingModel HostingModel { get; set; }
         public Func<INodeInstance> NodeInstanceFactory { get; set; }
         public string ProjectPath { get; set; }
@@ -24,6 +48,6 @@ namespace Microsoft.AspNetCore.NodeServices
         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/NodeServicesServiceCollectionExtensions.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/HostingModels/HttpNodeInstance.cs

@@ -35,7 +35,7 @@ namespace Microsoft.AspNetCore.NodeServices.HostingModels
         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/OutOfProcessNodeInstance.cs

@@ -49,7 +49,7 @@ If you haven't yet installed node-inspector, you can do so as follows:
             ILogger nodeOutputLogger,
             IDictionary<string, string> environmentVars,
             bool launchWithDebugging,
-            int? debuggingPort)
+            int debuggingPort)
         {
             if (nodeOutputLogger == null)
             {
@@ -101,12 +101,12 @@ If you haven't yet installed node-inspector, you can do so as follows:
         // 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/SocketNodeInstance.cs

@@ -40,7 +40,7 @@ namespace Microsoft.AspNetCore.NodeServices.HostingModels
 
         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),

src/Microsoft.AspNetCore.NodeServices/README.md

@@ -95,7 +95,9 @@ In other types of .NET Core app, where you don't have ASP.NET supplying an `ISer
 ```csharp
 // Remember to add 'using Microsoft.AspNetCore.NodeServices;' at the top of your file
 var services = new ServiceCollection();
-services.AddNodeServices(new NodeServicesOptions { /* your options here */ });
+services.AddNodeServices(options => {
+    // Set any properties that you want on 'options' here
+});
 ```
 
 Now you can ask it to supply the shared `INodeServices` instance:
@@ -108,8 +110,8 @@ var nodeServices = serviceProvider.GetRequiredService<INodeServices>();
 Or, if you want to obtain a separate (non-shared) `INodeServices` instance:
 
 ```csharp
-var options = new NodeServicesOptions { /* your options here */ };
+var options = new NodeServicesOptions(serviceProvider) { /* Assign/override any other options here */ };
-var nodeServices = Microsoft.AspNetCore.NodeServices.Configuration.CreateNodeServices(serviceProvider, options);
+var nodeServices = NodeServicesFactory.CreateNodeServices(options);
 ```
 
 Besides this, the usage is the same as described for ASP.NET above, so you can now call `nodeServices.InvokeAsync<T>(...)` etc.
@@ -126,7 +128,7 @@ NodeServices instances are thread-safe - you can call `InvokeAsync<T>` simultane
 
 ```csharp
 AddNodeServices()
-AddNodeServices(NodeServicesOptions options)
+AddNodeServices(Action<NodeServicesOptions> setupAction)
 ```
 
 This is an extension method on `IServiceCollection`. It registers NodeServices with ASP.NET Core's DI system. Typically you should call this from the `ConfigureServices` method in your `Startup.cs` file.
@@ -148,23 +150,21 @@ services.AddNodeServices();
 Or, specifying options:
 
 ```csharp
-services.AddNodeServices(new NodeServicesOptions
+services.AddNodeServices(options =>
 {
-    WatchFileExtensions = new[] { ".coffee", ".sass" },
+    options.WatchFileExtensions = new[] { ".coffee", ".sass" };
     // ... etc. - see other properties below
 });
 ```
 
 **Parameters**
 
- * `options` - type: `NodeServicesOptions`
+ * `setupAction` - type: `Action<NodeServicesOptions>`
-   * Optional. If specified, configures how the `NodeServices` instances will work.
+   * Optional. If not specified, defaults will be used.
-   * Properties:
+   * Properties on `NodeServicesOptions`:
      * `HostingModel` - an `NodeHostingModel` enum value. See: [hosting models](#hosting-models)
      * `ProjectPath` - if specified, controls the working directory used when launching Node instances. This affects, for example, the location that `require` statements resolve relative paths against. If not specified, your application root directory is used.
-     * `WatchFileExtensions` - if specified, the launched Node instance will watch for changes to any files with these extensions, and auto-restarts when any are changed.
+     * `WatchFileExtensions` - if specified, the launched Node instance will watch for changes to any files with these extensions, and auto-restarts when any are changed. The default array includes `.js`, `.jsx`, `.ts`, `.tsx`, `.json`, and `.html`.
-
-If no `options` is passed, the default `WatchFileExtensions` array includes `.js`, `.jsx`, `.ts`, `.tsx`, `.json`, and `.html`.
 
 **Return type**: None. But once you've done this, you can get `NodeServices` instances out of ASP.NET's DI system. Typically it will be a singleton instance.
 
@@ -173,22 +173,19 @@ If no `options` is passed, the default `WatchFileExtensions` array includes `.js
 **Signature:**
 
 ```csharp
-CreateNodeServices(IServiceProvider serviceProvider, NodeServicesOptions options)
+CreateNodeServices(NodeServicesOptions options)
 ```
 
-Supplies a new (non-shared) instance of `NodeServices`. It takes configuration from the .NET DI system (hence requiring an `IServiceProvider`), though some aspects of configuration can be overridden via the `options` parameter.
+Supplies a new (non-shared) instance of `NodeServices`.
 
 **Example**
 
 ```csharp
-var nodeServices = Configuration.CreateNodeServices(serviceProvider, new NodeServicesOptions {
+var options = new NodeServicesOptions(serviceProvider); // Obtains default options from DI config
-    HostingModel = NodeHostingModel.Socket
+var nodeServices = NodeServicesFactory.CreateNodeServices(options);
-});
 ```
 
 **Parameters**
- * `serviceProvider` - type: `IServiceProvider`
-   * An instance of .NET's standard DI service provider. You can get an instance of this by calling `BuildServiceProvider` on an `IServiceCollection` object. See the example usage of `CreateNodeServices` earlier in this document.
  * `options` - type: `NodeServicesOptions`.
    * Configures the returned `NodeServices` instance.
    * Properties:
@@ -343,12 +340,12 @@ People have asked about using [VroomJS](https://github.com/fogzot/vroomjs) as a
 
 ### Built-in hosting models
 
-Normally, you can just use the default hosting model, and not worry about it. But if you have some special requirements, select a hosting model by passing an `options` parameter to `AddNodeServices` or `CreateNodeServices`, and populate its `HostingModel` property. Example:
+Normally, you can just use the default hosting model, and not worry about it. But if you have some special requirements, select a hosting model by setting the `HostingModel` property on the `options` object in `AddNodeServices`. Example:
 
 ```csharp
-services.AddNodeServices(new NodeServicesOptions
+services.AddNodeServices(options =>
 {
-    HostingModel = NodeHostingModel.Socket
+    options.HostingModel = NodeHostingModel.Socket;
 });
 ```
 
@@ -365,12 +362,11 @@ The default transport may change from `Http` to `Socket` in the near future, bec
 
 ### Custom hosting models
 
-If you implement a custom hosting model (by implementing `INodeInstance`), then you can cause it to be used by populating `NodeInstanceFactory` on a `NodeServicesOptions`:
+If you implement a custom hosting model (by implementing `INodeInstance`), then you can cause it to be used by populating `NodeInstanceFactory` on your options:
 
 ```csharp
-var options = new NodeServicesOptions {
+services.AddNodeServices(options =>
-    NodeInstanceFactory = () => new MyCustomNodeInstance()
+{
-};
+    options.NodeInstanceFactory = () => new MyCustomNodeInstance();
+});
 ```
-
-Now you can pass this `options` object to [`AddNodeServices`](#addnodeservices) or [`CreateNodeServices`](#createnodeservices).

src/Microsoft.AspNetCore.SpaServices/Prerendering/PrerenderTagHelper.cs

@@ -33,9 +33,8 @@ namespace Microsoft.AspNetCore.SpaServices.Prerendering
             // in your startup file, but then again it might be confusing that you don't need to.
             if (_nodeServices == null)
             {
-                _nodeServices = _fallbackNodeServices = Configuration.CreateNodeServices(
+                _nodeServices = _fallbackNodeServices = NodeServicesFactory.CreateNodeServices(
-                    serviceProvider,
+                    new NodeServicesOptions(serviceProvider));
-                    new NodeServicesOptions());
             }
         }
 

src/Microsoft.AspNetCore.SpaServices/Webpack/WebpackDevMiddleware.cs

@@ -40,12 +40,9 @@ namespace Microsoft.AspNetCore.Builder
             // because it must *not* restart when files change (if it did, you'd lose all the benefits of Webpack
             // middleware). And since this is a dev-time-only feature, it doesn't matter if the default transport isn't
             // as fast as some theoretical future alternative.
-            var nodeServices = Configuration.CreateNodeServices(
+            var nodeServicesOptions = new NodeServicesOptions(appBuilder.ApplicationServices);
-                appBuilder.ApplicationServices,
+            nodeServicesOptions.WatchFileExtensions = new string[] {}; // Don't watch anything
-                new NodeServicesOptions
+            var nodeServices = NodeServicesFactory.CreateNodeServices(nodeServicesOptions);
-                {
-                    WatchFileExtensions = new string[] { } // Don't watch anything
-                });
 
             // Get a filename matching the middleware Node script
             var script = EmbeddedResourceReader.Read(typeof(WebpackDevMiddleware),