From bb6e5e2dd06fb9047a1a3c414a0ae73ff1e0d2c1 Mon Sep 17 00:00:00 2001 From: Alex Rickabaugh Date: Wed, 3 Mar 2021 11:54:40 -0800 Subject: [PATCH] fix(language-service): don't show external template diagnostics in ts files (#41070) The compiler considers template diagnostics to "belong" to the source file of the component using the template. This means that when diagnostics for a source file are reported, it returns diagnostics of TS structures in the actual source file, diagnostics for any inline templates, and diagnostics of any external templates. The Language Service uses a different model, and wants to show template diagnostics in the actual .html file. Thus, it's not necessary (and in fact incorrect) to include such diagnostics for the actual .ts file as well. Doing this currently causes a bug where external diagnostics appear in the TS file with "random" source spans. This commit changes the Language Service to filter the set of diagnostics returned by the compiler and only include those diagnostics with spans actually within the .ts file itself. Fixes #41032 PR Close #41070 --- .../language-service/ivy/language_service.ts | 23 +++++++- .../ivy/test/compiler_spec.ts | 4 +- .../ivy/test/diagnostic_spec.ts | 54 +++++++++++++++++-- 3 files changed, 73 insertions(+), 8 deletions(-) diff --git a/packages/language-service/ivy/language_service.ts b/packages/language-service/ivy/language_service.ts index 9c367c4822..21908703a9 100644 --- a/packages/language-service/ivy/language_service.ts +++ b/packages/language-service/ivy/language_service.ts @@ -70,7 +70,28 @@ export class LanguageService { const program = compiler.getNextProgram(); const sourceFile = program.getSourceFile(fileName); if (sourceFile) { - diagnostics.push(...compiler.getDiagnosticsForFile(sourceFile, OptimizeFor.SingleFile)); + const ngDiagnostics = compiler.getDiagnosticsForFile(sourceFile, OptimizeFor.SingleFile); + // There are several kinds of diagnostics returned by `NgCompiler` for a source file: + // + // 1. Angular-related non-template diagnostics from decorated classes within that file. + // 2. Template diagnostics for components with direct inline templates (a string literal). + // 3. Template diagnostics for components with indirect inline templates (templates computed + // by expression). + // 4. Template diagnostics for components with external templates. + // + // When showing diagnostics for a TS source file, we want to only include kinds 1 and 2 - + // those diagnostics which are reported at a location within the TS file itself. Diagnostics + // for external templates will be shown when editing that template file (the `else` block) + // below. + // + // Currently, indirect inline template diagnostics (kind 3) are not shown at all by the + // Language Service, because there is no sensible location in the user's code for them. Such + // templates are an edge case, though, and should not be common. + // + // TODO(alxhub): figure out a good user experience for indirect template diagnostics and + // show them from within the Language Service. + diagnostics.push(...ngDiagnostics.filter( + diag => diag.file !== undefined && diag.file.fileName === sourceFile.fileName)); } } else { const components = compiler.getComponentsWithTemplateFile(fileName); diff --git a/packages/language-service/ivy/test/compiler_spec.ts b/packages/language-service/ivy/test/compiler_spec.ts index 027c32ff3a..33ba654ffb 100644 --- a/packages/language-service/ivy/test/compiler_spec.ts +++ b/packages/language-service/ivy/test/compiler_spec.ts @@ -30,11 +30,11 @@ describe('language-service/compiler integration', () => { 'test.html': `Test` }); - expect(project.getDiagnosticsForFile('test.ts').length).toBeGreaterThan(0); + expect(project.getDiagnosticsForFile('test.html').length).toBeGreaterThan(0); const tmplFile = project.openFile('test.html'); tmplFile.contents = '
Test
'; - expect(project.getDiagnosticsForFile('test.ts').length).toEqual(0); + expect(project.getDiagnosticsForFile('test.html').length).toEqual(0); }); it('should not produce errors from inline test declarations mixing with those of the app', () => { diff --git a/packages/language-service/ivy/test/diagnostic_spec.ts b/packages/language-service/ivy/test/diagnostic_spec.ts index 5288e06814..97f7239d33 100644 --- a/packages/language-service/ivy/test/diagnostic_spec.ts +++ b/packages/language-service/ivy/test/diagnostic_spec.ts @@ -75,6 +75,44 @@ describe('getSemanticDiagnostics', () => { expect(diags).toEqual([]); }); + it('should not report external template diagnostics on the TS file', () => { + const files = { + 'app.ts': ` + import {Component, NgModule} from '@angular/core'; + + @Component({ + templateUrl: './app.html' + }) + export class AppComponent {} + `, + 'app.html': '{{nope}}' + }; + + const project = createModuleAndProjectWithDeclarations(env, 'test', files); + const diags = project.getDiagnosticsForFile('app.ts'); + expect(diags).toEqual([]); + }); + + it('should report diagnostics in inline templates', () => { + const files = { + 'app.ts': ` + import {Component, NgModule} from '@angular/core'; + + @Component({ + template: '{{nope}}', + }) + export class AppComponent {} + ` + }; + const project = createModuleAndProjectWithDeclarations(env, 'test', files); + const diags = project.getDiagnosticsForFile('app.ts'); + expect(diags.length).toBe(1); + const {category, file, messageText} = diags[0]; + expect(category).toBe(ts.DiagnosticCategory.Error); + expect(file?.fileName).toBe('/test/app.ts'); + expect(messageText).toBe(`Property 'nope' does not exist on type 'AppComponent'.`); + }); + it('should report member does not exist in external template', () => { const files = { 'app.ts': ` @@ -179,11 +217,17 @@ describe('getSemanticDiagnostics', () => { }; const project = env.addProject('test', files); - const diags = project.getDiagnosticsForFile('app.ts'); - expect(diags.map(x => x.messageText).sort()).toEqual([ - 'Parser Error: Bindings cannot contain assignments at column 8 in [{{nope = false}}] in /test/app1.html@0:0', - 'Parser Error: Bindings cannot contain assignments at column 8 in [{{nope = true}}] in /test/app2.html@0:0' - ]); + const diags1 = project.getDiagnosticsForFile('app1.html'); + expect(diags1.length).toBe(1); + expect(diags1[0].messageText) + .toBe( + 'Parser Error: Bindings cannot contain assignments at column 8 in [{{nope = false}}] in /test/app1.html@0:0'); + + const diags2 = project.getDiagnosticsForFile('app2.html'); + expect(diags2.length).toBe(1); + expect(diags2[0].messageText) + .toBe( + 'Parser Error: Bindings cannot contain assignments at column 8 in [{{nope = true}}] in /test/app2.html@0:0'); }); it('reports a diagnostic for a component without a template', () => {