From 4ba478267ec4081ebeab1760968b2d58156948b8 Mon Sep 17 00:00:00 2001 From: ayazhafiz Date: Sun, 12 Jan 2020 14:15:50 -0800 Subject: [PATCH] feat(language-service): specific suggestions for template context diags (#34751) This commit elaborates diagnostics produced for invalid template contexts by including the name of the embedded template type using the template context, and in the common case that the implicity property is being referenced (e.g. in a `for .. of ..` expression), suggesting to refine the type of the context. This suggestion is provided because users will sometimes use a base class as the type of the context in the embedded view, and a more specific context later on (e.g. in an `ngOnChanges` method). Closes https://github.com/angular/vscode-ng-language-service/issues/251 PR Close #34751 --- .../src/expression_diagnostics.ts | 6 ++- .../language-service/test/diagnostics_spec.ts | 41 ++++++++++++++----- .../language-service/test/project/app/main.ts | 1 + .../test/project/app/parsing-cases.ts | 20 ++++++++- 4 files changed, 55 insertions(+), 13 deletions(-) diff --git a/packages/language-service/src/expression_diagnostics.ts b/packages/language-service/src/expression_diagnostics.ts index 124f63e77a..84a6ff480f 100644 --- a/packages/language-service/src/expression_diagnostics.ts +++ b/packages/language-service/src/expression_diagnostics.ts @@ -269,10 +269,12 @@ class ExpressionDiagnosticsVisitor extends RecursiveTemplateAstVisitor { if (context && !context.has(ast.value)) { if (ast.value === '$implicit') { this.reportError( - 'The template context does not have an implicit value', spanOf(ast.sourceSpan)); + `The template context of '${directive.type.reference.name}' does not define an implicit value.\n` + + `If the context type is a base type, consider refining it to a more specific type.`, + spanOf(ast.sourceSpan)); } else { this.reportError( - `The template context does not define a member called '${ast.value}'`, + `The template context of '${directive.type.reference.name}' does not define a member called '${ast.value}'`, spanOf(ast.sourceSpan)); } } diff --git a/packages/language-service/test/diagnostics_spec.ts b/packages/language-service/test/diagnostics_spec.ts index 4acdd08551..7ae5bf4e5e 100644 --- a/packages/language-service/test/diagnostics_spec.ts +++ b/packages/language-service/test/diagnostics_spec.ts @@ -25,7 +25,6 @@ import {MockTypescriptHost} from './test_utils'; const EXPRESSION_CASES = '/app/expression-cases.ts'; const NG_FOR_CASES = '/app/ng-for-cases.ts'; -const NG_IF_CASES = '/app/ng-if-cases.ts'; const TEST_TEMPLATE = '/app/test.ng'; const APP_COMPONENT = '/app/app.component.ts'; @@ -242,11 +241,6 @@ describe('diagnostics', () => { `and element references do not contain such a member`); }); - it('should report an unknown context reference', () => { - const diags = ngLS.getSemanticDiagnostics(NG_FOR_CASES).map(d => d.messageText); - expect(diags).toContain(`The template context does not define a member called 'even_1'`); - }); - it('should report an unknown value in a key expression', () => { const diags = ngLS.getSemanticDiagnostics(NG_FOR_CASES).map(d => d.messageText); expect(diags).toContain( @@ -256,10 +250,37 @@ describe('diagnostics', () => { }); }); - describe('in ng-if-cases.ts', () => { - it('should report an implicit context reference', () => { - const diags = ngLS.getSemanticDiagnostics(NG_IF_CASES).map(d => d.messageText); - expect(diags).toContain(`The template context does not define a member called 'unknown'`); + describe('embedded templates', () => { + it('should suggest refining a template context missing a property', () => { + mockHost.override( + TEST_TEMPLATE, + ``); + const diags = ngLS.getSemanticDiagnostics(TEST_TEMPLATE); + expect(diags.length).toBe(1); + const {messageText, start, length} = diags[0]; + expect(messageText) + .toBe( + `The template context of 'CounterDirective' does not define an implicit value.\n` + + `If the context type is a base type, consider refining it to a more specific type.`, ); + + const span = mockHost.getLocationMarkerFor(TEST_TEMPLATE, 'emb'); + expect(start).toBe(span.start); + expect(length).toBe(span.length); + }); + + it('should report an unknown context reference', () => { + mockHost.override( + TEST_TEMPLATE, + `
`); + const diags = ngLS.getSemanticDiagnostics(TEST_TEMPLATE); + expect(diags.length).toBe(1); + const {messageText, start, length} = diags[0]; + expect(messageText) + .toBe(`The template context of 'NgForOf' does not define a member called 'even_1'`); + + const span = mockHost.getLocationMarkerFor(TEST_TEMPLATE, 'emb'); + expect(start).toBe(span.start); + expect(length).toBe(span.length); }); }); diff --git a/packages/language-service/test/project/app/main.ts b/packages/language-service/test/project/app/main.ts index 31dab5c379..6de049e260 100644 --- a/packages/language-service/test/project/app/main.ts +++ b/packages/language-service/test/project/app/main.ts @@ -33,6 +33,7 @@ import * as ParsingCases from './parsing-cases'; ParsingCases.CaseIncompleteOpen, ParsingCases.CaseMissingClosing, ParsingCases.CaseUnknown, + ParsingCases.CounterDirective, ParsingCases.EmptyInterpolation, ParsingCases.HintModel, 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 77e6753add..90c90a9eaf 100644 --- a/packages/language-service/test/project/app/parsing-cases.ts +++ b/packages/language-service/test/project/app/parsing-cases.ts @@ -6,7 +6,7 @@ * found in the LICENSE file at https://angular.io/license */ -import {Component, Directive, EventEmitter, Input, Output} from '@angular/core'; +import {Component, Directive, EventEmitter, Input, OnChanges, Output, SimpleChanges, TemplateRef, ViewContainerRef} from '@angular/core'; import {Hero} from './app.component'; @@ -109,6 +109,24 @@ export class AsyncForUsingComponent { export class References { } +class CounterDirectiveContext { + constructor(public $implicit: T) {} +} + +@Directive({selector: '[counterOf]'}) +export class CounterDirective implements OnChanges { + // Object does not have an "$implicit" property. + constructor(private container: ViewContainerRef, private template: TemplateRef) {} + + @Input('counterOf') counter: number = 0; + ngOnChanges(_changes: SimpleChanges) { + this.container.clear(); + for (let i = 0; i < this.counter; ++i) { + this.container.createEmbeddedView(this.template, new CounterDirectiveContext(i + 1)); + } + } +} + /** * This Component provides the `test-comp` selector. */