fix(migrations): avoid trailing whitespaces in unused imports migration (#61698)

crisbeto · kirjs · commit e9820a6d4862 · 2025-05-30T11:07:21.000-04:00
Follow-up to #61674 where we were leaving behind some whitespace, e.g. `[One, Two, Three]` would turn into `[One ]`. These changes only preserve the whitespace if the node is preceded by a newline. This wasn't caught by tests, because they were stripping away whitespaces before asserting. I've also reworked the tests to be sensitive to formatting changes. PR Close #61698
diff --git a/packages/core/schematics/ng-generate/cleanup-unused-imports/unused_imports_migration.ts b/packages/core/schematics/ng-generate/cleanup-unused-imports/unused_imports_migration.ts
@@ -364,15 +364,18 @@ function getArrayElementRemovalUpdate(
   // trailing comma at the end of the line is fine.
   if (parent.elements[parent.elements.length - 1] === node) {
     for (let i = position - 1; i >= 0; i--) {
-      if (sourceText[i] === ',' || sourceText[i] === ' ') {
+      const char = sourceText[i];
+      if (char === ',' || char === ' ') {
         position--;
       } else {
+        if (whitespaceOrLineFeed.test(char)) {
+          // Replace the node with its leading whitespace to preserve the formatting.
+          // This only needs to happen if we're breaking on a newline.
+          toInsert = getLeadingLineWhitespaceOfNode(node);
+        }
         break;
       }
     }
-
-    // Replace the node with its leading whitespace to preserve the formatting.
-    toInsert = getLeadingLineWhitespaceOfNode(node);
   }
 
   return new TextUpdate({position, end, toInsert});
diff --git a/packages/core/schematics/test/cleanup_unused_imports_migration_spec.ts b/packages/core/schematics/test/cleanup_unused_imports_migration_spec.ts
@@ -29,10 +29,6 @@ describe('cleanup unused imports schematic', () => {
     return runner.runSchematic('cleanup-unused-imports', {}, tree);
   }
 
-  function stripWhitespace(content: string) {
-    return content.replace(/\s+/g, '');
-  }
-
   beforeEach(() => {
     runner = new SchematicTestRunner('test', runfiles.resolvePackageRelative('../collection.json'));
     host = new TempScopedNodeJsSyncHost();
@@ -95,19 +91,10 @@ describe('cleanup unused imports schematic', () => {
 
     await runMigration();
 
+    const contents = tree.readContent('comp.ts');
     expect(logs.pop()).toBe('Removed 2 imports in 1 file');
-    expect(stripWhitespace(tree.readContent('comp.ts'))).toBe(
-      stripWhitespace(`
-        import {Component} from '@angular/core';
-        import {One} from './directives';
-
-        @Component({
-          imports: [One],
-          template: '<div one></div>',
-        })
-        export class Comp {}
-    `),
-    );
+    expect(contents).toContain('imports: [One],');
+    expect(contents).toContain(`import {One} from './directives';`);
   });
 
   it('should clean up an array where all imports are not used', async () => {
@@ -127,18 +114,10 @@ describe('cleanup unused imports schematic', () => {
 
     await runMigration();
 
+    const contents = tree.readContent('comp.ts');
     expect(logs.pop()).toBe('Removed 3 imports in 1 file');
-    expect(stripWhitespace(tree.readContent('comp.ts'))).toBe(
-      stripWhitespace(`
-        import {Component} from '@angular/core';
-
-        @Component({
-          imports: [],
-          template: '',
-        })
-        export class Comp {}
-    `),
-    );
+    expect(contents).toContain('imports: [],');
+    expect(contents).not.toContain(`from './directives'`);
   });
 
   it('should clean up an array where aliased imports are not used', async () => {
@@ -157,20 +136,11 @@ describe('cleanup unused imports schematic', () => {
     );
 
     await runMigration();
+    const contents = tree.readContent('comp.ts');
 
     expect(logs.pop()).toBe('Removed 2 imports in 1 file');
-    expect(stripWhitespace(tree.readContent('comp.ts'))).toBe(
-      stripWhitespace(`
-        import {Component} from '@angular/core';
-        import {One as OneAlias} from './directives';
-
-        @Component({
-          imports: [OneAlias],
-          template: '<div one></div>',
-        })
-        export class Comp {}
-    `),
-    );
+    expect(contents).toContain('imports: [OneAlias],');
+    expect(contents).toContain(`import {One as OneAlias} from './directives';`);
   });
 
   it('should preserve import declaration if unused import is still used within the file', async () => {
@@ -195,26 +165,11 @@ describe('cleanup unused imports schematic', () => {
     );
 
     await runMigration();
+    const contents = tree.readContent('comp.ts');
 
     expect(logs.pop()).toBe('Removed 1 import in 1 file');
-    expect(stripWhitespace(tree.readContent('comp.ts'))).toBe(
-      stripWhitespace(`
-        import {Component} from '@angular/core';
-        import {One} from './directives';
-
-        @Component({
-          imports: [],
-          template: '',
-        })
-        export class Comp {}
-
-        @Component({
-          imports: [One],
-          template: '<div one></div>',
-        })
-        export class OtherComp {}
-    `),
-    );
+    expect(contents).toContain('imports: [],');
+    expect(contents).toContain(`import {One} from './directives';`);
   });
 
   it('should not touch a file where all imports are used', async () => {
@@ -282,62 +237,51 @@ describe('cleanup unused imports schematic', () => {
     );
 
     await runMigration();
+    const contents = tree.readContent('comp.ts');
 
     expect(logs.pop()).toBe('Removed 2 imports in 1 file');
-    expect(stripWhitespace(tree.readContent('comp.ts'))).toBe(
-      stripWhitespace(`
-        import {Component} from '@angular/core';
-        import {One} from './directives';
-
-        @Component({
-          imports: [One],
-          template: '<div one></div>',
-        })
-        export class Comp {}
-    `),
-    );
+    expect(contents).toContain('imports: [One],');
+    expect(contents).toContain(`import {One} from './directives';`);
   });
 
   it('should preserve comments when removing unused imports', async () => {
     writeFile(
       'comp.ts',
-      `
-        import {Component} from '@angular/core';
-        import {One, Two, Three} from './directives';
-
-        @Component({
-          imports: [
-            // Start
-            Three,
-            One,
-            Two,
-            // End
-          ],
-          template: '<div one></div>',
-        })
-        export class Comp {}
-      `,
+      [
+        `import {Component} from '@angular/core';`,
+        `import {One, Two, Three} from './directives';`,
+        ``,
+        `@Component({`,
+        `  imports: [`,
+        `    // Start`,
+        `    Three,`,
+        `    One,`,
+        `    Two,`,
+        `    // End`,
+        `  ],`,
+        `  template: '<div one></div>',`,
+        `})`,
+        `export class Comp {}`,
+      ].join('\n'),
     );
 
     await runMigration();
 
     expect(logs.pop()).toBe('Removed 2 imports in 1 file');
-    expect(stripWhitespace(tree.readContent('comp.ts'))).toBe(
-      stripWhitespace(`
-        import {Component} from '@angular/core';
-        import {One} from './directives';
-
-        @Component({
-          imports: [
-            // Start
-            One,
-            // End
-          ],
-          template: '<div one></div>',
-        })
-        export class Comp {}
-    `),
-    );
+    expect(tree.readContent('comp.ts').split('\n')).toEqual([
+      `import {Component} from '@angular/core';`,
+      `import {One} from './directives';`,
+      ``,
+      `@Component({`,
+      `  imports: [`,
+      `    // Start`,
+      `    One,`,
+      `    // End`,
+      `  ],`,
+      `  template: '<div one></div>',`,
+      `})`,
+      `export class Comp {}`,
+    ]);
   });
 
   it('should preserve inline comments and strip trailing comma when removing imports from same line', async () => {
@@ -356,19 +300,10 @@ describe('cleanup unused imports schematic', () => {
     );
 
     await runMigration();
+    const contents = tree.readContent('comp.ts');
 
     expect(logs.pop()).toBe('Removed 2 imports in 1 file');
-    expect(stripWhitespace(tree.readContent('comp.ts'))).toBe(
-      stripWhitespace(`
-        import {Component} from '@angular/core';
-        import {One} from './directives';
-
-        @Component({
-          imports: [/* Start */ One /* End */],
-          template: '<div one></div>',
-        })
-        export class Comp {}
-    `),
-    );
+    expect(contents).toContain(' imports: [/* Start */ One/* End */],');
+    expect(contents).toContain(`import {One} from './directives';`);
   });
 });
