From 49eec5d8726117f763b54aeea61a613b67cf0a49 Mon Sep 17 00:00:00 2001 From: Keen Yee Liau Date: Mon, 21 Oct 2019 18:49:32 -0700 Subject: [PATCH] fix(language-service): Add directive selectors & banana-in-a-box to completions (#33311) This commit refactors attribute completions and fixes two bugs: 1. selectors for directives are not provided 2. banana-in-a-box (two way binding) syntax are not provided PR closes https://github.com/angular/vscode-ng-language-service/issues/358 PR Close #33311 --- packages/language-service/src/completions.ts | 234 +++++++----------- packages/language-service/src/utils.ts | 22 +- .../language-service/test/completions_spec.ts | 29 ++- .../test/project/app/parsing-cases.ts | 8 +- 4 files changed, 130 insertions(+), 163 deletions(-) diff --git a/packages/language-service/src/completions.ts b/packages/language-service/src/completions.ts index 2cfa93921f..dd48b802ba 100644 --- a/packages/language-service/src/completions.ts +++ b/packages/language-service/src/completions.ts @@ -13,7 +13,7 @@ import {AstResult, AttrInfo} from './common'; import {getExpressionCompletions} from './expressions'; import {attributeNames, elementNames, eventNames, propertyNames} from './html_info'; import {CompletionKind, Span, Symbol, SymbolTable, TemplateSource} from './types'; -import {diagnosticInfoFromTemplateInfo, findTemplateAstAt, flatten, getSelectors, hasTemplateReference, inSpan, removeSuffix, spanOf} from './utils'; +import {diagnosticInfoFromTemplateInfo, findTemplateAstAt, getSelectors, hasTemplateReference, inSpan, spanOf} from './utils'; const TEMPLATE_ATTR_PREFIX = '*'; @@ -101,98 +101,46 @@ export function getTemplateCompletions( function attributeCompletions(info: AstResult, path: AstPath): ts.CompletionEntry[] { const item = path.tail instanceof Element ? path.tail : path.parentOf(path.tail); if (item instanceof Element) { - return attributeCompletionsForElement(info, item.name, item); + return attributeCompletionsForElement(info, item.name); } return []; } function attributeCompletionsForElement( - info: AstResult, elementName: string, element?: Element): ts.CompletionEntry[] { - const attributes = getAttributeInfosForElement(info, elementName, element); - - // Map all the attributes to a completion - return attributes.map(attr => { - const kind = attr.fromHtml ? CompletionKind.HTML_ATTRIBUTE : CompletionKind.ATTRIBUTE; - return { - name: nameOfAttr(attr), - // Need to cast to unknown because Angular's CompletionKind includes HTML - // entites. - kind: kind as unknown as ts.ScriptElementKind, - sortText: attr.name, - }; - }); -} - -function getAttributeInfosForElement( - info: AstResult, elementName: string, element?: Element): AttrInfo[] { - const attributes: AttrInfo[] = []; + info: AstResult, elementName: string): ts.CompletionEntry[] { + const results: ts.CompletionEntry[] = []; // Add html attributes - const htmlAttributes = attributeNames(elementName) || []; - if (htmlAttributes) { - attributes.push(...htmlAttributes.map(name => ({name, fromHtml: true}))); + for (const name of attributeNames(elementName)) { + results.push({ + name, + kind: CompletionKind.HTML_ATTRIBUTE as unknown as ts.ScriptElementKind, + sortText: name, + }); } // Add html properties - const htmlProperties = propertyNames(elementName); - if (htmlProperties) { - attributes.push(...htmlProperties.map(name => ({name, input: true}))); + for (const name of propertyNames(elementName)) { + results.push({ + name: `[${name}]`, + kind: CompletionKind.ATTRIBUTE as unknown as ts.ScriptElementKind, + sortText: name, + }); } // Add html events - const htmlEvents = eventNames(elementName); - if (htmlEvents) { - attributes.push(...htmlEvents.map(name => ({name, output: true}))); + for (const name of eventNames(elementName)) { + results.push({ + name: `(${name})`, + kind: CompletionKind.ATTRIBUTE as unknown as ts.ScriptElementKind, + sortText: name, + }); } - const {selectors, map: selectorMap} = getSelectors(info); - if (selectors && selectors.length) { - // All the attributes that are selectable should be shown. - const applicableSelectors = - selectors.filter(selector => !selector.element || selector.element === elementName); - const selectorAndAttributeNames = - applicableSelectors.map(selector => ({selector, attrs: selector.attrs.filter(a => !!a)})); - let attrs = flatten(selectorAndAttributeNames.map(selectorAndAttr => { - const directive = selectorMap.get(selectorAndAttr.selector) !; - const result = selectorAndAttr.attrs.map( - name => ({name, input: name in directive.inputs, output: name in directive.outputs})); - return result; - })); + // Add Angular attributes + results.push(...angularAttributes(info, elementName)); - // Add template attribute if a directive contains a template reference - selectorAndAttributeNames.forEach(selectorAndAttr => { - const selector = selectorAndAttr.selector; - const directive = selectorMap.get(selector); - if (directive && hasTemplateReference(directive.type) && selector.attrs.length && - selector.attrs[0]) { - attrs.push({name: selector.attrs[0], template: true}); - } - }); - - // All input and output properties of the matching directives should be added. - const elementSelector = element ? - createElementCssSelector(element) : - createElementCssSelector(new Element(elementName, [], [], null !, null, null)); - - const matcher = new SelectorMatcher(); - matcher.addSelectables(selectors); - matcher.match(elementSelector, selector => { - const directive = selectorMap.get(selector); - if (directive) { - const {inputs, outputs} = directive; - attrs.push(...Object.keys(inputs).map(name => ({name: inputs[name], input: true}))); - attrs.push(...Object.keys(outputs).map(name => ({name: outputs[name], output: true}))); - } - }); - - // If a name shows up twice, fold it into a single value. - attrs = foldAttrs(attrs); - - // Now expand them back out to ensure that input/output shows up as well as input and - // output. - attributes.push(...flatten(attrs.map(expandedAttr))); - } - return attributes; + return results; } function attributeValueCompletions( @@ -482,27 +430,6 @@ function getSourceText(template: TemplateSource, span: Span): string { return template.source.substring(span.start, span.end); } -function nameOfAttr(attr: AttrInfo): string { - let name = attr.name; - if (attr.output) { - name = removeSuffix(name, 'Events'); - name = removeSuffix(name, 'Changed'); - } - const result = [name]; - if (attr.input) { - result.unshift('['); - result.push(']'); - } - if (attr.output) { - result.unshift('('); - result.push(')'); - } - if (attr.template) { - result.unshift('*'); - } - return result.join(''); -} - const templateAttr = /^(\w+:)?(template$|^\*)/; function createElementCssSelector(element: Element): CssSelector { const cssSelector = new CssSelector(); @@ -523,48 +450,75 @@ function createElementCssSelector(element: Element): CssSelector { return cssSelector; } -function foldAttrs(attrs: AttrInfo[]): AttrInfo[] { - const inputOutput = new Map(); - const templates = new Map(); - const result: AttrInfo[] = []; - attrs.forEach(attr => { - if (attr.fromHtml) { - return attr; - } - if (attr.template) { - const duplicate = templates.get(attr.name); - if (!duplicate) { - result.push({name: attr.name, template: true}); - templates.set(attr.name, attr); - } - } - if (attr.input || attr.output) { - const duplicate = inputOutput.get(attr.name); - if (duplicate) { - duplicate.input = duplicate.input || attr.input; - duplicate.output = duplicate.output || attr.output; - } else { - const cloneAttr: AttrInfo = {name: attr.name}; - if (attr.input) cloneAttr.input = true; - if (attr.output) cloneAttr.output = true; - result.push(cloneAttr); - inputOutput.set(attr.name, cloneAttr); - } - } - }); - return result; -} - -function expandedAttr(attr: AttrInfo): AttrInfo[] { - if (attr.input && attr.output) { - return [ - attr, {name: attr.name, input: true, output: false}, - {name: attr.name, input: false, output: true} - ]; - } - return [attr]; -} - function lowerName(name: string): string { return name && (name[0].toLowerCase() + name.substr(1)); } + +function angularAttributes(info: AstResult, elementName: string): ts.CompletionEntry[] { + const {selectors, map: selectorMap} = getSelectors(info); + const templateRefs = new Set(); + const inputs = new Set(); + const outputs = new Set(); + const others = new Set(); + 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); + } + } + } + for (const input of Object.values(summary.inputs)) { + inputs.add(input); + } + for (const output of Object.values(summary.outputs)) { + outputs.add(output); + } + } + + const results: ts.CompletionEntry[] = []; + for (const name of templateRefs) { + results.push({ + name: `*${name}`, + kind: CompletionKind.ATTRIBUTE as unknown as ts.ScriptElementKind, + sortText: name, + }); + } + for (const name of inputs) { + results.push({ + name: `[${name}]`, + kind: CompletionKind.ATTRIBUTE as unknown as ts.ScriptElementKind, + 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: CompletionKind.ATTRIBUTE as unknown as ts.ScriptElementKind, + sortText: name, + }); + } + } + for (const name of outputs) { + results.push({ + name: `(${name})`, + kind: CompletionKind.ATTRIBUTE as unknown as ts.ScriptElementKind, + sortText: name, + }); + } + for (const name of others) { + results.push({ + name, + kind: CompletionKind.ATTRIBUTE as unknown as ts.ScriptElementKind, + sortText: name, + }); + } + return results; +} diff --git a/packages/language-service/src/utils.ts b/packages/language-service/src/utils.ts index e2ee41416b..faa674a6f0 100644 --- a/packages/language-service/src/utils.ts +++ b/packages/language-service/src/utils.ts @@ -69,21 +69,15 @@ export function hasTemplateReference(type: CompileTypeMetadata): boolean { export function getSelectors(info: AstResult): SelectorInfo { const map = new Map(); - const selectors: CssSelector[] = flatten(info.directives.map(directive => { + const results: CssSelector[] = []; + for (const directive of info.directives) { const selectors: CssSelector[] = CssSelector.parse(directive.selector !); - selectors.forEach(selector => map.set(selector, directive)); - return selectors; - })); - return {selectors, map}; -} - -export function flatten(a: T[][]) { - return ([]).concat(...a); -} - -export function removeSuffix(value: string, suffix: string) { - if (value.endsWith(suffix)) return value.substring(0, value.length - suffix.length); - return value; + for (const selector of selectors) { + results.push(selector); + map.set(selector, directive); + } + } + return {selectors: results, map}; } export function isTypescriptVersion(low: string, high?: string) { diff --git a/packages/language-service/test/completions_spec.ts b/packages/language-service/test/completions_spec.ts index e7311dd5a5..b5177bc132 100644 --- a/packages/language-service/test/completions_spec.ts +++ b/packages/language-service/test/completions_spec.ts @@ -40,7 +40,7 @@ describe('completions', () => { } }); - it('should be able to return element directives', () => { + it('should be able to return component directives', () => { const marker = mockHost.getLocationMarkerFor(APP_COMPONENT, 'empty'); const completions = ngLS.getCompletionsAt(APP_COMPONENT, marker.start); expectContain(completions, CompletionKind.COMPONENT, [ @@ -51,6 +51,12 @@ describe('completions', () => { ]); }); + it('should be able to return attribute directives', () => { + const marker = mockHost.getLocationMarkerFor(APP_COMPONENT, 'h1-after-space'); + const completions = ngLS.getCompletionsAt(APP_COMPONENT, marker.start); + expectContain(completions, CompletionKind.ATTRIBUTE, ['string-model', 'number-model']); + }); + it('should be able to return angular pseudo elements', () => { const marker = mockHost.getLocationMarkerFor(APP_COMPONENT, 'empty'); const completions = ngLS.getCompletionsAt(APP_COMPONENT, marker.start); @@ -279,7 +285,18 @@ describe('completions', () => { expectContain(completions, CompletionKind.METHOD, ['modelChanged']); }); - it('should be able to complete a two-way binding', () => { + 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)]', + ]); + }); + + it('should be able to complete a the RHS of a two-way binding', () => { const marker = mockHost.getLocationMarkerFor(PARSING_CASES, 'two-way-binding-model'); const completions = ngLS.getCompletionsAt(PARSING_CASES, marker.start); expectContain(completions, CompletionKind.PROPERTY, ['test']); @@ -288,7 +305,7 @@ describe('completions', () => { 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]', '(model)']); + expectContain(c1, CompletionKind.ATTRIBUTE, ['[model]', '(modelChange)', '[(model)]']); const m2 = mockHost.getLocationMarkerFor(PARSING_CASES, 'number-marker'); const c2 = ngLS.getCompletionsAt(PARSING_CASES, m2.start); @@ -347,7 +364,7 @@ describe('completions', () => { function expectContain( completions: ts.CompletionInfo | undefined, kind: CompletionKind, names: string[]) { expect(completions).toBeDefined(); - expect(completions !.entries).toEqual(jasmine.arrayContaining(names.map(name => { - return jasmine.objectContaining({name, kind}); - }) as any)); + for (const name of names) { + expect(completions !.entries).toContain(jasmine.objectContaining({ name, kind } as any)); + } } diff --git a/packages/language-service/test/project/app/parsing-cases.ts b/packages/language-service/test/project/app/parsing-cases.ts index 958aea6bcd..a67b216bf0 100644 --- a/packages/language-service/test/project/app/parsing-cases.ts +++ b/packages/language-service/test/project/app/parsing-cases.ts @@ -69,7 +69,9 @@ export class EventBinding { } @Component({ - template: '

', + template: ` +

+ `, }) export class TwoWayBinding { test: string = 'test'; @@ -80,7 +82,7 @@ export class TwoWayBinding { }) export class StringModel { @Input() model: string = 'model'; - @Output() modelChanged: EventEmitter = new EventEmitter(); + @Output() modelChange: EventEmitter = new EventEmitter(); } @Directive({ @@ -88,7 +90,7 @@ export class StringModel { }) export class NumberModel { @Input('inputAlias') model: number = 0; - @Output('outputAlias') modelChanged: EventEmitter = new EventEmitter(); + @Output('outputAlias') modelChange: EventEmitter = new EventEmitter(); } @Component({