Design review: Always instantiate via DI

Author: SteveSandersonMS

samples/misc/LatencyTest/Program.cs

@@ -3,6 +3,7 @@
 using System.IO;
 using System.Threading.Tasks;
 using Microsoft.AspNetCore.NodeServices;
+using Microsoft.Extensions.DependencyInjection;
 
 namespace ConsoleApplication
 {
@@ -12,7 +13,17 @@ namespace ConsoleApplication
     public class Program
     {
         public static void Main(string[] args) {
-            using (var nodeServices = CreateNodeServices(NodeServicesOptions.DefaultNodeHostingModel)) {
+            // 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
+            });
+            var serviceProvider = services.BuildServiceProvider();
+
+            // Now instantiate an INodeServices and use it
+            using (var nodeServices = serviceProvider.GetRequiredService<INodeServices>()) {
                 MeasureLatency(nodeServices).Wait();
             }
         }
@@ -34,13 +45,5 @@ private static async Task MeasureLatency(INodeServices nodeServices) {
             Console.WriteLine("\nTotal time: {0:F2} milliseconds", 1000 * elapsedSeconds);
             Console.WriteLine("\nTime per invocation: {0:F2} milliseconds", 1000 * elapsedSeconds / requestCount);
         }
-
-        private static INodeServices CreateNodeServices(NodeHostingModel hostingModel) {
-            return Configuration.CreateNodeServices(new NodeServicesOptions {
-                HostingModel = hostingModel,
-                ProjectPath = Directory.GetCurrentDirectory(),
-                WatchFileExtensions = new string[] {} // Don't watch anything
-            });
-        }
     }
 }

samples/misc/LatencyTest/project.json

@@ -8,7 +8,8 @@
       "version": "1.0.0",
       "type": "platform"
     },
-    "Microsoft.AspNetCore.NodeServices": "1.0.0-*"
+    "Microsoft.AspNetCore.NodeServices": "1.0.0-*",
+    "Microsoft.Extensions.DependencyInjection": "1.0.0"
   },
   "frameworks": {
     "netcoreapp1.0": {

samples/misc/Webpack/webpack.config.js

@@ -10,7 +10,7 @@ module.exports = merge({
     },
     module: {
         loaders: [
-            { test: /\.ts(x?)$/, exclude: /node_modules/, loader: 'ts-loader' }
+            { test: /\.ts(x?)$/, exclude: /node_modules/, loader: 'ts-loader?silent' }
         ],
     },
     entry: {

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

@@ -4,6 +4,7 @@
 using Microsoft.AspNetCore.NodeServices.HostingModels;
 using Microsoft.Extensions.Logging;
 using Microsoft.Extensions.Logging.Console;
+using System.Collections.Generic;
 
 namespace Microsoft.AspNetCore.NodeServices
 {
@@ -16,48 +17,18 @@ public static void AddNodeServices(this IServiceCollection serviceCollection)
 
         public static void AddNodeServices(this IServiceCollection serviceCollection, NodeServicesOptions options)
         {
-            serviceCollection.AddSingleton(typeof(INodeServices), serviceProvider =>
-            {
-                // Since this instance is being created through DI, we can access the IHostingEnvironment
-                // to populate options.ProjectPath if it wasn't explicitly specified.                
-                var hostEnv = serviceProvider.GetRequiredService<IHostingEnvironment>();
-                if (string.IsNullOrEmpty(options.ProjectPath))
-                {
-                    options.ProjectPath = hostEnv.ContentRootPath;
-                }
-
-                // Similarly, we can determine the 'is development' value from the hosting environment
-                options.AddDefaultEnvironmentVariables(hostEnv.IsDevelopment());
-
-                // Likewise, if no logger was specified explicitly, we should use the one from DI.
-                // If it doesn't provide one, CreateNodeInstance will set up a default.
-                if (options.NodeInstanceOutputLogger == null)
-                {
-                    var loggerFactory = serviceProvider.GetService<ILoggerFactory>();
-                    if (loggerFactory != null)
-                    {
-                        options.NodeInstanceOutputLogger = loggerFactory.CreateLogger(LogCategoryName); 
-                    }
-                }
-
-                return new NodeServicesImpl(options, () => CreateNodeInstance(options));
-            });
+            serviceCollection.AddSingleton(
+                typeof(INodeServices),
+                serviceProvider => CreateNodeServices(serviceProvider, options));
         }
 
-        public static INodeServices CreateNodeServices(NodeServicesOptions options)
+        public static INodeServices CreateNodeServices(IServiceProvider serviceProvider, NodeServicesOptions options)
         {
-            return new NodeServicesImpl(options, () => CreateNodeInstance(options));
+            return new NodeServicesImpl(() => CreateNodeInstance(serviceProvider, options));
         }
 
-        private static INodeInstance CreateNodeInstance(NodeServicesOptions options)
+        private static INodeInstance CreateNodeInstance(IServiceProvider serviceProvider, NodeServicesOptions options)
         {
-            // If you've specified no logger, fall back on a default console logger
-            var logger = options.NodeInstanceOutputLogger;
-            if (logger == null)
-            {
-                logger = new ConsoleLogger(LogCategoryName, null, false);
-            }
-
             if (options.NodeInstanceFactory != null)
             {
                 // If you've explicitly supplied an INodeInstance factory, we'll use that. This is useful for
@@ -66,17 +37,51 @@ private static INodeInstance CreateNodeInstance(NodeServicesOptions options)
             }
             else
             {
-                // Otherwise we'll construct the type of INodeInstance specified by the HostingModel property,
-                // which itself has a useful default value.
+                // 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(options.ProjectPath, options.WatchFileExtensions, logger, 
-                            options.EnvironmentVariables, options.LaunchWithDebugging, options.DebuggingPort, /* port */ 0);
+                        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(options.ProjectPath, options.WatchFileExtensions, pipeName, logger,
-                            options.EnvironmentVariables, options.LaunchWithDebugging, options.DebuggingPort);
+                        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/NodeServicesOptions.cs

@@ -25,21 +25,5 @@ public NodeServicesOptions()
         public bool LaunchWithDebugging { get; set; }
         public IDictionary<string, string> EnvironmentVariables { get; set; }
         public int? DebuggingPort { get; set; }
-
-        public NodeServicesOptions AddDefaultEnvironmentVariables(bool isDevelopmentMode)
-        {
-            if (EnvironmentVariables == null)
-            {
-                EnvironmentVariables = new Dictionary<string, string>();
-            }
-
-            if (!EnvironmentVariables.ContainsKey("NODE_ENV"))
-            {
-                // These strings are a de-facto standard in Node
-                EnvironmentVariables["NODE_ENV"] = isDevelopmentMode ? "development" : "production";
-            }
-
-            return this;
-        }
     }
 }

src/Microsoft.AspNetCore.NodeServices/NodeServicesImpl.cs

@@ -19,15 +19,13 @@ namespace Microsoft.AspNetCore.NodeServices
     internal class NodeServicesImpl : INodeServices
     {
         private static TimeSpan ConnectionDrainingTimespan = TimeSpan.FromSeconds(15);
-        private NodeServicesOptions _options;
         private Func<INodeInstance> _nodeInstanceFactory;
         private INodeInstance _currentNodeInstance;
         private object _currentNodeInstanceAccessLock = new object();
         private Exception _instanceDelayedDisposalException;
 
-        internal NodeServicesImpl(NodeServicesOptions options, Func<INodeInstance> nodeInstanceFactory)
+        internal NodeServicesImpl(Func<INodeInstance> nodeInstanceFactory)
         {
-            _options = options;
             _nodeInstanceFactory = nodeInstanceFactory;
         }
 

src/Microsoft.AspNetCore.NodeServices/README.md

@@ -90,18 +90,33 @@ If you want to put `addNumber.js` inside a subfolder rather than the root of you
 
 ## For non-ASP.NET apps
 
-In other types of .NET app where you don't have ASP.NET Core's DI system, you can get an instance of `NodeServices` as follows:
+In other types of .NET Core app, where you don't have ASP.NET supplying an `IServiceCollection` to you, you'll need to instantiate your own DI container. For example, add a reference to the .NET package `Microsoft.Extensions.DependencyInjection`, and then you can construct an `IServiceCollection`, then register NodeServices as usual:
 
 ```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 */ });
+```
+
+Now you can ask it to supply the shared `INodeServices` instance:
+
+```csharp
+var serviceProvider = services.BuildServiceProvider();
+var nodeServices = serviceProvider.GetRequiredService<INodeServices>();
+```
+
+Or, if you want to obtain a separate (non-shared) `INodeServices` instance:
 
-var nodeServices = Configuration.CreateNodeServices(new NodeServicesOptions());
+```csharp
+var options = new NodeServicesOptions { /* your options here */ };
+var nodeServices = Microsoft.AspNetCore.NodeServices.Configuration.CreateNodeServices(serviceProvider, options);
 ```
 
 Besides this, the usage is the same as described for ASP.NET above, so you can now call `nodeServices.InvokeAsync<T>(...)` etc.
 
-You can dispose the `nodeServices` object whenever you are done with it (and it will shut down the associated Node.js instance), but because these instances are expensive to create, you should whenever possible retain and reuse instances. They are thread-safe - you can call `InvokeAsync<T>` simultaneously from multiple threads. Also, `NodeServices` instances are smart enough to detect if the associated Node instance has died and will automatically start a new Node instance if needed.
+You can dispose the `nodeServices` object whenever you are done with it (and it will shut down the associated Node.js instance), but because these instances are expensive to create, you should whenever possible retain and reuse instances. Don't dispose the shared instance returned from `serviceProvider.GetRequiredService` (except perhaps if you know your application is shutting down, although .NET's finalizers will dispose it anyway if the shutdown is graceful).
 
+NodeServices instances are thread-safe - you can call `InvokeAsync<T>` simultaneously from multiple threads. Also, they are smart enough to detect if the associated Node instance has died and will automatically start a new Node instance if needed.
 
 # API Reference
 
@@ -158,21 +173,22 @@ If no `options` is passed, the default `WatchFileExtensions` array includes `.js
 **Signature:**
 
 ```csharp
-CreateNodeServices(NodeServicesOptions options)
+CreateNodeServices(IServiceProvider serviceProvider, NodeServicesOptions options)
 ```
 
-Directly supplies an instance of `NodeServices` without using ASP.NET's DI system.
+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.
 
 **Example**
 
 ```csharp
-var nodeServices = Configuration.CreateNodeServices(new NodeServicesOptions {
+var nodeServices = Configuration.CreateNodeServices(serviceProvider, new NodeServicesOptions {
     HostingModel = NodeHostingModel.Socket
 });
 ```
 
 **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:

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

@@ -33,10 +33,9 @@ public PrerenderTagHelper(IServiceProvider serviceProvider)
             // in your startup file, but then again it might be confusing that you don't need to.
             if (_nodeServices == null)
             {
-                _nodeServices = _fallbackNodeServices = Configuration.CreateNodeServices(new NodeServicesOptions
-                {
-                    ProjectPath = _applicationBasePath
-                }.AddDefaultEnvironmentVariables(hostEnv.IsDevelopment()));
+                _nodeServices = _fallbackNodeServices = Configuration.CreateNodeServices(
+                    serviceProvider,
+                    new NodeServicesOptions());
             }
         }
 

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

@@ -35,26 +35,26 @@ public static void UseWebpackDevMiddleware(
                     "To enable ReactHotModuleReplacement, you must also enable HotModuleReplacement.");
             }
 
-            var hostEnv = (IHostingEnvironment)appBuilder.ApplicationServices.GetService(typeof(IHostingEnvironment));
-            var projectPath = options.ProjectPath ?? hostEnv.ContentRootPath;
-
             // Unlike other consumers of NodeServices, WebpackDevMiddleware dosen't share Node instances, nor does it
             // use your DI configuration. It's important for WebpackDevMiddleware to have its own private Node instance
             // 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(new NodeServicesOptions
-            {
-                ProjectPath = projectPath,
-                WatchFileExtensions = new string[] { } // Don't watch anything
-            }.AddDefaultEnvironmentVariables(hostEnv.IsDevelopment()));
+            var nodeServices = Configuration.CreateNodeServices(
+                appBuilder.ApplicationServices,
+                new NodeServicesOptions
+                {
+                    WatchFileExtensions = new string[] { } // Don't watch anything
+                });
 
             // Get a filename matching the middleware Node script
             var script = EmbeddedResourceReader.Read(typeof(WebpackDevMiddleware),
                 "/Content/Node/webpack-dev-middleware.js");
             var nodeScript = new StringAsTempFile(script); // Will be cleaned up on process exit
 
             // Tell Node to start the server hosting webpack-dev-middleware
+            var hostEnv = (IHostingEnvironment)appBuilder.ApplicationServices.GetService(typeof(IHostingEnvironment));
+            var projectPath = options.ProjectPath ?? hostEnv.ContentRootPath;
             var devServerOptions = new
             {
                 webpackConfigPath = Path.Combine(projectPath, options.ConfigFile ?? DefaultConfigFile),