PR Feedback

Author: Paul van Brenk

Nodejs/Product/Nodejs/SharedProject/OutputWindowWrapper.cs

@@ -17,61 +17,41 @@ public sealed class OutputPaneWrapper
 
         // This is the package manager pane that ships with VS2015, and we should print there if available.
         private static readonly Guid VSPackageManagerPaneGuid = new Guid("C7E31C31-1451-4E05-B6BE-D11B6829E8BB");
-
         private static readonly Guid TscPaneGuid = new Guid("7CCFA622-001E-4459-9D8B-5B60BE0A18C2");
 
         [Import]
         private SVsServiceProvider ServiceProvider { get; set; }
 
         private Dictionary<OutputWindowTarget, IVsOutputWindowPane> lazyOutputPaneCollection = new Dictionary<OutputWindowTarget, IVsOutputWindowPane>();
 
-        public OutputPaneWrapper()
-        {
-            //...
-        }
-
-        private void InitializeOutputPane(OutputWindowTarget target)
+        private IVsOutputWindowPane InitializeOutputPane(string title, Guid paneId)
         {
             ThreadHelper.ThrowIfNotOnUIThread();
-            var (title, guid) = GetPaneInfo(target);
             var outputWindow = (IVsOutputWindow)this.ServiceProvider.GetService(typeof(SVsOutputWindow));
 
             // Try to get the workspace pane if it has already been registered
-            var hr = outputWindow.GetPane(guid, out var lazyOutputPane);
+            var hr = outputWindow.GetPane(paneId, out var lazyOutputPane);
 
             // If the workspace pane has not been registered before, create it
             if (lazyOutputPane == null || ErrorHandler.Failed(hr))
             {
 
-                if (ErrorHandler.Failed(outputWindow.CreatePane(guid, title, fInitVisible: 1, fClearWithSolution: 1)) ||
-                    ErrorHandler.Failed(outputWindow.GetPane(guid, out lazyOutputPane)))
+                if (ErrorHandler.Failed(outputWindow.CreatePane(paneId, title, fInitVisible: 1, fClearWithSolution: 1)) ||
+                    ErrorHandler.Failed(outputWindow.GetPane(paneId, out lazyOutputPane)))
                 {
-                    return;
+                    return null;
                 }
 
                 // Must activate the workspace pane for it to show up in the output window
                 lazyOutputPane.Activate();
             }
 
-            this.lazyOutputPaneCollection.Add(target, lazyOutputPane);
-
-            (string title, Guid guid) GetPaneInfo(OutputWindowTarget pane)
-            {
-                switch (pane)
-                {
-                    case OutputWindowTarget.Npm:
-                        return ("Npm", VSPackageManagerPaneGuid);
-                    case OutputWindowTarget.Tsc:
-                        return ("Tsc", TscPaneGuid); /* no need to localize since name of exe */
-                    default:
-                        throw new ArgumentOutOfRangeException(nameof(pane));
-                }
-            }
+            return lazyOutputPane;
         }
 
         public void WriteLine(string message, OutputWindowTarget target = OutputWindowTarget.Npm)
         {
-            if (!this.lazyOutputPaneCollection.TryGetValue(target, out var lazyOutputPane))
+            if (!this.lazyOutputPaneCollection.TryGetValue(target, out var lazyOutputPane) || lazyOutputPane == null)
             {
                 throw new InvalidOperationException("You need to initialize the output panes before using them.");
             }
@@ -84,8 +64,10 @@ public void InitializeOutputPanes()
         {
             ThreadHelper.ThrowIfNotOnUIThread();
 
-            InitializeOutputPane(OutputWindowTarget.Npm);
-            InitializeOutputPane(OutputWindowTarget.Tsc);
+            var npmPane = InitializeOutputPane("Npm", VSPackageManagerPaneGuid);
+            this.lazyOutputPaneCollection.Add(OutputWindowTarget.Npm, npmPane);
+            var tscPane = InitializeOutputPane("Tsc", TscPaneGuid);
+            this.lazyOutputPaneCollection.Add(OutputWindowTarget.Tsc, tscPane);
         }
     }
 

Nodejs/Product/Nodejs/Workspace/ContextMenuProvider.cs

@@ -11,12 +11,10 @@
 using Microsoft.NodejsTools.NpmUI;
 using Microsoft.VisualStudio;
 using Microsoft.VisualStudio.OLE.Interop;
-using Microsoft.VisualStudio.Shell;
 using Microsoft.VisualStudio.Workspace;
 using Microsoft.VisualStudio.Workspace.Debug;
 using Microsoft.VisualStudio.Workspace.VSIntegration.UI;
 using Microsoft.VisualStudioTools.Project;
-using ShellInterop = Microsoft.VisualStudio.Shell.Interop;
 
 namespace Microsoft.NodejsTools.Workspace
 {
@@ -80,24 +78,26 @@ public int Exec(List<WorkspaceVisualNodeBase> selection, Guid pguidCmdGroup, uin
                 {
                     this.outputPane.InitializeOutputPanes();
 
+                    var fileNode = ((IFileNode)node).FullPath;
+
                     switch (nCmdID)
                     {
                         case PkgCmdId.cmdidWorkSpaceNpmInstallMissing:
-                            ExecNpmInstallMissing((IFileNode)node);
+                            ExecNpmInstallMissing(fileNode);
                             return VSConstants.S_OK;
 
                         case PkgCmdId.cmdidWorkSpaceNpmInstallNew:
-                            ExecNpmInstallNew((IFileNode)node);
+                            ExecNpmInstallNew(fileNode);
                             return VSConstants.S_OK;
 
                         case PkgCmdId.cmdidWorkSpaceNpmUpdate:
-                            ExecNpmUpdate((IFileNode)node);
+                            ExecNpmUpdate(fileNode);
                             return VSConstants.S_OK;
                     }
 
                     if (nCmdID >= PkgCmdId.cmdidWorkSpaceNpmDynamicScript && nCmdID < PkgCmdId.cmdidWorkSpaceNpmDynamicScriptMax)
                     {
-                        ExecDynamic((IFileNode)node, nCmdID);
+                        ExecDynamic(fileNode, nCmdID);
                         return VSConstants.S_OK;
                     }
                 }
@@ -119,40 +119,39 @@ public int Exec(List<WorkspaceVisualNodeBase> selection, Guid pguidCmdGroup, uin
             // Note: all the Exec commands are async, this allows us to call them in a fire and forget
             // pattern, without blocking the UI or losing any logging
 
-            private async void ExecNpmInstallMissing(IFileNode node)
+            private async void ExecNpmInstallMissing(string filePath)
             {
-                using (var npmController = this.CreateController(node.FullPath))
+                using (var npmController = this.CreateController(filePath))
                 using (var commander = npmController.CreateNpmCommander())
                 {
                     await commander.Install();
                 }
             }
 
-            private void ExecNpmInstallNew(IFileNode node)
+            private void ExecNpmInstallNew(string filePath)
             {
-                using (var npmController = this.CreateController(node.FullPath))
+                using (var npmController = this.CreateController(filePath))
                 using (var npmWorker = new NpmWorker(npmController))
                 using (var manager = new NpmPackageInstallWindow(npmController, npmWorker))
                 {
                     manager.ShowModal();
                 }
             }
 
-            private async void ExecNpmUpdate(IFileNode node)
+            private async void ExecNpmUpdate(string filePath)
             {
-                using (var npmController = this.CreateController(node.FullPath))
+                using (var npmController = this.CreateController(filePath))
                 using (var commander = npmController.CreateNpmCommander())
                 {
                     await commander.UpdatePackagesAsync();
                 }
             }
 
-            private async void ExecDynamic(IFileNode node, uint nCmdID)
+            private async void ExecDynamic(string filePath, uint nCmdID)
             {
                 // Unfortunately the NpmController (and NpmCommander), used for the install and update commands
                 // doesn't support running arbitrary scripts. And changing that is outside
                 // the scope of these changes.
-                var filePath = node.FullPath;
                 if (TryGetCommand(nCmdID, filePath, out var commandName))
                 {
                     using (var npmController = this.CreateController(filePath))
@@ -174,7 +173,7 @@ private async void ExecDebugAsync(WorkspaceVisualNodeBase node)
                 }
 
                 //invoke debuglaunchtargetprovider on this file
-                var fileContextActions = await node.Workspace.GetFileContextActionsAsync(packageJson.Main, new[] { DebugLaunchActionContext.ContextTypeGuid });
+                var fileContextActions = await workspace.GetFileContextActionsAsync(packageJson.Main, new[] { DebugLaunchActionContext.ContextTypeGuid });
                 if (fileContextActions.Any())
                 {
                     // we requested a single context, so there should be a single grouping. Use the First action, since they're ordered by priority.
@@ -274,7 +273,8 @@ private static bool TryGetCommand(uint nCmdID, string filePath, out string comma
 
             private INpmController CreateController(string packageJsonPath)
             {
-                Debug.Assert(Path.IsPathRooted(packageJsonPath) && PackageJsonFactory.IsPackageJsonFile(packageJsonPath));
+                Debug.Assert(Path.IsPathRooted(packageJsonPath));
+                Debug.Assert(PackageJsonFactory.IsPackageJsonFile(packageJsonPath));
 
                 var projectHome = Path.GetDirectoryName(packageJsonPath);
 

Nodejs/Product/Npm/SPI/NpmCommand.cs

@@ -106,7 +106,7 @@ await NpmHelpers.ExecuteNpmCommandAsync(
                 cancelled = true;
             }
             OnCommandCompleted(this.Arguments, redirector?.HasErrors ?? false, cancelled);
-            return redirector == null || !redirector.HasErrors; // return true when the command completed without errors, or redirector is null (which means we can't track)
+            return redirector?.HasErrors != true; // return true when the command completed without errors, or redirector is null (which means we can't track)
         }
 
         private sealed class NpmCommandRedirector : Redirector