From 5eaab85fc064e69a94d104796123ea38619587ef Mon Sep 17 00:00:00 2001 From: Keen Yee Liau Date: Mon, 16 Dec 2019 11:03:39 -0800 Subject: [PATCH] fix(language-service): Remove completions for let and of in ngFor (#34434) `let` and `of` should be considered reserved keywords in template syntax and thus should not be part of the autocomplete suggestions. For reference, TypeScript does not provide such completions. This commit removes these results and cleans up the code. PR Close #34434 --- packages/language-service/src/completions.ts | 36 ++----------------- packages/language-service/src/utils.ts | 5 ++- .../language-service/test/completions_spec.ts | 18 ---------- .../language-service/test/project/app/main.ts | 1 - .../test/project/app/parsing-cases.ts | 8 +---- .../test/typescript_host_spec.ts | 2 +- 6 files changed, 6 insertions(+), 64 deletions(-) diff --git a/packages/language-service/src/completions.ts b/packages/language-service/src/completions.ts index d1944bb3d28..19fe6c7a495 100644 --- a/packages/language-service/src/completions.ts +++ b/packages/language-service/src/completions.ts @@ -6,7 +6,7 @@ * found in the LICENSE file at https://angular.io/license */ -import {AST, AstPath, Attribute, BoundDirectivePropertyAst, BoundElementPropertyAst, BoundEventAst, BoundTextAst, CssSelector, Element, ElementAst, ImplicitReceiver, NAMED_ENTITIES, Node as HtmlAst, NullTemplateVisitor, ParseSpan, PropertyRead, TagContentType, TemplateBinding, Text, findNode, getHtmlTagDefinition} from '@angular/compiler'; +import {AST, AstPath, Attribute, BoundDirectivePropertyAst, BoundElementPropertyAst, BoundEventAst, BoundTextAst, Element, ElementAst, ImplicitReceiver, NAMED_ENTITIES, Node as HtmlAst, NullTemplateVisitor, ParseSpan, PropertyRead, TagContentType, TemplateBinding, Text, findNode, getHtmlTagDefinition} from '@angular/compiler'; import {$$, $_, isAsciiLetter, isDigit} from '@angular/compiler/src/chars'; import {AstResult} from './common'; @@ -246,19 +246,7 @@ function attributeValueCompletions( const visitor = new ExpressionVisitor(info, position, () => getExpressionScope(dinfo, path, false), attr); path.tail.visit(visitor, null); - const {results} = visitor; - if (results.length) { - return results; - } - // Try allowing widening the path - const widerPath = findTemplateAstAt(info.templateAst, position, /* allowWidening */ true); - if (widerPath.tail) { - const widerVisitor = new ExpressionVisitor( - info, position, () => getExpressionScope(dinfo, widerPath, false), attr); - widerPath.tail.visit(widerVisitor, null); - return widerVisitor.results; - } - return results; + return visitor.results; } function elementCompletions(info: AstResult): ng.CompletionEntry[] { @@ -415,24 +403,6 @@ class ExpressionVisitor extends NullTemplateVisitor { } } - private addKeysToCompletions(selector: CssSelector, key: string) { - if (key !== 'ngFor') { - return; - } - this.completions.set('let', { - name: 'let', - kind: ng.CompletionKind.KEY, - sortText: 'let', - }); - if (selector.attrs.some(attr => attr === 'ngForOf')) { - this.completions.set('of', { - name: 'of', - kind: ng.CompletionKind.KEY, - sortText: 'of', - }); - } - } - private addSymbolsToCompletions(symbols: ng.Symbol[]) { for (const s of symbols) { if (s.name.startsWith('__') || !s.public || this.completions.has(s.name)) { @@ -507,8 +477,6 @@ class ExpressionVisitor extends NullTemplateVisitor { this.addAttributeValuesToCompletions(binding.expression.ast, this.position); return; } - - this.addKeysToCompletions(selector, key); } } diff --git a/packages/language-service/src/utils.ts b/packages/language-service/src/utils.ts index 9863d09ef91..65a187c7deb 100644 --- a/packages/language-service/src/utils.ts +++ b/packages/language-service/src/utils.ts @@ -101,15 +101,14 @@ export function diagnosticInfoFromTemplateInfo(info: AstResult): DiagnosticTempl }; } -export function findTemplateAstAt( - ast: TemplateAst[], position: number, allowWidening: boolean = false): TemplateAstPath { +export function findTemplateAstAt(ast: TemplateAst[], position: number): TemplateAstPath { const path: TemplateAst[] = []; const visitor = new class extends RecursiveTemplateAstVisitor { visit(ast: TemplateAst, context: any): any { let span = spanOf(ast); if (inSpan(position, span)) { const len = path.length; - if (!len || allowWidening || isNarrower(span, spanOf(path[len - 1]))) { + if (!len || isNarrower(span, spanOf(path[len - 1]))) { path.push(ast); } } else { diff --git a/packages/language-service/test/completions_spec.ts b/packages/language-service/test/completions_spec.ts index 4bb1b0fda85..54541c662d1 100644 --- a/packages/language-service/test/completions_spec.ts +++ b/packages/language-service/test/completions_spec.ts @@ -296,12 +296,6 @@ describe('completions', () => { }); describe('with a *ngFor', () => { - it('should include a let for empty attribute', () => { - const marker = mockHost.getLocationMarkerFor(PARSING_CASES, 'for-empty'); - const completions = ngLS.getCompletionsAt(PARSING_CASES, marker.start); - expectContain(completions, CompletionKind.KEY, ['let', 'of']); - }); - it('should suggest NgForRow members for let initialization expression', () => { const marker = mockHost.getLocationMarkerFor(PARSING_CASES, 'for-let-i-equal'); const completions = ngLS.getCompletionsAt(PARSING_CASES, marker.start); @@ -317,18 +311,6 @@ describe('completions', () => { ]); }); - it('should include a let', () => { - const marker = mockHost.getLocationMarkerFor(PARSING_CASES, 'for-let'); - const completions = ngLS.getCompletionsAt(PARSING_CASES, marker.start); - expectContain(completions, CompletionKind.KEY, ['let', 'of']); - }); - - it('should include an "of"', () => { - const marker = mockHost.getLocationMarkerFor(PARSING_CASES, 'for-of'); - const completions = ngLS.getCompletionsAt(PARSING_CASES, marker.start); - expectContain(completions, CompletionKind.KEY, ['let', 'of']); - }); - it('should include field reference', () => { const marker = mockHost.getLocationMarkerFor(PARSING_CASES, 'for-people'); const completions = ngLS.getCompletionsAt(PARSING_CASES, marker.start); diff --git a/packages/language-service/test/project/app/main.ts b/packages/language-service/test/project/app/main.ts index 344c23c8195..7df8dc7aa46 100644 --- a/packages/language-service/test/project/app/main.ts +++ b/packages/language-service/test/project/app/main.ts @@ -38,7 +38,6 @@ import * as ParsingCases from './parsing-cases'; ParsingCases.EventBinding, ParsingCases.FooComponent, ParsingCases.ForLetIEqual, - ParsingCases.ForOfEmpty, ParsingCases.ForOfLetEmpty, ParsingCases.ForUsingComponent, ParsingCases.NoValueAttribute, diff --git a/packages/language-service/test/project/app/parsing-cases.ts b/packages/language-service/test/project/app/parsing-cases.ts index aef10496f1d..c21400b82d8 100644 --- a/packages/language-service/test/project/app/parsing-cases.ts +++ b/packages/language-service/test/project/app/parsing-cases.ts @@ -111,12 +111,6 @@ interface Person { street: string; } -@Component({ - template: '
', -}) -export class ForOfEmpty { -} - @Component({ template: '
', }) @@ -131,7 +125,7 @@ export class ForLetIEqual { @Component({ template: ` -
+
Name: {{~{for-interp-person}person.~{for-interp-name}name}} Age: {{person.~{for-interp-age}age}}
`, diff --git a/packages/language-service/test/typescript_host_spec.ts b/packages/language-service/test/typescript_host_spec.ts index b7a0a53d168..ef64b6e94d6 100644 --- a/packages/language-service/test/typescript_host_spec.ts +++ b/packages/language-service/test/typescript_host_spec.ts @@ -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(18); + expect(templates.length).toBe(17); }); it('should be able to find external template', () => {