From 0223aa6121bf23d92d9b1d42a81b96ac023ca679 Mon Sep 17 00:00:00 2001 From: Keen Yee Liau Date: Thu, 23 Jan 2020 12:02:47 -0800 Subject: [PATCH] fix(language-service): Diagnostic span should point to class name (#34932) Right now, if an Angular diagnostic is generated for a TypeScript node, the span points to the decorator Identifier, i.e. the Identifier node like `@NgModule`, `@Component`, etc. This is weird. It should point to the class name instead. Note, we do not have a more fine-grained breakdown of the span when diagnostics are emitted, this work remains to be done. PR Close #34932 --- packages/language-service/src/typescript_host.ts | 6 +++--- packages/language-service/src/utils.ts | 16 ++++++++-------- .../language-service/test/diagnostics_spec.ts | 14 ++++---------- packages/language-service/test/utils_spec.ts | 6 +++--- 4 files changed, 18 insertions(+), 24 deletions(-) diff --git a/packages/language-service/src/typescript_host.ts b/packages/language-service/src/typescript_host.ts index a9576638b4..573164ac9b 100644 --- a/packages/language-service/src/typescript_host.ts +++ b/packages/language-service/src/typescript_host.ts @@ -288,9 +288,9 @@ export class TypeScriptServiceHost implements LanguageServiceHost { const visit = (child: tss.Node) => { const candidate = getDirectiveClassLike(child); if (candidate) { - const {decoratorId, classDecl} = candidate; - const declarationSpan = spanOf(decoratorId); - const className = classDecl.name !.text; + const {classId} = candidate; + const declarationSpan = spanOf(classId); + const className = classId.getText(); const classSymbol = this.reflector.getStaticSymbol(sourceFile.fileName, className); // Ask the resolver to check if candidate is actually Angular directive if (!this.resolver.isDirective(classSymbol)) { diff --git a/packages/language-service/src/utils.ts b/packages/language-service/src/utils.ts index 934457e9ce..59d1f0fad4 100644 --- a/packages/language-service/src/utils.ts +++ b/packages/language-service/src/utils.ts @@ -164,8 +164,8 @@ export function findTightestNode(node: ts.Node, position: number): ts.Node|undef } interface DirectiveClassLike { - decoratorId: ts.Identifier; // decorator identifier - classDecl: ts.ClassDeclaration; + decoratorId: ts.Identifier; // decorator identifier, like @Component + classId: ts.Identifier; } /** @@ -178,11 +178,11 @@ interface DirectiveClassLike { * * For example, * v---------- `decoratorId` - * @NgModule({ - * declarations: [], - * }) - * class AppModule {} - * ^----- `classDecl` + * @NgModule({ < + * declarations: [], < classDecl + * }) < + * class AppModule {} < + * ^----- `classId` * * @param node Potential node that represents an Angular directive. */ @@ -200,7 +200,7 @@ export function getDirectiveClassLike(node: ts.Node): DirectiveClassLike|undefin if (ts.isObjectLiteralExpression(arg)) { return { decoratorId: expr.expression, - classDecl: node, + classId: node.name, }; } } diff --git a/packages/language-service/test/diagnostics_spec.ts b/packages/language-service/test/diagnostics_spec.ts index 7ae5bf4e5e..bc75d329de 100644 --- a/packages/language-service/test/diagnostics_spec.ts +++ b/packages/language-service/test/diagnostics_spec.ts @@ -354,9 +354,7 @@ describe('diagnostics', () => { .toBe( `Component 'MyComponent' is not included in a module and will not be available inside a template. Consider adding it to a NgModule declaration.`); const content = mockHost.readFile(fileName) !; - const keyword = '@Component'; - expect(start).toBe(content.lastIndexOf(keyword) + 1); // exclude leading '@' - expect(length).toBe(keyword.length - 1); // exclude leading '@' + expect(content.substring(start !, start ! + length !)).toBe('MyComponent'); }); @@ -596,9 +594,7 @@ describe('diagnostics', () => { .toBe( 'Invalid providers for "AppComponent in /app/app.component.ts" - only instances of Provider and Type are allowed, got: [?null?]'); // TODO: Looks like this is the wrong span. Should point to 'null' instead. - const keyword = '@Component'; - expect(start).toBe(content.lastIndexOf(keyword) + 1); // exclude leading '@' - expect(length).toBe(keyword.length - 1); // exclude leading '@ + expect(content.substring(start !, start ! + length !)).toBe('AppComponent'); }); // Issue #15768 @@ -767,8 +763,7 @@ describe('diagnostics', () => { const {file, messageText, start, length} = diags[0]; expect(file !.fileName).toBe(APP_COMPONENT); expect(messageText).toBe(`Component 'AppComponent' must have a template or templateUrl`); - expect(start).toBe(content.indexOf(`@Component`) + 1); - expect(length).toBe('Component'.length); + expect(content.substring(start !, start ! + length !)).toBe('AppComponent'); }); it('should report diagnostic for both template and templateUrl', () => { @@ -787,8 +782,7 @@ describe('diagnostics', () => { expect(file !.fileName).toBe(APP_COMPONENT); expect(messageText) .toBe(`Component 'AppComponent' must not have both template and templateUrl`); - expect(start).toBe(content.indexOf(`@Component`) + 1); - expect(length).toBe('Component'.length); + expect(content.substring(start !, start ! + length !)).toBe('AppComponent'); }); it('should report errors for invalid styleUrls', () => { diff --git a/packages/language-service/test/utils_spec.ts b/packages/language-service/test/utils_spec.ts index 8419924c2b..ee0670ea93 100644 --- a/packages/language-service/test/utils_spec.ts +++ b/packages/language-service/test/utils_spec.ts @@ -28,10 +28,10 @@ describe('getDirectiveClassLike()', () => { } }); expect(result).toBeTruthy(); - const {decoratorId, classDecl} = result !; + const {decoratorId, classId} = result !; expect(decoratorId.kind).toBe(ts.SyntaxKind.Identifier); - expect((decoratorId as ts.Identifier).text).toBe('NgModule'); - expect(classDecl.name !.text).toBe('AppModule'); + expect(decoratorId.text).toBe('NgModule'); + expect(classId.text).toBe('AppModule'); }); });