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
This commit is contained in:
parent
b50ed5c22c
commit
0223aa6121
|
@ -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)) {
|
||||
|
|
|
@ -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,
|
||||
};
|
||||
}
|
||||
}
|
||||
|
|
|
@ -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', () => {
|
||||
|
|
|
@ -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');
|
||||
});
|
||||
});
|
||||
|
||||
|
|
Loading…
Reference in New Issue