refactor(core): undecorated classes migration should not decorate classes if not needed (#32319)

Currently the undecorated classes migration decorates base classes if no
explicit constructor is defined on all classes in the inheritance chain.

We only want to decorate base classes which define a constructor that is
inherited. Additionally for best practice, all classes in between the class
that inherits the constructor and the one that defines it are also decorated.

PR Close #32319

packages/core/schematics/migrations/undecorated-classes-with-di/transform.ts

@@ -97,6 +97,21 @@ export class UndecoratedClassesTransform {
   }
 
   private _migrateProviderBaseClass(node: ts.ClassDeclaration): TransformFailure[] {
+    return this._migrateDecoratedClassWithInheritedCtor(
+        node, symbol => this.metadataResolver.isInjectable(symbol),
+        node => this._addInjectableDecorator(node));
+  }
+
+  private _migrateDirectiveBaseClass(node: ts.ClassDeclaration): TransformFailure[] {
+    return this._migrateDecoratedClassWithInheritedCtor(
+        node, symbol => this.metadataResolver.isDirective(symbol),
+        node => this._addAbstractDirectiveDecorator(node));
+  }
+
+
+  private _migrateDecoratedClassWithInheritedCtor(
+      node: ts.ClassDeclaration, isClassDecorated: (symbol: StaticSymbol) => boolean,
+      addClassDecorator: (node: ts.ClassDeclaration) => void): TransformFailure[] {
     // In case the provider has an explicit constructor, we don't need to do anything
     // because the class is already decorated and does not inherit a constructor.
     if (hasExplicitConstructor(node)) {
@@ -104,77 +119,43 @@ export class UndecoratedClassesTransform {
     }
 
     const orderedBaseClasses = findBaseClassDeclarations(node, this.typeChecker);
-    let lastDecoratedClass: ts.ClassDeclaration = node;
+    const undecoratedBaseClasses: ts.ClassDeclaration[] = [];
 
     for (let {node: baseClass, identifier} of orderedBaseClasses) {
       const baseClassFile = baseClass.getSourceFile();
 
       if (hasExplicitConstructor(baseClass)) {
+        // All classes in between the decorated class and the undecorated class
+        // that defines the constructor need to be decorated as well.
+        undecoratedBaseClasses.forEach(b => addClassDecorator(b));
+
         if (baseClassFile.isDeclarationFile) {
           const staticSymbol = this._getStaticSymbolOfIdentifier(identifier);
 
           // If the base class is decorated through metadata files, we don't
           // need to add a comment to the derived class for the external base class.
-          if (staticSymbol && this.metadataResolver.isInjectable(staticSymbol)) {
+          if (staticSymbol && isClassDecorated(staticSymbol)) {
             break;
           }
 
-          // If the base class is not decorated, we cannot decorate the base class and
+          // Find the last class in the inheritance chain that is decorated and will be
-          // need to a comment to the last decorated class.
+          // used as anchor for a comment explaining that the class that defines the
+          // constructor cannot be decorated automatically.
+          const lastDecoratedClass =
+              undecoratedBaseClasses[undecoratedBaseClasses.length - 1] || node;
           return this._addMissingExplicitConstructorTodo(lastDecoratedClass);
         }
 
-        this._addInjectableDecorator(baseClass);
+        // Decorate the class that defines the constructor that is inherited.
+        addClassDecorator(baseClass);
         break;
       }
 
-      // Add the "@Injectable" decorator for all base classes in the inheritance chain
+      // Add the class decorator for all base classes in the inheritance chain until
-      // until the base class with the explicit constructor. The decorator will be only
+      // the base class with the explicit constructor. The decorator will be only
       // added for base classes which can be modified.
       if (!baseClassFile.isDeclarationFile) {
-        this._addInjectableDecorator(baseClass);
+        undecoratedBaseClasses.push(baseClass);
-        lastDecoratedClass = baseClass;
-      }
-    }
-    return [];
-  }
-
-  private _migrateDirectiveBaseClass(node: ts.ClassDeclaration): TransformFailure[] {
-    // In case the directive has an explicit constructor, we don't need to do
-    // anything because the class is already decorated with "@Directive" or "@Component"
-    if (hasExplicitConstructor(node)) {
-      return [];
-    }
-
-    const orderedBaseClasses = findBaseClassDeclarations(node, this.typeChecker);
-    let lastDecoratedClass: ts.ClassDeclaration = node;
-
-    for (let {node: baseClass, identifier} of orderedBaseClasses) {
-      const baseClassFile = baseClass.getSourceFile();
-
-      if (hasExplicitConstructor(baseClass)) {
-        if (baseClassFile.isDeclarationFile) {
-          // If the base class is decorated through metadata files, we don't
-          // need to add a comment to the derived class for the external base class.
-          if (this._hasDirectiveMetadata(identifier)) {
-            break;
-          }
-
-          // If the base class is not decorated, we cannot decorate the base class and
-          // need to a comment to the last decorated class.
-          return this._addMissingExplicitConstructorTodo(lastDecoratedClass);
-        }
-
-        this._addAbstractDirectiveDecorator(baseClass);
-        break;
-      }
-
-      // Add the abstract directive decorator for all base classes in the inheritance
-      // chain until the base class with the explicit constructor. The decorator will
-      // be only added for base classes which can be modified.
-      if (!baseClassFile.isDeclarationFile) {
-        this._addAbstractDirectiveDecorator(baseClass);
-        lastDecoratedClass = baseClass;
       }
     }
     return [];
@@ -387,20 +368,6 @@ export class UndecoratedClassesTransform {
     }
   }
 
-  /**
-   * Whether the given identifier resolves to a class declaration that
-   * has metadata for a directive.
-   */
-  private _hasDirectiveMetadata(node: ts.Identifier): boolean {
-    const symbol = this._getStaticSymbolOfIdentifier(node);
-
-    if (!symbol) {
-      return false;
-    }
-
-    return this.metadataResolver.isDirective(symbol);
-  }
-
   /**
    * Resolves the declaration metadata of a given static symbol. The metadata
    * is determined by resolving metadata for the static symbol.

packages/core/schematics/test/undecorated_classes_with_di_migration_spec.ts

@@ -209,6 +209,39 @@ describe('Undecorated classes with DI migration', () => {
       export class MyPipe extends PipeTransform {}`);
   });
 
+  it('should not decorate base class if no constructor is inherited', () => {
+    writeFile('/index.ts', dedent `
+      import {Component, NgModule, Directive} from '@angular/core';
+
+      export class BaseClassWithoutCtor {
+        someUnrelatedProp = true;
+      }
+
+      @Directive({selector: 'my-dir'})
+      export class MyDirective extends BaseClassWithoutCtor {}
+
+      @Pipe()
+      export class MyPipe extends BaseClassWithoutCtor {}
+
+      @NgModule({declarations: [MyDirective, MyPipe]})
+      export class AppModule {}
+    `);
+
+    runMigration();
+
+    expect(tree.readContent('/index.ts')).toContain(dedent `
+
+      export class BaseClassWithoutCtor {
+        someUnrelatedProp = true;
+      }
+
+      @Directive({selector: 'my-dir'})
+      export class MyDirective extends BaseClassWithoutCtor {}
+
+      @Pipe()
+      export class MyPipe extends BaseClassWithoutCtor {}`);
+  });
+
   it('should not decorate base class if directive/component/provider defines a constructor', () => {
     writeFile('/index.ts', dedent `
       import {Component, Injectable, NgModule, NgZone} from '@angular/core';