From b0c1282fbed04771a8649f8cf9b355a4076136a2 Mon Sep 17 00:00:00 2001 From: Paul Gschwendtner Date: Thu, 11 Apr 2019 18:59:12 +0200 Subject: [PATCH] refactor(core): migrations do not properly handle multiple templates in source file (#29841) Currently if there are multiple source files within a given TypeScript source file, only the last template in the source file is checked as we store templates in a `Map` with the source file paths as keys. This is problematic as multiple templates can live within the same source file and we therefore accidentally overwrite existing entries in the resolved templates map. PR Close #29841 --- .../analyze_template.ts | 6 ++--- .../noTemplateVariableAssignmentRule.ts | 5 +++-- .../template-var-assignment/index.ts | 5 +++-- .../template_var_assignment_migration_spec.ts | 22 +++++++++++++++++++ .../schematics/utils/ng_component_template.ts | 6 ++--- 5 files changed, 34 insertions(+), 10 deletions(-) diff --git a/packages/core/schematics/migrations/template-var-assignment/analyze_template.ts b/packages/core/schematics/migrations/template-var-assignment/analyze_template.ts index f732c7b760..c140770232 100644 --- a/packages/core/schematics/migrations/template-var-assignment/analyze_template.ts +++ b/packages/core/schematics/migrations/template-var-assignment/analyze_template.ts @@ -24,9 +24,9 @@ export interface TemplateVariableAssignment { * Analyzes a given resolved template by looking for property assignments to local * template variables within bound events. */ -export function analyzeResolvedTemplate( - filePath: string, template: ResolvedTemplate): TemplateVariableAssignment[]|null { - const templateNodes = parseHtmlGracefully(template.content, filePath); +export function analyzeResolvedTemplate(template: ResolvedTemplate): TemplateVariableAssignment[]| + null { + const templateNodes = parseHtmlGracefully(template.content, template.filePath); if (!templateNodes) { return null; diff --git a/packages/core/schematics/migrations/template-var-assignment/google3/noTemplateVariableAssignmentRule.ts b/packages/core/schematics/migrations/template-var-assignment/google3/noTemplateVariableAssignmentRule.ts index 34adc22a0d..0bd7077ab5 100644 --- a/packages/core/schematics/migrations/template-var-assignment/google3/noTemplateVariableAssignmentRule.ts +++ b/packages/core/schematics/migrations/template-var-assignment/google3/noTemplateVariableAssignmentRule.ts @@ -33,8 +33,9 @@ export class Rule extends Rules.TypedRule { // Analyze each resolved template and print a warning for property writes to // template variables. - resolvedTemplates.forEach((template, filePath) => { - const nodes = analyzeResolvedTemplate(filePath, template); + resolvedTemplates.forEach(template => { + const filePath = template.filePath; + const nodes = analyzeResolvedTemplate(template); const templateFile = template.inline ? sourceFile : createHtmlSourceFile(filePath, template.content); diff --git a/packages/core/schematics/migrations/template-var-assignment/index.ts b/packages/core/schematics/migrations/template-var-assignment/index.ts index 43502a4c6f..f6d534cefe 100644 --- a/packages/core/schematics/migrations/template-var-assignment/index.ts +++ b/packages/core/schematics/migrations/template-var-assignment/index.ts @@ -71,8 +71,9 @@ function runTemplateVariableAssignmentCheck( // Analyze each resolved template and print a warning for property writes to // template variables. - resolvedTemplates.forEach((template, filePath) => { - const nodes = analyzeResolvedTemplate(filePath, template); + resolvedTemplates.forEach(template => { + const filePath = template.filePath; + const nodes = analyzeResolvedTemplate(template); if (!nodes) { return; diff --git a/packages/core/schematics/test/template_var_assignment_migration_spec.ts b/packages/core/schematics/test/template_var_assignment_migration_spec.ts index c3bac52433..87a3ba4f9e 100644 --- a/packages/core/schematics/test/template_var_assignment_migration_spec.ts +++ b/packages/core/schematics/test/template_var_assignment_migration_spec.ts @@ -197,4 +197,26 @@ describe('template variable assignment migration', () => { expect(warnOutput.length).toBe(0); }); + + it('should be able to report multiple templates within the same source file', () => { + writeFile('/index.ts', ` + import {Component} from '@angular/core'; + + @Component({ + template: '', + }) + export class MyComp {} + + @Component({ + template: '', + }) + export class MyComp2 {} + `); + + runMigration(); + + expect(warnOutput.length).toBe(2); + expect(warnOutput[0]).toMatch(/^⮑ {3}index.ts@5:56: Found assignment/); + expect(warnOutput[1]).toMatch(/^⮑ {3}index.ts@10:53: Found assignment/); + }); }); diff --git a/packages/core/schematics/utils/ng_component_template.ts b/packages/core/schematics/utils/ng_component_template.ts index aac6620fde..d905d0e8d5 100644 --- a/packages/core/schematics/utils/ng_component_template.ts +++ b/packages/core/schematics/utils/ng_component_template.ts @@ -39,7 +39,7 @@ export interface ResolvedTemplate { * TypeScript source files (inline templates or external referenced templates) */ export class NgComponentTemplateVisitor { - resolvedTemplates = new Map(); + resolvedTemplates: ResolvedTemplate[] = []; constructor(public typeChecker: ts.TypeChecker) {} @@ -95,7 +95,7 @@ export class NgComponentTemplateVisitor { // not part of the template content. const templateStartIdx = property.initializer.getStart() + 1; const filePath = resolve(sourceFileName); - this.resolvedTemplates.set(filePath, { + this.resolvedTemplates.push({ filePath: filePath, container: node, content: property.initializer.text, @@ -117,7 +117,7 @@ export class NgComponentTemplateVisitor { const fileContent = readFileSync(templatePath, 'utf8'); const lineStartsMap = computeLineStartsMap(fileContent); - this.resolvedTemplates.set(templatePath, { + this.resolvedTemplates.push({ filePath: templatePath, container: node, content: fileContent,