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;