From 2e5e1dd5f54a06d342e460f32dfcbad1549f57a7 Mon Sep 17 00:00:00 2001 From: JoostK Date: Sun, 20 Oct 2019 23:28:00 +0200 Subject: [PATCH] refactor(ngcc): rework undecorated parent migration (#33362) Previously, the (currently disabled) undecorated parent migration in ngcc would produce errors when a base class could not be determined statically or when a class extends from a class in another package. This is not ideal, as it would cause the library to fail compilation without a workaround, whereas those problems are not guaranteed to cause issues. Additionally, inheritance chains were not handled. This commit reworks the migration to address these limitations. PR Close #33362 --- .../undecorated_parent_migration.ts | 65 +++++++----- .../compiler-cli/ngcc/src/migrations/utils.ts | 10 +- .../undecorated_parent_migration_spec.ts | 100 ++++++++++++++++-- .../src/ngtsc/diagnostics/src/code.ts | 11 -- 4 files changed, 134 insertions(+), 52 deletions(-) diff --git a/packages/compiler-cli/ngcc/src/migrations/undecorated_parent_migration.ts b/packages/compiler-cli/ngcc/src/migrations/undecorated_parent_migration.ts index 921242983fe6e..4b77e3eec4aa3 100644 --- a/packages/compiler-cli/ngcc/src/migrations/undecorated_parent_migration.ts +++ b/packages/compiler-cli/ngcc/src/migrations/undecorated_parent_migration.ts @@ -6,12 +6,14 @@ * found in the LICENSE file at https://angular.io/license */ import * as ts from 'typescript'; -import {ErrorCode, makeDiagnostic} from '../../../src/ngtsc/diagnostics'; + +import {Reference} from '../../../src/ngtsc/imports'; import {ClassDeclaration} from '../../../src/ngtsc/reflection'; -import {isRelativePath} from '../utils'; + import {Migration, MigrationHost} from './migration'; import {createDirectiveDecorator, hasConstructor, hasDirectiveDecorator, isClassDeclaration} from './utils'; + /** * Ensure that the parents of directives and components that have no constructor are also decorated * as a `Directive`. @@ -59,36 +61,49 @@ export class UndecoratedParentMigration implements Migration { } // Only interested in `clazz` if it inherits from a base class. - const baseClassExpr = host.reflectionHost.getBaseClassExpression(clazz); - if (baseClassExpr === null) { - return null; - } + let baseClazzRef = determineBaseClass(clazz, host); + while (baseClazzRef !== null) { + const baseClazz = baseClazzRef.node; - if (!ts.isIdentifier(baseClassExpr)) { - return makeDiagnostic( - ErrorCode.NGCC_MIGRATION_EXTERNAL_BASE_CLASS, baseClassExpr, - `${clazz.name.text} class has a dynamic base class ${baseClassExpr.getText()}, so it is not possible to migrate.`); - } + // Do not proceed if the base class already has a decorator, or is not in scope of the + // entry-point that is currently being compiled. + if (hasDirectiveDecorator(host, baseClazz) || !host.isInScope(baseClazz)) { + break; + } - const baseClazz = host.reflectionHost.getDeclarationOfIdentifier(baseClassExpr) !.node; - if (baseClazz === null || !isClassDeclaration(baseClazz)) { - return null; - } + // Inject an `@Directive()` decorator for the base class. + host.injectSyntheticDecorator(baseClazz, createDirectiveDecorator(baseClazz)); - // Only interested in this base class if it doesn't have a `Directive` or `Component` decorator. - if (hasDirectiveDecorator(host, baseClazz)) { - return null; - } + // If the base class has a constructor, there's no need to continue walking up the + // inheritance chain. The injected decorator ensures that a factory is generated that does + // not delegate to the base class. + if (hasConstructor(host, baseClazz)) { + break; + } - const importInfo = host.reflectionHost.getImportOfIdentifier(baseClassExpr); - if (importInfo !== null && !isRelativePath(importInfo.from)) { - return makeDiagnostic( - ErrorCode.NGCC_MIGRATION_EXTERNAL_BASE_CLASS, baseClassExpr, - 'The base class was imported from an external entry-point so we cannot add a directive to it.'); + // Continue with another level of class inheritance. + baseClazzRef = determineBaseClass(baseClazz, host); } - host.injectSyntheticDecorator(baseClazz, createDirectiveDecorator(baseClazz)); + return null; + } +} +/** + * Computes a reference to the base class, or `null` if the class has no base class or if it could + * not be statically determined. + */ +function determineBaseClass( + clazz: ClassDeclaration, host: MigrationHost): Reference|null { + const baseClassExpr = host.reflectionHost.getBaseClassExpression(clazz); + if (baseClassExpr === null) { return null; } + + const baseClass = host.evaluator.evaluate(baseClassExpr); + if (!(baseClass instanceof Reference) || !isClassDeclaration(baseClass.node as ts.Declaration)) { + return null; + } + + return baseClass as Reference; } diff --git a/packages/compiler-cli/ngcc/src/migrations/utils.ts b/packages/compiler-cli/ngcc/src/migrations/utils.ts index a305986de0772..e988dde29bc86 100644 --- a/packages/compiler-cli/ngcc/src/migrations/utils.ts +++ b/packages/compiler-cli/ngcc/src/migrations/utils.ts @@ -19,7 +19,8 @@ export function isClassDeclaration(clazz: ts.Declaration): clazz is ClassDeclara * Returns true if the `clazz` is decorated as a `Directive` or `Component`. */ export function hasDirectiveDecorator(host: MigrationHost, clazz: ClassDeclaration): boolean { - return host.metadata.getDirectiveMetadata(new Reference(clazz)) !== null; + const ref = new Reference(clazz); + return host.metadata.getDirectiveMetadata(ref) !== null; } /** @@ -33,18 +34,13 @@ export function hasConstructor(host: MigrationHost, clazz: ClassDeclaration): bo * Create an empty `Directive` decorator that will be associated with the `clazz`. */ export function createDirectiveDecorator(clazz: ClassDeclaration): Decorator { - const selectorArg = ts.createObjectLiteral([ - // TODO: At the moment ngtsc does not accept a directive with no selector - ts.createPropertyAssignment('selector', ts.createStringLiteral('NGCC_DUMMY')), - ]); - return { name: 'Directive', identifier: null, import: {name: 'Directive', from: '@angular/core'}, node: null, synthesizedFor: clazz.name, - args: [reifySourceFile(selectorArg)], + args: [], }; } diff --git a/packages/compiler-cli/ngcc/test/migrations/undecorated_parent_migration_spec.ts b/packages/compiler-cli/ngcc/test/migrations/undecorated_parent_migration_spec.ts index 065ea13612b83..74536c1b9085a 100644 --- a/packages/compiler-cli/ngcc/test/migrations/undecorated_parent_migration_spec.ts +++ b/packages/compiler-cli/ngcc/test/migrations/undecorated_parent_migration_spec.ts @@ -26,19 +26,20 @@ runInEachFileSystem(() => { }); it('should ignore undecorated classes', () => { - const {program, analysis} = setUpAndAnalyzeProgram([{ + const {program, analysis, errors} = setUpAndAnalyzeProgram([{ name: INDEX_FILENAME, contents: ` export class DerivedClass extends BaseClass {} export class BaseClass {} ` }]); + expect(errors).toEqual([]); const file = analysis.get(program.getSourceFile(INDEX_FILENAME) !); expect(file).toBeUndefined(); }); it('should ignore an undecorated base class if the derived class has a constructor', () => { - const {program, analysis} = setUpAndAnalyzeProgram([{ + const {program, analysis, errors} = setUpAndAnalyzeProgram([{ name: INDEX_FILENAME, contents: ` import {Directive, ViewContainerRef} from '@angular/core'; @@ -54,6 +55,7 @@ runInEachFileSystem(() => { export class BaseClass {} ` }]); + expect(errors).toEqual([]); const file = analysis.get(program.getSourceFile(INDEX_FILENAME) !) !; expect(file.compiledClasses.find(c => c.name === 'DerivedClass')).toBeDefined(); expect(file.compiledClasses.find(c => c.name === 'BaseClass')).toBeUndefined(); @@ -61,7 +63,7 @@ runInEachFileSystem(() => { it('should add a decorator to an undecorated base class if the derived class is a Directive with no constructor', () => { - const {program, analysis} = setUpAndAnalyzeProgram([{ + const {program, analysis, errors} = setUpAndAnalyzeProgram([{ name: INDEX_FILENAME, contents: ` import {Directive, ViewContainerRef} from '@angular/core'; @@ -78,20 +80,96 @@ runInEachFileSystem(() => { ]; ` }]); + expect(errors).toEqual([]); const file = analysis.get(program.getSourceFile(INDEX_FILENAME) !) !; expect(file.compiledClasses.find(c => c.name === 'DerivedClass')).toBeDefined(); const baseClass = file.compiledClasses.find(c => c.name === 'BaseClass') !; expect(baseClass.decorators !.length).toEqual(1); const decorator = baseClass.decorators ![0]; expect(decorator.name).toEqual('Directive'); + expect(decorator.identifier).toBeNull('The decorator must be synthesized'); expect(decorator.import).toEqual({from: '@angular/core', name: 'Directive'}); - expect(decorator.args !.length).toEqual(1); + expect(decorator.args !.length).toEqual(0); + }); + + it('should not add a decorator to a base class that is already decorated', () => { + const {program, analysis, errors} = setUpAndAnalyzeProgram([{ + name: INDEX_FILENAME, + contents: ` + import {Directive, ViewContainerRef} from '@angular/core'; + export class DerivedClass extends BaseClass { + } + DerivedClass.decorators = [ + { type: Directive, args: [{ selector: '[dir]' }] } + ]; + export class BaseClass { + constructor(private vcr: ViewContainerRef) {} + } + BaseClass.decorators = [ + { type: Directive, args: [] } + ]; + BaseClass.ctorParameters = () => [ + { type: ViewContainerRef, } + ]; + ` + }]); + expect(errors).toEqual([]); + const file = analysis.get(program.getSourceFile(INDEX_FILENAME) !) !; + expect(file.compiledClasses.find(c => c.name === 'DerivedClass')).toBeDefined(); + const baseClass = file.compiledClasses.find(c => c.name === 'BaseClass') !; + expect(baseClass.decorators !.length).toEqual(1); + const decorator = baseClass.decorators ![0]; + expect(decorator.name).toEqual('Directive'); + expect(decorator.identifier).not.toBeNull('The decorator must not be synthesized'); + }); + + it('should add decorators to all classes in an inheritance chain until a constructor is found', + () => { + const {program, analysis, errors} = setUpAndAnalyzeProgram([{ + name: INDEX_FILENAME, + contents: ` + import {Directive, ViewContainerRef} from '@angular/core'; + export class DerivedClass extends IntermediateClass { + } + DerivedClass.decorators = [ + { type: Directive, args: [{ selector: '[dir]' }] } + ]; + export class IntermediateClass extends BaseClass {} + export class BaseClass extends RealBaseClass { + constructor(private vcr: ViewContainerRef) {} + } + BaseClass.ctorParameters = () => [ + { type: ViewContainerRef, } + ]; + export class RealBaseClass {} + ` + }]); + expect(errors).toEqual([]); + const file = analysis.get(program.getSourceFile(INDEX_FILENAME) !) !; + expect(file.compiledClasses.find(c => c.name === 'DerivedClass')).toBeDefined(); + expect(file.compiledClasses.find(c => c.name === 'RealBaseClass')).toBeUndefined(); + + const intermediateClass = file.compiledClasses.find(c => c.name === 'IntermediateClass') !; + expect(intermediateClass.decorators !.length).toEqual(1); + const intermediateDecorator = intermediateClass.decorators ![0]; + expect(intermediateDecorator.name).toEqual('Directive'); + expect(intermediateDecorator.identifier).toBeNull('The decorator must be synthesized'); + expect(intermediateDecorator.import).toEqual({from: '@angular/core', name: 'Directive'}); + expect(intermediateDecorator.args !.length).toEqual(0); + + const baseClass = file.compiledClasses.find(c => c.name === 'BaseClass') !; + expect(baseClass.decorators !.length).toEqual(1); + const baseDecorator = baseClass.decorators ![0]; + expect(baseDecorator.name).toEqual('Directive'); + expect(baseDecorator.identifier).toBeNull('The decorator must be synthesized'); + expect(baseDecorator.import).toEqual({from: '@angular/core', name: 'Directive'}); + expect(baseDecorator.args !.length).toEqual(0); }); it('should handle the base class being in a different file (same package) as the derived class', () => { const BASE_FILENAME = _('/node_modules/test-package/base.js'); - const {program, analysis} = setUpAndAnalyzeProgram([ + const {program, analysis, errors} = setUpAndAnalyzeProgram([ { name: INDEX_FILENAME, contents: ` @@ -116,18 +194,20 @@ runInEachFileSystem(() => { ` } ]); + expect(errors).toEqual([]); const file = analysis.get(program.getSourceFile(BASE_FILENAME) !) !; const baseClass = file.compiledClasses.find(c => c.name === 'BaseClass') !; expect(baseClass.decorators !.length).toEqual(1); const decorator = baseClass.decorators ![0]; expect(decorator.name).toEqual('Directive'); + expect(decorator.identifier).toBeNull('The decorator must be synthesized'); expect(decorator.import).toEqual({from: '@angular/core', name: 'Directive'}); - expect(decorator.args !.length).toEqual(1); + expect(decorator.args !.length).toEqual(0); }); - it('should error if the base class being is a different package from the derived class', () => { + it('should skip the base class if it is in a different package from the derived class', () => { const BASE_FILENAME = _('/node_modules/other-package/index.js'); - const {errors} = setUpAndAnalyzeProgram([ + const {program, analysis, errors} = setUpAndAnalyzeProgram([ { name: INDEX_FILENAME, contents: ` @@ -152,7 +232,9 @@ runInEachFileSystem(() => { ` } ]); - expect(errors.length).toEqual(1); + expect(errors).toEqual([]); + const file = analysis.get(program.getSourceFile(BASE_FILENAME) !); + expect(file).toBeUndefined(); }); }); diff --git a/packages/compiler-cli/src/ngtsc/diagnostics/src/code.ts b/packages/compiler-cli/src/ngtsc/diagnostics/src/code.ts index c3528a9a9c46a..cbad3b5f84b8d 100644 --- a/packages/compiler-cli/src/ngtsc/diagnostics/src/code.ts +++ b/packages/compiler-cli/src/ngtsc/diagnostics/src/code.ts @@ -75,17 +75,6 @@ export enum ErrorCode { */ NGCC_MIGRATION_DECORATOR_INJECTION_ERROR = 7001, - /** - * Raised when ngcc tries to decorate a base class that was imported from outside the package. - */ - NGCC_MIGRATION_EXTERNAL_BASE_CLASS = 7002, - - /** - * Raised when ngcc tries to migrate a class that is extended from a dynamic base class - * expression. - */ - NGCC_MIGRATION_DYNAMIC_BASE_CLASS = 7003, - /** * An element name failed validation against the DOM schema. */