Skip to content

Commit

Permalink
refactor(ngcc): rework undecorated parent migration (#33362)
Browse files Browse the repository at this point in the history 
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
  • Loading branch information
@JoostK @AndrewKushnir
JoostK authored and AndrewKushnir committed Oct 25, 2019
1 parent 3858b26 commit 2e5e1dd
Show file tree
Hide file tree
Showing 4 changed files with 134 additions and 52 deletions.
65 changes: 40 additions & 25 deletions packages/compiler-cli/ngcc/src/migrations/undecorated_parent_migration.ts
View file
Expand Up @@ -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`.
Expand Down Expand Up @@ -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<ClassDeclaration>|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<ClassDeclaration>;
}
10 changes: 3 additions & 7 deletions packages/compiler-cli/ngcc/src/migrations/utils.ts
View file
Expand Up @@ -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;
}

/**
Expand All @@ -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: [],
};
}

Expand Down
100 changes: 91 additions & 9 deletions packages/compiler-cli/ngcc/test/migrations/undecorated_parent_migration_spec.ts
View file
Expand Up @@ -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';
Expand All @@ -54,14 +55,15 @@ 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();
});

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';
Expand All @@ -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: `
Expand All @@ -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: `
Expand All @@ -152,7 +232,9 @@ runInEachFileSystem(() => {
`
}
]);
expect(errors.length).toEqual(1);
expect(errors).toEqual([]);
const file = analysis.get(program.getSourceFile(BASE_FILENAME) !);
expect(file).toBeUndefined();
});
});

Expand Down
11 changes: 0 additions & 11 deletions packages/compiler-cli/src/ngtsc/diagnostics/src/code.ts
View file
Expand Up @@ -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.
*/
Expand Down

0 comments on commit 2e5e1dd

Please sign in to comment.