honeymoose
/
angular-cn
1
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

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
This commit is contained in:
Andrew Scott 2020-10-09 10:58:16 -07:00 committed by atscott
parent b0b4953fd6
commit 437e563507
2 changed files with 68 additions and 41 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

47
packages/language-service/ivy/definitions.ts
View File

@ -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);
}
}

62
packages/language-service/ivy/test/definitions_spec.ts
View File

@ -22,12 +22,15 @@ describe('definitions', () => {
});
describe('elements', () => {
it('should return nothing for native elements', () => {
const {position} = service.overwriteInlineTemplate(APP_COMPONENT, `<butt¦on></button>`);
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: `<butt¦on></button>`,
expectedSpanText: `<button></button>`,
});
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: `<div string-model¦></div>`,
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: `<div *¦ngFor="let item of heroes"></div>`,
expectedSpanText: 'ngFor',
});
expect(definitions).toEqual([]);
});
it('should work for structural directives', () => {
const definitions = getDefinitionsAndAssertBoundSpan({
templateOverride: `<div *¦ngFor="let item of heroes"></div>`,
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<T, U extends NgIterable<T> = NgIterable<T>> 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: `<ng-template ngF¦or let-item [ngForOf]="items">{{item}}</ng-template>`,
expectedSpanText: 'ngFor',
let defs = getDefinitionsAndAssertBoundSpan({
templateOverride: `<button com¦pound custom-button></button>`,
expectedSpanText: 'compound',
});
expect(definitions).toEqual([]);
expect(defs.length).toEqual(1);
expect(defs[0].contextSpan).toContain('export class CompoundCustomButtonDirective');
defs = getDefinitionsAndAssertBoundSpan({
templateOverride: `<button compound cu¦stom-button></button>`,
expectedSpanText: 'custom-button',
});
expect(defs.length).toEqual(1);
expect(defs[0].contextSpan).toContain('export class CompoundCustomButtonDirective');
});
});