fix: move ngcc operation to server

This commit fixes a bug in which ngcc could not be resolved from the client
when the extension is running in the production vscode environment.

This is because internally, the client depends on `require.resolve` to look
for `ngcc` given the directory where the `tsconfig.json` is.
However, vscode provides its own [`require` mechanism][1], which is different
from Node.js. In particular, the bug is due to `require.resolve` not honoring
the `paths` parameter provided to the method. See Node.js [documentation][2] on
`require.resolve`.

We did not catch this bug because the custom `require` is not used
in the extension development host.

In order to mitigate this, we have a few options:

1. Use an external [`resolve` package][3] in the client to perform node module resolution
2. Bundle ngcc with the client
3. Move ngcc operation to the server

(1) is not ideal because it adds an external dependency to the extension

(2) could potentially lead to compatibility issues due to different version
of ngcc in the extension vs user's project.

(3) would not only solve this issue, but also provides the additional benefit
of reducing the work needed to integrate the language server for clients
other than vscode (vim, emacs, for example).
This is because the client needs to do some extra work to find ngcc then
launch it, and report back to the server that the operation has completed.
With this change, the server handles ngcc completely, and no work is needed
on the client side.

As an aside, the initial motivation for implementing ngcc on the client
side is to provide the guarantee that the server is idempotent, i.e. it should
not produce the side effect of modifying files on disk, which ngcc does.
However, at this point, the benefits outweigh the cons, so ngcc operation is
moved to the server.

[1]: https://github.com/microsoft/vscode/blob/92adef0bac1260a6273443a2132428fdfc28a31f/src/vs/loader.js#L743-L745
[2]: https://nodejs.org/docs/latest-v12.x/api/modules.html#modules_require_resolve_request_options
[3]: https://www.npmjs.com/package/resolve

Author: kyliau

client/src/extension.ts

@@ -11,11 +11,11 @@ import * as path from 'path';
 import * as vscode from 'vscode';
 import * as lsp from 'vscode-languageclient';
 
-import * as notification from '../common/notifications';
+import {ProjectLoadingFinish, ProjectLoadingStart} from '../common/notifications';
+import {NgccProgress, NgccProgressToken, NgccProgressType} from '../common/progress';
 
-import {resolveAndRunNgcc} from './command-ngcc';
 import {registerCommands} from './commands';
-import {withProgress} from './progress-reporter';
+import {ProgressReporter} from './progress-reporter';
 
 export function activate(context: vscode.ExtensionContext) {
   // If the extension is launched in debug mode then the debug server options are used
@@ -59,50 +59,54 @@ export function activate(context: vscode.ExtensionContext) {
   client.onDidChangeState((e: lsp.StateChangeEvent) => {
     if (e.newState === lsp.State.Running) {
       registerNotificationHandlers(client);
+      registerProgressHandlers(client, context);
     }
   });
 }
 
 function registerNotificationHandlers(client: lsp.LanguageClient) {
-  client.onNotification(notification.ProjectLoadingStart, () => {
+  client.onNotification(ProjectLoadingStart, () => {
     vscode.window.withProgress(
         {
           location: vscode.ProgressLocation.Window,
           title: 'Initializing Angular language features',
         },
         () => new Promise<void>((resolve) => {
-          client.onNotification(notification.ProjectLoadingFinish, resolve);
+          client.onNotification(ProjectLoadingFinish, resolve);
         }),
     );
   });
+}
 
-  client.onNotification(notification.RunNgcc, async (params: notification.RunNgccParams) => {
-    const {configFilePath} = params;
-    try {
-      await withProgress(
-          {
-            location: vscode.ProgressLocation.Window,
-            title: `Running ngcc for project ${configFilePath}`,
-            cancellable: false,
-          },
-          (progress: vscode.Progress<string>) => {
-            return resolveAndRunNgcc(configFilePath, progress);
-          },
-      );
-      client.sendNotification(notification.NgccComplete, {
-        configFilePath,
-        success: true,
-      });
-    } catch (e) {
-      vscode.window.showWarningMessage(
-          `Failed to run ngcc. Ivy language service might not function correctly. Please see the log file for more information.`);
-      client.sendNotification(notification.NgccComplete, {
-        configFilePath,
-        success: false,
-        error: e.message,
+function registerProgressHandlers(client: lsp.LanguageClient, context: vscode.ExtensionContext) {
+  const progressReporters = new Map<string, ProgressReporter>();
+  const disposable =
+      client.onProgress(NgccProgressType, NgccProgressToken, async (params: NgccProgress) => {
+        const {configFilePath} = params;
+        if (!progressReporters.has(configFilePath)) {
+          progressReporters.set(configFilePath, new ProgressReporter());
+        }
+        const reporter = progressReporters.get(configFilePath)!;
+        if (params.done) {
+          reporter.finish();
+          progressReporters.delete(configFilePath);
+          if (!params.success) {
+            const selection = await vscode.window.showErrorMessage(
+                `Failed to run ngcc. Ivy language service is disabled. ` +
+                    `Please see the extension output for more information.`,
+                {modal: true},
+                'See error message',
+            );
+            if (selection) {
+              client.outputChannel.show();
+            }
+          }
+        } else {
+          reporter.report(params.message);
+        }
       });
-    }
-  });
+  // Dispose the progress handler on exit
+  context.subscriptions.push(disposable);
 }
 
 /**

client/src/progress-reporter.ts

@@ -10,43 +10,18 @@ import * as vscode from 'vscode';
 
 const EMPTY_DISPOSABLE = vscode.Disposable.from();
 
-class ProgressReporter implements vscode.Progress<unknown> {
+export class ProgressReporter implements vscode.Progress<unknown> {
   private lastMessage: vscode.Disposable = EMPTY_DISPOSABLE;
 
   report(value: unknown) {
     this.lastMessage.dispose();  // clear the last message
     // See https://code.visualstudio.com/api/references/icons-in-labels for
     // icons available in vscode. "~spin" animates the icon.
-    this.lastMessage = vscode.window.setStatusBarMessage(`$(loading~spin) Angular: ${value}`);
+    this.lastMessage = vscode.window.setStatusBarMessage(`$(sync~spin) Angular: ${value}`);
   }
 
   finish() {
     this.lastMessage.dispose();
     this.lastMessage = EMPTY_DISPOSABLE;
   }
 }
-
-interface Task<R> {
-  (progress: vscode.Progress<string>): Promise<R>;
-}
-
-/**
- * Show progress in the editor. Progress is shown while running the given `task`
- * callback and while the promise it returns is in the pending state.
- * If the given `task` returns a rejected promise, this function will reject with
- * the same promise.
- */
-export async function withProgress<R>(options: vscode.ProgressOptions, task: Task<R>): Promise<R> {
-  // Although not strictly compatible, the signature of this function follows
-  // the signature of vscode.window.withProgress() to make it easier to switch
-  // to the official API if we choose to do so later.
-  const reporter = new ProgressReporter();
-  if (options.title) {
-    reporter.report(options.title);
-  }
-  try {
-    return await task(reporter);
-  } finally {
-    reporter.finish();
-  }
-}

common/notifications.ts

@@ -18,19 +18,3 @@ export interface ProjectLanguageServiceParams {
 
 export const ProjectLanguageService =
     new NotificationType<ProjectLanguageServiceParams>('angular/projectLanguageService');
-
-export interface RunNgccParams {
-  configFilePath: string;
-}
-
-export const RunNgcc = new NotificationType<RunNgccParams>('angular/runNgcc');
-
-export type NgccCompleteParams = {
-  configFilePath: string; success: true;
-}|{
-  configFilePath: string;
-  success: false;
-  error: string;
-};
-
-export const NgccComplete = new NotificationType<NgccCompleteParams>('angular/ngccComplete');

common/progress.ts

@@ -0,0 +1,21 @@
+/**
+ * @license
+ * Copyright Google Inc. All Rights Reserved.
+ *
+ * Use of this source code is governed by an MIT-style license that can be
+ * found in the LICENSE file at https://angular.io/license
+ */
+
+import {ProgressType} from 'vscode-jsonrpc';
+
+export type NgccProgress = {
+  done: false,
+  configFilePath: string,
+  message: string,
+}|{
+  done: true,
+  configFilePath: string,
+  success: boolean,
+};
+export const NgccProgressToken = 'ngcc';
+export const NgccProgressType = new ProgressType<NgccProgress>();

integration/lsp/ivy_spec.ts

@@ -9,7 +9,8 @@
 import {MessageConnection} from 'vscode-jsonrpc';
 import * as lsp from 'vscode-languageserver-protocol';
 
-import {NgccComplete, ProjectLanguageService, ProjectLanguageServiceParams, RunNgcc, RunNgccParams} from '../../common/notifications';
+import {ProjectLanguageService, ProjectLanguageServiceParams} from '../../common/notifications';
+import {NgccProgress, NgccProgressToken, NgccProgressType} from '../../common/progress';
 
 import {APP_COMPONENT, createConnection, FOO_TEMPLATE, initializeServer, openTextDocument} from './test_utils';
 
@@ -30,9 +31,9 @@ describe('Angular Ivy language server', () => {
     client.dispose();
   });
 
-  it('should send ngcc notification after a project has finished loading', async () => {
+  it('should send ngcc progress after a project has finished loading', async () => {
     openTextDocument(client, APP_COMPONENT);
-    const configFilePath = await onRunNgccNotification(client);
+    const configFilePath = await onNgccProgress(client);
     expect(configFilePath.endsWith('integration/project/tsconfig.json')).toBeTrue();
   });
 
@@ -82,10 +83,12 @@ describe('Angular Ivy language server', () => {
   });
 });
 
-function onRunNgccNotification(client: MessageConnection): Promise<string> {
+function onNgccProgress(client: MessageConnection): Promise<string> {
   return new Promise(resolve => {
-    client.onNotification(RunNgcc, (params: RunNgccParams) => {
-      resolve(params.configFilePath);
+    client.onProgress(NgccProgressType, NgccProgressToken, (params: NgccProgress) => {
+      if (params.done) {
+        resolve(params.configFilePath);
+      }
     });
   });
 }
@@ -111,11 +114,6 @@ function getDiagnosticsForFile(
 }
 
 async function waitForNgcc(client: MessageConnection): Promise<boolean> {
-  const configFilePath = await onRunNgccNotification(client);
-  // We run ngcc before the test, so no need to do anything here.
-  client.sendNotification(NgccComplete, {
-    success: true,
-    configFilePath,
-  });
+  await onNgccProgress(client);
   return onLanguageServiceStateNotification(client);
 }

scripts/test.sh

@@ -6,7 +6,6 @@ set -ex -o pipefail
 (
   cd integration/project
   yarn
-  yarn ngcc
 )
 
 # Server unit tests

client/src/command-ngcc.ts renamed to server/src/ngcc.ts

@@ -6,9 +6,8 @@
  * found in the LICENSE file at https://angular.io/license
  */
 
-import {spawn} from 'child_process';
+import {fork} from 'child_process';
 import * as path from 'path';
-import * as vscode from 'vscode';
 
 function resolveNgccFrom(directory: string): string|null {
   try {
@@ -20,18 +19,21 @@ function resolveNgccFrom(directory: string): string|null {
   }
 }
 
+interface Progress {
+  report(msg: string): void;
+}
+
 /**
  * Resolve ngcc from the directory that contains the specified `tsconfig` and
  * run ngcc.
  */
-export async function resolveAndRunNgcc(
-    tsconfig: string, progress: vscode.Progress<string>): Promise<void> {
+export async function resolveAndRunNgcc(tsconfig: string, progress: Progress): Promise<void> {
   const directory = path.dirname(tsconfig);
   const ngcc = resolveNgccFrom(directory);
   if (!ngcc) {
     throw new Error(`Failed to resolve ngcc from ${directory}`);
   }
-  const childProcess = spawn(
+  const childProcess = fork(
       ngcc,
       [
         '--tsconfig',
@@ -41,7 +43,12 @@ export async function resolveAndRunNgcc(
         cwd: directory,
       });
 
-  childProcess.stdout.on('data', (data: Buffer) => {
+  let stderr = '';
+  childProcess.stderr?.on('data', (data: Buffer) => {
+    stderr += data.toString();
+  });
+
+  childProcess.stdout?.on('data', (data: Buffer) => {
     for (let entry of data.toString().split('\n')) {
       entry = entry.trim();
       if (entry) {
@@ -58,7 +65,8 @@ export async function resolveAndRunNgcc(
       if (code === 0) {
         resolve();
       } else {
-        throw new Error(`ngcc for ${tsconfig} returned exit code ${code}`);
+        reject(
+            new Error(`ngcc for ${tsconfig} returned exit code ${code}, stderr: ${stderr.trim()}`));
       }
     });
   });