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
This commit is contained in:
Paul Gschwendtner 2019-04-11 18:59:12 +02:00 committed by Alex Rickabaugh
parent 446e3573e3
commit b0c1282fbe
5 changed files with 34 additions and 10 deletions

View File

@ -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;

View File

@ -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);

View File

@ -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;

View File

@ -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: '<ng-template let-one><a (sayHello)="one=true"></a></ng-template>',
})
export class MyComp {}
@Component({
template: '<ng-template let-two><b (greet)="two=true"></b></ng-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/);
});
});

View File

@ -39,7 +39,7 @@ export interface ResolvedTemplate {
* TypeScript source files (inline templates or external referenced templates)
*/
export class NgComponentTemplateVisitor {
resolvedTemplates = new Map<string, ResolvedTemplate>();
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,