Skip to content

Commit

Permalink
feat(ngcc): add a Migration for undecorated child classes
Browse files Browse the repository at this point in the history
  • Loading branch information
alxhub committed Oct 23, 2019
1 parent 52004c4 commit 0750fc8
Show file tree
Hide file tree
Showing 16 changed files with 394 additions and 24 deletions.
Expand Up @@ -21,6 +21,7 @@ import {CompileResult, DecoratorHandler, DetectResult, HandlerPrecedence} from '
import {NgccClassSymbol, NgccReflectionHost} from '../host/ngcc_host';
import {Migration} from '../migrations/migration';
import {MissingInjectableMigration} from '../migrations/missing_injectable_migration';
import {UndecoratedChildMigration} from '../migrations/undecorated_child_migration';
import {UndecoratedParentMigration} from '../migrations/undecorated_parent_migration';
import {EntryPointBundle} from '../packages/entry_point_bundle';
import {isDefined} from '../utils';
Expand All @@ -29,6 +30,7 @@ import {DefaultMigrationHost} from './migration_host';
import {AnalyzedClass, AnalyzedFile, CompiledClass, CompiledFile, DecorationAnalyses} from './types';
import {analyzeDecorators, isWithinPackage} from './util';


/**
* Simple class that resolves and loads files directly from the filesystem.
*/
Expand Down Expand Up @@ -104,7 +106,11 @@ export class DecorationAnalyzer {
this.reflectionHost, this.evaluator, this.metaRegistry, NOOP_DEFAULT_IMPORT_RECORDER,
this.isCore),
];
migrations: Migration[] = [new UndecoratedParentMigration(), new MissingInjectableMigration()];
migrations: Migration[] = [
new UndecoratedParentMigration(),
new UndecoratedChildMigration(),
new MissingInjectableMigration(),
];

constructor(
private fs: FileSystem, private bundle: EntryPointBundle,
Expand Down
7 changes: 4 additions & 3 deletions packages/compiler-cli/ngcc/src/analysis/migration_host.ts
Expand Up @@ -12,7 +12,7 @@ import {AbsoluteFsPath} from '../../../src/ngtsc/file_system';
import {MetadataReader} from '../../../src/ngtsc/metadata';
import {PartialEvaluator} from '../../../src/ngtsc/partial_evaluator';
import {ClassDeclaration, Decorator} from '../../../src/ngtsc/reflection';
import {DecoratorHandler} from '../../../src/ngtsc/transform';
import {DecoratorHandler, HandlerFlags} from '../../../src/ngtsc/transform';
import {NgccReflectionHost} from '../host/ngcc_host';
import {MigrationHost} from '../migrations/migration';

Expand All @@ -29,9 +29,10 @@ export class DefaultMigrationHost implements MigrationHost {
readonly evaluator: PartialEvaluator, private handlers: DecoratorHandler<any, any>[],
private entryPointPath: AbsoluteFsPath, private analyzedFiles: AnalyzedFile[]) {}

injectSyntheticDecorator(clazz: ClassDeclaration, decorator: Decorator): void {
injectSyntheticDecorator(clazz: ClassDeclaration, decorator: Decorator, flags?: HandlerFlags):
void {
const classSymbol = this.reflectionHost.getClassSymbol(clazz) !;
const newAnalyzedClass = analyzeDecorators(classSymbol, [decorator], this.handlers);
const newAnalyzedClass = analyzeDecorators(classSymbol, [decorator], this.handlers, flags);
if (newAnalyzedClass === null) {
return;
}
Expand Down
6 changes: 3 additions & 3 deletions packages/compiler-cli/ngcc/src/analysis/util.ts
Expand Up @@ -10,7 +10,7 @@ import * as ts from 'typescript';
import {isFatalDiagnosticError} from '../../../src/ngtsc/diagnostics';
import {AbsoluteFsPath, absoluteFromSourceFile, relative} from '../../../src/ngtsc/file_system';
import {Decorator} from '../../../src/ngtsc/reflection';
import {DecoratorHandler, DetectResult, HandlerPrecedence} from '../../../src/ngtsc/transform';
import {DecoratorHandler, DetectResult, HandlerFlags, HandlerPrecedence} from '../../../src/ngtsc/transform';
import {NgccClassSymbol} from '../host/ngcc_host';

import {AnalyzedClass, MatchingHandler} from './types';
Expand All @@ -21,7 +21,7 @@ export function isWithinPackage(packagePath: AbsoluteFsPath, sourceFile: ts.Sour

export function analyzeDecorators(
classSymbol: NgccClassSymbol, decorators: Decorator[] | null,
handlers: DecoratorHandler<any, any>[]): AnalyzedClass|null {
handlers: DecoratorHandler<any, any>[], flags?: HandlerFlags): AnalyzedClass|null {
const declaration = classSymbol.declaration.valueDeclaration;
const matchingHandlers = handlers
.map(handler => {
Expand Down Expand Up @@ -64,7 +64,7 @@ export function analyzeDecorators(
const allDiagnostics: ts.Diagnostic[] = [];
for (const {handler, detected} of detections) {
try {
const {analysis, diagnostics} = handler.analyze(declaration, detected.metadata);
const {analysis, diagnostics} = handler.analyze(declaration, detected.metadata, flags);
if (diagnostics !== undefined) {
allDiagnostics.push(...diagnostics);
}
Expand Down
6 changes: 5 additions & 1 deletion packages/compiler-cli/ngcc/src/migrations/migration.ts
Expand Up @@ -6,11 +6,14 @@
* found in the LICENSE file at https://angular.io/license
*/
import * as ts from 'typescript';

import {MetadataReader} from '../../../src/ngtsc/metadata';
import {PartialEvaluator} from '../../../src/ngtsc/partial_evaluator';
import {ClassDeclaration, Decorator} from '../../../src/ngtsc/reflection';
import {HandlerFlags} from '../../../src/ngtsc/transform';
import {NgccReflectionHost} from '../host/ngcc_host';


/**
* Implement this interface and add it to the `DecorationAnalyzer.migrations` collection to get ngcc
* to modify the analysis of the decorators in the program in order to migrate older code to work
Expand Down Expand Up @@ -41,7 +44,8 @@ export interface MigrationHost {
* @param clazz the class to receive the new decorator.
* @param decorator the decorator to inject.
*/
injectSyntheticDecorator(clazz: ClassDeclaration, decorator: Decorator): void;
injectSyntheticDecorator(clazz: ClassDeclaration, decorator: Decorator, flags?: HandlerFlags):
void;

/**
* Retrieves all decorators that are associated with the class, including synthetic decorators
Expand Down
@@ -0,0 +1,82 @@
/**
* @license
* Copyright Google Inc. All Rights Reserved.
*
* Use of this source code is governed by an MIT-style license that can be
* found in the LICENSE file at https://angular.io/license
*/

import * as ts from 'typescript';

import {readBaseClass} from '../../../src/ngtsc/annotations/src/util';
import {Reference} from '../../../src/ngtsc/imports';
import {ClassDeclaration} from '../../../src/ngtsc/reflection';
import {HandlerFlags} from '../../../src/ngtsc/transform';

import {Migration, MigrationHost} from './migration';
import {createComponentDecorator, createDirectiveDecorator, hasDirectiveDecorator, hasPipeDecorator} from './utils';

export class UndecoratedChildMigration implements Migration {
constructor() {}

apply(clazz: ClassDeclaration, host: MigrationHost): ts.Diagnostic|null {
// This migration looks at NgModules and considers the directives (and pipes) it declares.
// It verifies that these classes have decorators.
const moduleMeta = host.metadata.getNgModuleMetadata(new Reference(clazz));
if (moduleMeta === null) {
// Not an NgModule; don't care.
return null;
}

// Examine each of the declarations to see if it needs to be migrated.
for (const decl of moduleMeta.declarations) {
const diag = this.maybeMigrate(decl, host);
if (diag !== null) {
return diag;
}
}

return null;
}

maybeMigrate(ref: Reference<ClassDeclaration>, host: MigrationHost): ts.Diagnostic|null {
if (hasDirectiveDecorator(host, ref.node) || hasPipeDecorator(host, ref.node)) {
// Stop if one of the classes in the chain is actually decorated with @Directive.
console.error(ref.debugName, 'has decorator');
return null;
}

const baseRef = readBaseClass(ref.node, host.reflectionHost, host.evaluator);
if (baseRef === null) {
// Stop: can't migrate a class with no parent.
return null;
} else if (baseRef === 'dynamic') {
// Stop: can't migrate a class with an indeterminate parent.
return null;
}

// Apply the migration recursively, to handle inheritance chains.
this.maybeMigrate(baseRef, host);

// After the above call, `host.metadata` should have metadata for the base class, if indeed this
// is a directive inheritance chain.
const baseMeta = host.metadata.getDirectiveMetadata(baseRef);
if (baseMeta === null) {
// Stop: this isn't a directive inheritance chain after all.
return null;
}

// Otherwise, decorate the class with @Component() or @Directive(), as appropriate.
if (baseMeta.isComponent) {
host.injectSyntheticDecorator(
ref.node, createComponentDecorator(ref.node, baseMeta), HandlerFlags.FULL_INHERITANCE);
} else {
host.injectSyntheticDecorator(
ref.node, createDirectiveDecorator(ref.node, baseMeta), HandlerFlags.FULL_INHERITANCE);
}


// Success!
return null;
}
}
67 changes: 65 additions & 2 deletions packages/compiler-cli/ngcc/src/migrations/utils.ts
Expand Up @@ -23,6 +23,14 @@ export function hasDirectiveDecorator(host: MigrationHost, clazz: ClassDeclarati
return host.metadata.getDirectiveMetadata(ref) !== null || host.metadata.isAbstractDirective(ref);
}

/**
* Returns true if the `clazz` is decorated as a `Directive` or `Component`.
*/
export function hasPipeDecorator(host: MigrationHost, clazz: ClassDeclaration): boolean {
const ref = new Reference(clazz);
return host.metadata.getPipeMetadata(ref) !== null;
}

/**
* Returns true if the `clazz` has its own constructor function.
*/
Expand All @@ -33,13 +41,46 @@ 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 {
export function createDirectiveDecorator(
clazz: ClassDeclaration, metadata?: {selector: string, exportAs: string[] | null}): Decorator {
const args: ts.Expression[] = [];
if (metadata !== undefined) {
const metaArgs: ts.PropertyAssignment[] = [
property('selector', metadata.selector),
];
if (metadata.exportAs !== null) {
metaArgs.push(property('exportAs', metadata.exportAs));
}
args.push(reifySourceFile(ts.createObjectLiteral(metaArgs)));
}
return {
name: 'Directive',
identifier: null,
import: {name: 'Directive', from: '@angular/core'},
node: clazz.name, args,
};
}

/**
* Create an empty `Component` decorator that will be associated with the `clazz`.
*/
export function createComponentDecorator(
clazz: ClassDeclaration, metadata: {selector: string, exportAs: string[] | null}): Decorator {
const metaArgs: ts.PropertyAssignment[] = [
property('selector', metadata.selector),
property('template', ''),
];
if (metadata.exportAs !== null) {
metaArgs.push(property('exportAs', metadata.exportAs));
}
return {
name: 'Component',
identifier: null,
import: {name: 'Component', from: '@angular/core'},
node: clazz.name,
args: [],
args: [
reifySourceFile(ts.createObjectLiteral(metaArgs)),
],
};
}

Expand All @@ -55,3 +96,25 @@ export function createInjectableDecorator(clazz: ClassDeclaration): Decorator {
args: [],
};
}

function property(name: string, value: string | string[]): ts.PropertyAssignment {
if (typeof value === 'string') {
return ts.createPropertyAssignment(name, ts.createStringLiteral(value));
} else {
return ts.createPropertyAssignment(
name, ts.createArrayLiteral(value.map(v => ts.createStringLiteral(v))));
}
}
const EMPTY_SF = ts.createSourceFile('(empty)', '', ts.ScriptTarget.Latest);

function reifySourceFile(expr: ts.Expression): ts.Expression {
const printer = ts.createPrinter();
const exprText = printer.printNode(ts.EmitHint.Unspecified, expr, EMPTY_SF);
const sf = ts.createSourceFile(
'(synthetic)', `const expr = ${exprText};`, ts.ScriptTarget.Latest, true, ts.ScriptKind.JS);
const stmt = sf.statements[0];
if (!ts.isVariableStatement(stmt)) {
throw new Error(`Expected VariableStatement, got ${ts.SyntaxKind[stmt.kind]}`);
}
return stmt.declarationList.declarations[0].initializer !;
}
1 change: 1 addition & 0 deletions packages/compiler-cli/ngcc/test/BUILD.bazel
Expand Up @@ -56,6 +56,7 @@ ts_library(
"//packages/compiler-cli/src/ngtsc/testing",
"//packages/compiler-cli/test/helpers",
"@npm//rxjs",
"@npm//typescript",
],
)

Expand Down
86 changes: 86 additions & 0 deletions packages/compiler-cli/ngcc/test/integration/ngcc_spec.ts
Expand Up @@ -19,6 +19,7 @@ import {EntryPointJsonProperty, EntryPointPackageJson, SUPPORTED_FORMAT_PROPERTI
import {Transformer} from '../../src/packages/transformer';
import {DirectPackageJsonUpdater, PackageJsonUpdater} from '../../src/writing/package_json_updater';
import {MockLogger} from '../helpers/mock_logger';
import {genNodeModules} from './util';


const testFiles = loadStandardTestFiles({fakeCore: false, rxjs: true});
Expand Down Expand Up @@ -752,6 +753,91 @@ runInEachFileSystem(() => {
});
});

describe('undecorated child class migration', () => {
it('should generate a directive definition with CopyDefinitionFeature for an undecorated child directive',
() => {
genNodeModules({
'test-package': {
'/index.ts': `
import {Directive, NgModule} from '@angular/core';
@Directive({
selector: '[base]',
})
export class BaseDir {}
export class DerivedDir extends BaseDir {}
@NgModule({
declarations: [DerivedDir],
})
export class Module {}
`,
},
});

mainNgcc({
basePath: '/node_modules',
targetEntryPointPath: 'test-package',
propertiesToConsider: ['main'],
});


const jsContents = fs.readFile(_(`/node_modules/test-package/index.js`));
expect(jsContents)
.toContain(
'DerivedDir.ɵdir = ɵngcc0.ɵɵdefineDirective({ type: DerivedDir, selectors: [["", "base", ""]], ' +
'features: [ɵngcc0.ɵɵInheritDefinitionFeature, ɵngcc0.ɵɵCopyDefinitionFeature] });');

const dtsContents = fs.readFile(_(`/node_modules/test-package/index.d.ts`));
expect(dtsContents)
.toContain(
'static ɵdir: ɵngcc0.ɵɵDirectiveDefWithMeta<DerivedDir, "[base]", never, {}, {}, never>;');
});

it('should generate a component definition with CopyDefinitionFeature for an undecorated child component',
() => {
genNodeModules({
'test-package': {
'/index.ts': `
import {Component, NgModule} from '@angular/core';
@Component({
selector: '[base]',
template: '<span>This is the base template</span>',
})
export class BaseCmp {}
export class DerivedCmp extends BaseCmp {}
@NgModule({
declarations: [DerivedCmp],
})
export class Module {}
`,
},
});

mainNgcc({
basePath: '/node_modules',
targetEntryPointPath: 'test-package',
propertiesToConsider: ['main'],
});


const jsContents = fs.readFile(_(`/node_modules/test-package/index.js`));
expect(jsContents).toContain('DerivedCmp.ɵcmp = ɵngcc0.ɵɵdefineComponent');
expect(jsContents)
.toContain(
'features: [ɵngcc0.ɵɵInheritDefinitionFeature, ɵngcc0.ɵɵCopyDefinitionFeature]');

const dtsContents = fs.readFile(_(`/node_modules/test-package/index.d.ts`));
expect(dtsContents)
.toContain(
'static ɵcmp: ɵngcc0.ɵɵComponentDefWithMeta<DerivedCmp, "[base]", never, {}, {}, never>;');
});
});

describe('aliasing re-exports in commonjs', () => {
it('should add re-exports to commonjs files', () => {
loadTestFiles([
Expand Down

0 comments on commit 0750fc8

Please sign in to comment.