perf: prevent requests from being sent to the server when outside Angular context

atscott · atscott · commit fcbdf938e080 · 2021-03-10T15:34:00.000-08:00
The Angular Language Service extension of vscode has several pieces to it: * The client, which communicates directly with vscode * The server, which handles LSP requests over an lsp connection * @angular/language-service which provides concrete answers to LS queries We added an optimization the @angular/language-service which exits early when a request is outside the Angular context. This prevents unnecessary Angular analysis of a file when we know from the start that there are no Angular-specific results at a location. This commit provides an additional optimization by adding a similar preventing short-circuit from the client side. This prevents requests from even being sent to the server when we know there is no Angular information at a location. This optimization is a necessary addition because the server can be blocked from processing requests if another one is taking a while to respond. We found that requests for diagnostics, which are necessary when opening a new file, block other server requests from being processed. This commit would prevent that block from happening by never making the request to the server in the first place. fixes #1176
diff --git a/.vscode/launch.json b/.vscode/launch.json
@@ -7,8 +7,12 @@
       "request": "launch",
       "name": "Launch Client",
       "runtimeExecutable": "${execPath}",
-      "args": ["--extensionDevelopmentPath=${workspaceRoot}"],
-      "outFiles": ["${workspaceRoot}/dist/client/*.js"],
+      "args": [
+        "--extensionDevelopmentPath=${workspaceFolder}"
+      ],
+      "outFiles": [
+        "${workspaceFolder}/dist/client/*.js"
+      ],
       "preLaunchTask": {
         "type": "npm",
         "script": "watch"
@@ -20,7 +24,19 @@
       "name": "Attach to Server",
       "port": 6009,
       "restart": true,
-      "outFiles": ["${workspaceRoot}/dist/server/*.js"]
+      "outFiles": [
+        "${workspaceFolder}/dist/server/*.js"
+      ]
+    },
+    {
+      "type": "node",
+      "request": "attach",
+      "name": "Attach to Jasmine",
+      "port": 9229,
+      "restart": true,
+      "outFiles": [
+        "${workspaceFolder}/dist/**/*.js"
+      ],
     },
     {
       "name": "Integration test: Attach to server",
@@ -29,7 +45,9 @@
       "skipFiles": [
         "<node_internals>/**"
       ],
-      "outFiles": ["${workspaceRoot}/dist/integration/lsp/*.js"],
+      "outFiles": [
+        "${workspaceFolder}/dist/integration/lsp/*.js"
+      ],
       "type": "node"
     },
     {
@@ -38,11 +56,13 @@
       "request": "launch",
       "runtimeExecutable": "${execPath}",
       "args": [
-        "--extensionDevelopmentPath=${workspaceRoot}",
-        "--extensionTestsPath=${workspaceRoot}/dist/integration/e2e",
-        "${workspaceRoot}/integration/project"
+        "--extensionDevelopmentPath=${workspaceFolder}",
+        "--extensionTestsPath=${workspaceFolder}/dist/integration/e2e",
+        "${workspaceFolder}/integration/project"
+      ],
+      "outFiles": [
+        "${workspaceFolder}/dist/integration/e2e/*.js"
       ],
-      "outFiles": ["${workspaceRoot}/dist/integration/e2e/*.js"],
       "preLaunchTask": {
         "type": "npm",
         "script": "compile:integration"
@@ -52,7 +72,10 @@
   "compounds": [
     {
       "name": "Client + Server",
-      "configurations": ["Launch Client", "Attach to Server"]
+      "configurations": [
+        "Launch Client",
+        "Attach to Server"
+      ]
     }
   ]
-}
+}
diff --git a/client/src/client.ts b/client/src/client.ts
@@ -15,6 +15,7 @@ import {ProjectLoadingFinish, ProjectLoadingStart, SuggestIvyLanguageService, Su
 import {NgccProgress, NgccProgressToken, NgccProgressType} from '../common/progress';
 import {GetTcbRequest} from '../common/requests';
 
+import {isInsideComponentDecorator, isInsideInlineTemplateRegion} from './embedded_support';
 import {ProgressReporter} from './progress-reporter';
 
 interface GetTcbResponse {
@@ -50,6 +51,37 @@ export class AngularLanguageClient implements vscode.Disposable {
       // Don't let our output console pop open
       revealOutputChannelOn: lsp.RevealOutputChannelOn.Never,
       outputChannel: this.outputChannel,
+      middleware: {
+        provideDefinition: async (
+            document: vscode.TextDocument, position: vscode.Position,
+            token: vscode.CancellationToken, next: lsp.ProvideDefinitionSignature) => {
+          if (isInsideComponentDecorator(document, position)) {
+            return next(document, position, token);
+          }
+        },
+        provideTypeDefinition: async (
+            document: vscode.TextDocument, position: vscode.Position,
+            token: vscode.CancellationToken, next) => {
+          if (isInsideInlineTemplateRegion(document, position)) {
+            return next(document, position, token);
+          }
+        },
+        provideHover: async (
+            document: vscode.TextDocument, position: vscode.Position,
+            token: vscode.CancellationToken, next: lsp.ProvideHoverSignature) => {
+          if (isInsideInlineTemplateRegion(document, position)) {
+            return next(document, position, token);
+          }
+        },
+        provideCompletionItem: async (
+            document: vscode.TextDocument, position: vscode.Position,
+            context: vscode.CompletionContext, token: vscode.CancellationToken,
+            next: lsp.ProvideCompletionItemsSignature) => {
+          if (isInsideInlineTemplateRegion(document, position)) {
+            return next(document, position, context, token);
+          }
+        }
+      }
     };
   }
 
@@ -313,4 +345,4 @@ function getServerOptions(ctx: vscode.ExtensionContext, debug: boolean): lsp.Nod
       execArgv: debug ? devExecArgv : prodExecArgv,
     },
   };
-}
+}
diff --git a/client/src/embedded_support.ts b/client/src/embedded_support.ts
@@ -0,0 +1,94 @@
+/**
+ * @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 * as ts from 'typescript';
+import * as vscode from 'vscode';
+
+/** Determines if the position is inside an inline template. */
+export function isInsideInlineTemplateRegion(
+    document: vscode.TextDocument, position: vscode.Position): boolean {
+  if (document.languageId !== 'typescript') {
+    return true;
+  }
+  return isPropertyAssignmentToStringOrStringInArray(
+      document.getText(), document.offsetAt(position), ['template']);
+}
+
+/** Determines if the position is inside an inline template, templateUrl, or string in styleUrls. */
+export function isInsideComponentDecorator(
+    document: vscode.TextDocument, position: vscode.Position): boolean {
+  if (document.languageId !== 'typescript') {
+    return true;
+  }
+  return isPropertyAssignmentToStringOrStringInArray(
+      document.getText(), document.offsetAt(position), ['template', 'templateUrl', 'styleUrls']);
+}
+
+/**
+ * Basic scanner to determine if we're inside a string of a property with one of the given names.
+ *
+ * This scanner is not currently robust or perfect but provides us with an accurate answer _most_ of
+ * the time.
+ *
+ * False positives are OK here. Though this will give some false positives for determining if a
+ * position is within an Angular context, i.e. an object like `{template: ''}` that is not inside an
+ * `@Component` or `{styleUrls: [someFunction('stringL¦iteral')]}`, the @angular/language-service
+ * will always give us the correct answer. This helper gives us a quick win for optimizing the
+ * number of requests we send to the server.
+ */
+function isPropertyAssignmentToStringOrStringInArray(
+    documentText: string, offset: number, propertyAssignmentNames: string[]): boolean {
+  const scanner = ts.createScanner(ts.ScriptTarget.ESNext, true /* skipTrivia */);
+  scanner.setText(documentText);
+
+  let token: ts.SyntaxKind = scanner.scan();
+  let lastToken: ts.SyntaxKind|undefined;
+  let lastTokenText: string|undefined;
+  let unclosedBraces = 0;
+  let unclosedBrackets = 0;
+  let propertyAssignmentContext = false;
+  while (token !== ts.SyntaxKind.EndOfFileToken && scanner.getStartPos() < offset) {
+    if (lastToken === ts.SyntaxKind.Identifier && lastTokenText !== undefined &&
+        propertyAssignmentNames.includes(lastTokenText) && token === ts.SyntaxKind.ColonToken) {
+      propertyAssignmentContext = true;
+      token = scanner.scan();
+      continue;
+    }
+    if (unclosedBraces === 0 && unclosedBrackets === 0 && isPropertyAssignmentTerminator(token)) {
+      propertyAssignmentContext = false;
+    }
+
+    if (token === ts.SyntaxKind.OpenBracketToken) {
+      unclosedBrackets++;
+    } else if (token === ts.SyntaxKind.OpenBraceToken) {
+      unclosedBraces++;
+    } else if (token === ts.SyntaxKind.CloseBracketToken) {
+      unclosedBrackets--;
+    } else if (token === ts.SyntaxKind.CloseBraceToken) {
+      unclosedBraces--;
+    }
+
+    const isStringToken = token === ts.SyntaxKind.StringLiteral ||
+        token === ts.SyntaxKind.NoSubstitutionTemplateLiteral;
+    const isCursorInToken = scanner.getStartPos() <= offset &&
+        scanner.getStartPos() + scanner.getTokenText().length >= offset;
+    if (propertyAssignmentContext && isCursorInToken && isStringToken) {
+      return true;
+    }
+
+    lastTokenText = scanner.getTokenText();
+    lastToken = token;
+    token = scanner.scan();
+  }
+
+  return false;
+}
+
+function isPropertyAssignmentTerminator(token: ts.SyntaxKind) {
+  return token === ts.SyntaxKind.EndOfFileToken || token === ts.SyntaxKind.CommaToken ||
+      token === ts.SyntaxKind.SemicolonToken || token === ts.SyntaxKind.CloseBraceToken;
+}
diff --git a/client/src/tests/embedded_support_spec.ts b/client/src/tests/embedded_support_spec.ts
@@ -0,0 +1,115 @@
+/**
+ * @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 * as vscode from 'vscode';
+import {DocumentUri, TextDocument} from 'vscode-languageserver-textdocument';
+
+import {isInsideComponentDecorator, isInsideInlineTemplateRegion} from '../embedded_support';
+
+describe('embedded language support', () => {
+  describe('isInsideInlineTemplateRegion', () => {
+    it('empty file', () => {
+      test('¦', isInsideInlineTemplateRegion, false);
+    });
+
+    it('just after template', () => {
+      test(`template: '<div></div>'¦`, isInsideInlineTemplateRegion, false);
+    });
+
+    it('just before template', () => {
+      // Note that while it seems that this should be `false`, we should still consider this inside
+      // the string because the visual mode of vim appears to have a position on top of the open
+      // quote while the cursor position is before it.
+      test(`template: ¦'<div></div>'`, isInsideInlineTemplateRegion, true);
+    });
+
+    it('two spaces before template', () => {
+      test(`template:¦ '<div></div>'`, isInsideInlineTemplateRegion, false);
+    });
+
+    it('at beginning of template', () => {
+      test(`template: '¦<div></div>'`, isInsideInlineTemplateRegion, true);
+    });
+
+    it('at end of template', () => {
+      test(`template: '<div></div>¦'`, isInsideInlineTemplateRegion, true);
+    });
+  });
+
+  describe('isInsideAngularContext', () => {
+    it('empty file', () => {
+      test('¦', isInsideComponentDecorator, false);
+    });
+
+    it('just after template', () => {
+      test(`template: '<div></div>'¦`, isInsideComponentDecorator, false);
+    });
+
+    it('inside template', () => {
+      test(`template: '<div>¦</div>'`, isInsideComponentDecorator, true);
+    });
+
+    it('just after templateUrl', () => {
+      test(`templateUrl: './abc.html'¦`, isInsideComponentDecorator, false);
+    });
+
+    it('inside templateUrl', () => {
+      test(`templateUrl: './abc¦.html'`, isInsideComponentDecorator, true);
+    });
+
+    it('just after styleUrls', () => {
+      test(`styleUrls: ['./abc.css']¦`, isInsideComponentDecorator, false);
+    });
+
+    it('inside first item of styleUrls', () => {
+      test(`styleUrls: ['./abc.c¦ss', 'def.css']`, isInsideComponentDecorator, true);
+    });
+
+    it('inside second item of styleUrls', () => {
+      test(`styleUrls: ['./abc.css', 'def¦.css']`, isInsideComponentDecorator, true);
+    });
+
+    it('inside second item of styleUrls, when first is complicated function', () => {
+      test(
+          `styleUrls: [getCss({strict: true, dirs: ['apple', 'banana']}), 'def¦.css']`,
+          isInsideComponentDecorator, true);
+    });
+
+    it('inside non-string item of styleUrls', () => {
+      test(
+          `styleUrls: [getCss({strict: true¦, dirs: ['apple', 'banana']}), 'def.css']`,
+          isInsideComponentDecorator, false);
+    });
+  });
+});
+
+function test(
+    fileWithCursor: string,
+    testFn: (doc: vscode.TextDocument, position: vscode.Position) => boolean,
+    expectation: boolean): void {
+  const {cursor, text} = extractCursorInfo(fileWithCursor);
+  const vdoc = TextDocument.create('test' as DocumentUri, 'typescript', 0, text) as {} as
+      vscode.TextDocument;
+  const actual = testFn(vdoc, vdoc.positionAt(cursor));
+  expect(actual).toBe(expectation);
+}
+
+/**
+ * Given a text snippet which contains exactly one cursor symbol ('¦'), extract both the offset of
+ * that cursor within the text as well as the text snippet without the cursor.
+ */
+function extractCursorInfo(textWithCursor: string): {cursor: number, text: string} {
+  const cursor = textWithCursor.indexOf('¦');
+  if (cursor === -1 || textWithCursor.indexOf('¦', cursor + 1) !== -1) {
+    throw new Error(`Expected to find exactly one cursor symbol '¦'`);
+  }
+
+  return {
+    cursor,
+    text: textWithCursor.substr(0, cursor) + textWithCursor.substr(cursor + 1),
+  };
+}
diff --git a/client/src/tests/tsconfig.json b/client/src/tests/tsconfig.json
@@ -0,0 +1,14 @@
+{
+  "extends": "../../../tsconfig.json",
+  "compilerOptions": {
+    "outDir": "../../../dist/client/tests"
+  },
+  "references": [
+    {
+      "path": "../../tsconfig.json"
+    }
+  ],
+  "include": [
+    "*.ts"
+  ]
+}
diff --git a/client/tsconfig.json b/client/tsconfig.json
@@ -1,12 +1,23 @@
 {
   "extends": "../tsconfig.json",
   "compilerOptions": {
+    "composite": true,
     "outDir": "../dist/client",
-    "rootDirs": [".", "../dist"]
+    "rootDir": "src",
+    "rootDirs": [
+      ".",
+      "../dist"
+    ]
   },
   "references": [
-    {"path": "../common"}
+    {
+      "path": "../common"
+    }
   ],
-  "include": ["src"],
-  "exclude": ["node_modules"]
-}
+  "include": [
+    "src"
+  ],
+  "exclude": [
+    "node_modules"
+  ]
+}
diff --git a/jasmine.json b/jasmine.json
@@ -0,0 +1,7 @@
+{
+  "spec_dir": "dist",
+  "spec_files": [
+    "client/tests/*_spec.js",
+    "server/tests/*_spec.js"
+  ]
+}
diff --git a/package.json b/package.json
diff --git a/yarn.lock b/yarn.lock
