fix(language-service): Proper completions for properties and events (#34445)

This commit fixes autocompletions for properties and events bindings.

The language service will no longer provide bindings like (click) or [id].
Instead, it'll infer the context based on the brackets and provide suggestions
without any brackets.

This fix also adds support for alternative binding syntax such as
`bind-`, `on-`, and `bindon`.

PR closes https://github.com/angular/vscode-ng-language-service/issues/398
PR closes https://github.com/angular/vscode-ng-language-service/issues/474

PR Close #34445
This commit is contained in:
Keen Yee Liau 2019-12-12 15:46:27 -08:00 committed by Kara Erickson
parent 9d1175e2b2
commit a04f7c0d5f
7 changed files with 241 additions and 159 deletions

View File

@ -45,6 +45,33 @@ const ANGULAR_ELEMENTS: ReadonlyArray<ng.CompletionEntry> = [
},
];
// This is adapted from packages/compiler/src/render3/r3_template_transform.ts
// to allow empty binding names.
const BIND_NAME_REGEXP =
/^(?:(?:(?:(bind-)|(let-)|(ref-|#)|(on-)|(bindon-)|(@))(.*))|\[\(([^\)]*)\)\]|\[([^\]]*)\]|\(([^\)]*)\))$/;
enum ATTR {
// Group 1 = "bind-"
KW_BIND_IDX = 1,
// Group 2 = "let-"
KW_LET_IDX = 2,
// Group 3 = "ref-/#"
KW_REF_IDX = 3,
// Group 4 = "on-"
KW_ON_IDX = 4,
// Group 5 = "bindon-"
KW_BINDON_IDX = 5,
// Group 6 = "@"
KW_AT_IDX = 6,
// Group 7 = the identifier after "bind-", "let-", "ref-/#", "on-", "bindon-" or "@"
IDENT_KW_IDX = 7,
// Group 8 = identifier inside [()]
IDENT_BANANA_BOX_IDX = 8,
// Group 9 = identifier inside []
IDENT_PROPERTY_IDX = 9,
// Group 10 = identifier inside ()
IDENT_EVENT_IDX = 10,
}
function isIdentifierPart(code: number) {
// Identifiers consist of alphanumeric characters, '_', or '$'.
return isAsciiLetter(code) || isDigit(code) || code == $$ || code == $_;
@ -139,7 +166,7 @@ export function getTemplateCompletions(
} else if (templatePosition < startTagSpan.end) {
// We are in the attribute section of the element (but not in an attribute).
// Return the attribute completions.
result = attributeCompletions(templateInfo, path);
result = attributeCompletionsForElement(templateInfo, ast.name);
}
},
visitAttribute(ast) {
@ -190,11 +217,52 @@ export function getTemplateCompletions(
}
function attributeCompletions(info: AstResult, path: AstPath<HtmlAst>): ng.CompletionEntry[] {
const item = path.tail instanceof Element ? path.tail : path.parentOf(path.tail);
if (item instanceof Element) {
return attributeCompletionsForElement(info, item.name);
const attr = path.tail;
const elem = path.parentOf(attr);
if (!(attr instanceof Attribute) || !(elem instanceof Element)) {
return [];
}
return [];
// TODO: Consider parsing the attrinute name to a proper AST instead of
// matching using regex. This is because the regexp would incorrectly identify
// bind parts for cases like [()|]
// ^ cursor is here
const bindParts = attr.name.match(BIND_NAME_REGEXP);
// TemplateRef starts with '*'. See https://angular.io/api/core/TemplateRef
const isTemplateRef = attr.name.startsWith('*');
const isBinding = bindParts !== null || isTemplateRef;
if (!isBinding) {
return attributeCompletionsForElement(info, elem.name);
}
const results: string[] = [];
const ngAttrs = angularAttributes(info, elem.name);
if (!bindParts) {
// If bindParts is null then this must be a TemplateRef.
results.push(...ngAttrs.templateRefs);
} else if (
bindParts[ATTR.KW_BIND_IDX] !== undefined ||
bindParts[ATTR.IDENT_PROPERTY_IDX] !== undefined) {
// property binding via bind- or []
results.push(...propertyNames(elem.name), ...ngAttrs.inputs);
} else if (
bindParts[ATTR.KW_ON_IDX] !== undefined || bindParts[ATTR.IDENT_EVENT_IDX] !== undefined) {
// event binding via on- or ()
results.push(...eventNames(elem.name), ...ngAttrs.outputs);
} else if (
bindParts[ATTR.KW_BINDON_IDX] !== undefined ||
bindParts[ATTR.IDENT_BANANA_BOX_IDX] !== undefined) {
// banana-in-a-box binding via bindon- or [()]
results.push(...ngAttrs.bananas);
}
return results.map(name => {
return {
name,
kind: ng.CompletionKind.ATTRIBUTE,
sortText: name,
};
});
}
function attributeCompletionsForElement(
@ -212,26 +280,15 @@ function attributeCompletionsForElement(
}
}
// Add html properties
for (const name of propertyNames(elementName)) {
results.push({
name: `[${name}]`,
kind: ng.CompletionKind.ATTRIBUTE,
sortText: name,
});
}
// Add html events
for (const name of eventNames(elementName)) {
results.push({
name: `(${name})`,
kind: ng.CompletionKind.ATTRIBUTE,
sortText: name,
});
}
// Add Angular attributes
results.push(...angularAttributes(info, elementName));
const ngAttrs = angularAttributes(info, elementName);
for (const name of ngAttrs.others) {
results.push({
name,
kind: ng.CompletionKind.ATTRIBUTE,
sortText: name,
});
}
return results;
}
@ -484,24 +541,54 @@ function getSourceText(template: ng.TemplateSource, span: ng.Span): string {
return template.source.substring(span.start, span.end);
}
function angularAttributes(info: AstResult, elementName: string): ng.CompletionEntry[] {
interface AngularAttributes {
/**
* Attributes that support the * syntax. See https://angular.io/api/core/TemplateRef
*/
templateRefs: Set<string>;
/**
* Attributes with the @Input annotation.
*/
inputs: Set<string>;
/**
* Attributes with the @Output annotation.
*/
outputs: Set<string>;
/**
* Attributes that support the [()] or bindon- syntax.
*/
bananas: Set<string>;
/**
* General attributes that match the specified element.
*/
others: Set<string>;
}
/**
* Return all Angular-specific attributes for the element with `elementName`.
* @param info
* @param elementName
*/
function angularAttributes(info: AstResult, elementName: string): AngularAttributes {
const {selectors, map: selectorMap} = getSelectors(info);
const templateRefs = new Set<string>();
const inputs = new Set<string>();
const outputs = new Set<string>();
const bananas = new Set<string>();
const others = new Set<string>();
for (const selector of selectors) {
if (selector.element && selector.element !== elementName) {
continue;
}
const summary = selectorMap.get(selector) !;
for (const attr of selector.attrs) {
if (attr) {
if (hasTemplateReference(summary.type)) {
templateRefs.add(attr);
} else {
others.add(attr);
}
const isTemplateRef = hasTemplateReference(summary.type);
// attributes are listed in (attribute, value) pairs
for (let i = 0; i < selector.attrs.length; i += 2) {
const attr = selector.attrs[i];
if (isTemplateRef) {
templateRefs.add(attr);
} else {
others.add(attr);
}
}
for (const input of Object.values(summary.inputs)) {
@ -511,44 +598,12 @@ function angularAttributes(info: AstResult, elementName: string): ng.CompletionE
outputs.add(output);
}
}
const results: ng.CompletionEntry[] = [];
for (const name of templateRefs) {
results.push({
name: `*${name}`,
kind: ng.CompletionKind.ATTRIBUTE,
sortText: name,
});
}
for (const name of inputs) {
results.push({
name: `[${name}]`,
kind: ng.CompletionKind.ATTRIBUTE,
sortText: name,
});
// Add banana-in-a-box syntax
// https://angular.io/guide/template-syntax#two-way-binding-
if (outputs.has(`${name}Change`)) {
results.push({
name: `[(${name})]`,
kind: ng.CompletionKind.ATTRIBUTE,
sortText: name,
});
bananas.add(name);
}
}
for (const name of outputs) {
results.push({
name: `(${name})`,
kind: ng.CompletionKind.ATTRIBUTE,
sortText: name,
});
}
for (const name of others) {
results.push({
name,
kind: ng.CompletionKind.ATTRIBUTE,
sortText: name,
});
}
return results;
return {templateRefs, inputs, outputs, bananas, others};
}

View File

@ -6,7 +6,7 @@
* found in the LICENSE file at https://angular.io/license
*/
import {AstPath, CompileDirectiveSummary, CompileTypeMetadata, CssSelector, DirectiveAst, ElementAst, EmbeddedTemplateAst, HtmlAstPath, Node, ParseSourceSpan, RecursiveTemplateAstVisitor, RecursiveVisitor, TemplateAst, TemplateAstPath, identifierName, templateVisitAll, visitAll} from '@angular/compiler';
import {AstPath, CompileDirectiveSummary, CompileTypeMetadata, CssSelector, DirectiveAst, ElementAst, EmbeddedTemplateAst, HtmlAstPath, Identifiers, Node, ParseSourceSpan, RecursiveTemplateAstVisitor, RecursiveVisitor, TemplateAst, TemplateAstPath, identifierName, templateVisitAll, visitAll} from '@angular/compiler';
import * as ts from 'typescript';
import {AstResult, SelectorInfo} from './common';
@ -57,11 +57,9 @@ export function isNarrower(spanA: Span, spanB: Span): boolean {
}
export function hasTemplateReference(type: CompileTypeMetadata): boolean {
if (type.diDeps) {
for (let diDep of type.diDeps) {
if (diDep.token && diDep.token.identifier &&
identifierName(diDep.token !.identifier !) === 'TemplateRef')
return true;
for (const diDep of type.diDeps) {
if (diDep.token && identifierName(diDep.token.identifier) === Identifiers.TemplateRef.name) {
return true;
}
}
return false;

View File

@ -80,14 +80,15 @@ describe('completions', () => {
]);
});
it('should be able to find common angular attributes', () => {
it('should be able to find common Angular attributes', () => {
const marker = mockHost.getLocationMarkerFor(APP_COMPONENT, 'div-attributes');
const completions = ngLS.getCompletionsAt(APP_COMPONENT, marker.start);
expectContain(completions, CompletionKind.ATTRIBUTE, [
'(click)',
'[ngClass]',
'*ngIf',
'*ngFor',
'ngClass',
'ngForm',
'ngModel',
'string-model',
'number-model',
]);
});
@ -117,46 +118,21 @@ describe('completions', () => {
expectContain(completions, CompletionKind.PROPERTY, ['id', 'name']);
});
describe('property completions for members of an indexed type', () => {
it('should work with numeric index signatures (arrays)', () => {
mockHost.override(TEST_TEMPLATE, `{{ heroes[0].~{heroes-number-index}}}`);
const marker = mockHost.getLocationMarkerFor(TEST_TEMPLATE, 'heroes-number-index');
const completions = ngLS.getCompletionsAt(TEST_TEMPLATE, marker.start);
expectContain(completions, CompletionKind.PROPERTY, ['id', 'name']);
});
it('should work with numeric index signatures (tuple arrays)', () => {
mockHost.override(TEST_TEMPLATE, `{{ tupleArray[1].~{tuple-array-number-index}}}`);
const marker = mockHost.getLocationMarkerFor(TEST_TEMPLATE, 'tuple-array-number-index');
const completions = ngLS.getCompletionsAt(TEST_TEMPLATE, marker.start);
expectContain(completions, CompletionKind.PROPERTY, ['id', 'name']);
});
describe('with string index signatures', () => {
it('should work with index notation', () => {
mockHost.override(TEST_TEMPLATE, `{{ heroesByName['Jacky'].~{heroes-string-index}}}`);
const marker = mockHost.getLocationMarkerFor(TEST_TEMPLATE, 'heroes-string-index');
const completions = ngLS.getCompletionsAt(TEST_TEMPLATE, marker.start);
expectContain(completions, CompletionKind.PROPERTY, ['id', 'name']);
});
it('should work with dot notation', () => {
mockHost.override(TEST_TEMPLATE, `{{ heroesByName.jacky.~{heroes-string-index}}}`);
const marker = mockHost.getLocationMarkerFor(TEST_TEMPLATE, 'heroes-string-index');
const completions = ngLS.getCompletionsAt(TEST_TEMPLATE, marker.start);
expectContain(completions, CompletionKind.PROPERTY, ['id', 'name']);
});
it('should work with dot notation if stringIndexType is a primitive type', () => {
mockHost.override(TEST_TEMPLATE, `{{ primitiveIndexType.test.~{string-primitive-type}}}`);
const marker = mockHost.getLocationMarkerFor(TEST_TEMPLATE, 'string-primitive-type');
const completions = ngLS.getCompletionsAt(TEST_TEMPLATE, marker.start);
expectContain(completions, CompletionKind.METHOD, ['substring']);
});
});
it('should suggest template refereces', () => {
mockHost.override(TEST_TEMPLATE, `<div *~{cursor}></div>`);
const marker = mockHost.getLocationMarkerFor(TEST_TEMPLATE, 'cursor');
const completions = ngLS.getCompletionsAt(TEST_TEMPLATE, marker.start);
expectContain(completions, CompletionKind.ATTRIBUTE, [
'ngFor',
'ngForOf',
'ngIf',
'ngSwitchCase',
'ngSwitchDefault',
'ngPluralCase',
]);
});
it('should be able to return attribute names with an incompete attribute', () => {
it('should be able to return attribute names with an incomplete attribute', () => {
const marker = mockHost.getLocationMarkerFor(PARSING_CASES, 'no-value-attribute');
const completions = ngLS.getCompletionsAt(PARSING_CASES, marker.start);
expectContain(completions, CompletionKind.HTML_ATTRIBUTE, ['id', 'class', 'dir', 'lang']);
@ -275,14 +251,16 @@ describe('completions', () => {
expect(entries).not.toContain(jasmine.objectContaining({name: 'onmouseup'}));
});
it('should be able to find common angular attributes', () => {
const marker = mockHost.getLocationMarkerFor(TEST_TEMPLATE, 'div-attributes');
it('should be able to find common Angular attributes', () => {
mockHost.override(TEST_TEMPLATE, `<div ~{cursor}></div>`);
const marker = mockHost.getLocationMarkerFor(TEST_TEMPLATE, 'cursor');
const completions = ngLS.getCompletionsAt(TEST_TEMPLATE, marker.start);
expectContain(completions, CompletionKind.ATTRIBUTE, [
'(click)',
'[ngClass]',
'*ngIf',
'*ngFor',
'ngClass',
'ngForm',
'ngModel',
'string-model',
'number-model',
]);
});
});
@ -366,14 +344,10 @@ describe('completions', () => {
});
it('should be able to complete a the LHS of a two-way binding', () => {
const marker = mockHost.getLocationMarkerFor(PARSING_CASES, 'two-way-binding-input');
const completions = ngLS.getCompletionsAt(PARSING_CASES, marker.start);
expectContain(completions, CompletionKind.ATTRIBUTE, [
'ngModel',
'[ngModel]',
'(ngModelChange)',
'[(ngModel)]',
]);
mockHost.override(TEST_TEMPLATE, `<div [(~{cursor})]></div>`);
const marker = mockHost.getLocationMarkerFor(TEST_TEMPLATE, 'cursor');
const completions = ngLS.getCompletionsAt(TEST_TEMPLATE, marker.start);
expectContain(completions, CompletionKind.ATTRIBUTE, ['ngModel']);
});
it('should be able to complete a the RHS of a two-way binding', () => {
@ -382,14 +356,46 @@ describe('completions', () => {
expectContain(completions, CompletionKind.PROPERTY, ['test']);
});
it('should work with input and output', () => {
const m1 = mockHost.getLocationMarkerFor(PARSING_CASES, 'string-marker');
const c1 = ngLS.getCompletionsAt(PARSING_CASES, m1.start);
expectContain(c1, CompletionKind.ATTRIBUTE, ['[model]', '(modelChange)', '[(model)]']);
it('should suggest property binding for input', () => {
// Property binding via []
mockHost.override(TEST_TEMPLATE, `<div number-model [~{cursor}]></div>`);
const m1 = mockHost.getLocationMarkerFor(TEST_TEMPLATE, 'cursor');
const c1 = ngLS.getCompletionsAt(TEST_TEMPLATE, m1.start);
expectContain(c1, CompletionKind.ATTRIBUTE, ['inputAlias']);
const m2 = mockHost.getLocationMarkerFor(PARSING_CASES, 'number-marker');
const c2 = ngLS.getCompletionsAt(PARSING_CASES, m2.start);
expectContain(c2, CompletionKind.ATTRIBUTE, ['[inputAlias]', '(outputAlias)']);
// Property binding via bind-
mockHost.override(TEST_TEMPLATE, `<div number-model bind-~{cursor}></div>`);
const m2 = mockHost.getLocationMarkerFor(TEST_TEMPLATE, 'cursor');
const c2 = ngLS.getCompletionsAt(TEST_TEMPLATE, m2.start);
expectContain(c2, CompletionKind.ATTRIBUTE, ['inputAlias']);
});
it('should suggest event binding for output', () => {
// Event binding via ()
mockHost.override(TEST_TEMPLATE, `<div number-model (~{cursor})></div>`);
const m1 = mockHost.getLocationMarkerFor(TEST_TEMPLATE, 'cursor');
const c1 = ngLS.getCompletionsAt(TEST_TEMPLATE, m1.start);
expectContain(c1, CompletionKind.ATTRIBUTE, ['outputAlias']);
// Event binding via on-
mockHost.override(TEST_TEMPLATE, `<div number-mode on-~{cursor}></div>`);
const m2 = mockHost.getLocationMarkerFor(TEST_TEMPLATE, 'cursor');
const c2 = ngLS.getCompletionsAt(TEST_TEMPLATE, m2.start);
expectContain(c2, CompletionKind.ATTRIBUTE, ['outputAlias']);
});
it('should suggest two-way binding for input and output', () => {
// Banana-in-a-box via [()]
mockHost.override(TEST_TEMPLATE, `<div string-model [(~{cursor})]></div>`);
const m1 = mockHost.getLocationMarkerFor(TEST_TEMPLATE, 'cursor');
const c1 = ngLS.getCompletionsAt(TEST_TEMPLATE, m1.start);
expectContain(c1, CompletionKind.ATTRIBUTE, ['model']);
// Banana-in-a-box via bindon-
mockHost.override(TEST_TEMPLATE, `<div string-model bindon-~{cursor}></div>`);
const m2 = mockHost.getLocationMarkerFor(TEST_TEMPLATE, 'cursor');
const c2 = ngLS.getCompletionsAt(TEST_TEMPLATE, m2.start);
expectContain(c2, CompletionKind.ATTRIBUTE, ['model']);
});
});
@ -543,7 +549,7 @@ describe('completions', () => {
@Component({
selector: 'foo-component',
template: \`
<div cl~{click}></div>
<div (cl~{click})></div>
\`,
})
export class FooComponent {}
@ -551,9 +557,9 @@ describe('completions', () => {
const location = mockHost.getLocationMarkerFor(fileName, 'click');
const completions = ngLS.getCompletionsAt(fileName, location.start) !;
expect(completions).toBeDefined();
const completion = completions.entries.find(entry => entry.name === '(click)') !;
const completion = completions.entries.find(entry => entry.name === 'click') !;
expect(completion).toBeDefined();
expect(completion.kind).toBe('attribute');
expect(completion.kind).toBe(CompletionKind.ATTRIBUTE);
expect(completion.replacementSpan).toEqual({start: location.start - 2, length: 2});
});
@ -602,7 +608,7 @@ describe('completions', () => {
@Component({
selector: 'foo-component',
template: \`
<input ngMod~{model} />
<input [(ngMod~{model})] />
\`,
})
export class FooComponent {}
@ -610,12 +616,51 @@ describe('completions', () => {
const location = mockHost.getLocationMarkerFor(fileName, 'model');
const completions = ngLS.getCompletionsAt(fileName, location.start) !;
expect(completions).toBeDefined();
const completion = completions.entries.find(entry => entry.name === '[(ngModel)]') !;
const completion = completions.entries.find(entry => entry.name === 'ngModel') !;
expect(completion).toBeDefined();
expect(completion.kind).toBe('attribute');
expect(completion.kind).toBe(CompletionKind.ATTRIBUTE);
expect(completion.replacementSpan).toEqual({start: location.start - 5, length: 5});
});
});
describe('property completions for members of an indexed type', () => {
it('should work with numeric index signatures (arrays)', () => {
mockHost.override(TEST_TEMPLATE, `{{ heroes[0].~{heroes-number-index}}}`);
const marker = mockHost.getLocationMarkerFor(TEST_TEMPLATE, 'heroes-number-index');
const completions = ngLS.getCompletionsAt(TEST_TEMPLATE, marker.start);
expectContain(completions, CompletionKind.PROPERTY, ['id', 'name']);
});
it('should work with numeric index signatures (tuple arrays)', () => {
mockHost.override(TEST_TEMPLATE, `{{ tupleArray[1].~{tuple-array-number-index}}}`);
const marker = mockHost.getLocationMarkerFor(TEST_TEMPLATE, 'tuple-array-number-index');
const completions = ngLS.getCompletionsAt(TEST_TEMPLATE, marker.start);
expectContain(completions, CompletionKind.PROPERTY, ['id', 'name']);
});
describe('with string index signatures', () => {
it('should work with index notation', () => {
mockHost.override(TEST_TEMPLATE, `{{ heroesByName['Jacky'].~{heroes-string-index}}}`);
const marker = mockHost.getLocationMarkerFor(TEST_TEMPLATE, 'heroes-string-index');
const completions = ngLS.getCompletionsAt(TEST_TEMPLATE, marker.start);
expectContain(completions, CompletionKind.PROPERTY, ['id', 'name']);
});
it('should work with dot notation', () => {
mockHost.override(TEST_TEMPLATE, `{{ heroesByName.jacky.~{heroes-string-index}}}`);
const marker = mockHost.getLocationMarkerFor(TEST_TEMPLATE, 'heroes-string-index');
const completions = ngLS.getCompletionsAt(TEST_TEMPLATE, marker.start);
expectContain(completions, CompletionKind.PROPERTY, ['id', 'name']);
});
it('should work with dot notation if stringIndexType is a primitive type', () => {
mockHost.override(TEST_TEMPLATE, `{{ primitiveIndexType.test.~{string-primitive-type}}}`);
const marker = mockHost.getLocationMarkerFor(TEST_TEMPLATE, 'string-primitive-type');
const completions = ngLS.getCompletionsAt(TEST_TEMPLATE, marker.start);
expectContain(completions, CompletionKind.METHOD, ['substring']);
});
});
});
});
function expectContain(

View File

@ -36,7 +36,6 @@ import * as ParsingCases from './parsing-cases';
ParsingCases.CaseUnknown,
ParsingCases.EmptyInterpolation,
ParsingCases.EventBinding,
ParsingCases.FooComponent,
ParsingCases.ForLetIEqual,
ParsingCases.ForOfLetEmpty,
ParsingCases.ForUsingComponent,

View File

@ -93,18 +93,6 @@ export class NumberModel {
@Output('outputAlias') modelChange: EventEmitter<number> = new EventEmitter();
}
@Component({
selector: 'foo-component',
template: `
<div string-model ~{string-marker}="text"></div>
<div number-model ~{number-marker}="value"></div>
`,
})
export class FooComponent {
text: string = 'some text';
value: number = 42;
}
interface Person {
name: string;
age: number;

View File

@ -4,7 +4,4 @@
</h1>
~{after-h1}<h2>{{~{h2-hero}hero.~{h2-name}name}} details!</h2>
<div><label>id: </label>{{~{label-hero}hero.~{label-id}id}}</div>
<div ~{div-attributes}>
<label>name: </label>
</div>
&~{entity-amp}amp;

View File

@ -94,7 +94,7 @@ describe('TypeScriptServiceHost', () => {
const tsLS = ts.createLanguageService(tsLSHost);
const ngLSHost = new TypeScriptServiceHost(tsLSHost, tsLS);
const templates = ngLSHost.getTemplates('/app/parsing-cases.ts');
expect(templates.length).toBe(17);
expect(templates.length).toBe(16);
});
it('should be able to find external template', () => {