fix(client,server): Correctly use bundled version unless tsdk is specified (#1970)

* refactor(client): Move server options to client class

This means not having to pass context around and will also give those functions
access to other things directly like outputChannel for logging.

* refactor(client): Log warning to the output channel for when block syntax is disabled

We should log this somewhere or there would be no way to know when this happened to a user.

* refactor(client): Inline server options rather than helper function

A function with a flag is frequently a code smell and in this case it
is. There isn't much code shared between the two modes so inlining is
better.

* fix(client,server): Correctly use bundled version unless tsdk is specified

When the `typescript.tsdk` is empty, it should be ignored. Instead, the
logic previously would _always_ add an empty string tsdk to the
beginning of the probe locations. This would result in resolving the ts
server from the user's node modules rather than prefering the bundled
version as intended unless a different tsdk is specifically set in the
config.

Additionally, if the workspace has many roots, the first one would be
seen as a tsdk. While this would probably still result in the correct
behavior, the code is much easier to follow when the tsdk is a separate
option passed to the server.

* refactor(server): Update tsserver version log to reflect actual version used

The version that's currently logged is whatever the resolve logic gives. However,
this resolve logic actually only affects the version when the banner is used
to override the require statement. This won't happen when debugging the extension
and stumped me for quite some time since the log version was using the tsdk
I specified but I was not seeing the behavior I expected with that version
(I was instead seeing the behavior expected from the bundled version).

Author: atscott

client/src/client.ts

@@ -217,13 +217,47 @@ export class AngularLanguageClient implements vscode.Disposable {
       throw new Error(`An existing client is running. Call stop() first.`);
     }
 
+    // Node module for the language server
+    const args = this.constructArgs();
+    const prodBundle = this.context.asAbsolutePath('server');
+    const devBundle =
+        this.context.asAbsolutePath(path.join('bazel-bin', 'server', 'src', 'server.js'));
+
     // If the extension is launched in debug mode then the debug server options are used
     // Otherwise the run options are used
     const serverOptions: lsp.ServerOptions = {
-      run: getServerOptions(this.context, false /* debug */),
-      debug: getServerOptions(this.context, true /* debug */),
+      run: {
+        module: this.context.asAbsolutePath('server'),
+        transport: lsp.TransportKind.ipc,
+        args,
+      },
+      debug: {
+        // VS Code Insider launches extensions in debug mode by default but users
+        // install prod bundle so we have to check whether dev bundle exists.
+        module: fs.existsSync(devBundle) ? devBundle : prodBundle,
+        transport: lsp.TransportKind.ipc,
+        options: {
+          // Argv options for Node.js
+          execArgv: [
+            // do not lazily evaluate the code so all breakpoints are respected
+            '--nolazy',
+            // If debugging port is changed, update .vscode/launch.json as well
+            '--inspect=6009',
+          ],
+          env: {
+            NG_DEBUG: true,
+          }
+        },
+        args
+      },
     };
 
+    if (!extensionVersionCompatibleWithAllProjects(serverOptions.run.module)) {
+      vscode.window.showWarningMessage(
+          `A project in the workspace is using a newer version of Angular than the language service extension. ` +
+          `This may cause the extension to show incorrect diagnostics.`);
+    }
+
     // Create the language client and start the client.
     const forceDebug = process.env['NG_DEBUG'] === 'true';
     this.client = new lsp.LanguageClient(
@@ -242,6 +276,62 @@ export class AngularLanguageClient implements vscode.Disposable {
     this.disposables.push(registerNotificationHandlers(this.client));
   }
 
+  /**
+   * Construct the arguments that's used to spawn the server process.
+   * @param ctx vscode extension context
+   */
+  private constructArgs(): string[] {
+    const config = vscode.workspace.getConfiguration();
+    const args: string[] = ['--logToConsole'];
+
+    const ngLog: string = config.get('angular.log', 'off');
+    if (ngLog !== 'off') {
+      // Log file does not yet exist on disk. It is up to the server to create the file.
+      const logFile = path.join(this.context.logUri.fsPath, 'nglangsvc.log');
+      args.push('--logFile', logFile);
+      args.push('--logVerbosity', ngLog);
+    }
+
+    const ngProbeLocations = getProbeLocations(this.context.extensionPath);
+    args.push('--ngProbeLocations', ngProbeLocations.join(','));
+
+    const includeAutomaticOptionalChainCompletions =
+        config.get<boolean>('angular.suggest.includeAutomaticOptionalChainCompletions');
+    if (includeAutomaticOptionalChainCompletions) {
+      args.push('--includeAutomaticOptionalChainCompletions');
+    }
+
+    const includeCompletionsWithSnippetText =
+        config.get<boolean>('angular.suggest.includeCompletionsWithSnippetText');
+    if (includeCompletionsWithSnippetText) {
+      args.push('--includeCompletionsWithSnippetText');
+    }
+
+    const angularVersions = getAngularVersionsInWorkspace();
+    // Only disable block syntax if we find angular/core and every one we find does not support
+    // block syntax
+    if (angularVersions.size > 0 && Array.from(angularVersions).every(v => v.version.major < 17)) {
+      args.push('--disableBlockSyntax');
+      this.outputChannel.appendLine(
+          `All workspace roots are using versions of Angular that do not support control flow block syntax.` +
+          ` Block syntax parsing in templates will be disabled.`);
+    }
+
+    const forceStrictTemplates = config.get<boolean>('angular.forceStrictTemplates');
+    if (forceStrictTemplates) {
+      args.push('--forceStrictTemplates');
+    }
+
+    const tsdk = config.get('typescript.tsdk', '');
+    if (tsdk.trim().length > 0) {
+      args.push('--tsdk', tsdk);
+    }
+    const tsProbeLocations = [...getProbeLocations(this.context.extensionPath)];
+    args.push('--tsProbeLocations', tsProbeLocations.join(','));
+
+    return args;
+  }
+
   /**
    * Kill the language client and perform some clean ups.
    */
@@ -401,97 +491,6 @@ function getProbeLocations(bundled: string): string[] {
   return locations;
 }
 
-/**
- * Construct the arguments that's used to spawn the server process.
- * @param ctx vscode extension context
- */
-function constructArgs(ctx: vscode.ExtensionContext): string[] {
-  const config = vscode.workspace.getConfiguration();
-  const args: string[] = ['--logToConsole'];
-
-  const ngLog: string = config.get('angular.log', 'off');
-  if (ngLog !== 'off') {
-    // Log file does not yet exist on disk. It is up to the server to create the file.
-    const logFile = path.join(ctx.logUri.fsPath, 'nglangsvc.log');
-    args.push('--logFile', logFile);
-    args.push('--logVerbosity', ngLog);
-  }
-
-  const ngProbeLocations = getProbeLocations(ctx.extensionPath);
-  args.push('--ngProbeLocations', ngProbeLocations.join(','));
-
-  const includeAutomaticOptionalChainCompletions =
-      config.get<boolean>('angular.suggest.includeAutomaticOptionalChainCompletions');
-  if (includeAutomaticOptionalChainCompletions) {
-    args.push('--includeAutomaticOptionalChainCompletions');
-  }
-
-  const includeCompletionsWithSnippetText =
-      config.get<boolean>('angular.suggest.includeCompletionsWithSnippetText');
-  if (includeCompletionsWithSnippetText) {
-    args.push('--includeCompletionsWithSnippetText');
-  }
-
-  const angularVersions = getAngularVersionsInWorkspace();
-  // Only disable block syntax if we find angular/core and every one we find does not support block
-  // syntax
-  if (angularVersions.size > 0 && Array.from(angularVersions).every(v => v.version.major < 17)) {
-    args.push('--disableBlockSyntax');
-  }
-
-  const forceStrictTemplates = config.get<boolean>('angular.forceStrictTemplates');
-  if (forceStrictTemplates) {
-    args.push('--forceStrictTemplates');
-  }
-
-  const tsdk: string|null = config.get('typescript.tsdk', null);
-  const tsProbeLocations = [tsdk, ...getProbeLocations(ctx.extensionPath)];
-  args.push('--tsProbeLocations', tsProbeLocations.join(','));
-
-  return args;
-}
-
-function getServerOptions(ctx: vscode.ExtensionContext, debug: boolean): lsp.NodeModule {
-  // Environment variables for server process
-  const prodEnv = {};
-  const devEnv = {
-    ...prodEnv,
-    NG_DEBUG: true,
-  };
-
-  // Node module for the language server
-  const args = constructArgs(ctx);
-  const prodBundle = ctx.asAbsolutePath('server');
-  const devBundle = ctx.asAbsolutePath(path.join('bazel-bin', 'server', 'src', 'server.js'));
-  // VS Code Insider launches extensions in debug mode by default but users
-  // install prod bundle so we have to check whether dev bundle exists.
-  const latestServerModule = debug && fs.existsSync(devBundle) ? devBundle : prodBundle;
-
-  if (!extensionVersionCompatibleWithAllProjects(latestServerModule)) {
-    vscode.window.showWarningMessage(
-        `A project in the workspace is using a newer version of Angular than the language service extension. ` +
-        `This may cause the extension to show incorrect diagnostics.`);
-  }
-
-  // Argv options for Node.js
-  const prodExecArgv: string[] = [];
-  const devExecArgv: string[] = [
-    // do not lazily evaluate the code so all breakpoints are respected
-    '--nolazy',
-    // If debugging port is changed, update .vscode/launch.json as well
-    '--inspect=6009',
-  ];
-
-  return {
-    module: latestServerModule,
-    transport: lsp.TransportKind.ipc,
-    args,
-    options: {
-      env: debug ? devEnv : prodEnv,
-      execArgv: debug ? devExecArgv : prodExecArgv,
-    },
-  };
-}
 
 function extensionVersionCompatibleWithAllProjects(serverModuleLocation: string): boolean {
   const languageServiceVersion =

server/src/banner.ts

@@ -31,8 +31,8 @@ export function requireOverride(moduleName: string) {
     throw new Error(`Import '${TSSERVER}' instead of 'typescript'`);
   }
   if (moduleName === TSSERVER) {
-    const {tsProbeLocations} = parseCommandLine(process.argv);
-    moduleName = resolveTsServer(tsProbeLocations).resolvedPath;
+    const {tsProbeLocations, tsdk} = parseCommandLine(process.argv);
+    moduleName = resolveTsServer(tsProbeLocations, tsdk).resolvedPath;
   }
   return originalRequire(moduleName);
 }

server/src/cmdline_utils.ts

@@ -33,6 +33,7 @@ interface CommandLineOptions {
   logToConsole: boolean;
   ngProbeLocations: string[];
   tsProbeLocations: string[];
+  tsdk: string|null;
   includeAutomaticOptionalChainCompletions: boolean;
   includeCompletionsWithSnippetText: boolean;
   forceStrictTemplates: boolean;
@@ -47,6 +48,7 @@ export function parseCommandLine(argv: string[]): CommandLineOptions {
     logToConsole: hasArgument(argv, '--logToConsole'),
     ngProbeLocations: parseStringArray(argv, '--ngProbeLocations'),
     tsProbeLocations: parseStringArray(argv, '--tsProbeLocations'),
+    tsdk: findArgument(argv, '--tsdk') ?? null,
     includeAutomaticOptionalChainCompletions:
         hasArgument(argv, '--includeAutomaticOptionalChainCompletions'),
     includeCompletionsWithSnippetText: hasArgument(argv, '--includeCompletionsWithSnippetText'),

server/src/server.ts

@@ -6,6 +6,8 @@
  * found in the LICENSE file at https://angular.io/license
  */
 
+import {version as tsServerVersion} from 'typescript/lib/tsserverlibrary';
+
 import {generateHelpMessage, parseCommandLine} from './cmdline_utils';
 
 // Parse command line arguments
@@ -27,7 +29,7 @@ function main() {
     logVerbosity: options.logVerbosity,
   });
 
-  const ts = resolveTsServer(options.tsProbeLocations);
+  const ts = resolveTsServer(options.tsProbeLocations, options.tsdk);
   const ng = resolveNgLangSvc(options.ngProbeLocations);
 
   const isG3 = ts.resolvedPath.includes('/google3/');
@@ -51,7 +53,7 @@ function main() {
 
   // Log initialization info
   session.info(`Angular language server process ID: ${process.pid}`);
-  session.info(`Using ${ts.name} v${ts.version} from ${ts.resolvedPath}`);
+  session.info(`Imported typescript/lib/tsserverlibrary is version ${tsServerVersion}.`);
   session.info(`Using ${ng.name} v${ng.version} from ${ng.resolvedPath}`);
   if (logger.loggingEnabled()) {
     session.info(`Log file: ${logger.getLogFileName()}`);

server/src/tests/version_provider_spec.ts

@@ -14,7 +14,7 @@ describe('Node Module Resolver', () => {
   const probeLocations = [__dirname];
 
   it('should be able to resolve tsserver', () => {
-    const result = resolveTsServer(probeLocations);
+    const result = resolveTsServer(probeLocations, null);
     expect(result).toBeDefined();
     expect(result.resolvedPath).toMatch(/typescript\/lib\/tsserverlibrary.js$/);
   });
@@ -23,7 +23,7 @@ describe('Node Module Resolver', () => {
     // Resolve relative to cwd.
     const absPath = resolve('node_modules/typescript/lib');
     expect(isAbsolute(absPath)).toBeTrue();
-    const result = resolveTsServer([absPath]);
+    const result = resolveTsServer(probeLocations, absPath);
     expect(result.resolvedPath.endsWith('typescript/lib/tsserverlibrary.js')).toBeTrue();
   });
 

server/src/version_provider.ts

@@ -47,10 +47,9 @@ function resolveWithMinVersion(
  * Resolve `typescript/lib/tsserverlibrary` from the given locations.
  * @param probeLocations
  */
-export function resolveTsServer(probeLocations: string[]): NodeModule {
-  if (probeLocations.length > 0) {
-    // The first probe location is `typescript.tsdk` if it is specified.
-    const resolvedFromTsdk = resolveTsServerFromTsdk(probeLocations[0], probeLocations.slice(1));
+export function resolveTsServer(probeLocations: string[], tsdk: string|null): NodeModule {
+  if (tsdk !== null) {
+    const resolvedFromTsdk = resolveTsServerFromTsdk(tsdk, probeLocations);
     const minVersion = new Version(MIN_TS_VERSION);
     if (resolvedFromTsdk !== undefined) {
       if (resolvedFromTsdk.version.greaterThanOrEqual(minVersion)) {