Skip to content

Commit

Permalink
perf: prevent requests from being sent to the server when outside Ang…
Browse files Browse the repository at this point in the history
…ular context

This reverts commit fcbdf93 (revert of
the revert).
  • Loading branch information
atscott committed Mar 15, 2021
1 parent 1223b3a commit da3fbc7
Show file tree
Hide file tree
Showing 9 changed files with 306 additions and 16 deletions.
26 changes: 18 additions & 8 deletions .vscode/launch.json
Original file line number Diff line number Diff line change
Expand Up @@ -9,10 +9,10 @@
"runtimeExecutable": "${execPath}",
"args": [
"--disable-extensions",
"--extensionDevelopmentPath=${workspaceRoot}"
"--extensionDevelopmentPath=${workspaceFolder}"
],
"outFiles": [
"${workspaceRoot}/dist/client/*.js"
"${workspaceFolder}/dist/client/*.js"
],
"preLaunchTask": {
"type": "npm",
Expand All @@ -26,9 +26,19 @@
"port": 6009,
"restart": true,
"outFiles": [
"${workspaceRoot}/dist/server/*.js"
"${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",
"port": 9330,
Expand All @@ -37,7 +47,7 @@
"<node_internals>/**"
],
"outFiles": [
"${workspaceRoot}/dist/integration/lsp/*.js"
"${workspaceFolder}/dist/integration/lsp/*.js"
],
"type": "node"
},
Expand All @@ -47,12 +57,12 @@
"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": [
"${workspaceRoot}/dist/integration/e2e/*.js"
"${workspaceFolder}/dist/integration/e2e/*.js"
],
"preLaunchTask": {
"type": "npm",
Expand Down
34 changes: 33 additions & 1 deletion client/src/client.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -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);
}
}
}
};
}

Expand Down Expand Up @@ -312,4 +344,4 @@ function getServerOptions(ctx: vscode.ExtensionContext, debug: boolean): lsp.Nod
execArgv: debug ? devExecArgv : prodExecArgv,
},
};
}
}
94 changes: 94 additions & 0 deletions client/src/embedded_support.ts
Original file line number Diff line number Diff line change
@@ -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;
}
115 changes: 115 additions & 0 deletions client/src/tests/embedded_support_spec.ts
Original file line number Diff line number Diff line change
@@ -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),
};
}
14 changes: 14 additions & 0 deletions client/src/tests/tsconfig.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
{
"extends": "../../../tsconfig.json",
"compilerOptions": {
"outDir": "../../../dist/client/tests"
},
"references": [
{
"path": "../../tsconfig.json"
}
],
"include": [
"*.ts"
]
}
21 changes: 16 additions & 5 deletions client/tsconfig.json
Original file line number Diff line number Diff line change
@@ -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"
]
}
7 changes: 7 additions & 0 deletions jasmine.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
{
"spec_dir": "dist",
"spec_files": [
"client/tests/*_spec.js",
"server/tests/*_spec.js"
]
}
Loading

0 comments on commit da3fbc7

Please sign in to comment.