fix(migrations): Add support for removing imports post migration (#52763)

This update removes imports from component decorators and at the top of the files. It only removes standalone imports though. It does not remove CommonModule if that is the only import.

PR Close #52763

packages/core/schematics/ng-generate/control-flow-migration/index.ts

@@ -82,11 +82,11 @@ function runControlFlowMigration(
     const content = tree.readText(relativePath);
     const update = tree.beginUpdate(relativePath);
 
-    for (const [start, end] of ranges) {
+    for (const {start, end, node, type} of ranges) {
       const template = content.slice(start, end);
       const length = (end ?? content.length) - start;
 
-      const {migrated, errors} = migrateTemplate(template);
+      const {migrated, errors} = migrateTemplate(template, type, node, file);
 
       if (migrated !== null) {
         update.remove(start, length);

packages/core/schematics/ng-generate/control-flow-migration/migration.ts

@@ -6,26 +6,37 @@
  * found in the LICENSE file at https://angular.io/license
  */
 
+import ts from 'typescript';
+
 import {migrateFor} from './fors';
 import {migrateIf} from './ifs';
 import {migrateSwitch} from './switches';
-import {MigrateError} from './types';
-import {processNgTemplates} from './util';
+import {AnalyzedFile, MigrateError} from './types';
+import {canRemoveCommonModule, processNgTemplates, removeImports} from './util';
 
 /**
  * Actually migrates a given template to the new syntax
  */
-export function migrateTemplate(template: string): {migrated: string, errors: MigrateError[]} {
-  const ifResult = migrateIf(template);
-  const forResult = migrateFor(ifResult.migrated);
-  const switchResult = migrateSwitch(forResult.migrated);
+export function migrateTemplate(
+    template: string, templateType: string, node: ts.Node,
+    file: AnalyzedFile): {migrated: string, errors: MigrateError[]} {
+  let errors: MigrateError[] = [];
+  let migrated = template;
+  if (templateType === 'template') {
+    const ifResult = migrateIf(template);
+    const forResult = migrateFor(ifResult.migrated);
+    const switchResult = migrateSwitch(forResult.migrated);
+    migrated = processNgTemplates(switchResult.migrated);
+    file.removeCommonModule = canRemoveCommonModule(template);
 
-  const migrated = processNgTemplates(switchResult.migrated);
+    errors = [
+      ...ifResult.errors,
+      ...forResult.errors,
+      ...switchResult.errors,
+    ];
+  } else {
+    migrated = removeImports(template, node, file.removeCommonModule);
+  }
 
-  const errors = [
-    ...ifResult.errors,
-    ...forResult.errors,
-    ...switchResult.errors,
-  ];
   return {migrated, errors};
 }

packages/core/schematics/ng-generate/control-flow-migration/types.ts

@@ -6,15 +6,59 @@
  * found in the LICENSE file at https://angular.io/license
  */
 
-import {Attribute, Element, RecursiveVisitor} from '@angular/compiler';
+import {Attribute, Element, RecursiveVisitor, Text} from '@angular/compiler';
+import ts from 'typescript';
 
 export const ngtemplate = 'ng-template';
 
+function allFormsOf(selector: string): string[] {
+  return [
+    selector,
+    `*${selector}`,
+    `[${selector}]`,
+  ];
+}
+
+const commonModuleDirectives = new Set([
+  ...allFormsOf('ngComponentOutlet'),
+  ...allFormsOf('ngTemplateOutlet'),
+  ...allFormsOf('ngClass'),
+  ...allFormsOf('ngPlural'),
+  ...allFormsOf('ngPluralCase'),
+  ...allFormsOf('ngStyle'),
+  ...allFormsOf('ngTemplateOutlet'),
+  ...allFormsOf('ngComponentOutlet'),
+]);
+
+function pipeMatchRegExpFor(name: string): RegExp {
+  return new RegExp(`\\|\\s*${name}`);
+}
+
+const commonModulePipes = [
+  'date',
+  'async',
+  'currency',
+  'number',
+  'i18nPlural',
+  'i18nSelect',
+  'json',
+  'keyvalue',
+  'slice',
+  'lowercase',
+  'uppercase',
+  'titlecase',
+  'percent',
+  'titlecase',
+].map(name => pipeMatchRegExpFor(name));
+
 /**
  * Represents a range of text within a file. Omitting the end
  * means that it's until the end of the file.
  */
-type Range = [start: number, end?: number];
+type Range = {
+  start: number,
+  end?: number, node: ts.Node, type: string,
+};
 
 export type Offsets = {
   pre: number,
@@ -104,10 +148,17 @@ export class Template {
 /** Represents a file that was analyzed by the migration. */
 export class AnalyzedFile {
   private ranges: Range[] = [];
+  removeCommonModule = false;
 
   /** Returns the ranges in the order in which they should be migrated. */
   getSortedRanges(): Range[] {
-    return this.ranges.slice().sort(([aStart], [bStart]) => bStart - aStart);
+    const templateRanges = this.ranges.slice()
+                               .filter(x => x.type === 'template')
+                               .sort((aStart, bStart) => bStart.start - aStart.start);
+    const importRanges = this.ranges.slice()
+                             .filter(x => x.type === 'import')
+                             .sort((aStart, bStart) => bStart.start - aStart.start);
+    return [...templateRanges, ...importRanges];
   }
 
   /**
@@ -125,7 +176,7 @@ export class AnalyzedFile {
     }
 
     const duplicate =
-        analysis.ranges.find(current => current[0] === range[0] && current[1] === range[1]);
+        analysis.ranges.find(current => current.start === range.start && current.end === range.end);
 
     if (!duplicate) {
       analysis.ranges.push(range);
@@ -133,6 +184,36 @@ export class AnalyzedFile {
   }
 }
 
+/** Finds all non-control flow elements from common module. */
+export class CommonCollector extends RecursiveVisitor {
+  count = 0;
+
+  override visitElement(el: Element): void {
+    if (el.attrs.length > 0) {
+      for (const attr of el.attrs) {
+        if (this.hasDirectives(attr.name) || this.hasPipes(attr.value)) {
+          this.count++;
+        }
+      }
+    }
+    super.visitElement(el, null);
+  }
+
+  override visitText(ast: Text) {
+    if (this.hasPipes(ast.value)) {
+      this.count++;
+    }
+  }
+
+  private hasDirectives(input: string): boolean {
+    return commonModuleDirectives.has(input);
+  }
+
+  private hasPipes(input: string): boolean {
+    return commonModulePipes.some(regexp => regexp.test(input));
+  }
+}
+
 /** Finds all elements with ngif structural directives. */
 export class ElementCollector extends RecursiveVisitor {
   readonly elements: ElementToMigrate[] = [];

packages/core/schematics/ng-generate/control-flow-migration/util.ts

@@ -10,7 +10,10 @@ import {Attribute, HtmlParser, ParseTreeResult, visitAll} from '@angular/compile
 import {dirname, join} from 'path';
 import ts from 'typescript';
 
-import {AnalyzedFile, ElementCollector, ElementToMigrate, Template, TemplateCollector} from './types';
+import {AnalyzedFile, CommonCollector, ElementCollector, ElementToMigrate, Template, TemplateCollector} from './types';
+
+const importRemovals = ['NgIf', 'NgFor', 'NgSwitch', 'NgSwitchCase', 'NgSwitchDefault'];
+const importWithCommonRemovals = [...importRemovals, 'CommonModule'];
 
 /**
  * Analyzes a source file to find file that need to be migrated and the text ranges within them.
@@ -19,50 +22,131 @@ import {AnalyzedFile, ElementCollector, ElementToMigrate, Template, TemplateColl
  */
 export function analyze(sourceFile: ts.SourceFile, analyzedFiles: Map<string, AnalyzedFile>) {
   forEachClass(sourceFile, node => {
-    // Note: we have a utility to resolve the Angular decorators from a class declaration already.
-    // We don't use it here, because it requires access to the type checker which makes it more
-    // time-consuming to run internally.
-    const decorator = ts.getDecorators(node)?.find(dec => {
-      return ts.isCallExpression(dec.expression) && ts.isIdentifier(dec.expression.expression) &&
-          dec.expression.expression.text === 'Component';
-    }) as (ts.Decorator & {expression: ts.CallExpression}) |
-        undefined;
-
-    const metadata = decorator && decorator.expression.arguments.length > 0 &&
-            ts.isObjectLiteralExpression(decorator.expression.arguments[0]) ?
-        decorator.expression.arguments[0] :
-        null;
-
-    if (!metadata) {
-      return;
-    }
-
-    for (const prop of metadata.properties) {
-      // All the properties we care about should have static
-      // names and be initialized to a static string.
-      if (!ts.isPropertyAssignment(prop) || !ts.isStringLiteralLike(prop.initializer) ||
-          (!ts.isIdentifier(prop.name) && !ts.isStringLiteralLike(prop.name))) {
-        continue;
-      }
-
-      switch (prop.name.text) {
-        case 'template':
-          // +1/-1 to exclude the opening/closing characters from the range.
-          AnalyzedFile.addRange(
-              sourceFile.fileName, analyzedFiles,
-              [prop.initializer.getStart() + 1, prop.initializer.getEnd() - 1]);
-          break;
-
-        case 'templateUrl':
-          // Leave the end as undefined which means that the range is until the end of the file.
-          const path = join(dirname(sourceFile.fileName), prop.initializer.text);
-          AnalyzedFile.addRange(path, analyzedFiles, [0]);
-          break;
-      }
+    if (ts.isClassDeclaration(node)) {
+      analyzeDecorators(node, sourceFile, analyzedFiles);
+    } else {
+      analyzeImportDeclarations(node, sourceFile, analyzedFiles);
     }
   });
 }
 
+function updateImportDeclaration(decl: ts.ImportDeclaration, removeCommonModule: boolean): string {
+  const clause = decl.getChildAt(1) as ts.ImportClause;
+  const updatedClause = updateImportClause(clause, removeCommonModule);
+  if (updatedClause === null) {
+    return '';
+  }
+  const printer = ts.createPrinter();
+  const updated = ts.factory.updateImportDeclaration(
+      decl, decl.modifiers, updatedClause, decl.moduleSpecifier, undefined);
+  return printer.printNode(ts.EmitHint.Unspecified, updated, clause.getSourceFile());
+}
+
+function updateImportClause(clause: ts.ImportClause, removeCommonModule: boolean): ts.ImportClause|
+    null {
+  if (clause.namedBindings && ts.isNamedImports(clause.namedBindings)) {
+    const removals = removeCommonModule ? importWithCommonRemovals : importRemovals;
+    const elements = clause.namedBindings.elements.filter(el => !removals.includes(el.getText()));
+    if (elements.length === 0) {
+      return null;
+    }
+    clause = ts.factory.updateImportClause(
+        clause, clause.isTypeOnly, clause.name, ts.factory.createNamedImports(elements));
+  }
+  return clause;
+}
+
+function updateClassImports(
+    propAssignment: ts.PropertyAssignment, removeCommonModule: boolean): string {
+  const printer = ts.createPrinter();
+  const importList = propAssignment.initializer as ts.ArrayLiteralExpression;
+  const removals = removeCommonModule ? importWithCommonRemovals : importRemovals;
+  const elements = importList.elements.filter(el => !removals.includes(el.getText()));
+  const updatedElements = ts.factory.updateArrayLiteralExpression(importList, elements);
+  const updatedAssignment =
+      ts.factory.updatePropertyAssignment(propAssignment, propAssignment.name, updatedElements);
+  return printer.printNode(
+      ts.EmitHint.Unspecified, updatedAssignment, updatedAssignment.getSourceFile());
+}
+
+function analyzeImportDeclarations(
+    node: ts.ImportDeclaration, sourceFile: ts.SourceFile,
+    analyzedFiles: Map<string, AnalyzedFile>) {
+  if (node.getText().indexOf('@angular/common') === -1) {
+    return;
+  }
+  const clause = node.getChildAt(1) as ts.ImportClause;
+  if (clause.namedBindings && ts.isNamedImports(clause.namedBindings)) {
+    const elements =
+        clause.namedBindings.elements.filter(el => importWithCommonRemovals.includes(el.getText()));
+    if (elements.length > 0) {
+      AnalyzedFile.addRange(
+          sourceFile.fileName, analyzedFiles,
+          {start: node.getStart(), end: node.getEnd(), node, type: 'import'});
+    }
+  }
+}
+
+function analyzeDecorators(
+    node: ts.ClassDeclaration, sourceFile: ts.SourceFile,
+    analyzedFiles: Map<string, AnalyzedFile>) {
+  // Note: we have a utility to resolve the Angular decorators from a class declaration already.
+  // We don't use it here, because it requires access to the type checker which makes it more
+  // time-consuming to run internally.
+  const decorator = ts.getDecorators(node)?.find(dec => {
+    return ts.isCallExpression(dec.expression) && ts.isIdentifier(dec.expression.expression) &&
+        dec.expression.expression.text === 'Component';
+  }) as (ts.Decorator & {expression: ts.CallExpression}) |
+      undefined;
+
+  const metadata = decorator && decorator.expression.arguments.length > 0 &&
+          ts.isObjectLiteralExpression(decorator.expression.arguments[0]) ?
+      decorator.expression.arguments[0] :
+      null;
+
+  if (!metadata) {
+    return;
+  }
+
+  for (const prop of metadata.properties) {
+    // All the properties we care about should have static
+    // names and be initialized to a static string.
+    if (!ts.isPropertyAssignment(prop) ||
+        (!ts.isIdentifier(prop.name) && !ts.isStringLiteralLike(prop.name))) {
+      continue;
+    }
+
+    switch (prop.name.text) {
+      case 'template':
+        // +1/-1 to exclude the opening/closing characters from the range.
+        AnalyzedFile.addRange(sourceFile.fileName, analyzedFiles, {
+          start: prop.initializer.getStart() + 1,
+          end: prop.initializer.getEnd() - 1,
+          node: prop,
+          type: 'template'
+        });
+        break;
+
+      case 'imports':
+        AnalyzedFile.addRange(sourceFile.fileName, analyzedFiles, {
+          start: prop.name.getStart(),
+          end: prop.initializer.getEnd(),
+          node: prop,
+          type: 'import'
+        });
+        break;
+
+      case 'templateUrl':
+        // Leave the end as undefined which means that the range is until the end of the file.
+        if (ts.isStringLiteralLike(prop.initializer)) {
+          const path = join(dirname(sourceFile.fileName), prop.initializer.text);
+          AnalyzedFile.addRange(path, analyzedFiles, {start: 0, node: prop, type: 'template'});
+        }
+        break;
+    }
+  }
+}
+
 /**
  * returns the level deep a migratable element is nested
  */
@@ -216,6 +300,33 @@ export function processNgTemplates(template: string): string {
   return template;
 }
 
+/**
+ * determines if the CommonModule can be safely removed from imports
+ */
+export function canRemoveCommonModule(template: string): boolean {
+  const parsed = parseTemplate(template);
+  let removeCommonModule = false;
+  if (parsed !== null) {
+    const visitor = new CommonCollector();
+    visitAll(visitor, parsed.rootNodes);
+    removeCommonModule = visitor.count === 0;
+  }
+  return removeCommonModule;
+}
+
+/**
+ * removes imports from template imports and import declarations
+ */
+export function removeImports(
+    template: string, node: ts.Node, removeCommonModule: boolean): string {
+  if (template.startsWith('imports') && ts.isPropertyAssignment(node)) {
+    return updateClassImports(node, removeCommonModule);
+  } else if (ts.isImportDeclaration(node)) {
+    return updateImportDeclaration(node, removeCommonModule);
+  }
+  return template;
+}
+
 /**
  * retrieves the original block of text in the template for length comparison during migration
  * processing
@@ -283,9 +394,10 @@ export function getMainBlock(etm: ElementToMigrate, tmpl: string, offset: number
 }
 
 /** Executes a callback on each class declaration in a file. */
-function forEachClass(sourceFile: ts.SourceFile, callback: (node: ts.ClassDeclaration) => void) {
+function forEachClass(
+    sourceFile: ts.SourceFile, callback: (node: ts.ClassDeclaration|ts.ImportDeclaration) => void) {
   sourceFile.forEachChild(function walk(node) {
-    if (ts.isClassDeclaration(node)) {
+    if (ts.isClassDeclaration(node) || ts.isImportDeclaration(node)) {
       callback(node);
     }
     node.forEachChild(walk);

packages/core/schematics/test/control_flow_migration_spec.ts

@@ -105,14 +105,12 @@ describe('control flow migration', () => {
     });
 
     it('should only migrate the paths that were passed in', async () => {
-      let error: string|null = null;
-
       writeFile('comp.ts', `
         import {Component} from '@angular/core';
         import {NgIf} from '@angular/common';
 
         @Component({
-          imports: [NgIf],
+          imports: [NgIf, NgFor,NgSwitch,NgSwitchCase ,NgSwitchDefault],
           template: \`<div><span *ngIf="toggle">This should be hidden</span></div>\`
         })
         class Comp {
@@ -139,7 +137,11 @@ describe('control flow migration', () => {
 
       expect(migratedContent)
           .toContain('template: `<div>@if (toggle) {<span>This should be hidden</span>}</div>`');
+      expect(migratedContent).toContain('imports: []');
+      expect(migratedContent).not.toContain(`import {NgIf} from '@angular/common';`);
       expect(skippedContent).toContain('template: `<div *ngIf="show">Show me</div>`');
+      expect(skippedContent).toContain('imports: [NgIf]');
+      expect(skippedContent).toContain(`import {NgIf} from '@angular/common';`);
     });
   });
 
@@ -2730,6 +2732,126 @@ describe('control flow migration', () => {
     });
   });
 
+  describe('imports', () => {
+    it('should remove common module imports post migration', async () => {
+      writeFile('/comp.ts', [
+        `import {Component} from '@angular/core';`,
+        `import {NgIf} from '@angular/common';`,
+        `@Component({`,
+        `  imports: [NgIf],`,
+        `  template: \`<div><span *ngIf="toggle">shrug</span></div>\``,
+        `})`,
+        `class Comp {`,
+        `  toggle = false;`,
+        `}`,
+      ].join('\n'));
+
+      await runMigration();
+      const actual = tree.readContent('/comp.ts');
+      const expected = [
+        `import {Component} from '@angular/core';\n`,
+        `@Component({`,
+        `  imports: [],`,
+        `  template: \`<div>@if (toggle) {<span>shrug</span>}</div>\``,
+        `})`,
+        `class Comp {`,
+        `  toggle = false;`,
+        `}`,
+      ].join('\n');
+
+      expect(actual).toBe(expected);
+    });
+
+    it('should leave non-cf common module imports post migration', async () => {
+      writeFile('/comp.ts', [
+        `import {Component} from '@angular/core';`,
+        `import {NgIf, AsyncPipe} from '@angular/common';\n`,
+        `@Component({`,
+        `  imports: [NgIf, AsyncPipe],`,
+        `  template: \`<div><span *ngIf="toggle">shrug</span></div>\``,
+        `})`,
+        `class Comp {`,
+        `  toggle = false;`,
+        `}`,
+      ].join('\n'));
+
+      await runMigration();
+      const actual = tree.readContent('/comp.ts');
+      const expected = [
+        `import {Component} from '@angular/core';`,
+        `import { AsyncPipe } from '@angular/common';\n`,
+        `@Component({`,
+        `  imports: [AsyncPipe],`,
+        `  template: \`<div>@if (toggle) {<span>shrug</span>}</div>\``,
+        `})`,
+        `class Comp {`,
+        `  toggle = false;`,
+        `}`,
+      ].join('\n');
+
+      expect(actual).toBe(expected);
+    });
+
+    it('should remove common module post migration', async () => {
+      writeFile('/comp.ts', [
+        `import {Component} from '@angular/core';`,
+        `import {CommonModule} from '@angular/common';`,
+        `@Component({`,
+        `  imports: [CommonModule],`,
+        `  template: \`<div><span *ngIf="toggle">shrug</span></div>\``,
+        `})`,
+        `class Comp {`,
+        `  toggle = false;`,
+        `}`,
+      ].join('\n'));
+
+      await runMigration();
+      const actual = tree.readContent('/comp.ts');
+      const expected = [
+        `import {Component} from '@angular/core';\n`,
+        `@Component({`,
+        `  imports: [],`,
+        `  template: \`<div>@if (toggle) {<span>shrug</span>}</div>\``,
+        `})`,
+        `class Comp {`,
+        `  toggle = false;`,
+        `}`,
+      ].join('\n');
+
+      expect(actual).toBe(expected);
+    });
+
+    it('should leave common module post migration if other common module deps exist', async () => {
+      writeFile('/comp.ts', [
+        `import {Component} from '@angular/core';`,
+        `import {CommonModule} from '@angular/common';\n`,
+        `@Component({`,
+        `  imports: [CommonModule],`,
+        `  template: \`<div><span *ngIf="toggle">{{ shrug | lowercase }}</span></div>\``,
+        `})`,
+        `class Comp {`,
+        `  toggle = false;`,
+        `}`,
+      ].join('\n'));
+
+      await runMigration();
+      const actual = tree.readContent('/comp.ts');
+      const expected = [
+        `import {Component} from '@angular/core';`,
+        `import { CommonModule } from '@angular/common';\n`,
+        `@Component({`,
+        `  imports: [CommonModule],`,
+        `  template: \`<div>@if (toggle) {<span>{{ shrug | lowercase }}</span>}</div>\``,
+        `})`,
+        `class Comp {`,
+        `  toggle = false;`,
+        `}`,
+      ].join('\n');
+
+      expect(actual).toBe(expected);
+    });
+  });
+
   describe('no migration needed', () => {
     it('should do nothing when no control flow is present', async () => {
       writeFile('/comp.ts', `