From 2dffe65cfdc1329819a5ef0d0f2069efb0b07ff8 Mon Sep 17 00:00:00 2001 From: Keen Yee Liau Date: Wed, 18 Dec 2019 11:08:31 -0800 Subject: [PATCH] fix(language-service): completions after "let x of |" in ngFor (#34473) This commit fixes a bug in which we do testing for completions. Subsequently, this exposes another bug in our implementation whereby suggestions are not provided in "ngFor" where there should have been. Currently, multiple test cases are grouped together in a single template. This requires the template to be somewhat complete so that test cases that depend on variables declared earlier would pass. Consider the following example: ``` template: `
Name: {{~{for-interp-person}person.~{for-interp-name}name}} Age: {{person.~{for-interp-age}age}}
`, ``` In order to test `~{for-interp-person}`, `people` has to be included after `~{for-people}`. This means the test case for `~{for-people}` is not reflective of the actual use case because the variable is already there! In real case, the expression would be incomplete, and our implementation failed to take that into account. This commit breaks such test into individual tests, and fix the bugs in the underlying implementation. PR Close #34473 --- packages/language-service/src/completions.ts | 15 +++++- .../language-service/test/completions_spec.ts | 53 +++++++++++++------ .../language-service/test/project/app/main.ts | 3 -- .../test/project/app/parsing-cases.ts | 23 -------- .../test/typescript_host_spec.ts | 2 +- 5 files changed, 52 insertions(+), 44 deletions(-) diff --git a/packages/language-service/src/completions.ts b/packages/language-service/src/completions.ts index 622083edb1..79ea098da0 100644 --- a/packages/language-service/src/completions.ts +++ b/packages/language-service/src/completions.ts @@ -511,7 +511,7 @@ class ExpressionVisitor extends NullTemplateVisitor { if (binding.keyIsVar) { const equalLocation = attr.value.indexOf('='); - if (equalLocation >= 0 && valueRelativePosition >= equalLocation) { + if (equalLocation > 0 && valueRelativePosition > equalLocation) { // We are after the '=' in a let clause. The valid values here are the members of the // template reference's type parameter. const directiveMetadata = selectorInfo.map.get(selector); @@ -531,6 +531,19 @@ class ExpressionVisitor extends NullTemplateVisitor { this.addAttributeValuesToCompletions(binding.expression.ast); return; } + + // If the expression is incomplete, for example *ngFor="let x of |" + // binding.expression is null. We could still try to provide suggestions + // by looking for symbols that are in scope. + const KW_OF = ' of '; + const ofLocation = attr.value.indexOf(KW_OF); + if (ofLocation > 0 && valueRelativePosition >= ofLocation + KW_OF.length) { + const span = new ParseSpan(0, attr.value.length); + const offset = attr.sourceSpan.start.offset; + const receiver = new ImplicitReceiver(span, span.toAbsolute(offset)); + const expressionAst = new PropertyRead(span, span.toAbsolute(offset), receiver, ''); + this.addAttributeValuesToCompletions(expressionAst); + } } } diff --git a/packages/language-service/test/completions_spec.ts b/packages/language-service/test/completions_spec.ts index 3bce45f1b2..e94977dd9f 100644 --- a/packages/language-service/test/completions_spec.ts +++ b/packages/language-service/test/completions_spec.ts @@ -118,7 +118,7 @@ describe('completions', () => { expectContain(completions, CompletionKind.PROPERTY, ['id', 'name']); }); - it('should suggest template refereces', () => { + it('should suggest template references', () => { mockHost.override(TEST_TEMPLATE, `
`); const marker = mockHost.getLocationMarkerFor(TEST_TEMPLATE, 'cursor'); const completions = ngLS.getCompletionsAt(TEST_TEMPLATE, marker.start); @@ -275,8 +275,9 @@ describe('completions', () => { describe('with a *ngFor', () => { 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); + mockHost.override(TEST_TEMPLATE, `
`); + const marker = mockHost.getLocationMarkerFor(TEST_TEMPLATE, 'cursor'); + const completions = ngLS.getCompletionsAt(TEST_TEMPLATE, marker.start); expectContain(completions, CompletionKind.PROPERTY, [ '$implicit', 'ngForOf', @@ -289,24 +290,44 @@ describe('completions', () => { ]); }); - it('should include field reference', () => { - const marker = mockHost.getLocationMarkerFor(PARSING_CASES, 'for-people'); - const completions = ngLS.getCompletionsAt(PARSING_CASES, marker.start); - expectContain(completions, CompletionKind.PROPERTY, ['people']); + it('should not provide suggestion before the = sign', () => { + mockHost.override(TEST_TEMPLATE, `
`); + const marker = mockHost.getLocationMarkerFor(TEST_TEMPLATE, 'cursor'); + const completions = ngLS.getCompletionsAt(TEST_TEMPLATE, marker.start); + expect(completions).toBeUndefined(); }); - it('should include person in the let scope', () => { - const marker = mockHost.getLocationMarkerFor(PARSING_CASES, 'for-interp-person'); - const completions = ngLS.getCompletionsAt(PARSING_CASES, marker.start); - expectContain(completions, CompletionKind.VARIABLE, ['person']); + it('should include field reference', () => { + mockHost.override(TEST_TEMPLATE, `
`); + const marker = mockHost.getLocationMarkerFor(TEST_TEMPLATE, 'cursor'); + const completions = ngLS.getCompletionsAt(TEST_TEMPLATE, marker.start); + expectContain(completions, CompletionKind.PROPERTY, ['title', 'heroes', 'league']); + // the symbol 'x' declared in *ngFor is also in scope. This asserts that + // we are actually taking the AST into account and not just referring to + // the symbol table of the Component. + expectContain(completions, CompletionKind.VARIABLE, ['x']); + }); + + it('should include variable in the let scope in interpolation', () => { + mockHost.override(TEST_TEMPLATE, ` +
+ {{~{cursor}}} +
+ `); + const marker = mockHost.getLocationMarkerFor(TEST_TEMPLATE, 'cursor'); + const completions = ngLS.getCompletionsAt(TEST_TEMPLATE, marker.start); + expectContain(completions, CompletionKind.VARIABLE, ['h']); }); it('should be able to infer the type of a ngForOf', () => { - for (const location of ['for-interp-name', 'for-interp-age']) { - const marker = mockHost.getLocationMarkerFor(PARSING_CASES, location); - const completions = ngLS.getCompletionsAt(PARSING_CASES, marker.start); - expectContain(completions, CompletionKind.PROPERTY, ['name', 'age', 'street']); - } + mockHost.override(TEST_TEMPLATE, ` +
+ {{ h.~{cursor} }} +
+ `); + const marker = mockHost.getLocationMarkerFor(TEST_TEMPLATE, 'cursor'); + const completions = ngLS.getCompletionsAt(TEST_TEMPLATE, marker.start); + expectContain(completions, CompletionKind.PROPERTY, ['id', 'name']); }); it('should be able to infer the type of a ngForOf with an async pipe', () => { diff --git a/packages/language-service/test/project/app/main.ts b/packages/language-service/test/project/app/main.ts index 6ef6479ace..701780c7c4 100644 --- a/packages/language-service/test/project/app/main.ts +++ b/packages/language-service/test/project/app/main.ts @@ -36,9 +36,6 @@ import * as ParsingCases from './parsing-cases'; ParsingCases.CaseUnknown, ParsingCases.EmptyInterpolation, ParsingCases.EventBinding, - ParsingCases.ForLetIEqual, - ParsingCases.ForOfLetEmpty, - ParsingCases.ForUsingComponent, ParsingCases.NoValueAttribute, ParsingCases.NumberModel, ParsingCases.Pipes, diff --git a/packages/language-service/test/project/app/parsing-cases.ts b/packages/language-service/test/project/app/parsing-cases.ts index 537574f70f..7b2b3fe36c 100644 --- a/packages/language-service/test/project/app/parsing-cases.ts +++ b/packages/language-service/test/project/app/parsing-cases.ts @@ -99,29 +99,6 @@ interface Person { street: string; } -@Component({ - template: '
', -}) -export class ForOfLetEmpty { -} - -@Component({ - template: '
', -}) -export class ForLetIEqual { -} - -@Component({ - template: ` -
- Name: {{~{for-interp-person}person.~{for-interp-name}name}} - Age: {{person.~{for-interp-age}age}} -
`, -}) -export class ForUsingComponent { - people: Person[] = []; -} - @Component({ template: `
diff --git a/packages/language-service/test/typescript_host_spec.ts b/packages/language-service/test/typescript_host_spec.ts index f9527c2d8e..816bb81f34 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(16); + expect(templates.length).toBe(13); }); it('should be able to find external template', () => {