From 437e5635072a7b3259b612055761955ab12d32af Mon Sep 17 00:00:00 2001 From: Andrew Scott Date: Fri, 9 Oct 2020 10:58:16 -0700 Subject: [PATCH] refactor(language-service): Allow "go to definition" for directives in Ivy (#39228) For directives/components, it would be generally more appropriate for "go to type definition" to be the function which navigates to the class definition. However, for a better user experience, we should do this for "go to definition" as well. PR Close #39228 --- packages/language-service/ivy/definitions.ts | 47 ++++++++------ .../ivy/test/definitions_spec.ts | 62 ++++++++++++------- 2 files changed, 68 insertions(+), 41 deletions(-) diff --git a/packages/language-service/ivy/definitions.ts b/packages/language-service/ivy/definitions.ts index a5a4d4ea93..7de4ecc63a 100644 --- a/packages/language-service/ivy/definitions.ts +++ b/packages/language-service/ivy/definitions.ts @@ -8,7 +8,7 @@ import {AST, TmplAstNode, TmplAstTextAttribute} from '@angular/compiler'; import {NgCompiler} from '@angular/compiler-cli/src/ngtsc/core'; -import {ShimLocation, Symbol, SymbolKind} from '@angular/compiler-cli/src/ngtsc/typecheck/api'; +import {DirectiveSymbol, DomBindingSymbol, ElementSymbol, ShimLocation, Symbol, SymbolKind, TemplateSymbol} from '@angular/compiler-cli/src/ngtsc/typecheck/api'; import * as ts from 'typescript'; import {findNodeAtPosition} from './hybrid_visitor'; @@ -52,17 +52,12 @@ export class DefinitionBuilder { case SymbolKind.Element: case SymbolKind.Template: case SymbolKind.DomBinding: - // `Template` and `Element` types should not return anything because their "definitions" are - // the template locations themselves. Instead, `getTypeDefinitionAtPosition` should return - // the directive class / native element interface. `Directive` would have similar reasoning, - // though the `TemplateTypeChecker` only returns it as a list on `DomBinding`, `Element`, or - // `Template` so it's really only here for switch case completeness (it wouldn't ever appear - // here). - // - // `DomBinding` also does not return anything because the value assignment is internal to - // the TCB. Again, `getTypeDefinitionAtPosition` could return a possible directive the - // attribute binds to or the property in the native interface. - return []; + // Though it is generally more appropriate for the above symbol definitions to be + // associated with "type definitions" since the location in the template is the + // actual definition location, the better user experience would be to allow + // LS users to "go to definition" on an item in the template that maps to a class and be + // taken to the directive or HTML class. + return this.getTypeDefinitionsForTemplateInstance(symbol, node); case SymbolKind.Input: case SymbolKind.Output: return this.getDefinitionsForSymbols(...symbol.bindings); @@ -110,14 +105,32 @@ export class DefinitionBuilder { } const {symbol, node} = definitionMeta; + switch (symbol.kind) { + case SymbolKind.Directive: + case SymbolKind.DomBinding: + case SymbolKind.Element: + case SymbolKind.Template: + return this.getTypeDefinitionsForTemplateInstance(symbol, node); + case SymbolKind.Output: + case SymbolKind.Input: + return this.getTypeDefinitionsForSymbols(...symbol.bindings); + case SymbolKind.Reference: + case SymbolKind.Expression: + case SymbolKind.Variable: + return this.getTypeDefinitionsForSymbols(symbol); + } + } + + private getTypeDefinitionsForTemplateInstance( + symbol: TemplateSymbol|ElementSymbol|DomBindingSymbol|DirectiveSymbol, + node: AST|TmplAstNode): ts.DefinitionInfo[] { switch (symbol.kind) { case SymbolKind.Template: { const matches = getDirectiveMatchesForElementTag(symbol.templateNode, symbol.directives); return this.getTypeDefinitionsForSymbols(...matches); } case SymbolKind.Element: { - const matches = getDirectiveMatchesForAttribute( - symbol.templateNode.name, symbol.templateNode, symbol.directives); + const matches = getDirectiveMatchesForElementTag(symbol.templateNode, symbol.directives); // If one of the directive matches is a component, we should not include the native element // in the results because it is replaced by the component. return Array.from(matches).some(dir => dir.isComponent) ? @@ -132,13 +145,7 @@ export class DefinitionBuilder { node.name, symbol.host.templateNode, symbol.host.directives); return this.getTypeDefinitionsForSymbols(...dirs); } - case SymbolKind.Output: - case SymbolKind.Input: - return this.getTypeDefinitionsForSymbols(...symbol.bindings); - case SymbolKind.Reference: case SymbolKind.Directive: - case SymbolKind.Expression: - case SymbolKind.Variable: return this.getTypeDefinitionsForSymbols(symbol); } } diff --git a/packages/language-service/ivy/test/definitions_spec.ts b/packages/language-service/ivy/test/definitions_spec.ts index 8f4dc42364..99313d2328 100644 --- a/packages/language-service/ivy/test/definitions_spec.ts +++ b/packages/language-service/ivy/test/definitions_spec.ts @@ -22,12 +22,15 @@ describe('definitions', () => { }); describe('elements', () => { - it('should return nothing for native elements', () => { - const {position} = service.overwriteInlineTemplate(APP_COMPONENT, ``); - const definitionAndBoundSpan = ngLS.getDefinitionAndBoundSpan(APP_COMPONENT, position); - // The "definition" is this location itself so we should return nothing. - // getTypeDefinitionAtPosition would return the HTMLButtonElement interface. - expect(definitionAndBoundSpan!.definitions).toEqual([]); + it('should work for native elements', () => { + const defs = getDefinitionsAndAssertBoundSpan({ + templateOverride: ``, + expectedSpanText: ``, + }); + expect(defs.length).toEqual(2); + expect(defs[0].fileName).toContain('lib.dom.d.ts'); + expect(defs[0].contextSpan).toContain('interface HTMLButtonElement extends HTMLElement'); + expect(defs[1].contextSpan).toContain('declare var HTMLButtonElement'); }); }); @@ -42,11 +45,13 @@ describe('definitions', () => { describe('directives', () => { it('should work for directives', () => { - const definitions = getDefinitionsAndAssertBoundSpan({ + const defs = getDefinitionsAndAssertBoundSpan({ templateOverride: `
`, expectedSpanText: 'string-model', }); - expect(definitions).toEqual([]); + expect(defs.length).toEqual(1); + expect(defs[0].contextSpan).toContain('@Directive'); + expect(defs[0].contextSpan).toContain('export class StringModel'); }); it('should work for components', () => { @@ -58,17 +63,25 @@ describe('definitions', () => { templateOverride, expectedSpanText: templateOverride.replace('¦', '').trim(), }); - expect(definitions).toEqual([]); + expect(definitions.length).toEqual(1); + + expect(definitions.length).toEqual(1); + expect(definitions[0].textSpan).toEqual('TestComponent'); + expect(definitions[0].contextSpan).toContain('@Component'); }); - it('should not return anything for structural directives where the key does not map to a binding', - () => { - const definitions = getDefinitionsAndAssertBoundSpan({ - templateOverride: `
`, - expectedSpanText: 'ngFor', - }); - expect(definitions).toEqual([]); - }); + it('should work for structural directives', () => { + const definitions = getDefinitionsAndAssertBoundSpan({ + templateOverride: `
`, + expectedSpanText: 'ngFor', + }); + expect(definitions.length).toEqual(1); + expect(definitions[0].fileName).toContain('ng_for_of.d.ts'); + expect(definitions[0].textSpan).toEqual('NgForOf'); + expect(definitions[0].contextSpan) + .toContain( + 'export declare class NgForOf = NgIterable> implements DoCheck'); + }); it('should return binding for structural directive where key maps to a binding', () => { const definitions = getDefinitionsAndAssertBoundSpan({ @@ -83,11 +96,18 @@ describe('definitions', () => { }); it('should work for directives with compound selectors', () => { - const definitions = getDefinitionsAndAssertBoundSpan({ - templateOverride: `{{item}}`, - expectedSpanText: 'ngFor', + let defs = getDefinitionsAndAssertBoundSpan({ + templateOverride: ``, + expectedSpanText: 'compound', }); - expect(definitions).toEqual([]); + expect(defs.length).toEqual(1); + expect(defs[0].contextSpan).toContain('export class CompoundCustomButtonDirective'); + defs = getDefinitionsAndAssertBoundSpan({ + templateOverride: ``, + expectedSpanText: 'custom-button', + }); + expect(defs.length).toEqual(1); + expect(defs[0].contextSpan).toContain('export class CompoundCustomButtonDirective'); }); });