From 71ada483bfa91a54b7adf6f172e46804cab91035 Mon Sep 17 00:00:00 2001 From: Keen Yee Liau Date: Thu, 15 Aug 2019 14:09:33 -0700 Subject: [PATCH] refactor(language-service): Omit typechecking for finding directives (#32156) Remove unnecessary private method `getDeclarationFromNode` and moved some logic to utils instead so that it can be tested in isolation of the Language Service infrastructure. The use of typechecker to check the directive is also not necessary, since resolve.getNonNormalizedDirectiveMetadata() will check if the directive is actually an Angular entity. PR Close #32156 --- .../language-service/src/typescript_host.ts | 102 ++++++++---------- packages/language-service/src/utils.ts | 47 +++++++- .../language-service/test/template_spec.ts | 21 +++- packages/language-service/test/utils_spec.ts | 34 ++++++ 4 files changed, 141 insertions(+), 63 deletions(-) create mode 100644 packages/language-service/test/utils_spec.ts diff --git a/packages/language-service/src/typescript_host.ts b/packages/language-service/src/typescript_host.ts index 575d0912f4..ee1bce4b29 100644 --- a/packages/language-service/src/typescript_host.ts +++ b/packages/language-service/src/typescript_host.ts @@ -14,8 +14,8 @@ import {AstResult, TemplateInfo} from './common'; import {createLanguageService} from './language_service'; import {ReflectorHost} from './reflector_host'; import {ExternalTemplate, InlineTemplate, getClassDeclFromTemplateNode} from './template'; -import {Declaration, DeclarationError, Declarations, Diagnostic, DiagnosticKind, DiagnosticMessageChain, LanguageService, LanguageServiceHost, Span, TemplateSource} from './types'; -import {findTighestNode} from './utils'; +import {Declaration, DeclarationError, Diagnostic, DiagnosticKind, DiagnosticMessageChain, LanguageService, LanguageServiceHost, Span, TemplateSource} from './types'; +import {findTightestNode, getDirectiveClassLike} from './utils'; /** * Create a `LanguageServiceHost` @@ -194,24 +194,50 @@ export class TypeScriptServiceHost implements LanguageServiceHost { return results; } - getDeclarations(fileName: string): Declarations { + /** + * Return metadata about all class declarations in the file that are Angular + * directives. Potential matches are `@NgModule`, `@Component`, `@Directive`, + * `@Pipes`, etc. class declarations. + * + * @param fileName TS file + */ + getDeclarations(fileName: string): Declaration[] { if (!fileName.endsWith('.ts')) { return []; } - const result: Declarations = []; const sourceFile = this.getSourceFile(fileName); - if (sourceFile) { - const visit = (child: ts.Node) => { - const declaration = this.getDeclarationFromNode(sourceFile, child); - if (declaration) { - result.push(declaration); - } else { - ts.forEachChild(child, visit); - } - }; - ts.forEachChild(sourceFile, visit); + if (!sourceFile) { + return []; } - return result; + const results: Declaration[] = []; + const visit = (child: ts.Node) => { + const candidate = getDirectiveClassLike(child); + if (candidate) { + const {decoratorId, classDecl} = candidate; + const declarationSpan = spanOf(decoratorId); + const className = classDecl.name !.text; + const classSymbol = this.reflector.getStaticSymbol(sourceFile.fileName, className); + // Ask the resolver to check if candidate is actually Angular directive + if (!this.resolver.isDirective(classSymbol)) { + return; + } + const data = this.resolver.getNonNormalizedDirectiveMetadata(classSymbol); + if (!data) { + return; + } + results.push({ + type: classSymbol, + declarationSpan, + metadata: data.metadata, + errors: this.getCollectedErrors(declarationSpan, sourceFile), + }); + } else { + child.forEachChild(visit); + } + }; + ts.forEachChild(sourceFile, visit); + + return results; } getSourceFile(fileName: string): ts.SourceFile|undefined { @@ -277,7 +303,6 @@ export class TypeScriptServiceHost implements LanguageServiceHost { * class AppComponent {} * ^---- class declaration node * - * * @param node Potential template node */ private getInternalTemplate(node: ts.Node): TemplateSource|undefined { @@ -355,49 +380,6 @@ export class TypeScriptServiceHost implements LanguageServiceHost { }); } - private getDeclarationFromNode(sourceFile: ts.SourceFile, node: ts.Node): Declaration|undefined { - if (node.kind == ts.SyntaxKind.ClassDeclaration && node.decorators && - (node as ts.ClassDeclaration).name) { - for (const decorator of node.decorators) { - if (decorator.expression && decorator.expression.kind == ts.SyntaxKind.CallExpression) { - const classDeclaration = node as ts.ClassDeclaration; - if (classDeclaration.name) { - const call = decorator.expression as ts.CallExpression; - const target = call.expression; - const type = this.program.getTypeChecker().getTypeAtLocation(target); - if (type) { - const staticSymbol = - this.reflector.getStaticSymbol(sourceFile.fileName, classDeclaration.name.text); - try { - if (this.resolver.isDirective(staticSymbol as any)) { - const {metadata} = - this.resolver.getNonNormalizedDirectiveMetadata(staticSymbol as any) !; - const declarationSpan = spanOf(target); - return { - type: staticSymbol, - declarationSpan, - metadata, - errors: this.getCollectedErrors(declarationSpan, sourceFile) - }; - } - } catch (e) { - if (e.message) { - this.collectError(e, sourceFile.fileName); - const declarationSpan = spanOf(target); - return { - type: staticSymbol, - declarationSpan, - errors: this.getCollectedErrors(declarationSpan, sourceFile) - }; - } - } - } - } - } - } - } - } - /** * Return the parsed template for the template at the specified `position`. * @param fileName TS or HTML file @@ -411,7 +393,7 @@ export class TypeScriptServiceHost implements LanguageServiceHost { return; } // Find the node that most closely matches the position - const node = findTighestNode(sourceFile, position); + const node = findTightestNode(sourceFile, position); if (!node) { return; } diff --git a/packages/language-service/src/utils.ts b/packages/language-service/src/utils.ts index 89d77874be..0a116d72d4 100644 --- a/packages/language-service/src/utils.ts +++ b/packages/language-service/src/utils.ts @@ -183,8 +183,51 @@ export function findTemplateAstAt( * @param node * @param position */ -export function findTighestNode(node: ts.Node, position: number): ts.Node|undefined { +export function findTightestNode(node: ts.Node, position: number): ts.Node|undefined { if (node.getStart() <= position && position < node.getEnd()) { - return node.forEachChild(c => findTighestNode(c, position)) || node; + return node.forEachChild(c => findTightestNode(c, position)) || node; + } +} + +interface DirectiveClassLike { + decoratorId: ts.Identifier; // decorator identifier + classDecl: ts.ClassDeclaration; +} + +/** + * Return metadata about `node` if it looks like an Angular directive class. + * In this case, potential matches are `@NgModule`, `@Component`, `@Directive`, + * `@Pipe`, etc. + * These class declarations all share some common attributes, namely their + * decorator takes exactly one parameter and the parameter must be an object + * literal. + * + * For example, + * v---------- `decoratorId` + * @NgModule({ + * declarations: [], + * }) + * class AppModule {} + * ^----- `classDecl` + * + * @param node Potential node that represents an Angular directive. + */ +export function getDirectiveClassLike(node: ts.Node): DirectiveClassLike|undefined { + if (!ts.isClassDeclaration(node) || !node.name || !node.decorators) { + return; + } + for (const d of node.decorators) { + const expr = d.expression; + if (!ts.isCallExpression(expr) || expr.arguments.length !== 1 || + !ts.isIdentifier(expr.expression)) { + continue; + } + const arg = expr.arguments[0]; + if (ts.isObjectLiteralExpression(arg)) { + return { + decoratorId: expr.expression, + classDecl: node, + }; + } } } diff --git a/packages/language-service/test/template_spec.ts b/packages/language-service/test/template_spec.ts index 4f159211cf..1992af4650 100644 --- a/packages/language-service/test/template_spec.ts +++ b/packages/language-service/test/template_spec.ts @@ -12,7 +12,26 @@ import {toh} from './test_data'; import {MockTypescriptHost} from './test_utils'; describe('getClassDeclFromTemplateNode', () => { - it('should return class declaration', () => { + + it('should find class declaration in syntax-only mode', () => { + const sourceFile = ts.createSourceFile( + 'foo.ts', ` + @Component({ + template: '
' + }) + class MyComponent {}`, + ts.ScriptTarget.ES2015, true /* setParentNodes */); + function visit(node: ts.Node): ts.ClassDeclaration|undefined { + return getClassDeclFromTemplateNode(node) || node.forEachChild(visit); + } + const classDecl = sourceFile.forEachChild(visit); + expect(classDecl).toBeTruthy(); + expect(classDecl !.kind).toBe(ts.SyntaxKind.ClassDeclaration); + expect((classDecl as ts.ClassDeclaration).name !.text).toBe('MyComponent'); + }); + + + it('should return class declaration for AppComponent', () => { const host = new MockTypescriptHost(['/app/app.component.ts'], toh); const tsLS = ts.createLanguageService(host); const sourceFile = tsLS.getProgram() !.getSourceFile('/app/app.component.ts'); diff --git a/packages/language-service/test/utils_spec.ts b/packages/language-service/test/utils_spec.ts new file mode 100644 index 0000000000..f032da97b5 --- /dev/null +++ b/packages/language-service/test/utils_spec.ts @@ -0,0 +1,34 @@ +/** + * @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 {getDirectiveClassLike} from '../src/utils'; + +describe('getDirectiveClassLike()', () => { + it('should return a directive class', () => { + const sourceFile = ts.createSourceFile( + 'foo.ts', ` + @NgModule({ + declarations: [], + }) + class AppModule {} + `, + ts.ScriptTarget.ES2015); + const result = sourceFile.forEachChild(c => { + const directive = getDirectiveClassLike(c); + if (directive) { + return directive; + } + }); + expect(result).toBeTruthy(); + const {decoratorId, classDecl} = result !; + expect(decoratorId.kind).toBe(ts.SyntaxKind.Identifier); + expect((decoratorId as ts.Identifier).text).toBe('NgModule'); + expect(classDecl.name !.text).toBe('AppModule'); + }); +});