From c24ad560fa6a14d067969d5588749ba1261bcb98 Mon Sep 17 00:00:00 2001 From: Paul Gschwendtner Date: Fri, 14 Feb 2020 18:59:29 +0100 Subject: [PATCH] feat(core): undecorated-classes migration should handle derived abstract classes (#35339) In version 10, undecorated base classes that use Angular features need to be decorated explicitly with `@Directive()`. Additionally, derived classes of abstract directives need to be decorated. The migration already handles this for undecorated classes that are not explicitly decorated, but since in V9, abstract directives can be used, we also need to handle this for explicitly decorated abstract directives. e.g. ``` @Directive() export class Base {...} // needs to be decorated by migration when updating from v9 to v10 export class Wrapped extends Base {} @Component(...) export class Cmp extends Wrapped {} ``` PR Close #35339 --- .../undecorated-classes-with-fields.ts | 17 ++- ...ndecorated-classes-with-fields_expected.ts | 19 ++- .../BUILD.bazel | 2 + .../transform.ts | 112 +++++++++++++----- ...es_with_decorated_fields_migration_spec.ts | 33 ++++++ .../schematics/utils/typescript/functions.ts | 6 +- 6 files changed, 157 insertions(+), 32 deletions(-) diff --git a/integration/ng_update_migrations/src/app/migration-tests/undecorated-classes-with-fields.ts b/integration/ng_update_migrations/src/app/migration-tests/undecorated-classes-with-fields.ts index 8988a6cd25..c3ea7d460e 100644 --- a/integration/ng_update_migrations/src/app/migration-tests/undecorated-classes-with-fields.ts +++ b/integration/ng_update_migrations/src/app/migration-tests/undecorated-classes-with-fields.ts @@ -1,4 +1,12 @@ -import {Component, ElementRef, HostBinding, HostListener, Input, NgModule} from '@angular/core'; +import { + Component, + Directive, + ElementRef, + HostBinding, + HostListener, + Input, + NgModule +} from '@angular/core'; export class NonAngularBaseClass { greet() {} @@ -45,3 +53,10 @@ export class WrappedMyComp extends MyComp {} @NgModule({declarations: [MyComp, WrappedMyComp]}) export class TestModule {} + +@Directive({selector: null}) +export class AbstractDir {} + +export class DerivedAbstractDir extends AbstractDir {} + +export class WrappedDerivedAbstractDir extends DerivedAbstractDir {} diff --git a/integration/ng_update_migrations/src/app/migration-tests/undecorated-classes-with-fields_expected.ts b/integration/ng_update_migrations/src/app/migration-tests/undecorated-classes-with-fields_expected.ts index 37cb721ec5..ebc267f02a 100644 --- a/integration/ng_update_migrations/src/app/migration-tests/undecorated-classes-with-fields_expected.ts +++ b/integration/ng_update_migrations/src/app/migration-tests/undecorated-classes-with-fields_expected.ts @@ -1,4 +1,12 @@ -import { Component, ElementRef, HostBinding, HostListener, Input, NgModule, Directive } from '@angular/core'; +import { + Component, + Directive, + ElementRef, + HostBinding, + HostListener, + Input, + NgModule +} from '@angular/core'; export class NonAngularBaseClass { greet() {} @@ -55,3 +63,12 @@ export class WrappedMyComp extends MyComp {} @NgModule({declarations: [MyComp, WrappedMyComp]}) export class TestModule {} + +@Directive({selector: null}) +export class AbstractDir {} + +@Directive() +export class DerivedAbstractDir extends AbstractDir {} + +@Directive() +export class WrappedDerivedAbstractDir extends DerivedAbstractDir {} diff --git a/packages/core/schematics/migrations/undecorated-classes-with-decorated-fields/BUILD.bazel b/packages/core/schematics/migrations/undecorated-classes-with-decorated-fields/BUILD.bazel index 9831bbf557..040810b57e 100644 --- a/packages/core/schematics/migrations/undecorated-classes-with-decorated-fields/BUILD.bazel +++ b/packages/core/schematics/migrations/undecorated-classes-with-decorated-fields/BUILD.bazel @@ -11,6 +11,8 @@ ts_library( "//packages/core/schematics/test:__pkg__", ], deps = [ + "//packages/compiler-cli/src/ngtsc/partial_evaluator", + "//packages/compiler-cli/src/ngtsc/reflection", "//packages/core/schematics/utils", "@npm//@angular-devkit/schematics", "@npm//@types/node", diff --git a/packages/core/schematics/migrations/undecorated-classes-with-decorated-fields/transform.ts b/packages/core/schematics/migrations/undecorated-classes-with-decorated-fields/transform.ts index c8e30ec86a..159f65d6c8 100644 --- a/packages/core/schematics/migrations/undecorated-classes-with-decorated-fields/transform.ts +++ b/packages/core/schematics/migrations/undecorated-classes-with-decorated-fields/transform.ts @@ -6,17 +6,33 @@ * found in the LICENSE file at https://angular.io/license */ +import {PartialEvaluator} from '@angular/compiler-cli/src/ngtsc/partial_evaluator'; +import {TypeScriptReflectionHost, reflectObjectLiteral} from '@angular/compiler-cli/src/ngtsc/reflection'; import * as ts from 'typescript'; import {ImportManager} from '../../utils/import_manager'; -import {getAngularDecorators} from '../../utils/ng_decorators'; +import {NgDecorator, getAngularDecorators} from '../../utils/ng_decorators'; import {findBaseClassDeclarations} from '../../utils/typescript/find_base_classes'; +import {unwrapExpression} from '../../utils/typescript/functions'; import {UpdateRecorder} from './update_recorder'; + +/** Analyzed class declaration. */ +interface AnalyzedClass { + /** Whether the class is decorated with @Directive or @Component. */ + isDirectiveOrComponent: boolean; + /** Whether the class is an abstract directive. */ + isAbstractDirective: boolean; + /** Whether the class uses any Angular features. */ + usesAngularFeatures: boolean; +} + export class UndecoratedClassesWithDecoratedFieldsTransform { private printer = ts.createPrinter(); private importManager = new ImportManager(this.getUpdateRecorder, this.printer); + private reflectionHost = new TypeScriptReflectionHost(this.typeChecker); + private partialEvaluator = new PartialEvaluator(this.reflectionHost, this.typeChecker, null); constructor( private typeChecker: ts.TypeChecker, @@ -28,7 +44,7 @@ export class UndecoratedClassesWithDecoratedFieldsTransform { * https://hackmd.io/vuQfavzfRG6KUCtU7oK_EA */ migrate(sourceFiles: ts.SourceFile[]) { - this._findUndecoratedDirectives(sourceFiles).forEach(node => { + this._findUndecoratedAbstractDirectives(sourceFiles).forEach(node => { const sourceFile = node.getSourceFile(); const recorder = this.getUpdateRecorder(sourceFile); const directiveExpr = @@ -42,54 +58,96 @@ export class UndecoratedClassesWithDecoratedFieldsTransform { /** Records all changes that were made in the import manager. */ recordChanges() { this.importManager.recordChanges(); } - /** Finds undecorated directives in the specified source files. */ - private _findUndecoratedDirectives(sourceFiles: ts.SourceFile[]) { - const typeChecker = this.typeChecker; - const undecoratedDirectives = new Set(); + /** Finds undecorated abstract directives in the specified source files. */ + private _findUndecoratedAbstractDirectives(sourceFiles: ts.SourceFile[]) { + const result = new Set(); const undecoratedClasses = new Set(); - const decoratedDirectives = new WeakSet(); + const nonAbstractDirectives = new WeakSet(); + const abstractDirectives = new WeakSet(); const visitNode = (node: ts.Node) => { node.forEachChild(visitNode); if (!ts.isClassDeclaration(node)) { return; } - const ngDecorators = node.decorators && getAngularDecorators(typeChecker, node.decorators); - const isDirectiveOrComponent = ngDecorators !== undefined && - ngDecorators.some(({name}) => name === 'Directive' || name === 'Component'); + const {isDirectiveOrComponent, isAbstractDirective, usesAngularFeatures} = + this._analyzeClassDeclaration(node); if (isDirectiveOrComponent) { - decoratedDirectives.add(node); - } else { - if (this._hasAngularDecoratedClassMember(node)) { - undecoratedDirectives.add(node); + if (isAbstractDirective) { + abstractDirectives.add(node); } else { - undecoratedClasses.add(node); + nonAbstractDirectives.add(node); } + } else if (usesAngularFeatures) { + abstractDirectives.add(node); + result.add(node); + } else { + undecoratedClasses.add(node); } }; sourceFiles.forEach(sourceFile => sourceFile.forEachChild(visitNode)); - // We collected all class declarations that use Angular features but are not decorated. For - // such undecorated directives, the derived classes also need to be migrated. To achieve this, - // we walk through all undecorated classes and mark those which extend from an undecorated - // directive as undecorated directive too. + // We collected all undecorated class declarations which inherit from abstract directives. + // For such abstract directives, the derived classes also need to be migrated. undecoratedClasses.forEach(node => { for (const {node: baseClass} of findBaseClassDeclarations(node, this.typeChecker)) { - // If the undecorated class inherits from a decorated directive, skip the current class. - // We do this because undecorated classes which inherit from directives/components are - // handled in the `undecorated-classes-with-di` migration which copies inherited metadata. - if (decoratedDirectives.has(baseClass)) { + // If the undecorated class inherits from a non-abstract directive, skip the current + // class. We do this because undecorated classes which inherit metadata from non-abstract + // directives are handle in the `undecorated-classes-with-di` migration that copies + // inherited metadata into an explicit decorator. + if (nonAbstractDirectives.has(baseClass)) { break; - } else if (undecoratedDirectives.has(baseClass)) { - undecoratedDirectives.add(node); - undecoratedClasses.delete(node); + } else if (abstractDirectives.has(baseClass)) { + result.add(node); break; } } }); - return undecoratedDirectives; + return result; + } + + /** + * Analyzes the given class declaration by determining whether the class + * is a directive, is an abstract directive, or uses Angular features. + */ + private _analyzeClassDeclaration(node: ts.ClassDeclaration): AnalyzedClass { + const ngDecorators = node.decorators && getAngularDecorators(this.typeChecker, node.decorators); + const usesAngularFeatures = this._hasAngularDecoratedClassMember(node); + if (ngDecorators === undefined || ngDecorators.length === 0) { + return {isDirectiveOrComponent: false, isAbstractDirective: false, usesAngularFeatures}; + } + const directiveDecorator = ngDecorators.find(({name}) => name === 'Directive'); + const componentDecorator = ngDecorators.find(({name}) => name === 'Component'); + const isAbstractDirective = + directiveDecorator !== undefined && this._isAbstractDirective(directiveDecorator); + return { + isDirectiveOrComponent: !!directiveDecorator || !!componentDecorator, + isAbstractDirective, + usesAngularFeatures, + }; + } + + /** + * Checks whether the given decorator resolves to an abstract directive. An directive is + * considered "abstract" if there is no selector specified. + */ + private _isAbstractDirective({node}: NgDecorator): boolean { + const metadataArgs = node.expression.arguments; + if (metadataArgs.length === 0) { + return true; + } + const metadataExpr = unwrapExpression(metadataArgs[0]); + if (!ts.isObjectLiteralExpression(metadataExpr)) { + return false; + } + const metadata = reflectObjectLiteral(metadataExpr); + if (!metadata.has('selector')) { + return false; + } + const selector = this.partialEvaluator.evaluate(metadata.get('selector') !); + return selector == null; } private _hasAngularDecoratedClassMember(node: ts.ClassDeclaration): boolean { diff --git a/packages/core/schematics/test/undecorated_classes_with_decorated_fields_migration_spec.ts b/packages/core/schematics/test/undecorated_classes_with_decorated_fields_migration_spec.ts index c141898803..27c7ac65f2 100644 --- a/packages/core/schematics/test/undecorated_classes_with_decorated_fields_migration_spec.ts +++ b/packages/core/schematics/test/undecorated_classes_with_decorated_fields_migration_spec.ts @@ -259,6 +259,39 @@ describe('Undecorated classes with decorated fields migration', () => { expect(fileContent).toMatch(/}\s+export class MyCompWrapped/); }); + it('should add @Directive to derived undecorated classes of abstract directives', async() => { + writeFile('/index.ts', ` + import { Input, Directive, NgModule } from '@angular/core'; + + @Directive() + export class Base { + // ... + } + + export class DerivedA extends Base {} + export class DerivedB extends DerivedA {} + export class DerivedC extends DerivedB {} + + @Directive({selector: 'my-comp'}) + export class MyComp extends DerivedC {} + + export class MyCompWrapped extends MyComp {} + + @NgModule({declarations: [MyComp, MyCompWrapped]}) + export class AppModule {} + `); + + await runMigration(); + const fileContent = tree.readContent('/index.ts'); + expect(fileContent).toContain(`import { Input, Directive, NgModule } from '@angular/core';`); + expect(fileContent).toMatch(/core';\s+@Directive\(\)\s+export class Base/); + expect(fileContent).toMatch(/@Directive\(\)\s+export class DerivedA/); + expect(fileContent).toMatch(/@Directive\(\)\s+export class DerivedB/); + expect(fileContent).toMatch(/@Directive\(\)\s+export class DerivedC/); + expect(fileContent).toMatch(/}\s+@Directive\(\{selector: 'my-comp'}\)\s+export class MyComp/); + expect(fileContent).toMatch(/}\s+export class MyCompWrapped/); + }); + function writeFile(filePath: string, contents: string) { host.sync.write(normalize(filePath), virtualFs.stringToFileBuffer(contents)); } diff --git a/packages/core/schematics/utils/typescript/functions.ts b/packages/core/schematics/utils/typescript/functions.ts index 3e32af4132..b8cd376588 100644 --- a/packages/core/schematics/utils/typescript/functions.ts +++ b/packages/core/schematics/utils/typescript/functions.ts @@ -17,11 +17,11 @@ export function isFunctionLikeDeclaration(node: ts.Node): node is ts.FunctionLik /** * Unwraps a given expression TypeScript node. Expressions can be wrapped within multiple - * parentheses. e.g. "(((({exp}))))()". The function should return the TypeScript node - * referring to the inner expression. e.g "exp". + * parentheses or as expression. e.g. "(((({exp}))))()". The function should return the + * TypeScript node referring to the inner expression. e.g "exp". */ export function unwrapExpression(node: ts.Expression | ts.ParenthesizedExpression): ts.Expression { - if (ts.isParenthesizedExpression(node)) { + if (ts.isParenthesizedExpression(node) || ts.isAsExpression(node)) { return unwrapExpression(node.expression); } else { return node;