diff --git a/packages/core/schematics/migrations/undecorated-classes-with-decorated-fields/README.md b/packages/core/schematics/migrations/undecorated-classes-with-decorated-fields/README.md index 0fdec70e96..2c89d7b2b8 100644 --- a/packages/core/schematics/migrations/undecorated-classes-with-decorated-fields/README.md +++ b/packages/core/schematics/migrations/undecorated-classes-with-decorated-fields/README.md @@ -1,7 +1,11 @@ ## Undecorated classes with decorated fields migration -Automatically adds a `Directive` decorator to undecorated classes that have fields with Angular -decorators. Also adds the relevant imports, if necessary. +Automatically adds a `Directive` decorator to undecorated classes that use Angular features. A +class is considered using Angular features if a class member is decorated (e.g. `@Input()`), or +if the class defines any lifecycle hooks. + +This matches the undecorated classes compatibility logic in ngtsc that will be removed +as part of v10 so that the new mental model is enforced. #### Before ```ts diff --git a/packages/core/schematics/migrations/undecorated-classes-with-decorated-fields/google3/tslint_update_recorder.ts b/packages/core/schematics/migrations/undecorated-classes-with-decorated-fields/google3/tslint_update_recorder.ts index 311bdb15e8..e35bbf8ae0 100644 --- a/packages/core/schematics/migrations/undecorated-classes-with-decorated-fields/google3/tslint_update_recorder.ts +++ b/packages/core/schematics/migrations/undecorated-classes-with-decorated-fields/google3/tslint_update_recorder.ts @@ -16,6 +16,12 @@ export class TslintUpdateRecorder implements UpdateRecorder { constructor(private ruleName: string, private sourceFile: ts.SourceFile) {} + addClassTodo(node: ts.ClassDeclaration, message: string) { + this.failures.push(new RuleFailure( + this.sourceFile, node.getStart(), 0, message, this.ruleName, + Replacement.appendText(node.getStart(), `// TODO: ${message}`))); + } + /** Adds the specified decorator to the given class declaration. */ addClassDecorator(node: ts.ClassDeclaration, decoratorText: string) { // Adding a decorator should be the last replacement. Replacements/rule failures diff --git a/packages/core/schematics/migrations/undecorated-classes-with-decorated-fields/index.ts b/packages/core/schematics/migrations/undecorated-classes-with-decorated-fields/index.ts index 7b9698b1db..eb30af27b7 100644 --- a/packages/core/schematics/migrations/undecorated-classes-with-decorated-fields/index.ts +++ b/packages/core/schematics/migrations/undecorated-classes-with-decorated-fields/index.ts @@ -6,7 +6,7 @@ * found in the LICENSE file at https://angular.io/license */ -import {Rule, SchematicsException, Tree,} from '@angular-devkit/schematics'; +import {Rule, SchematicContext, SchematicsException, Tree,} from '@angular-devkit/schematics'; import {relative} from 'path'; import * as ts from 'typescript'; @@ -21,10 +21,11 @@ import {UpdateRecorder} from './update_recorder'; * https://hackmd.io/vuQfavzfRG6KUCtU7oK_EA */ export default function(): Rule { - return (tree: Tree) => { + return (tree: Tree, ctx: SchematicContext) => { const {buildPaths, testPaths} = getProjectTsConfigPaths(tree); const basePath = process.cwd(); const allPaths = [...buildPaths, ...testPaths]; + const failures: string[] = []; if (!allPaths.length) { throw new SchematicsException( @@ -32,12 +33,20 @@ export default function(): Rule { } for (const tsconfigPath of allPaths) { - runUndecoratedClassesMigration(tree, tsconfigPath, basePath); + failures.push(...runUndecoratedClassesMigration(tree, tsconfigPath, basePath)); + } + + if (failures.length) { + ctx.logger.info('Could not migrate all undecorated classes that use Angular features.'); + ctx.logger.info('Please manually fix the following failures:'); + failures.forEach(message => ctx.logger.warn(`⮑ ${message}`)); } }; } -function runUndecoratedClassesMigration(tree: Tree, tsconfigPath: string, basePath: string) { +function runUndecoratedClassesMigration( + tree: Tree, tsconfigPath: string, basePath: string): string[] { + const failures: string[] = []; const {program} = createMigrationProgram(tree, tsconfigPath, basePath); const typeChecker = program.getTypeChecker(); const sourceFiles = program.getSourceFiles().filter( @@ -47,7 +56,13 @@ function runUndecoratedClassesMigration(tree: Tree, tsconfigPath: string, basePa new UndecoratedClassesWithDecoratedFieldsTransform(typeChecker, getUpdateRecorder); // Migrate all source files in the project. - transform.migrate(sourceFiles); + transform.migrate(sourceFiles).forEach(({node, message}) => { + const nodeSourceFile = node.getSourceFile(); + const relativeFilePath = relative(basePath, nodeSourceFile.fileName); + const {line, character} = + ts.getLineAndCharacterOfPosition(node.getSourceFile(), node.getStart()); + failures.push(`${relativeFilePath}@${line + 1}:${character + 1}: ${message}`); + }); // Record the changes collected in the import manager. transform.recordChanges(); @@ -57,6 +72,8 @@ function runUndecoratedClassesMigration(tree: Tree, tsconfigPath: string, basePa // file in order to avoid shifted character offsets. updateRecorders.forEach(recorder => recorder.commitUpdate()); + return failures; + /** Gets the update recorder for the specified source file. */ function getUpdateRecorder(sourceFile: ts.SourceFile): UpdateRecorder { if (updateRecorders.has(sourceFile)) { @@ -64,6 +81,9 @@ function runUndecoratedClassesMigration(tree: Tree, tsconfigPath: string, basePa } const treeRecorder = tree.beginUpdate(relative(basePath, sourceFile.fileName)); const recorder: UpdateRecorder = { + addClassTodo(node: ts.ClassDeclaration, message: string) { + treeRecorder.insertRight(node.getStart(), `// TODO: ${message}\n`); + }, addClassDecorator(node: ts.ClassDeclaration, text: string) { // New imports should be inserted at the left while decorators should be inserted // at the right in order to ensure that imports are inserted before the decorator 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 012ff0c43a..323c96ec25 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 @@ -14,9 +14,40 @@ import {ImportManager} from '../../utils/import_manager'; import {getAngularDecorators, NgDecorator} from '../../utils/ng_decorators'; import {findBaseClassDeclarations} from '../../utils/typescript/find_base_classes'; import {unwrapExpression} from '../../utils/typescript/functions'; +import {getPropertyNameText} from '../../utils/typescript/property_name'; import {UpdateRecorder} from './update_recorder'; +/** + * Set of known decorators that indicate that the current class needs a directive + * definition. These decorators are always specific to directives. + */ +const DIRECTIVE_FIELD_DECORATORS = new Set([ + 'Input', 'Output', 'ViewChild', 'ViewChildren', 'ContentChild', 'ContentChildren', 'HostBinding', + 'HostListener' +]); + +/** + * Set of known lifecycle hooks that indicate that the current class needs a directive + * definition. These lifecycle hooks are always specific to directives. + */ +const DIRECTIVE_LIFECYCLE_HOOKS = new Set([ + 'ngOnChanges', 'ngOnInit', 'ngDoCheck', 'ngAfterViewInit', 'ngAfterViewChecked', + 'ngAfterContentInit', 'ngAfterContentChecked' +]); + +/** + * Set of known lifecycle hooks that indicate that a given class uses Angular + * features, but it's ambiguous whether it is a directive or service. + */ +const AMBIGUOUS_LIFECYCLE_HOOKS = new Set(['ngOnDestroy']); + +/** Describes how a given class is used in the context of Angular. */ +enum ClassKind { + DIRECTIVE, + AMBIGUOUS, + UNKNOWN, +} /** Analyzed class declaration. */ interface AnalyzedClass { @@ -24,8 +55,13 @@ interface AnalyzedClass { isDirectiveOrComponent: boolean; /** Whether the class is an abstract directive. */ isAbstractDirective: boolean; - /** Whether the class uses any Angular features. */ - usesAngularFeatures: boolean; + /** Kind of the given class in terms of Angular. */ + kind: ClassKind; +} + +interface AnalysisFailure { + node: ts.Node; + message: string; } export class UndecoratedClassesWithDecoratedFieldsTransform { @@ -40,11 +76,15 @@ export class UndecoratedClassesWithDecoratedFieldsTransform { /** * Migrates the specified source files. The transform adds the abstract `@Directive` - * decorator to classes that have Angular field decorators but are not decorated. - * https://hackmd.io/vuQfavzfRG6KUCtU7oK_EA + * decorator to undecorated classes that use Angular features. Class members which + * are decorated with any Angular decorator, or class members for lifecycle hooks are + * indicating that a given class uses Angular features. https://hackmd.io/vuQfavzfRG6KUCtU7oK_EA */ - migrate(sourceFiles: ts.SourceFile[]) { - this._findUndecoratedAbstractDirectives(sourceFiles).forEach(node => { + migrate(sourceFiles: ts.SourceFile[]): AnalysisFailure[] { + const {result, ambiguous} = this._findUndecoratedAbstractDirectives(sourceFiles); + + + result.forEach(node => { const sourceFile = node.getSourceFile(); const recorder = this.getUpdateRecorder(sourceFile); const directiveExpr = @@ -53,6 +93,25 @@ export class UndecoratedClassesWithDecoratedFieldsTransform { recorder.addClassDecorator( node, this.printer.printNode(ts.EmitHint.Unspecified, decoratorExpr, sourceFile)); }); + + // Ambiguous classes clearly use Angular features, but the migration is unable to + // determine whether the class is used as directive, service or pipe. The migration + // could potentially determine the type by checking NgModule definitions or inheritance + // of other known declarations, but this is out of scope and a TODO/failure is sufficient. + return Array.from(ambiguous).reduce((failures, node) => { + const sourceFile = node.getSourceFile(); + const recorder = this.getUpdateRecorder(sourceFile); + + // Add a TODO to the class that uses Angular features but is not decorated. + recorder.addClassTodo(node, `Add Angular decorator.`); + + // Add an error for the class that will be printed in the `ng update` output. + return failures.concat({ + node, + message: 'Class uses Angular features but cannot be migrated automatically. Please ' + + 'add an appropriate Angular decorator.' + }); + }, [] as AnalysisFailure[]); } /** Records all changes that were made in the import manager. */ @@ -60,19 +119,24 @@ export class UndecoratedClassesWithDecoratedFieldsTransform { this.importManager.recordChanges(); } - /** Finds undecorated abstract directives in the specified source files. */ + /** + * Finds undecorated abstract directives in the specified source files. Also returns + * a set of undecorated classes which could not be detected as guaranteed abstract + * directives. Those are ambiguous and could be either Directive, Pipe or service. + */ private _findUndecoratedAbstractDirectives(sourceFiles: ts.SourceFile[]) { const result = new Set(); const undecoratedClasses = new Set(); const nonAbstractDirectives = new WeakSet(); const abstractDirectives = new WeakSet(); + const ambiguous = new Set(); const visitNode = (node: ts.Node) => { node.forEachChild(visitNode); if (!ts.isClassDeclaration(node)) { return; } - const {isDirectiveOrComponent, isAbstractDirective, usesAngularFeatures} = + const {isDirectiveOrComponent, isAbstractDirective, kind} = this._analyzeClassDeclaration(node); if (isDirectiveOrComponent) { if (isAbstractDirective) { @@ -80,10 +144,13 @@ export class UndecoratedClassesWithDecoratedFieldsTransform { } else { nonAbstractDirectives.add(node); } - } else if (usesAngularFeatures) { + } else if (kind === ClassKind.DIRECTIVE) { abstractDirectives.add(node); result.add(node); } else { + if (kind === ClassKind.AMBIGUOUS) { + ambiguous.add(node); + } undecoratedClasses.add(node); } }; @@ -96,18 +163,21 @@ export class UndecoratedClassesWithDecoratedFieldsTransform { for (const {node: baseClass} of findBaseClassDeclarations(node, this.typeChecker)) { // 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 + // directives are handled in the `undecorated-classes-with-di` migration that copies // inherited metadata into an explicit decorator. if (nonAbstractDirectives.has(baseClass)) { break; } else if (abstractDirectives.has(baseClass)) { result.add(node); + // In case the undecorated class previously could not be detected as directive, + // remove it from the ambiguous set as we now know that it's a guaranteed directive. + ambiguous.delete(node); break; } } }); - return result; + return {result, ambiguous}; } /** @@ -116,9 +186,9 @@ export class UndecoratedClassesWithDecoratedFieldsTransform { */ private _analyzeClassDeclaration(node: ts.ClassDeclaration): AnalyzedClass { const ngDecorators = node.decorators && getAngularDecorators(this.typeChecker, node.decorators); - const usesAngularFeatures = this._hasAngularDecoratedClassMember(node); + const kind = this._determineClassKind(node); if (ngDecorators === undefined || ngDecorators.length === 0) { - return {isDirectiveOrComponent: false, isAbstractDirective: false, usesAngularFeatures}; + return {isDirectiveOrComponent: false, isAbstractDirective: false, kind}; } const directiveDecorator = ngDecorators.find(({name}) => name === 'Directive'); const componentDecorator = ngDecorators.find(({name}) => name === 'Component'); @@ -127,7 +197,7 @@ export class UndecoratedClassesWithDecoratedFieldsTransform { return { isDirectiveOrComponent: !!directiveDecorator || !!componentDecorator, isAbstractDirective, - usesAngularFeatures, + kind, }; } @@ -152,8 +222,41 @@ export class UndecoratedClassesWithDecoratedFieldsTransform { return selector == null; } - private _hasAngularDecoratedClassMember(node: ts.ClassDeclaration): boolean { - return node.members.some( - m => m.decorators && getAngularDecorators(this.typeChecker, m.decorators).length !== 0); + /** + * Determines the kind of a given class in terms of Angular. The method checks + * whether the given class has members that indicate the use of Angular features. + * e.g. lifecycle hooks or decorated members like `@Input` or `@Output` are + * considered Angular features.. + */ + private _determineClassKind(node: ts.ClassDeclaration): ClassKind { + let usage = ClassKind.UNKNOWN; + + for (const member of node.members) { + const propertyName = member.name !== undefined ? getPropertyNameText(member.name) : null; + + // If the class declares any of the known directive lifecycle hooks, we can + // immediately exit the loop as the class is guaranteed to be a directive. + if (propertyName !== null && DIRECTIVE_LIFECYCLE_HOOKS.has(propertyName)) { + return ClassKind.DIRECTIVE; + } + + const ngDecorators = member.decorators !== undefined ? + getAngularDecorators(this.typeChecker, member.decorators) : + []; + for (const {name} of ngDecorators) { + if (DIRECTIVE_FIELD_DECORATORS.has(name)) { + return ClassKind.DIRECTIVE; + } + } + + // If the class declares any of the lifecycle hooks that do not guarantee that + // the given class is a directive, update the kind and continue looking for other + // members that would unveil a more specific kind (i.e. being a directive). + if (propertyName !== null && AMBIGUOUS_LIFECYCLE_HOOKS.has(propertyName)) { + usage = ClassKind.AMBIGUOUS; + } + } + + return usage; } } diff --git a/packages/core/schematics/migrations/undecorated-classes-with-decorated-fields/update_recorder.ts b/packages/core/schematics/migrations/undecorated-classes-with-decorated-fields/update_recorder.ts index 7568256c35..0fceb6e2b2 100644 --- a/packages/core/schematics/migrations/undecorated-classes-with-decorated-fields/update_recorder.ts +++ b/packages/core/schematics/migrations/undecorated-classes-with-decorated-fields/update_recorder.ts @@ -15,5 +15,6 @@ import {ImportManagerUpdateRecorder} from '../../utils/import_manager'; */ export interface UpdateRecorder extends ImportManagerUpdateRecorder { addClassDecorator(node: ts.ClassDeclaration, text: string): void; + addClassTodo(node: ts.ClassDeclaration, message: string): void; commitUpdate(): void; } 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 e82adc2462..1866a7dfa8 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 @@ -18,6 +18,7 @@ describe('Undecorated classes with decorated fields migration', () => { let tree: UnitTestTree; let tmpDirPath: string; let previousWorkingDir: string; + let warnings: string[]; beforeEach(() => { runner = new SchematicTestRunner('test', require.resolve('../migrations.json')); @@ -29,6 +30,13 @@ describe('Undecorated classes with decorated fields migration', () => { projects: {t: {architect: {build: {options: {tsConfig: './tsconfig.json'}}}}} })); + warnings = []; + runner.logger.subscribe(entry => { + if (entry.level === 'warn') { + warnings.push(entry.message); + } + }); + previousWorkingDir = shx.pwd(); tmpDirPath = getSystemPath(host.root); @@ -228,6 +236,45 @@ describe('Undecorated classes with decorated fields migration', () => { expect(tree.readContent('/index.ts')).toContain(`@Directive()\nexport class Base {`); }); + it('should migrate undecorated class that uses "ngOnChanges" lifecycle hook', + () => assertLifecycleHookMigrated('ngOnChanges')); + it('should migrate undecorated class that uses "ngOnInit" lifecycle hook', + () => assertLifecycleHookMigrated('ngOnInit')); + it('should migrate undecorated class that uses "ngDoCheck" lifecycle hook', + () => assertLifecycleHookMigrated('ngDoCheck')); + it('should migrate undecorated class that uses "ngAfterViewInit" lifecycle hook', + () => assertLifecycleHookMigrated('ngAfterViewInit')); + it('should migrate undecorated class that uses "ngAfterViewChecked" lifecycle hook', + () => assertLifecycleHookMigrated('ngAfterViewChecked')); + it('should migrate undecorated class that uses "ngAfterContentInit" lifecycle hook', + () => assertLifecycleHookMigrated('ngAfterContentInit')); + it('should migrate undecorated class that uses "ngAfterContentChecked" lifecycle hook', + () => assertLifecycleHookMigrated('ngAfterContentChecked')); + + it(`should report an error and add a TODO for undecorated classes that only define ` + + `the "ngOnDestroy" lifecycle hook`, + async () => { + writeFile('/index.ts', ` + import { Input } from '@angular/core'; + + export class SomeClassWithAngularFeatures { + ngOnDestroy() { + // noop for testing + } + } + `); + + await runMigration(); + + expect(warnings.length).toBe(1); + expect(warnings[0]) + .toMatch( + 'index.ts@4:7: Class uses Angular features but cannot be migrated automatically. ' + + 'Please add an appropriate Angular decorator.'); + expect(tree.readContent('/index.ts')) + .toMatch(/TODO: Add Angular decorator\.\nexport class SomeClassWithAngularFeatures {/); + }); + it('should add @Directive to undecorated derived classes of a migrated class', async () => { writeFile('/index.ts', ` import { Input, Directive, NgModule } from '@angular/core'; @@ -314,6 +361,22 @@ describe('Undecorated classes with decorated fields migration', () => { expect(error).toBe(null); }); + async function assertLifecycleHookMigrated(lifecycleHookName: string) { + writeFile('/index.ts', ` + import { Input } from '@angular/core'; + + export class SomeClassWithAngularFeatures { + ${lifecycleHookName}() { + // noop for testing + } + } + `); + + await runMigration(); + expect(tree.readContent('/index.ts')) + .toContain(`@Directive()\nexport class SomeClassWithAngularFeatures {`); + } + function writeFile(filePath: string, contents: string) { host.sync.write(normalize(filePath), virtualFs.stringToFileBuffer(contents)); }