From 60a056d5dc498a8f0918dbf39de8459c18121e09 Mon Sep 17 00:00:00 2001 From: Paul Gschwendtner Date: Mon, 26 Aug 2019 18:34:15 +0200 Subject: [PATCH] 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 --- .../undecorated-classes-with-di/transform.ts | 97 ++++++------------- ...ecorated_classes_with_di_migration_spec.ts | 33 +++++++ 2 files changed, 65 insertions(+), 65 deletions(-) diff --git a/packages/core/schematics/migrations/undecorated-classes-with-di/transform.ts b/packages/core/schematics/migrations/undecorated-classes-with-di/transform.ts index 54e46f964e..c6d5f1e0f6 100644 --- a/packages/core/schematics/migrations/undecorated-classes-with-di/transform.ts +++ b/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 - // need to a comment to the last decorated class. + // Find the last class in the inheritance chain that is decorated and will be + // 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 - // until the base class with the explicit constructor. The decorator will be only + // Add the class 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._addInjectableDecorator(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; + undecoratedBaseClasses.push(baseClass); } } return []; @@ -229,7 +210,7 @@ export class UndecoratedClassesTransform { /** Adds a comment for adding an explicit constructor to the given class declaration. */ private _addMissingExplicitConstructorTodo(node: ts.ClassDeclaration): TransformFailure[] { // In case a todo comment has been already inserted to the given class, we don't - // want to add a comment or transform failure multiple times. + // want to add a comment or transform failure multiple times. if (this.missingExplicitConstructorClasses.has(node)) { 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. diff --git a/packages/core/schematics/test/undecorated_classes_with_di_migration_spec.ts b/packages/core/schematics/test/undecorated_classes_with_di_migration_spec.ts index 7f64e87558..de421641e1 100644 --- a/packages/core/schematics/test/undecorated_classes_with_di_migration_spec.ts +++ b/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';