fix(migrations): cf migration - ensure full check runs for all imports (#53637)

thePunderWoman · atscott · commit 5c2f2539e27b · 2023-12-19T10:11:47.000-08:00
In cases where CommonModule was unsafe to remove but other imports were present, the symbol check would be skipped. This should run for all the possibly removed symbols for safety. PR Close #53637
diff --git a/packages/core/schematics/ng-generate/control-flow-migration/types.ts b/packages/core/schematics/ng-generate/control-flow-migration/types.ts
@@ -311,26 +311,22 @@ export class AnalyzedFile {
    * It is only run on .ts files.
    */
   verifyCanRemoveImports() {
-    // if we already know it's not safe to remove the common module import
-    // skip this check entirely
-    if (this.removeCommonModule) {
-      const importDeclaration = this.importRanges.find(r => r.type === 'importDeclaration');
-      const instances = lookupIdentifiersInSourceFile(this.sourceFile, importWithCommonRemovals);
-      let foundImportDeclaration = false;
-      let count = 0;
-      for (let range of this.importRanges) {
-        for (let instance of instances) {
-          if (instance.getStart() >= range.start && instance.getEnd() <= range.end!) {
-            if (range === importDeclaration) {
-              foundImportDeclaration = true;
-            }
-            count++;
+    const importDeclaration = this.importRanges.find(r => r.type === 'importDeclaration');
+    const instances = lookupIdentifiersInSourceFile(this.sourceFile, importWithCommonRemovals);
+    let foundImportDeclaration = false;
+    let count = 0;
+    for (let range of this.importRanges) {
+      for (let instance of instances) {
+        if (instance.getStart() >= range.start && instance.getEnd() <= range.end!) {
+          if (range === importDeclaration) {
+            foundImportDeclaration = true;
           }
+          count++;
         }
       }
-      if (instances.size !== count && importDeclaration !== undefined && foundImportDeclaration) {
-        importDeclaration.remove = false;
-      }
+    }
+    if (instances.size !== count && importDeclaration !== undefined && foundImportDeclaration) {
+      importDeclaration.remove = false;
     }
   }
 }
diff --git a/packages/core/schematics/test/control_flow_migration_spec.ts b/packages/core/schematics/test/control_flow_migration_spec.ts
@@ -5226,6 +5226,56 @@ describe('control flow migration', () => {
 
       expect(actual).toBe(expected);
     });
+
+    it('should not remove other imports when mismatch in counts', async () => {
+      writeFile('/comp.ts', [
+        `import {DatePipe, NgIf} from '@angular/common';`,
+        `import {Component, NgModule, Pipe, PipeTransform} from '@angular/core';`,
+        `@Component({`,
+        `  selector: 'example',`,
+        `  template: \`<span>{{ example | date }}</span>\`,`,
+        `})`,
+        `export class ExampleCmp {`,
+        `  example: 'stuff',`,
+        `}`,
+        `const NG_MODULE_IMPORTS = [`,
+        `  DatePipe,`,
+        `  NgIf,`,
+        `];`,
+        `@NgModule({`,
+        `  declarations: [ExampleCmp],`,
+        `  imports: [NG_MODULE_IMPORTS],`,
+        `  exports: [ExampleCmp],`,
+        `})`,
+        `export class ExampleModule {}`,
+      ].join('\n'));
+
+      await runMigration();
+      const actual = tree.readContent('/comp.ts');
+      const expected = [
+        `import {DatePipe, NgIf} from '@angular/common';`,
+        `import {Component, NgModule, Pipe, PipeTransform} from '@angular/core';`,
+        `@Component({`,
+        `  selector: 'example',`,
+        `  template: \`<span>{{ example | date }}</span>\`,`,
+        `})`,
+        `export class ExampleCmp {`,
+        `  example: 'stuff',`,
+        `}`,
+        `const NG_MODULE_IMPORTS = [`,
+        `  DatePipe,`,
+        `  NgIf,`,
+        `];`,
+        `@NgModule({`,
+        `  declarations: [ExampleCmp],`,
+        `  imports: [NG_MODULE_IMPORTS],`,
+        `  exports: [ExampleCmp],`,
+        `})`,
+        `export class ExampleModule {}`,
+      ].join('\n');
+
+      expect(actual).toBe(expected);
+    });
   });
 
   describe('no migration needed', () => {
