diff --git a/packages/compiler-cli/ngcc/src/analysis/decoration_analyzer.ts b/packages/compiler-cli/ngcc/src/analysis/decoration_analyzer.ts index 410d0af0f86ed..e2cc4086b6d05 100644 --- a/packages/compiler-cli/ngcc/src/analysis/decoration_analyzer.ts +++ b/packages/compiler-cli/ngcc/src/analysis/decoration_analyzer.ts @@ -7,6 +7,7 @@ */ import {ConstantPool} from '@angular/compiler'; import * as ts from 'typescript'; + import {BaseDefDecoratorHandler, ComponentDecoratorHandler, DirectiveDecoratorHandler, InjectableDecoratorHandler, NgModuleDecoratorHandler, PipeDecoratorHandler, ReferencesRegistry, ResourceLoader} from '../../../src/ngtsc/annotations'; import {CycleAnalyzer, ImportGraph} from '../../../src/ngtsc/cycles'; import {isFatalDiagnosticError} from '../../../src/ngtsc/diagnostics'; @@ -18,13 +19,18 @@ import {ClassDeclaration} from '../../../src/ngtsc/reflection'; import {LocalModuleScopeRegistry, MetadataDtsModuleScopeResolver} from '../../../src/ngtsc/scope'; import {CompileResult, DecoratorHandler, DetectResult, HandlerPrecedence} from '../../../src/ngtsc/transform'; import {NgccClassSymbol, NgccReflectionHost} from '../host/ngcc_host'; -import {Migration, MigrationHost} from '../migrations/migration'; +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'; + 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. */ @@ -100,7 +106,11 @@ export class DecorationAnalyzer { this.reflectionHost, this.evaluator, this.metaRegistry, NOOP_DEFAULT_IMPORT_RECORDER, this.isCore), ]; - migrations: Migration[] = []; + migrations: Migration[] = [ + new UndecoratedParentMigration(), + new UndecoratedChildMigration(), + new MissingInjectableMigration(), + ]; constructor( private fs: FileSystem, private bundle: EntryPointBundle, @@ -118,9 +128,9 @@ export class DecorationAnalyzer { .filter(sourceFile => isWithinPackage(this.packagePath, sourceFile)) .map(sourceFile => this.analyzeFile(sourceFile)) .filter(isDefined); - const migrationHost = new DefaultMigrationHost( - this.reflectionHost, this.fullMetaReader, this.evaluator, this.handlers, analyzedFiles); - analyzedFiles.forEach(analyzedFile => this.migrateFile(migrationHost, analyzedFile)); + + this.applyMigrations(analyzedFiles); + analyzedFiles.forEach(analyzedFile => this.resolveFile(analyzedFile)); const compiledFiles = analyzedFiles.map(analyzedFile => this.compileFile(analyzedFile)); compiledFiles.forEach( @@ -146,21 +156,27 @@ export class DecorationAnalyzer { return analyzedClass; } - protected migrateFile(migrationHost: MigrationHost, analyzedFile: AnalyzedFile): void { - analyzedFile.analyzedClasses.forEach(({declaration}) => { - this.migrations.forEach(migration => { - try { - const result = migration.apply(declaration, migrationHost); - if (result !== null) { - this.diagnosticHandler(result); - } - } catch (e) { - if (isFatalDiagnosticError(e)) { - this.diagnosticHandler(e.toDiagnostic()); - } else { - throw e; + protected applyMigrations(analyzedFiles: AnalyzedFile[]): void { + const migrationHost = new DefaultMigrationHost( + this.reflectionHost, this.fullMetaReader, this.evaluator, this.handlers, + this.bundle.entryPoint.path, analyzedFiles); + + this.migrations.forEach(migration => { + analyzedFiles.forEach(analyzedFile => { + analyzedFile.analyzedClasses.forEach(({declaration}) => { + try { + const result = migration.apply(declaration, migrationHost); + if (result !== null) { + this.diagnosticHandler(result); + } + } catch (e) { + if (isFatalDiagnosticError(e)) { + this.diagnosticHandler(e.toDiagnostic()); + } else { + throw e; + } } - } + }); }); }); } diff --git a/packages/compiler-cli/ngcc/src/analysis/migration_host.ts b/packages/compiler-cli/ngcc/src/analysis/migration_host.ts index b575300318c6a..4ff258e44b661 100644 --- a/packages/compiler-cli/ngcc/src/analysis/migration_host.ts +++ b/packages/compiler-cli/ngcc/src/analysis/migration_host.ts @@ -6,15 +6,18 @@ * found in the LICENSE file at https://angular.io/license */ import * as ts from 'typescript'; + import {ErrorCode, FatalDiagnosticError} from '../../../src/ngtsc/diagnostics'; +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'; + import {AnalyzedClass, AnalyzedFile} from './types'; -import {analyzeDecorators} from './util'; +import {analyzeDecorators, isWithinPackage} from './util'; /** * The standard implementation of `MigrationHost`, which is created by the @@ -24,11 +27,12 @@ export class DefaultMigrationHost implements MigrationHost { constructor( readonly reflectionHost: NgccReflectionHost, readonly metadata: MetadataReader, readonly evaluator: PartialEvaluator, private handlers: DecoratorHandler[], - private analyzedFiles: AnalyzedFile[]) {} + 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; } @@ -41,6 +45,25 @@ export class DefaultMigrationHost implements MigrationHost { mergeAnalyzedClasses(oldAnalyzedClass, newAnalyzedClass); } } + + getAllDecorators(clazz: ClassDeclaration): Decorator[]|null { + const sourceFile = clazz.getSourceFile(); + const analyzedFile = this.analyzedFiles.find(file => file.sourceFile === sourceFile); + if (analyzedFile === undefined) { + return null; + } + + const analyzedClass = analyzedFile.analyzedClasses.find(c => c.declaration === clazz); + if (analyzedClass === undefined) { + return null; + } + + return analyzedClass.decorators; + } + + isInScope(clazz: ClassDeclaration): boolean { + return isWithinPackage(this.entryPointPath, clazz.getSourceFile()); + } } function getOrCreateAnalyzedFile( diff --git a/packages/compiler-cli/ngcc/src/analysis/util.ts b/packages/compiler-cli/ngcc/src/analysis/util.ts index b7837d9f90f2a..66530ed4af5e5 100644 --- a/packages/compiler-cli/ngcc/src/analysis/util.ts +++ b/packages/compiler-cli/ngcc/src/analysis/util.ts @@ -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'; @@ -21,7 +21,7 @@ export function isWithinPackage(packagePath: AbsoluteFsPath, sourceFile: ts.Sour export function analyzeDecorators( classSymbol: NgccClassSymbol, decorators: Decorator[] | null, - handlers: DecoratorHandler[]): AnalyzedClass|null { + handlers: DecoratorHandler[], flags?: HandlerFlags): AnalyzedClass|null { const declaration = classSymbol.declaration.valueDeclaration; const matchingHandlers = handlers .map(handler => { @@ -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); } diff --git a/packages/compiler-cli/ngcc/src/host/esm2015_host.ts b/packages/compiler-cli/ngcc/src/host/esm2015_host.ts index 5a14f1cd5f860..58bbdff8e5203 100644 --- a/packages/compiler-cli/ngcc/src/host/esm2015_host.ts +++ b/packages/compiler-cli/ngcc/src/host/esm2015_host.ts @@ -344,10 +344,18 @@ export class Esm2015ReflectionHost extends TypeScriptReflectionHost implements N return superDeclaration; } - /** Gets all decorators of the given class symbol. */ + /** + * Gets all decorators of the given class symbol. Any decorator that have been synthetically + * injected by a migration will not be present in the returned collection. + */ getDecoratorsOfSymbol(symbol: NgccClassSymbol): Decorator[]|null { const {classDecorators} = this.acquireDecoratorInfo(symbol); - return classDecorators; + if (classDecorators === null) { + return null; + } + + // Return a clone of the array to prevent consumers from mutating the cache. + return Array.from(classDecorators); } /** diff --git a/packages/compiler-cli/ngcc/src/migrations/migration.ts b/packages/compiler-cli/ngcc/src/migrations/migration.ts index 3854684eeff52..2738dc3e53a92 100644 --- a/packages/compiler-cli/ngcc/src/migrations/migration.ts +++ b/packages/compiler-cli/ngcc/src/migrations/migration.ts @@ -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 @@ -41,5 +44,22 @@ 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 + * that have been injected before. + * @param clazz the class for which all decorators are retrieved. + * @returns the list of the decorators, or null if the class was not decorated. + */ + getAllDecorators(clazz: ClassDeclaration): Decorator[]|null; + + /** + * Determines whether the provided class in within scope of the entry-point that is currently + * being compiled. + * @param clazz the class for which to determine whether it is within the current entry-point. + * @returns true if the file is part of the compiled entry-point, false otherwise. + */ + isInScope(clazz: ClassDeclaration): boolean; } diff --git a/packages/compiler-cli/ngcc/src/migrations/missing_injectable_migration.ts b/packages/compiler-cli/ngcc/src/migrations/missing_injectable_migration.ts new file mode 100644 index 0000000000000..8c590a08878d1 --- /dev/null +++ b/packages/compiler-cli/ngcc/src/migrations/missing_injectable_migration.ts @@ -0,0 +1,192 @@ +/** + * @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 {forwardRefResolver} from '../../../src/ngtsc/annotations'; +import {Reference} from '../../../src/ngtsc/imports'; +import {ResolvedValue, ResolvedValueMap} from '../../../src/ngtsc/partial_evaluator'; +import {ClassDeclaration, Decorator} from '../../../src/ngtsc/reflection'; + +import {Migration, MigrationHost} from './migration'; +import {createInjectableDecorator, isClassDeclaration} from './utils'; + +/** + * Ensures that classes that are provided as an Angular service in either `NgModule.providers` or + * `Directive.providers`/`Component.viewProviders` are decorated with one of the `@Injectable`, + * `@Directive`, `@Component` or `@Pipe` decorators, adding an `@Injectable()` decorator when none + * are present. + * + * At least one decorator is now mandatory, as otherwise the compiler would not compile an + * injectable definition for the service. This is unlike View Engine, where having just an unrelated + * decorator may have been sufficient for the service to become injectable. + * + * In essence, this migration operates on classes that are themselves an NgModule, Directive or + * Component. Their metadata is statically evaluated so that their "providers"/"viewProviders" + * properties can be analyzed. For any provider that refers to an undecorated class, the class will + * be migrated to have an `@Injectable()` decorator. + * + * This implementation mirrors the "missing-injectable" schematic. + */ +export class MissingInjectableMigration implements Migration { + apply(clazz: ClassDeclaration, host: MigrationHost): ts.Diagnostic|null { + const decorators = host.reflectionHost.getDecoratorsOfDeclaration(clazz); + if (decorators === null) { + return null; + } + + for (const decorator of decorators) { + const name = getAngularCoreDecoratorName(decorator); + if (name === 'NgModule') { + migrateNgModuleProviders(decorator, host); + } else if (name === 'Directive') { + migrateDirectiveProviders(decorator, host, /* isComponent */ false); + } else if (name === 'Component') { + migrateDirectiveProviders(decorator, host, /* isComponent */ true); + } + } + + return null; + } +} + +/** + * Iterates through all `NgModule.providers` and adds the `@Injectable()` decorator to any provider + * that is not otherwise decorated. + */ +function migrateNgModuleProviders(decorator: Decorator, host: MigrationHost): void { + if (decorator.args === null || decorator.args.length !== 1) { + return; + } + + const metadata = host.evaluator.evaluate(decorator.args[0], forwardRefResolver); + if (!(metadata instanceof Map)) { + return; + } + + migrateProviders(metadata, 'providers', host); + // TODO(alxhub): we should probably also check for `ModuleWithProviders` here. +} + +/** + * Iterates through all `Directive.providers` and if `isComponent` is set to true also + * `Component.viewProviders` and adds the `@Injectable()` decorator to any provider that is not + * otherwise decorated. + */ +function migrateDirectiveProviders( + decorator: Decorator, host: MigrationHost, isComponent: boolean): void { + if (decorator.args === null || decorator.args.length !== 1) { + return; + } + + const metadata = host.evaluator.evaluate(decorator.args[0], forwardRefResolver); + if (!(metadata instanceof Map)) { + return; + } + + migrateProviders(metadata, 'providers', host); + if (isComponent) { + migrateProviders(metadata, 'viewProviders', host); + } +} + +/** + * Given an object with decorator metadata, iterates through the list of providers to add + * `@Injectable()` to any provider that is not otherwise decorated. + */ +function migrateProviders(metadata: ResolvedValueMap, field: string, host: MigrationHost): void { + if (!metadata.has(field)) { + return; + } + const providers = metadata.get(field) !; + if (!Array.isArray(providers)) { + return; + } + + for (const provider of providers) { + migrateProvider(provider, host); + } +} + +/** + * Analyzes a single provider entry and determines the class that is required to have an + * `@Injectable()` decorator. + */ +function migrateProvider(provider: ResolvedValue, host: MigrationHost): void { + if (provider instanceof Map) { + if (!provider.has('provide') || provider.has('useValue') || provider.has('useFactory') || + provider.has('useExisting')) { + return; + } + if (provider.has('useClass')) { + // {provide: ..., useClass: SomeClass, deps: [...]} does not require a decorator on SomeClass, + // as the provider itself configures 'deps'. Only if 'deps' is missing will this require a + // factory to exist on SomeClass. + if (!provider.has('deps')) { + migrateProviderClass(provider.get('useClass') !, host); + } + } else { + migrateProviderClass(provider.get('provide') !, host); + } + } else if (Array.isArray(provider)) { + for (const v of provider) { + migrateProvider(v, host); + } + } else { + migrateProviderClass(provider, host); + } +} + +/** + * Given a provider class, adds the `@Injectable()` decorator if no other relevant Angular decorator + * is present on the class. + */ +function migrateProviderClass(provider: ResolvedValue, host: MigrationHost): void { + // Providers that do not refer to a class cannot be migrated. + if (!(provider instanceof Reference)) { + return; + } + + const clazz = provider.node as ts.Declaration; + if (isClassDeclaration(clazz) && host.isInScope(clazz) && needsInjectableDecorator(clazz, host)) { + host.injectSyntheticDecorator(clazz, createInjectableDecorator(clazz)); + } +} + +const NO_MIGRATE_DECORATORS = new Set(['Injectable', 'Directive', 'Component', 'Pipe']); + +/** + * Determines if the given class needs to be decorated with `@Injectable()` based on whether it + * already has an Angular decorator applied. + */ +function needsInjectableDecorator(clazz: ClassDeclaration, host: MigrationHost): boolean { + const decorators = host.getAllDecorators(clazz); + if (decorators === null) { + return true; + } + + for (const decorator of decorators) { + const name = getAngularCoreDecoratorName(decorator); + if (name !== null && NO_MIGRATE_DECORATORS.has(name)) { + return false; + } + } + + return true; +} + +/** + * Determines the original name of a decorator if it is from '@angular/core'. For other decorators, + * null is returned. + */ +export function getAngularCoreDecoratorName(decorator: Decorator): string|null { + if (decorator.import === null || decorator.import.from !== '@angular/core') { + return null; + } + + return decorator.import.name; +} diff --git a/packages/compiler-cli/ngcc/src/migrations/undecorated_child_migration.ts b/packages/compiler-cli/ngcc/src/migrations/undecorated_child_migration.ts new file mode 100644 index 0000000000000..cb100fe7c4ac3 --- /dev/null +++ b/packages/compiler-cli/ngcc/src/migrations/undecorated_child_migration.ts @@ -0,0 +1,75 @@ +/** + * @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 { + 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) { + this.maybeMigrate(decl, host); + } + + return null; + } + + maybeMigrate(ref: Reference, host: MigrationHost): void { + if (hasDirectiveDecorator(host, ref.node) || hasPipeDecorator(host, ref.node)) { + // Stop if one of the classes in the chain is actually decorated with @Directive, @Component, + // or @Pipe. + return; + } + + const baseRef = readBaseClass(ref.node, host.reflectionHost, host.evaluator); + if (baseRef === null) { + // Stop: can't migrate a class with no parent. + return; + } else if (baseRef === 'dynamic') { + // Stop: can't migrate a class with an indeterminate parent. + return; + } + + // 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; + } + + // 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! + } +} 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 4113d438766d4..5524661a022a5 100644 --- a/packages/compiler-cli/ngcc/src/migrations/utils.ts +++ b/packages/compiler-cli/ngcc/src/migrations/utils.ts @@ -19,7 +19,16 @@ 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; +} + +/** + * Returns true if the `clazz` is decorated as a `Pipe`. + */ +export function hasPipeDecorator(host: MigrationHost, clazz: ClassDeclaration): boolean { + const ref = new Reference(clazz); + return host.metadata.getPipeMetadata(ref) !== null; } /** @@ -32,32 +41,96 @@ 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')), - ]); - const decoratorType = ts.createIdentifier('Directive'); - const decoratorNode = ts.createObjectLiteral([ - ts.createPropertyAssignment('type', decoratorType), - ts.createPropertyAssignment('args', ts.createArrayLiteral([selectorArg])), - ]); - - setParentPointers(clazz.getSourceFile(), decoratorNode); - +export function createDirectiveDecorator( + clazz: ClassDeclaration, + metadata?: {selector: string | null, exportAs: string[] | null}): Decorator { + const args: ts.Expression[] = []; + if (metadata !== undefined) { + const metaArgs: ts.PropertyAssignment[] = []; + if (metadata.selector !== null) { + metaArgs.push(property('selector', metadata.selector)); + } + if (metadata.exportAs !== null) { + metaArgs.push(property('exportAs', metadata.exportAs)); + } + args.push(reifySourceFile(ts.createObjectLiteral(metaArgs))); + } return { name: 'Directive', - identifier: decoratorType, + identifier: null, import: {name: 'Directive', from: '@angular/core'}, - node: decoratorNode, - args: [selectorArg], + node: null, + synthesizedFor: clazz.name, args, + }; +} + +/** + * Create an empty `Component` decorator that will be associated with the `clazz`. + */ +export function createComponentDecorator( + clazz: ClassDeclaration, + metadata: {selector: string | null, exportAs: string[] | null}): Decorator { + const metaArgs: ts.PropertyAssignment[] = [ + property('template', ''), + ]; + if (metadata.selector !== null) { + metaArgs.push(property('selector', metadata.selector)); + } + if (metadata.exportAs !== null) { + metaArgs.push(property('exportAs', metadata.exportAs)); + } + return { + name: 'Component', + identifier: null, + import: {name: 'Component', from: '@angular/core'}, + node: null, + synthesizedFor: clazz.name, + args: [ + reifySourceFile(ts.createObjectLiteral(metaArgs)), + ], }; } /** - * Ensure that a tree of AST nodes have their parents wired up. + * Create an empty `Injectable` decorator that will be associated with the `clazz`. + */ +export function createInjectableDecorator(clazz: ClassDeclaration): Decorator { + return { + name: 'Injectable', + identifier: null, + import: {name: 'Injectable', from: '@angular/core'}, + node: null, + synthesizedFor: clazz.name, + 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); + +/** + * Takes a `ts.Expression` and returns the same `ts.Expression`, but with an associated + * `ts.SourceFile`. + * + * This transformation is necessary to use synthetic `ts.Expression`s with the `PartialEvaluator`, + * and many decorator arguments are interpreted in this way. */ -export function setParentPointers(parent: ts.Node, child: ts.Node): void { - child.parent = parent; - ts.forEachChild(child, grandchild => setParentPointers(child, grandchild)); +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 !; } diff --git a/packages/compiler-cli/ngcc/src/rendering/renderer.ts b/packages/compiler-cli/ngcc/src/rendering/renderer.ts index 958ec67588a62..171c1ec27eba9 100644 --- a/packages/compiler-cli/ngcc/src/rendering/renderer.ts +++ b/packages/compiler-cli/ngcc/src/rendering/renderer.ts @@ -130,6 +130,9 @@ export class Renderer { } clazz.decorators.forEach(dec => { + if (dec.node === null) { + return; + } const decoratorArray = dec.node.parent !; if (!decoratorsToRemove.has(decoratorArray)) { decoratorsToRemove.set(decoratorArray, [dec.node]); diff --git a/packages/compiler-cli/ngcc/test/BUILD.bazel b/packages/compiler-cli/ngcc/test/BUILD.bazel index 4a72d20160d2a..d0644397c23a6 100644 --- a/packages/compiler-cli/ngcc/test/BUILD.bazel +++ b/packages/compiler-cli/ngcc/test/BUILD.bazel @@ -56,6 +56,7 @@ ts_library( "//packages/compiler-cli/src/ngtsc/testing", "//packages/compiler-cli/test/helpers", "@npm//rxjs", + "@npm//typescript", ], ) diff --git a/packages/compiler-cli/ngcc/test/analysis/decoration_analyzer_spec.ts b/packages/compiler-cli/ngcc/test/analysis/decoration_analyzer_spec.ts index 9397c57cb5c9d..9f63637619e63 100644 --- a/packages/compiler-cli/ngcc/test/analysis/decoration_analyzer_spec.ts +++ b/packages/compiler-cli/ngcc/test/analysis/decoration_analyzer_spec.ts @@ -199,10 +199,10 @@ runInEachFileSystem(() => { it('should call `apply()` on each migration for each class', () => { expect(migrationLogs).toEqual([ 'migration1:MyComponent', - 'migration2:MyComponent', 'migration1:MyDirective', - 'migration2:MyDirective', 'migration1:MyOtherComponent', + 'migration2:MyComponent', + 'migration2:MyDirective', 'migration2:MyOtherComponent', ]); }); diff --git a/packages/compiler-cli/ngcc/test/analysis/migration_host_spec.ts b/packages/compiler-cli/ngcc/test/analysis/migration_host_spec.ts index f4f8981f1d20a..c0e52ef119beb 100644 --- a/packages/compiler-cli/ngcc/test/analysis/migration_host_spec.ts +++ b/packages/compiler-cli/ngcc/test/analysis/migration_host_spec.ts @@ -6,156 +6,256 @@ * found in the LICENSE file at https://angular.io/license */ import {ErrorCode} from '../../../src/ngtsc/diagnostics'; +import {AbsoluteFsPath, absoluteFrom} from '../../../src/ngtsc/file_system'; +import {runInEachFileSystem} from '../../../src/ngtsc/file_system/testing'; import {ClassDeclaration, Decorator} from '../../../src/ngtsc/reflection'; import {AnalysisOutput, CompileResult, DecoratorHandler, DetectResult, HandlerPrecedence} from '../../../src/ngtsc/transform'; import {DefaultMigrationHost} from '../../src/analysis/migration_host'; import {AnalyzedClass, AnalyzedFile} from '../../src/analysis/types'; import {NgccClassSymbol} from '../../src/host/ngcc_host'; -describe('DefaultMigrationHost', () => { - describe('injectSyntheticDecorator()', () => { - const mockHost: any = { - getClassSymbol: (node: any): NgccClassSymbol | undefined => { - const symbol = { valueDeclaration: node, name: node.name.text } as any; - return { - name: node.name.text, - declaration: symbol, - implementation: symbol, - }; - }, - }; - const mockMetadata: any = {}; - const mockEvaluator: any = {}; - const mockClazz: any = { - name: {text: 'MockClazz'}, - getSourceFile: () => { fileName: 'test-file.js'; }, - }; - const mockDecorator: any = {name: 'MockDecorator'}; - - it('should call `detect()` on each of the provided handlers', () => { - const log: string[] = []; - const handler1 = new TestHandler('handler1', log); - const handler2 = new TestHandler('handler2', log); - const host = - new DefaultMigrationHost(mockHost, mockMetadata, mockEvaluator, [handler1, handler2], []); - host.injectSyntheticDecorator(mockClazz, mockDecorator); - expect(log).toEqual([ - `handler1:detect:MockClazz:MockDecorator`, - `handler2:detect:MockClazz:MockDecorator`, - ]); +runInEachFileSystem(() => { + describe('DefaultMigrationHost', () => { + let _: typeof absoluteFrom; + let entryPointPath: AbsoluteFsPath; + let mockHost: any; + let mockMetadata: any = {}; + let mockEvaluator: any = {}; + let mockClazz: any; + let mockDecorator: any = {name: 'MockDecorator'}; + beforeEach(() => { + _ = absoluteFrom; + entryPointPath = _('/node_modules/some-package/entry-point'); + mockHost = { + getClassSymbol: (node: any): NgccClassSymbol | undefined => { + const symbol = { valueDeclaration: node, name: node.name.text } as any; + return { + name: node.name.text, + declaration: symbol, + implementation: symbol, + }; + }, + }; + const mockSourceFile: any = { + fileName: _('/node_modules/some-package/entry-point/test-file.js'), + }; + mockClazz = { + name: {text: 'MockClazz'}, + getSourceFile: () => mockSourceFile, + }; }); - it('should call `analyze()` on each of the provided handlers whose `detect()` call returns a result', - () => { - const log: string[] = []; - const handler1 = new TestHandler('handler1', log); - const handler2 = new AlwaysDetectHandler('handler2', log); - const handler3 = new TestHandler('handler3', log); - const host = new DefaultMigrationHost( - mockHost, mockMetadata, mockEvaluator, [handler1, handler2, handler3], []); - host.injectSyntheticDecorator(mockClazz, mockDecorator); - expect(log).toEqual([ - `handler1:detect:MockClazz:MockDecorator`, - `handler2:detect:MockClazz:MockDecorator`, - `handler3:detect:MockClazz:MockDecorator`, - 'handler2:analyze:MockClazz', - ]); - }); - - it('should add a newly `AnalyzedFile` to the `analyzedFiles` object', () => { - const log: string[] = []; - const handler = new AlwaysDetectHandler('handler', log); - const analyzedFiles: AnalyzedFile[] = []; - const host = - new DefaultMigrationHost(mockHost, mockMetadata, mockEvaluator, [handler], analyzedFiles); - host.injectSyntheticDecorator(mockClazz, mockDecorator); - expect(analyzedFiles.length).toEqual(1); - expect(analyzedFiles[0].analyzedClasses.length).toEqual(1); - expect(analyzedFiles[0].analyzedClasses[0].name).toEqual('MockClazz'); - }); + describe('injectSyntheticDecorator()', () => { + it('should call `detect()` on each of the provided handlers', () => { + const log: string[] = []; + const handler1 = new TestHandler('handler1', log); + const handler2 = new TestHandler('handler2', log); + const host = new DefaultMigrationHost( + mockHost, mockMetadata, mockEvaluator, [handler1, handler2], entryPointPath, []); + host.injectSyntheticDecorator(mockClazz, mockDecorator); + expect(log).toEqual([ + `handler1:detect:MockClazz:MockDecorator`, + `handler2:detect:MockClazz:MockDecorator`, + ]); + }); + + it('should call `analyze()` on each of the provided handlers whose `detect()` call returns a result', + () => { + const log: string[] = []; + const handler1 = new TestHandler('handler1', log); + const handler2 = new AlwaysDetectHandler('handler2', log); + const handler3 = new TestHandler('handler3', log); + const host = new DefaultMigrationHost( + mockHost, mockMetadata, mockEvaluator, [handler1, handler2, handler3], + entryPointPath, []); + host.injectSyntheticDecorator(mockClazz, mockDecorator); + expect(log).toEqual([ + `handler1:detect:MockClazz:MockDecorator`, + `handler2:detect:MockClazz:MockDecorator`, + `handler3:detect:MockClazz:MockDecorator`, + 'handler2:analyze:MockClazz', + ]); + }); + + it('should add a newly `AnalyzedFile` to the `analyzedFiles` object', () => { + const log: string[] = []; + const handler = new AlwaysDetectHandler('handler', log); + const analyzedFiles: AnalyzedFile[] = []; + const host = new DefaultMigrationHost( + mockHost, mockMetadata, mockEvaluator, [handler], entryPointPath, analyzedFiles); + host.injectSyntheticDecorator(mockClazz, mockDecorator); + expect(analyzedFiles.length).toEqual(1); + expect(analyzedFiles[0].analyzedClasses.length).toEqual(1); + expect(analyzedFiles[0].analyzedClasses[0].name).toEqual('MockClazz'); + }); + + it('should add a newly `AnalyzedClass` to an existing `AnalyzedFile` object', () => { + const DUMMY_CLASS_1: any = {}; + const DUMMY_CLASS_2: any = {}; + const log: string[] = []; + const handler = new AlwaysDetectHandler('handler', log); + const analyzedFiles: AnalyzedFile[] = [{ + sourceFile: mockClazz.getSourceFile(), + analyzedClasses: [DUMMY_CLASS_1, DUMMY_CLASS_2], + }]; + const host = new DefaultMigrationHost( + mockHost, mockMetadata, mockEvaluator, [handler], entryPointPath, analyzedFiles); + host.injectSyntheticDecorator(mockClazz, mockDecorator); + expect(analyzedFiles.length).toEqual(1); + expect(analyzedFiles[0].analyzedClasses.length).toEqual(3); + expect(analyzedFiles[0].analyzedClasses[2].name).toEqual('MockClazz'); + }); - it('should add a newly `AnalyzedClass` to an existing `AnalyzedFile` object', () => { - const DUMMY_CLASS_1: any = {}; - const DUMMY_CLASS_2: any = {}; - const log: string[] = []; - const handler = new AlwaysDetectHandler('handler', log); - const analyzedFiles: AnalyzedFile[] = [{ - sourceFile: mockClazz.getSourceFile(), - analyzedClasses: [DUMMY_CLASS_1, DUMMY_CLASS_2], - }]; - const host = - new DefaultMigrationHost(mockHost, mockMetadata, mockEvaluator, [handler], analyzedFiles); - host.injectSyntheticDecorator(mockClazz, mockDecorator); - expect(analyzedFiles.length).toEqual(1); - expect(analyzedFiles[0].analyzedClasses.length).toEqual(3); - expect(analyzedFiles[0].analyzedClasses[2].name).toEqual('MockClazz'); + it('should add a new decorator into an already existing `AnalyzedClass`', () => { + const analyzedClass: AnalyzedClass = { + name: 'MockClazz', + declaration: mockClazz, + matches: [], + decorators: null, + }; + const log: string[] = []; + const handler = new AlwaysDetectHandler('handler', log); + const analyzedFiles: AnalyzedFile[] = [{ + sourceFile: mockClazz.getSourceFile(), + analyzedClasses: [analyzedClass], + }]; + const host = new DefaultMigrationHost( + mockHost, mockMetadata, mockEvaluator, [handler], entryPointPath, analyzedFiles); + host.injectSyntheticDecorator(mockClazz, mockDecorator); + expect(analyzedFiles.length).toEqual(1); + expect(analyzedFiles[0].analyzedClasses.length).toEqual(1); + expect(analyzedFiles[0].analyzedClasses[0]).toBe(analyzedClass); + expect(analyzedClass.decorators !.length).toEqual(1); + expect(analyzedClass.decorators ![0].name).toEqual('MockDecorator'); + }); + + it('should merge a new decorator into pre-existing decorators an already existing `AnalyzedClass`', + () => { + const analyzedClass: AnalyzedClass = { + name: 'MockClazz', + declaration: mockClazz, + matches: [], + decorators: [{name: 'OtherDecorator'} as Decorator], + }; + const log: string[] = []; + const handler = new AlwaysDetectHandler('handler', log); + const analyzedFiles: AnalyzedFile[] = [{ + sourceFile: mockClazz.getSourceFile(), + analyzedClasses: [analyzedClass], + }]; + const host = new DefaultMigrationHost( + mockHost, mockMetadata, mockEvaluator, [handler], entryPointPath, analyzedFiles); + host.injectSyntheticDecorator(mockClazz, mockDecorator); + expect(analyzedFiles.length).toEqual(1); + expect(analyzedFiles[0].analyzedClasses.length).toEqual(1); + expect(analyzedFiles[0].analyzedClasses[0]).toBe(analyzedClass); + expect(analyzedClass.decorators !.length).toEqual(2); + expect(analyzedClass.decorators ![1].name).toEqual('MockDecorator'); + }); + + it('should throw an error if the injected decorator already exists', () => { + const analyzedClass: AnalyzedClass = { + name: 'MockClazz', + declaration: mockClazz, + matches: [], + decorators: [{name: 'MockDecorator'} as Decorator], + }; + const log: string[] = []; + const handler = new AlwaysDetectHandler('handler', log); + const analyzedFiles: AnalyzedFile[] = [{ + sourceFile: mockClazz.getSourceFile(), + analyzedClasses: [analyzedClass], + }]; + const host = new DefaultMigrationHost( + mockHost, mockMetadata, mockEvaluator, [handler], entryPointPath, analyzedFiles); + expect(() => host.injectSyntheticDecorator(mockClazz, mockDecorator)) + .toThrow(jasmine.objectContaining( + {code: ErrorCode.NGCC_MIGRATION_DECORATOR_INJECTION_ERROR})); + }); }); - it('should add a new decorator into an already existing `AnalyzedClass`', () => { - const analyzedClass: AnalyzedClass = { - name: 'MockClazz', - declaration: mockClazz, - matches: [], - decorators: null, - }; - const log: string[] = []; - const handler = new AlwaysDetectHandler('handler', log); - const analyzedFiles: AnalyzedFile[] = [{ - sourceFile: mockClazz.getSourceFile(), - analyzedClasses: [analyzedClass], - }]; - const host = - new DefaultMigrationHost(mockHost, mockMetadata, mockEvaluator, [handler], analyzedFiles); - host.injectSyntheticDecorator(mockClazz, mockDecorator); - expect(analyzedFiles.length).toEqual(1); - expect(analyzedFiles[0].analyzedClasses.length).toEqual(1); - expect(analyzedFiles[0].analyzedClasses[0]).toBe(analyzedClass); - expect(analyzedClass.decorators !.length).toEqual(1); - expect(analyzedClass.decorators ![0].name).toEqual('MockDecorator'); + describe('getAllDecorators', () => { + it('should be null for unknown source files', () => { + const log: string[] = []; + const handler = new AlwaysDetectHandler('handler', log); + const analyzedFiles: AnalyzedFile[] = []; + const host = new DefaultMigrationHost( + mockHost, mockMetadata, mockEvaluator, [handler], entryPointPath, analyzedFiles); + + const decorators = host.getAllDecorators(mockClazz); + expect(decorators).toBeNull(); + }); + + it('should be null for unknown classes', () => { + const log: string[] = []; + const handler = new AlwaysDetectHandler('handler', log); + const analyzedFiles: AnalyzedFile[] = []; + const host = new DefaultMigrationHost( + mockHost, mockMetadata, mockEvaluator, [handler], entryPointPath, analyzedFiles); + + const sourceFile: any = {}; + const unrelatedClass: any = { + getSourceFile: () => sourceFile, + }; + analyzedFiles.push({sourceFile, analyzedClasses: [unrelatedClass]}); + + const decorators = host.getAllDecorators(mockClazz); + expect(decorators).toBeNull(); + }); + + it('should include injected decorators', () => { + const log: string[] = []; + const handler = new AlwaysDetectHandler('handler', log); + const existingDecorator = { name: 'ExistingDecorator' } as Decorator; + const analyzedClass: AnalyzedClass = { + name: 'MockClazz', + declaration: mockClazz, + matches: [], + decorators: [existingDecorator], + }; + const analyzedFiles: AnalyzedFile[] = [{ + sourceFile: mockClazz.getSourceFile(), + analyzedClasses: [analyzedClass], + }]; + const host = new DefaultMigrationHost( + mockHost, mockMetadata, mockEvaluator, [handler], entryPointPath, analyzedFiles); + host.injectSyntheticDecorator(mockClazz, mockDecorator); + + const decorators = host.getAllDecorators(mockClazz) !; + expect(decorators.length).toBe(2); + expect(decorators[0]).toBe(existingDecorator); + expect(decorators[1]).toBe(mockDecorator); + }); }); - it('should merge a new decorator into pre-existing decorators an already existing `AnalyzedClass`', - () => { - const analyzedClass: AnalyzedClass = { - name: 'MockClazz', - declaration: mockClazz, - matches: [], - decorators: [{name: 'OtherDecorator'} as Decorator], - }; - const log: string[] = []; - const handler = new AlwaysDetectHandler('handler', log); - const analyzedFiles: AnalyzedFile[] = [{ - sourceFile: mockClazz.getSourceFile(), - analyzedClasses: [analyzedClass], - }]; - const host = new DefaultMigrationHost( - mockHost, mockMetadata, mockEvaluator, [handler], analyzedFiles); - host.injectSyntheticDecorator(mockClazz, mockDecorator); - expect(analyzedFiles.length).toEqual(1); - expect(analyzedFiles[0].analyzedClasses.length).toEqual(1); - expect(analyzedFiles[0].analyzedClasses[0]).toBe(analyzedClass); - expect(analyzedClass.decorators !.length).toEqual(2); - expect(analyzedClass.decorators ![1].name).toEqual('MockDecorator'); - }); - - it('should throw an error if the injected decorator already exists', () => { - const analyzedClass: AnalyzedClass = { - name: 'MockClazz', - declaration: mockClazz, - matches: [], - decorators: [{name: 'MockDecorator'} as Decorator], - }; - const log: string[] = []; - const handler = new AlwaysDetectHandler('handler', log); - const analyzedFiles: AnalyzedFile[] = [{ - sourceFile: mockClazz.getSourceFile(), - analyzedClasses: [analyzedClass], - }]; - const host = - new DefaultMigrationHost(mockHost, mockMetadata, mockEvaluator, [handler], analyzedFiles); - expect(() => host.injectSyntheticDecorator(mockClazz, mockDecorator)) - .toThrow( - jasmine.objectContaining({code: ErrorCode.NGCC_MIGRATION_DECORATOR_INJECTION_ERROR})); + describe('isInScope', () => { + it('should be true for nodes within the entry-point', () => { + const analyzedFiles: AnalyzedFile[] = []; + const host = new DefaultMigrationHost( + mockHost, mockMetadata, mockEvaluator, [], entryPointPath, analyzedFiles); + + const sourceFile: any = { + fileName: _('/node_modules/some-package/entry-point/relative.js'), + }; + const clazz: any = { + getSourceFile: () => sourceFile, + }; + expect(host.isInScope(clazz)).toBe(true); + }); + + it('should be false for nodes outside the entry-point', () => { + const analyzedFiles: AnalyzedFile[] = []; + const host = new DefaultMigrationHost( + mockHost, mockMetadata, mockEvaluator, [], entryPointPath, analyzedFiles); + + const sourceFile: any = { + fileName: _('/node_modules/some-package/other-entry/index.js'), + }; + const clazz: any = { + getSourceFile: () => sourceFile, + }; + expect(host.isInScope(clazz)).toBe(false); + }); }); }); }); diff --git a/packages/compiler-cli/ngcc/test/host/commonjs_host_import_helper_spec.ts b/packages/compiler-cli/ngcc/test/host/commonjs_host_import_helper_spec.ts index 8163a753110f3..b9bce75af33b0 100644 --- a/packages/compiler-cli/ngcc/test/host/commonjs_host_import_helper_spec.ts +++ b/packages/compiler-cli/ngcc/test/host/commonjs_host_import_helper_spec.ts @@ -86,7 +86,7 @@ exports.OtherDirective = OtherDirective; const decorator = decorators[0]; expect(decorator.name).toEqual('Directive'); - expect(decorator.identifier.getText()).toEqual('core.Directive'); + expect(decorator.identifier !.getText()).toEqual('core.Directive'); expect(decorator.import).toEqual({name: 'Directive', from: '@angular/core'}); expect(decorator.args !.map(arg => arg.getText())).toEqual([ '{ selector: \'[someDirective]\' }', diff --git a/packages/compiler-cli/ngcc/test/host/esm2015_host_import_helper_spec.ts b/packages/compiler-cli/ngcc/test/host/esm2015_host_import_helper_spec.ts index 2b22192e88455..3b617a2d9f414 100644 --- a/packages/compiler-cli/ngcc/test/host/esm2015_host_import_helper_spec.ts +++ b/packages/compiler-cli/ngcc/test/host/esm2015_host_import_helper_spec.ts @@ -175,7 +175,7 @@ runInEachFileSystem(() => { const decorator = decorators[0]; expect(decorator.name).toEqual('Directive'); - expect(decorator.identifier.getText()).toEqual('Directive'); + expect(decorator.identifier !.getText()).toEqual('Directive'); expect(decorator.import).toEqual({name: 'Directive', from: '@angular/core'}); expect(decorator.args !.map(arg => arg.getText())).toEqual([ '{ selector: \'[someDirective]\' }', @@ -197,7 +197,7 @@ runInEachFileSystem(() => { const decorator = decorators[0]; expect(decorator.name).toEqual('Directive'); - expect(decorator.identifier.getText()).toEqual('Directive'); + expect(decorator.identifier !.getText()).toEqual('Directive'); expect(decorator.import).toEqual({name: 'Directive', from: '@angular/core'}); expect(decorator.args !.map(arg => arg.getText())).toEqual([ '{ selector: \'[someDirective]\' }', @@ -219,7 +219,7 @@ runInEachFileSystem(() => { const decorator = decorators[0]; expect(decorator.name).toEqual('Directive'); - expect(decorator.identifier.getText()).toEqual('Directive'); + expect(decorator.identifier !.getText()).toEqual('Directive'); expect(decorator.import).toEqual({name: 'Directive', from: './directives'}); expect(decorator.args !.map(arg => arg.getText())).toEqual([ '{ selector: \'[someDirective]\' }', @@ -433,7 +433,7 @@ runInEachFileSystem(() => { const classNode = getDeclaration( program, _('/some_directive.js'), 'SomeDirective', isNamedVariableDeclaration); const classDecorators = host.getDecoratorsOfDeclaration(classNode) !; - const decoratorNode = classDecorators[0].node; + const decoratorNode = classDecorators[0].node !; const identifierOfDirective = ts.isCallExpression(decoratorNode) && ts.isIdentifier(decoratorNode.expression) ? decoratorNode.expression : diff --git a/packages/compiler-cli/ngcc/test/host/esm2015_host_spec.ts b/packages/compiler-cli/ngcc/test/host/esm2015_host_spec.ts index d85f29e9beec7..c29afece754e9 100644 --- a/packages/compiler-cli/ngcc/test/host/esm2015_host_spec.ts +++ b/packages/compiler-cli/ngcc/test/host/esm2015_host_spec.ts @@ -1931,6 +1931,20 @@ runInEachFileSystem(() => { expect(classDecoratorsSecondary.length).toEqual(1); expect(classDecoratorsSecondary[0] !.map(d => d.name)).toEqual(['Directive']); }); + + it('should return a cloned array on each invocation', () => { + loadTestFiles(DECORATED_FILES); + const {program} = makeTestBundleProgram(getRootFiles(DECORATED_FILES)[0]); + const host = new Esm2015ReflectionHost(new MockLogger(), false, program.getTypeChecker()); + const classDecl = + getDeclaration(program, DECORATED_FILES[0].name, 'A', ts.isClassDeclaration) !; + const classSymbol = host.getClassSymbol(classDecl) !; + + const firstResult = host.getDecoratorsOfSymbol(classSymbol); + const secondResult = host.getDecoratorsOfSymbol(classSymbol); + + expect(firstResult).not.toBe(secondResult); + }); }); describe('getDtsDeclarationsOfClass()', () => { diff --git a/packages/compiler-cli/ngcc/test/host/esm5_host_import_helper_spec.ts b/packages/compiler-cli/ngcc/test/host/esm5_host_import_helper_spec.ts index b467be4e19704..a2769b93e844f 100644 --- a/packages/compiler-cli/ngcc/test/host/esm5_host_import_helper_spec.ts +++ b/packages/compiler-cli/ngcc/test/host/esm5_host_import_helper_spec.ts @@ -202,7 +202,7 @@ export { SomeDirective }; const decorator = decorators[0]; expect(decorator.name).toEqual('Directive'); - expect(decorator.identifier.getText()).toEqual('Directive'); + expect(decorator.identifier !.getText()).toEqual('Directive'); expect(decorator.import).toEqual({name: 'Directive', from: '@angular/core'}); expect(decorator.args !.map(arg => arg.getText())).toEqual([ '{ selector: \'[someDirective]\' }', @@ -224,7 +224,7 @@ export { SomeDirective }; const decorator = decorators[0]; expect(decorator.name).toEqual('Directive'); - expect(decorator.identifier.getText()).toEqual('Directive'); + expect(decorator.identifier !.getText()).toEqual('Directive'); expect(decorator.import).toEqual({name: 'Directive', from: '@angular/core'}); expect(decorator.args !.map(arg => arg.getText())).toEqual([ '{ selector: \'[someDirective]\' }', @@ -245,7 +245,7 @@ export { SomeDirective }; const decorator = decorators[0]; expect(decorator.name).toEqual('Directive'); - expect(decorator.identifier.getText()).toEqual('Directive'); + expect(decorator.identifier !.getText()).toEqual('Directive'); expect(decorator.import).toEqual({name: 'Directive', from: './directives'}); expect(decorator.args !.map(arg => arg.getText())).toEqual([ '{ selector: \'[someDirective]\' }', @@ -470,7 +470,7 @@ export { SomeDirective }; const classNode = getDeclaration( program, _('/some_directive.js'), 'SomeDirective', isNamedVariableDeclaration); const classDecorators = host.getDecoratorsOfDeclaration(classNode) !; - const decoratorNode = classDecorators[0].node; + const decoratorNode = classDecorators[0].node !; const identifierOfDirective = ts.isCallExpression(decoratorNode) && ts.isIdentifier(decoratorNode.expression) ? diff --git a/packages/compiler-cli/ngcc/test/host/umd_host_import_helper_spec.ts b/packages/compiler-cli/ngcc/test/host/umd_host_import_helper_spec.ts index 0896665b0c43d..1cd4ee7a27803 100644 --- a/packages/compiler-cli/ngcc/test/host/umd_host_import_helper_spec.ts +++ b/packages/compiler-cli/ngcc/test/host/umd_host_import_helper_spec.ts @@ -79,7 +79,7 @@ runInEachFileSystem(() => { const decorator = decorators[0]; expect(decorator.name).toEqual('Directive'); - expect(decorator.identifier.getText()).toEqual('core.Directive'); + expect(decorator.identifier !.getText()).toEqual('core.Directive'); expect(decorator.import).toEqual({name: 'Directive', from: '@angular/core'}); expect(decorator.args !.map(arg => arg.getText())).toEqual([ '{ selector: \'[someDirective]\' }', diff --git a/packages/compiler-cli/ngcc/test/integration/ngcc_spec.ts b/packages/compiler-cli/ngcc/test/integration/ngcc_spec.ts index 1ee644a9c36e7..9832733ff84aa 100644 --- a/packages/compiler-cli/ngcc/test/integration/ngcc_spec.ts +++ b/packages/compiler-cli/ngcc/test/integration/ngcc_spec.ts @@ -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}); @@ -752,6 +753,135 @@ 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;'); + }); + + 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: 'This is the base template', + }) + 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;'); + }); + + it('should generate directive definitions with CopyDefinitionFeature for undecorated child directives in a long inheritance chain', + () => { + genNodeModules({ + 'test-package': { + '/index.ts': ` + import {Directive, NgModule} from '@angular/core'; + + @Directive({ + selector: '[base]', + }) + export class BaseDir {} + + export class DerivedDir1 extends BaseDir {} + + export class DerivedDir2 extends DerivedDir1 {} + + export class DerivedDir3 extends DerivedDir2 {} + + @NgModule({ + declarations: [DerivedDir3], + }) + export class Module {} + `, + }, + }); + + mainNgcc({ + basePath: '/node_modules', + targetEntryPointPath: 'test-package', + propertiesToConsider: ['main'], + }); + + const dtsContents = fs.readFile(_(`/node_modules/test-package/index.d.ts`)); + expect(dtsContents) + .toContain( + 'static ɵdir: ɵngcc0.ɵɵDirectiveDefWithMeta;'); + expect(dtsContents) + .toContain( + 'static ɵdir: ɵngcc0.ɵɵDirectiveDefWithMeta;'); + expect(dtsContents) + .toContain( + 'static ɵdir: ɵngcc0.ɵɵDirectiveDefWithMeta;'); + }); + }); + describe('aliasing re-exports in commonjs', () => { it('should add re-exports to commonjs files', () => { loadTestFiles([ diff --git a/packages/compiler-cli/ngcc/test/integration/util.ts b/packages/compiler-cli/ngcc/test/integration/util.ts new file mode 100644 index 0000000000000..a0271d9b0fadb --- /dev/null +++ b/packages/compiler-cli/ngcc/test/integration/util.ts @@ -0,0 +1,125 @@ +/** + * @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 {FileSystem, getFileSystem} from '../../../src/ngtsc/file_system'; +import {MockFileSystemPosix} from '../../../src/ngtsc/file_system/testing'; + +import {loadStandardTestFiles} from '../../../test/helpers'; + +export type NodeModulesDef = { + [name: string]: Package +}; + +export type Package = { + [path: string]: string; +}; + +/** + * Compile one or more testing packages into the top-level `FileSystem`. + * + * Instead of writing ESM5 code by hand, and manually describing the Angular Package Format + * structure of that code in a mock NPM package, `genNodeModules` allows for the generation of one + * or more NPM packages from TypeScript source code. Each named NPM package in `def` is + * independently transpiled with `compileNodeModuleToFs` and written into `node_modules` in the + * top-level filesystem, ready for use in testing ngcc. + */ +export function genNodeModules(def: NodeModulesDef): void { + const fs = getFileSystem(); + for (const pkgName of Object.keys(def)) { + compileNodeModuleToFs(fs, pkgName, def[pkgName]); + } +} + +/** + * Takes the TypeScript project defined in the `Package` structure, compiles it to ESM5, and sets it + * up as a package in `node_modules` in `fs`. + * + * TODO(alxhub): over time, expand this to other bundle formats and make it more faithful to the + * shape of real NPM packages. + */ +function compileNodeModuleToFs(fs: FileSystem, pkgName: string, pkg: Package): void { + const compileFs = new MockFileSystemPosix(true); + compileFs.init(loadStandardTestFiles({fakeCore: false})); + + const options: ts.CompilerOptions = { + declaration: true, + module: ts.ModuleKind.ESNext, + target: ts.ScriptTarget.ES5, + lib: [], + }; + + const rootNames = Object.keys(pkg); + + for (const fileName of rootNames) { + compileFs.writeFile(compileFs.resolve(fileName), pkg[fileName]); + } + + const host = new MockCompilerHost(compileFs); + const program = ts.createProgram({host, rootNames, options}); + program.emit(); + + // Copy over the JS and .d.ts files, and add a .metadata.json for each .d.ts file. + for (const inFileTs of rootNames) { + const inFileBase = inFileTs.replace(/\.ts$/, ''); + fs.writeFile( + fs.resolve(`/node_modules/${pkgName}/${inFileBase}.d.ts`), + compileFs.readFile(compileFs.resolve(`${inFileBase}.d.ts`))); + const jsContents = compileFs.readFile(compileFs.resolve(`${inFileBase}.js`)); + fs.writeFile(fs.resolve(`/node_modules/${pkgName}/${inFileBase}.js`), jsContents); + fs.writeFile(fs.resolve(`/node_modules/${pkgName}/${inFileBase}.metadata.json`), '{}'); + } + + // Write the package.json + const pkgJson: unknown = { + name: pkgName, + version: '0.0.1', + main: './index.js', + typings: './index.d.ts', + }; + + fs.writeFile( + fs.resolve(`/node_modules/${pkgName}/package.json`), JSON.stringify(pkgJson, null, 2)); +} + +/** + * A simple `ts.CompilerHost` that uses a `FileSystem` instead of the real FS. + * + * TODO(alxhub): convert this into a first class `FileSystemCompilerHost` and use it as the base for + * the entire compiler. + */ +class MockCompilerHost implements ts.CompilerHost { + constructor(private fs: FileSystem) {} + getSourceFile( + fileName: string, languageVersion: ts.ScriptTarget, + onError?: ((message: string) => void)|undefined, + shouldCreateNewSourceFile?: boolean|undefined): ts.SourceFile|undefined { + return ts.createSourceFile( + fileName, this.fs.readFile(this.fs.resolve(fileName)), languageVersion, true, + ts.ScriptKind.TS); + } + + getDefaultLibFileName(options: ts.CompilerOptions): string { + return ts.getDefaultLibFileName(options); + } + + writeFile(fileName: string, data: string): void { + this.fs.writeFile(this.fs.resolve(fileName), data); + } + + getCurrentDirectory(): string { return this.fs.pwd(); } + getCanonicalFileName(fileName: string): string { return fileName; } + useCaseSensitiveFileNames(): boolean { return true; } + getNewLine(): string { return '\n'; } + fileExists(fileName: string): boolean { return this.fs.exists(this.fs.resolve(fileName)); } + readFile(fileName: string): string|undefined { + const abs = this.fs.resolve(fileName); + return this.fs.exists(abs) ? this.fs.readFile(abs) : undefined; + } +} diff --git a/packages/compiler-cli/ngcc/test/migrations/missing_injectable_migration_spec.ts b/packages/compiler-cli/ngcc/test/migrations/missing_injectable_migration_spec.ts new file mode 100644 index 0000000000000..707dfc1a7dac0 --- /dev/null +++ b/packages/compiler-cli/ngcc/test/migrations/missing_injectable_migration_spec.ts @@ -0,0 +1,592 @@ +/** + * @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 {AbsoluteFsPath, absoluteFrom, getFileSystem} from '../../../src/ngtsc/file_system'; +import {TestFile, runInEachFileSystem} from '../../../src/ngtsc/file_system/testing'; +import {loadFakeCore, loadTestFiles} from '../../../test/helpers'; +import {DecorationAnalyzer} from '../../src/analysis/decoration_analyzer'; +import {NgccReferencesRegistry} from '../../src/analysis/ngcc_references_registry'; +import {DecorationAnalyses} from '../../src/analysis/types'; +import {Esm2015ReflectionHost} from '../../src/host/esm2015_host'; +import {MissingInjectableMigration, getAngularCoreDecoratorName} from '../../src/migrations/missing_injectable_migration'; +import {MockLogger} from '../helpers/mock_logger'; +import {getRootFiles, makeTestEntryPointBundle} from '../helpers/utils'; + +runInEachFileSystem(() => { + describe('MissingInjectableMigration', () => { + let _: typeof absoluteFrom; + let INDEX_FILENAME: AbsoluteFsPath; + beforeEach(() => { + _ = absoluteFrom; + INDEX_FILENAME = _('/node_modules/test-package/index.js'); + }); + + describe('NgModule', () => runTests('NgModule', 'providers')); + describe('Directive', () => runTests('Directive', 'providers')); + + describe('Component', () => { + runTests('Component', 'providers'); + runTests('Component', 'viewProviders'); + + it('should migrate all providers defined in "viewProviders" and "providers" in the same ' + + 'component', + () => { + const {program, analysis} = setUpAndAnalyzeProgram([{ + name: INDEX_FILENAME, + contents: ` + import {Component} from '@angular/core'; + + export class ServiceA {} + export class ServiceB {} + export class ServiceC {} + + export class TestClass {} + TestClass.decorators = [ + { type: Component, args: [{ + template: "", + providers: [ServiceA], + viewProviders: [ServiceB], + }] + } + ]; + `, + }]); + + const index = program.getSourceFile(INDEX_FILENAME) !; + expect(hasInjectableDecorator(index, analysis, 'ServiceA')).toBe(true); + expect(hasInjectableDecorator(index, analysis, 'ServiceB')).toBe(true); + expect(hasInjectableDecorator(index, analysis, 'ServiceC')).toBe(false); + }); + }); + + function runTests( + type: 'NgModule' | 'Directive' | 'Component', propName: 'providers' | 'viewProviders') { + const args = type === 'Component' ? 'template: "", ' : ''; + + it(`should migrate type provider in ${type}`, () => { + const {program, analysis} = setUpAndAnalyzeProgram([{ + name: INDEX_FILENAME, + contents: ` + import {${type}} from '@angular/core'; + + export class MyService {} + export class OtherService {} + + export class TestClass {} + TestClass.decorators = [ + { type: ${type}, args: [{${args}${propName}: [MyService]}] } + ]; + `, + }]); + + const index = program.getSourceFile(INDEX_FILENAME) !; + expect(hasInjectableDecorator(index, analysis, 'MyService')).toBe(true); + expect(hasInjectableDecorator(index, analysis, 'OtherService')).toBe(false); + }); + + it(`should migrate object literal provider in ${type}`, () => { + const {program, analysis} = setUpAndAnalyzeProgram([{ + name: INDEX_FILENAME, + contents: ` + import {${type}} from '@angular/core'; + + export class MyService {} + export class OtherService {} + + export class TestClass {} + TestClass.decorators = [ + { type: ${type}, args: [{${args}${propName}: [{provide: MyService}]}] } + ]; + `, + }]); + + const index = program.getSourceFile(INDEX_FILENAME) !; + expect(hasInjectableDecorator(index, analysis, 'MyService')).toBe(true); + expect(hasInjectableDecorator(index, analysis, 'OtherService')).toBe(false); + }); + + it(`should migrate object literal provider with forwardRef in ${type}`, async() => { + const {program, analysis} = setUpAndAnalyzeProgram([{ + name: INDEX_FILENAME, + contents: ` + import {${type}, forwardRef} from '@angular/core'; + + export class MyService {} + + export class TestClass {} + TestClass.decorators = [ + { type: ${type}, args: [{${args}${propName}: [{provide: forwardRef(() => MyService) }]}] } + ]; + `, + }]); + + const index = program.getSourceFile(INDEX_FILENAME) !; + expect(hasInjectableDecorator(index, analysis, 'MyService')).toBe(true); + }); + + it(`should not migrate object literal provider with "useValue" in ${type}`, () => { + const {program, analysis} = setUpAndAnalyzeProgram([{ + name: INDEX_FILENAME, + contents: ` + import {${type}} from '@angular/core'; + + export class MyService {} + + export class TestClass {} + TestClass.decorators = [ + { type: ${type}, args: [{${args}${propName}: [{provide: MyService, useValue: null }]}] } + ]; + `, + }]); + + const index = program.getSourceFile(INDEX_FILENAME) !; + expect(hasInjectableDecorator(index, analysis, 'MyService')).toBe(false); + }); + + it(`should not migrate object literal provider with "useFactory" in ${type}`, () => { + const {program, analysis} = setUpAndAnalyzeProgram([{ + name: INDEX_FILENAME, + contents: ` + import {${type}} from '@angular/core'; + + export class MyService {} + + export class TestClass {} + TestClass.decorators = [ + { type: ${type}, args: [{${args}${propName}: [{provide: MyService, useFactory: () => null }]}] } + ]; + `, + }]); + + const index = program.getSourceFile(INDEX_FILENAME) !; + expect(hasInjectableDecorator(index, analysis, 'MyService')).toBe(false); + }); + + it(`should not migrate object literal provider with "useExisting" in ${type}`, () => { + const {program, analysis} = setUpAndAnalyzeProgram([{ + name: INDEX_FILENAME, + contents: ` + import {${type}} from '@angular/core'; + + export class MyService {} + export class MyToken {} + export class MyTokenAlias {} + + export class TestClass {} + TestClass.decorators = [ + { type: ${type}, args: [{${args}${propName}: [ + MyService, + {provide: MyToken, useExisting: MyService}, + {provide: MyTokenAlias, useExisting: MyToken}, + ]}] } + ]; + `, + }]); + + const index = program.getSourceFile(INDEX_FILENAME) !; + expect(hasInjectableDecorator(index, analysis, 'MyService')).toBe(true); + expect(hasInjectableDecorator(index, analysis, 'MyToken')).toBe(false); + expect(hasInjectableDecorator(index, analysis, 'MyTokenAlias')).toBe(false); + }); + + it(`should migrate object literal provider with "useClass" in ${type}`, () => { + const {program, analysis} = setUpAndAnalyzeProgram([{ + name: INDEX_FILENAME, + contents: ` + import {${type}} from '@angular/core'; + + export class MyService {} + export class MyToken {} + + export class TestClass {} + TestClass.decorators = [ + { type: ${type}, args: [{${args}${propName}: [{provide: MyToken, useClass: MyService}]}] } + ]; + `, + }]); + + const index = program.getSourceFile(INDEX_FILENAME) !; + expect(hasInjectableDecorator(index, analysis, 'MyService')).toBe(true); + expect(hasInjectableDecorator(index, analysis, 'MyToken')).toBe(false); + }); + + it('should not migrate provider which is already decorated with @Injectable', () => { + const {program, analysis} = setUpAndAnalyzeProgram([{ + name: INDEX_FILENAME, + contents: ` + import {Injectable, ${type}} from '@angular/core'; + + export class MyService {} + MyService.decorators = [ + { type: Injectable } + ]; + + export class TestClass {} + TestClass.decorators = [ + { type: ${type}, args: [{${args}${propName}: [MyService]}] } + ]; + `, + }]); + + const index = program.getSourceFile(INDEX_FILENAME) !; + expect(getInjectableDecorators(index, analysis, 'MyService').length).toBe(1); + }); + + it('should not migrate provider which is already decorated with @Directive', () => { + const {program, analysis} = setUpAndAnalyzeProgram([{ + name: INDEX_FILENAME, + contents: ` + import {Directive, ${type}} from '@angular/core'; + + export class MyService {} + MyService.decorators = [ + { type: Directive } + ]; + + export class TestClass {} + TestClass.decorators = [ + { type: ${type}, args: [{${args}${propName}: [MyService]}] } + ]; + `, + }]); + + const index = program.getSourceFile(INDEX_FILENAME) !; + expect(hasInjectableDecorator(index, analysis, 'MyService')).toBe(false); + }); + + it('should not migrate provider which is already decorated with @Component', () => { + const {program, analysis} = setUpAndAnalyzeProgram([{ + name: INDEX_FILENAME, + contents: ` + import {Component, ${type}} from '@angular/core'; + + export class MyService {} + MyService.decorators = [ + { type: Component, args: [{template: ""}] } + ]; + + export class TestClass {} + TestClass.decorators = [ + { type: ${type}, args: [{${args}${propName}: [MyService]}] } + ]; + `, + }]); + + const index = program.getSourceFile(INDEX_FILENAME) !; + expect(hasInjectableDecorator(index, analysis, 'MyService')).toBe(false); + }); + + it('should not migrate provider which is already decorated with @Pipe', () => { + const {program, analysis} = setUpAndAnalyzeProgram([{ + name: INDEX_FILENAME, + contents: ` + import {Pipe, ${type}} from '@angular/core'; + + export class MyService {} + MyService.decorators = [ + { type: Pipe, args: [{name: "pipe"}] } + ]; + + export class TestClass {} + TestClass.decorators = [ + { type: ${type}, args: [{${args}${propName}: [MyService]}] } + ]; + `, + }]); + + const index = program.getSourceFile(INDEX_FILENAME) !; + expect(hasInjectableDecorator(index, analysis, 'MyService')).toBe(false); + }); + + it(`should migrate multiple providers in same ${type}`, () => { + const {program, analysis} = setUpAndAnalyzeProgram([{ + name: INDEX_FILENAME, + contents: ` + import {${type}} from '@angular/core'; + + export class ServiceA {} + export class ServiceB {} + + export class TestClass {} + TestClass.decorators = [ + { type: ${type}, args: [{${args}${propName}: [ServiceA, ServiceB]}] } + ]; + `, + }]); + + const index = program.getSourceFile(INDEX_FILENAME) !; + expect(hasInjectableDecorator(index, analysis, 'ServiceA')).toBe(true); + expect(hasInjectableDecorator(index, analysis, 'ServiceB')).toBe(true); + }); + + it(`should migrate multiple mixed providers in same ${type}`, () => { + const {program, analysis} = setUpAndAnalyzeProgram([{ + name: INDEX_FILENAME, + contents: ` + import {${type}} from '@angular/core'; + + export class ServiceA {} + export class ServiceB {} + export class ServiceC {} + export class ServiceD {} + + export class TestClass {} + TestClass.decorators = [ + { type: ${type}, args: [{${args}${propName}: [ + ServiceA, + {provide: ServiceB}, + {provide: SomeToken, useClass: ServiceC}, + ] + }] } + ]; + `, + }]); + + const index = program.getSourceFile(INDEX_FILENAME) !; + expect(hasInjectableDecorator(index, analysis, 'ServiceA')).toBe(true); + expect(hasInjectableDecorator(index, analysis, 'ServiceB')).toBe(true); + expect(hasInjectableDecorator(index, analysis, 'ServiceC')).toBe(true); + expect(hasInjectableDecorator(index, analysis, 'ServiceD')).toBe(false); + }); + + + it(`should migrate multiple nested providers in same ${type}`, () => { + const {program, analysis} = setUpAndAnalyzeProgram([{ + name: INDEX_FILENAME, + contents: ` + import {${type}} from '@angular/core'; + + export class ServiceA {} + export class ServiceB {} + export class ServiceC {} + export class ServiceD {} + + export class TestClass {} + TestClass.decorators = [ + { type: ${type}, args: [{${args}${propName}: [ + ServiceA, + [ + {provide: ServiceB}, + ServiceC, + ], + ]}] + } + ]; + `, + }]); + + const index = program.getSourceFile(INDEX_FILENAME) !; + expect(hasInjectableDecorator(index, analysis, 'ServiceA')).toBe(true); + expect(hasInjectableDecorator(index, analysis, 'ServiceB')).toBe(true); + expect(hasInjectableDecorator(index, analysis, 'ServiceC')).toBe(true); + expect(hasInjectableDecorator(index, analysis, 'ServiceD')).toBe(false); + }); + + it('should migrate providers referenced indirectly', () => { + const {program, analysis} = setUpAndAnalyzeProgram([{ + name: INDEX_FILENAME, + contents: ` + import {${type}} from '@angular/core'; + + export class ServiceA {} + export class ServiceB {} + export class ServiceC {} + + const PROVIDERS = [ServiceA, ServiceB]; + + export class TestClass {} + TestClass.decorators = [ + { type: ${type}, args: [{${args}${propName}: PROVIDERS}] } + ]; + ` + }]); + + const index = program.getSourceFile(INDEX_FILENAME) !; + expect(hasInjectableDecorator(index, analysis, 'ServiceA')).toBe(true); + expect(hasInjectableDecorator(index, analysis, 'ServiceB')).toBe(true); + expect(hasInjectableDecorator(index, analysis, 'ServiceC')).toBe(false); + }); + + it(`should migrate provider once if referenced in multiple ${type} definitions`, () => { + const {program, analysis} = setUpAndAnalyzeProgram([{ + name: INDEX_FILENAME, + contents: ` + import {${type}} from '@angular/core'; + + export class ServiceA {} + export class ServiceB {} + + export class TestClassA {} + TestClassA.decorators = [ + { type: ${type}, args: [{${args}${propName}: [ServiceA]}] } + ]; + + export class TestClassB {} + TestClassB.decorators = [ + { type: ${type}, args: [{${args}${propName}: [ServiceA, ServiceB]}] } + ]; + ` + }]); + + const index = program.getSourceFile(INDEX_FILENAME) !; + expect(getInjectableDecorators(index, analysis, 'ServiceA').length).toBe(1); + expect(getInjectableDecorators(index, analysis, 'ServiceB').length).toBe(1); + }); + + type !== 'Component' && it(`should support @${type} without metadata argument`, () => { + const {program, analysis} = setUpAndAnalyzeProgram([{ + name: INDEX_FILENAME, + contents: ` + import {${type}} from '@angular/core'; + + export class ServiceA {} + + export class TestClass {} + TestClass.decorators = [ + { type: ${type}, args: [] } + ]; + `, + }]); + + const index = program.getSourceFile(INDEX_FILENAME) !; + expect(hasInjectableDecorator(index, analysis, 'ServiceA')).toBe(false); + }); + + it(`should migrate services in a different file`, () => { + const SERVICE_FILENAME = _('/node_modules/test-package/service.js'); + const {program, analysis} = setUpAndAnalyzeProgram([ + { + name: INDEX_FILENAME, + contents: ` + import {${type}} from '@angular/core'; + import {MyService} from './service'; + + export class TestClass {} + TestClass.decorators = [ + { type: ${type}, args: [{${args}${propName}: [MyService]}] } + ]; + `, + }, + { + name: SERVICE_FILENAME, + contents: ` + export declare class MyService {} + `, + } + ]); + + const index = program.getSourceFile(SERVICE_FILENAME) !; + expect(hasInjectableDecorator(index, analysis, 'MyService')).toBe(true); + }); + + it(`should not migrate services in a different package`, () => { + const SERVICE_FILENAME = _('/node_modules/external/index.d.ts'); + const {program, analysis} = setUpAndAnalyzeProgram([ + { + name: INDEX_FILENAME, + contents: ` + import {${type}} from '@angular/core'; + import {MyService} from 'external'; + + export class TestClass {} + TestClass.decorators = [ + { type: ${type}, args: [{${args}${propName}: [MyService]}] } + ]; + `, + }, + { + name: SERVICE_FILENAME, + contents: ` + export declare class MyService {} + `, + } + ]); + + const index = program.getSourceFile(SERVICE_FILENAME) !; + expect(hasInjectableDecorator(index, analysis, 'MyService')).toBe(false); + }); + + it(`should deal with renamed imports for @${type}`, () => { + const {program, analysis} = setUpAndAnalyzeProgram([{ + name: INDEX_FILENAME, + contents: ` + import {${type} as Renamed} from '@angular/core'; + + export class MyService {} + + export class TestClass {} + TestClass.decorators = [ + { type: Renamed, args: [{${args}${propName}: [MyService]}] } + ]; + `, + }]); + + const index = program.getSourceFile(INDEX_FILENAME) !; + expect(hasInjectableDecorator(index, analysis, 'MyService')).toBe(true); + }); + + it(`should deal with decorators named @${type} not from '@angular/core'`, () => { + const {program, analysis} = setUpAndAnalyzeProgram([{ + name: INDEX_FILENAME, + contents: ` + import {${type}} from 'other'; + + export class MyService {} + + export class TestClass {} + TestClass.decorators = [ + { type: ${type}, args: [{${args}${propName}: [MyService]}] } + ]; + `, + }]); + + const index = program.getSourceFile(INDEX_FILENAME) !; + expect(hasInjectableDecorator(index, analysis, 'MyService')).toBe(false); + }); + } + + function setUpAndAnalyzeProgram(testFiles: TestFile[]) { + loadTestFiles(testFiles); + loadFakeCore(getFileSystem()); + const errors: ts.Diagnostic[] = []; + const rootFiles = getRootFiles(testFiles); + const bundle = makeTestEntryPointBundle('test-package', 'esm2015', false, rootFiles); + const program = bundle.src.program; + + const reflectionHost = + new Esm2015ReflectionHost(new MockLogger(), false, program.getTypeChecker()); + const referencesRegistry = new NgccReferencesRegistry(reflectionHost); + const analyzer = new DecorationAnalyzer( + getFileSystem(), bundle, reflectionHost, referencesRegistry, error => errors.push(error)); + analyzer.migrations = [new MissingInjectableMigration()]; + return {program, analysis: analyzer.analyzeProgram(), errors}; + } + + function getInjectableDecorators( + sourceFile: ts.SourceFile, analysis: DecorationAnalyses, className: string) { + const file = analysis.get(sourceFile); + if (file === undefined) { + return []; + } + + const clazz = file.compiledClasses.find(c => c.name === className); + if (clazz === undefined || clazz.decorators === null) { + return []; + } + + return clazz.decorators.filter( + decorator => getAngularCoreDecoratorName(decorator) === 'Injectable'); + } + + function hasInjectableDecorator( + sourceFile: ts.SourceFile, analysis: DecorationAnalyses, className: string) { + return getInjectableDecorators(sourceFile, analysis, className).length > 0; + } + }); +}); 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/ngcc/test/rendering/commonjs_rendering_formatter_spec.ts b/packages/compiler-cli/ngcc/test/rendering/commonjs_rendering_formatter_spec.ts index 6564513056537..14ddeeefdc328 100644 --- a/packages/compiler-cli/ngcc/test/rendering/commonjs_rendering_formatter_spec.ts +++ b/packages/compiler-cli/ngcc/test/rendering/commonjs_rendering_formatter_spec.ts @@ -356,7 +356,7 @@ SOME DEFINITION TEXT decorationAnalyses.get(sourceFile) !.compiledClasses.find(c => c.name === 'A') !; const decorator = compiledClass.decorators ![0]; const decoratorsToRemove = new Map(); - decoratorsToRemove.set(decorator.node.parent !, [decorator.node]); + decoratorsToRemove.set(decorator.node !.parent !, [decorator.node !]); renderer.removeDecorators(output, decoratorsToRemove); expect(output.toString()) .not.toContain(`{ type: core.Directive, args: [{ selector: '[a]' }] },`); @@ -377,7 +377,7 @@ SOME DEFINITION TEXT decorationAnalyses.get(sourceFile) !.compiledClasses.find(c => c.name === 'B') !; const decorator = compiledClass.decorators ![0]; const decoratorsToRemove = new Map(); - decoratorsToRemove.set(decorator.node.parent !, [decorator.node]); + decoratorsToRemove.set(decorator.node !.parent !, [decorator.node !]); renderer.removeDecorators(output, decoratorsToRemove); expect(output.toString()) .toContain(`{ type: core.Directive, args: [{ selector: '[a]' }] },`); @@ -398,7 +398,7 @@ SOME DEFINITION TEXT decorationAnalyses.get(sourceFile) !.compiledClasses.find(c => c.name === 'C') !; const decorator = compiledClass.decorators ![0]; const decoratorsToRemove = new Map(); - decoratorsToRemove.set(decorator.node.parent !, [decorator.node]); + decoratorsToRemove.set(decorator.node !.parent !, [decorator.node !]); renderer.removeDecorators(output, decoratorsToRemove); renderer.addDefinitions(output, compiledClass, 'SOME DEFINITION TEXT'); expect(output.toString()) @@ -423,7 +423,7 @@ SOME DEFINITION TEXT decorationAnalyses.get(sourceFile) !.compiledClasses.find(c => c.name === 'A') !; const decorator = compiledClass.decorators !.find(d => d.name === 'Directive') !; const decoratorsToRemove = new Map(); - decoratorsToRemove.set(decorator.node.parent !, [decorator.node]); + decoratorsToRemove.set(decorator.node !.parent !, [decorator.node !]); renderer.removeDecorators(output, decoratorsToRemove); expect(output.toString()).not.toContain(`Directive({ selector: '[a]' }),`); expect(output.toString()).toContain(`OtherA()`); @@ -440,7 +440,7 @@ SOME DEFINITION TEXT decorationAnalyses.get(sourceFile) !.compiledClasses.find(c => c.name === 'B') !; const decorator = compiledClass.decorators !.find(d => d.name === 'Directive') !; const decoratorsToRemove = new Map(); - decoratorsToRemove.set(decorator.node.parent !, [decorator.node]); + decoratorsToRemove.set(decorator.node !.parent !, [decorator.node !]); renderer.removeDecorators(output, decoratorsToRemove); expect(output.toString()).toContain(`Directive({ selector: '[a]' }),`); expect(output.toString()).toContain(`OtherA()`); @@ -458,7 +458,7 @@ SOME DEFINITION TEXT decorationAnalyses.get(sourceFile) !.compiledClasses.find(c => c.name === 'C') !; const decorator = compiledClass.decorators !.find(d => d.name === 'Directive') !; const decoratorsToRemove = new Map(); - decoratorsToRemove.set(decorator.node.parent !, [decorator.node]); + decoratorsToRemove.set(decorator.node !.parent !, [decorator.node !]); renderer.removeDecorators(output, decoratorsToRemove); expect(output.toString()).toContain(`Directive({ selector: '[a]' }),`); expect(output.toString()).toContain(`OtherA()`); diff --git a/packages/compiler-cli/ngcc/test/rendering/esm5_rendering_formatter_spec.ts b/packages/compiler-cli/ngcc/test/rendering/esm5_rendering_formatter_spec.ts index 387b5197d4b9c..1b7b64ee4d39a 100644 --- a/packages/compiler-cli/ngcc/test/rendering/esm5_rendering_formatter_spec.ts +++ b/packages/compiler-cli/ngcc/test/rendering/esm5_rendering_formatter_spec.ts @@ -345,7 +345,7 @@ SOME DEFINITION TEXT decorationAnalyses.get(sourceFile) !.compiledClasses.find(c => c.name === 'A') !; const decorator = compiledClass.decorators ![0]; const decoratorsToRemove = new Map(); - decoratorsToRemove.set(decorator.node.parent !, [decorator.node]); + decoratorsToRemove.set(decorator.node !.parent !, [decorator.node !]); renderer.removeDecorators(output, decoratorsToRemove); expect(output.toString()) .not.toContain(`{ type: Directive, args: [{ selector: '[a]' }] },`); @@ -364,7 +364,7 @@ SOME DEFINITION TEXT decorationAnalyses.get(sourceFile) !.compiledClasses.find(c => c.name === 'B') !; const decorator = compiledClass.decorators ![0]; const decoratorsToRemove = new Map(); - decoratorsToRemove.set(decorator.node.parent !, [decorator.node]); + decoratorsToRemove.set(decorator.node !.parent !, [decorator.node !]); renderer.removeDecorators(output, decoratorsToRemove); expect(output.toString()).toContain(`{ type: Directive, args: [{ selector: '[a]' }] },`); expect(output.toString()).toContain(`{ type: OtherA }`); @@ -383,7 +383,7 @@ SOME DEFINITION TEXT decorationAnalyses.get(sourceFile) !.compiledClasses.find(c => c.name === 'C') !; const decorator = compiledClass.decorators ![0]; const decoratorsToRemove = new Map(); - decoratorsToRemove.set(decorator.node.parent !, [decorator.node]); + decoratorsToRemove.set(decorator.node !.parent !, [decorator.node !]); renderer.removeDecorators(output, decoratorsToRemove); renderer.addDefinitions(output, compiledClass, 'SOME DEFINITION TEXT'); expect(output.toString()).toContain(`{ type: Directive, args: [{ selector: '[a]' }] },`); @@ -406,7 +406,7 @@ SOME DEFINITION TEXT decorationAnalyses.get(sourceFile) !.compiledClasses.find(c => c.name === 'A') !; const decorator = compiledClass.decorators !.find(d => d.name === 'Directive') !; const decoratorsToRemove = new Map(); - decoratorsToRemove.set(decorator.node.parent !, [decorator.node]); + decoratorsToRemove.set(decorator.node !.parent !, [decorator.node !]); renderer.removeDecorators(output, decoratorsToRemove); expect(output.toString()).not.toContain(`Directive({ selector: '[a]' }),`); expect(output.toString()).toContain(`OtherA()`); @@ -423,7 +423,7 @@ SOME DEFINITION TEXT decorationAnalyses.get(sourceFile) !.compiledClasses.find(c => c.name === 'B') !; const decorator = compiledClass.decorators !.find(d => d.name === 'Directive') !; const decoratorsToRemove = new Map(); - decoratorsToRemove.set(decorator.node.parent !, [decorator.node]); + decoratorsToRemove.set(decorator.node !.parent !, [decorator.node !]); renderer.removeDecorators(output, decoratorsToRemove); expect(output.toString()).toContain(`Directive({ selector: '[a]' }),`); expect(output.toString()).toContain(`OtherA()`); @@ -441,7 +441,7 @@ SOME DEFINITION TEXT decorationAnalyses.get(sourceFile) !.compiledClasses.find(c => c.name === 'C') !; const decorator = compiledClass.decorators !.find(d => d.name === 'Directive') !; const decoratorsToRemove = new Map(); - decoratorsToRemove.set(decorator.node.parent !, [decorator.node]); + decoratorsToRemove.set(decorator.node !.parent !, [decorator.node !]); renderer.removeDecorators(output, decoratorsToRemove); expect(output.toString()).toContain(`Directive({ selector: '[a]' }),`); expect(output.toString()).toContain(`OtherA()`); diff --git a/packages/compiler-cli/ngcc/test/rendering/esm_rendering_formatter_spec.ts b/packages/compiler-cli/ngcc/test/rendering/esm_rendering_formatter_spec.ts index bda5e1da02ad1..7dbb778cf05f8 100644 --- a/packages/compiler-cli/ngcc/test/rendering/esm_rendering_formatter_spec.ts +++ b/packages/compiler-cli/ngcc/test/rendering/esm_rendering_formatter_spec.ts @@ -255,7 +255,7 @@ A.decorators = [ decorationAnalyses.get(sourceFile) !.compiledClasses.find(c => c.name === 'A') !; const decorator = compiledClass.decorators ![0]; const decoratorsToRemove = new Map(); - decoratorsToRemove.set(decorator.node.parent !, [decorator.node]); + decoratorsToRemove.set(decorator.node !.parent !, [decorator.node !]); renderer.removeDecorators(output, decoratorsToRemove); expect(output.toString()) .not.toContain(`{ type: Directive, args: [{ selector: '[a]' }] },`); @@ -276,7 +276,7 @@ A.decorators = [ decorationAnalyses.get(sourceFile) !.compiledClasses.find(c => c.name === 'B') !; const decorator = compiledClass.decorators ![0]; const decoratorsToRemove = new Map(); - decoratorsToRemove.set(decorator.node.parent !, [decorator.node]); + decoratorsToRemove.set(decorator.node !.parent !, [decorator.node !]); renderer.removeDecorators(output, decoratorsToRemove); expect(output.toString()) .toContain(`{ type: Directive, args: [{ selector: '[a]' }] },`); @@ -304,7 +304,7 @@ A.decorators = [ decorationAnalyses.get(sourceFile) !.compiledClasses.find(c => c.name === 'A') !; const decorator = compiledClass.decorators ![0]; const decoratorsToRemove = new Map(); - decoratorsToRemove.set(decorator.node.parent !, [decorator.node]); + decoratorsToRemove.set(decorator.node !.parent !, [decorator.node !]); renderer.removeDecorators(output, decoratorsToRemove); // The decorator should have been removed correctly. expect(output.toString()).toContain('A.decorators = [ { type: OtherA }'); @@ -319,7 +319,7 @@ A.decorators = [ decorationAnalyses.get(sourceFile) !.compiledClasses.find(c => c.name === 'C') !; const decorator = compiledClass.decorators ![0]; const decoratorsToRemove = new Map(); - decoratorsToRemove.set(decorator.node.parent !, [decorator.node]); + decoratorsToRemove.set(decorator.node !.parent !, [decorator.node !]); renderer.removeDecorators(output, decoratorsToRemove); expect(output.toString()) .toContain(`{ type: Directive, args: [{ selector: '[a]' }] },`); @@ -385,7 +385,7 @@ export { D }; decorationAnalyses.get(sourceFile) !.compiledClasses.find(c => c.name === 'A') !; const decorator = compiledClass.decorators !.find(d => d.name === 'Directive') !; const decoratorsToRemove = new Map(); - decoratorsToRemove.set(decorator.node.parent !, [decorator.node]); + decoratorsToRemove.set(decorator.node !.parent !, [decorator.node !]); renderer.removeDecorators(output, decoratorsToRemove); expect(output.toString()).not.toContain(`Directive({ selector: '[a]' }),`); expect(output.toString()).toContain(`OtherA()`); @@ -402,7 +402,7 @@ export { D }; decorationAnalyses.get(sourceFile) !.compiledClasses.find(c => c.name === 'B') !; const decorator = compiledClass.decorators !.find(d => d.name === 'Directive') !; const decoratorsToRemove = new Map(); - decoratorsToRemove.set(decorator.node.parent !, [decorator.node]); + decoratorsToRemove.set(decorator.node !.parent !, [decorator.node !]); renderer.removeDecorators(output, decoratorsToRemove); expect(output.toString()).toContain(`Directive({ selector: '[a]' }),`); expect(output.toString()).toContain(`OtherA()`); @@ -420,7 +420,7 @@ export { D }; decorationAnalyses.get(sourceFile) !.compiledClasses.find(c => c.name === 'C') !; const decorator = compiledClass.decorators !.find(d => d.name === 'Directive') !; const decoratorsToRemove = new Map(); - decoratorsToRemove.set(decorator.node.parent !, [decorator.node]); + decoratorsToRemove.set(decorator.node !.parent !, [decorator.node !]); renderer.removeDecorators(output, decoratorsToRemove); expect(output.toString()).toContain(`Directive({ selector: '[a]' }),`); expect(output.toString()).toContain(`OtherA()`); diff --git a/packages/compiler-cli/ngcc/test/rendering/umd_rendering_formatter_spec.ts b/packages/compiler-cli/ngcc/test/rendering/umd_rendering_formatter_spec.ts index c67c5d5d7f735..1a5158dd05ba2 100644 --- a/packages/compiler-cli/ngcc/test/rendering/umd_rendering_formatter_spec.ts +++ b/packages/compiler-cli/ngcc/test/rendering/umd_rendering_formatter_spec.ts @@ -425,7 +425,7 @@ SOME DEFINITION TEXT decorationAnalyses.get(sourceFile) !.compiledClasses.find(c => c.name === 'A') !; const decorator = compiledClass.decorators ![0]; const decoratorsToRemove = new Map(); - decoratorsToRemove.set(decorator.node.parent !, [decorator.node]); + decoratorsToRemove.set(decorator.node !.parent !, [decorator.node !]); renderer.removeDecorators(output, decoratorsToRemove); expect(output.toString()) .not.toContain(`{ type: core.Directive, args: [{ selector: '[a]' }] },`); @@ -446,7 +446,7 @@ SOME DEFINITION TEXT decorationAnalyses.get(sourceFile) !.compiledClasses.find(c => c.name === 'B') !; const decorator = compiledClass.decorators ![0]; const decoratorsToRemove = new Map(); - decoratorsToRemove.set(decorator.node.parent !, [decorator.node]); + decoratorsToRemove.set(decorator.node !.parent !, [decorator.node !]); renderer.removeDecorators(output, decoratorsToRemove); expect(output.toString()) .toContain(`{ type: core.Directive, args: [{ selector: '[a]' }] },`); @@ -467,7 +467,7 @@ SOME DEFINITION TEXT decorationAnalyses.get(sourceFile) !.compiledClasses.find(c => c.name === 'C') !; const decorator = compiledClass.decorators ![0]; const decoratorsToRemove = new Map(); - decoratorsToRemove.set(decorator.node.parent !, [decorator.node]); + decoratorsToRemove.set(decorator.node !.parent !, [decorator.node !]); renderer.removeDecorators(output, decoratorsToRemove); renderer.addDefinitions(output, compiledClass, 'SOME DEFINITION TEXT'); expect(output.toString()) @@ -490,7 +490,7 @@ SOME DEFINITION TEXT decorationAnalyses.get(sourceFile) !.compiledClasses.find(c => c.name === 'A') !; const decorator = compiledClass.decorators !.find(d => d.name === 'Directive') !; const decoratorsToRemove = new Map(); - decoratorsToRemove.set(decorator.node.parent !, [decorator.node]); + decoratorsToRemove.set(decorator.node !.parent !, [decorator.node !]); renderer.removeDecorators(output, decoratorsToRemove); expect(output.toString()).not.toContain(`core.Directive({ selector: '[a]' }),`); expect(output.toString()).toContain(`OtherA()`); @@ -507,7 +507,7 @@ SOME DEFINITION TEXT decorationAnalyses.get(sourceFile) !.compiledClasses.find(c => c.name === 'B') !; const decorator = compiledClass.decorators !.find(d => d.name === 'Directive') !; const decoratorsToRemove = new Map(); - decoratorsToRemove.set(decorator.node.parent !, [decorator.node]); + decoratorsToRemove.set(decorator.node !.parent !, [decorator.node !]); renderer.removeDecorators(output, decoratorsToRemove); expect(output.toString()).toContain(`core.Directive({ selector: '[a]' }),`); expect(output.toString()).toContain(`OtherA()`); @@ -525,7 +525,7 @@ SOME DEFINITION TEXT decorationAnalyses.get(sourceFile) !.compiledClasses.find(c => c.name === 'C') !; const decorator = compiledClass.decorators !.find(d => d.name === 'Directive') !; const decoratorsToRemove = new Map(); - decoratorsToRemove.set(decorator.node.parent !, [decorator.node]); + decoratorsToRemove.set(decorator.node !.parent !, [decorator.node !]); renderer.removeDecorators(output, decoratorsToRemove); expect(output.toString()).toContain(`core.Directive({ selector: '[a]' }),`); expect(output.toString()).toContain(`OtherA()`); diff --git a/packages/compiler-cli/src/ngtsc/annotations/index.ts b/packages/compiler-cli/src/ngtsc/annotations/index.ts index 2790a74f868ac..3d5ff3cb64f7f 100644 --- a/packages/compiler-cli/src/ngtsc/annotations/index.ts +++ b/packages/compiler-cli/src/ngtsc/annotations/index.ts @@ -16,3 +16,4 @@ export {InjectableDecoratorHandler} from './src/injectable'; export {NgModuleDecoratorHandler} from './src/ng_module'; export {PipeDecoratorHandler} from './src/pipe'; export {NoopReferencesRegistry, ReferencesRegistry} from './src/references_registry'; +export {forwardRefResolver} from './src/util'; diff --git a/packages/compiler-cli/src/ngtsc/annotations/src/component.ts b/packages/compiler-cli/src/ngtsc/annotations/src/component.ts index 4578da8b48e02..28f8d03f8bc4b 100644 --- a/packages/compiler-cli/src/ngtsc/annotations/src/component.ts +++ b/packages/compiler-cli/src/ngtsc/annotations/src/component.ts @@ -19,7 +19,7 @@ import {flattenInheritedDirectiveMetadata} from '../../metadata/src/inheritance' import {EnumValue, PartialEvaluator} from '../../partial_evaluator'; import {ClassDeclaration, Decorator, ReflectionHost, reflectObjectLiteral} from '../../reflection'; import {ComponentScopeReader, LocalModuleScopeRegistry} from '../../scope'; -import {AnalysisOutput, CompileResult, DecoratorHandler, DetectResult, HandlerPrecedence, ResolveResult} from '../../transform'; +import {AnalysisOutput, CompileResult, DecoratorHandler, DetectResult, HandlerFlags, HandlerPrecedence, ResolveResult} from '../../transform'; import {TemplateSourceMapping, TypeCheckContext} from '../../typecheck'; import {NoopResourceDependencyRecorder, ResourceDependencyRecorder} from '../../util/src/resource_recorder'; import {tsSourceMapBug29300Fixed} from '../../util/src/ts_source_map_bug_29300'; @@ -137,7 +137,8 @@ export class ComponentDecoratorHandler implements } } - analyze(node: ClassDeclaration, decorator: Decorator): AnalysisOutput { + analyze(node: ClassDeclaration, decorator: Decorator, flags: HandlerFlags = HandlerFlags.NONE): + AnalysisOutput { const containingFile = node.getSourceFile().fileName; this.literalCache.delete(decorator); @@ -145,7 +146,7 @@ export class ComponentDecoratorHandler implements // on it. const directiveResult = extractDirectiveMetadata( node, decorator, this.reflector, this.evaluator, this.defaultImportRecorder, this.isCore, - this.elementSchemaRegistry.getDefaultComponentElementName()); + flags, this.elementSchemaRegistry.getDefaultComponentElementName()); if (directiveResult === undefined) { // `extractDirectiveMetadata` returns undefined when the @Directive has `jit: true`. In this // case, compilation of the decorator is skipped. Returning an empty object signifies @@ -509,7 +510,7 @@ export class ComponentDecoratorHandler implements } if (decorator.args === null || decorator.args.length !== 1) { throw new FatalDiagnosticError( - ErrorCode.DECORATOR_ARITY_WRONG, decorator.node, + ErrorCode.DECORATOR_ARITY_WRONG, Decorator.nodeForError(decorator), `Incorrect number of arguments to @Component decorator`); } const meta = unwrapExpression(decorator.args[0]); @@ -624,7 +625,8 @@ export class ComponentDecoratorHandler implements } { if (!component.has('template')) { throw new FatalDiagnosticError( - ErrorCode.COMPONENT_MISSING_TEMPLATE, decorator.node, 'component is missing a template'); + ErrorCode.COMPONENT_MISSING_TEMPLATE, Decorator.nodeForError(decorator), + 'component is missing a template'); } const templateExpr = component.get('template') !; diff --git a/packages/compiler-cli/src/ngtsc/annotations/src/directive.ts b/packages/compiler-cli/src/ngtsc/annotations/src/directive.ts index d4cd46cabdffb..adbb5069408d5 100644 --- a/packages/compiler-cli/src/ngtsc/annotations/src/directive.ts +++ b/packages/compiler-cli/src/ngtsc/annotations/src/directive.ts @@ -15,7 +15,7 @@ import {MetadataRegistry} from '../../metadata'; import {extractDirectiveGuards} from '../../metadata/src/util'; import {DynamicValue, EnumValue, PartialEvaluator} from '../../partial_evaluator'; import {ClassDeclaration, ClassMember, ClassMemberKind, Decorator, ReflectionHost, filterToMembersWithDecorator, reflectObjectLiteral} from '../../reflection'; -import {AnalysisOutput, CompileResult, DecoratorHandler, DetectResult, HandlerPrecedence} from '../../transform'; +import {AnalysisOutput, CompileResult, DecoratorHandler, DetectResult, HandlerFlags, HandlerPrecedence} from '../../transform'; import {compileNgFactoryDefField} from './factory'; import {generateSetClassMetadataCall} from './metadata'; @@ -51,9 +51,11 @@ export class DirectiveDecoratorHandler implements } } - analyze(node: ClassDeclaration, decorator: Decorator): AnalysisOutput { + analyze(node: ClassDeclaration, decorator: Decorator, flags = HandlerFlags.NONE): + AnalysisOutput { const directiveResult = extractDirectiveMetadata( - node, decorator, this.reflector, this.evaluator, this.defaultImportRecorder, this.isCore); + node, decorator, this.reflector, this.evaluator, this.defaultImportRecorder, this.isCore, + flags); const analysis = directiveResult && directiveResult.metadata; if (analysis === undefined) { return {}; @@ -111,7 +113,7 @@ export class DirectiveDecoratorHandler implements export function extractDirectiveMetadata( clazz: ClassDeclaration, decorator: Decorator, reflector: ReflectionHost, evaluator: PartialEvaluator, defaultImportRecorder: DefaultImportRecorder, isCore: boolean, - defaultSelector: string | null = null): { + flags: HandlerFlags, defaultSelector: string | null = null): { decorator: Map, metadata: R3DirectiveMetadata, decoratedElements: ClassMember[], @@ -121,7 +123,7 @@ export function extractDirectiveMetadata( directive = new Map(); } else if (decorator.args.length !== 1) { throw new FatalDiagnosticError( - ErrorCode.DECORATOR_ARITY_WRONG, decorator.node, + ErrorCode.DECORATOR_ARITY_WRONG, Decorator.nodeForError(decorator), `Incorrect number of arguments to @${decorator.name} decorator`); } else { const meta = unwrapExpression(decorator.args[0]); @@ -236,6 +238,7 @@ export function extractDirectiveMetadata( }, inputs: {...inputsFromMeta, ...inputsFromFields}, outputs: {...outputsFromMeta, ...outputsFromFields}, queries, viewQueries, selector, + fullInheritance: !!(flags & HandlerFlags.FULL_INHERITANCE), type: new WrappedNodeExpr(clazz.name), typeArgumentCount: reflector.getGenericArityOfClass(clazz) || 0, typeSourceSpan: EMPTY_SOURCE_SPAN, usesInheritance, exportAs, providers @@ -447,7 +450,7 @@ export function queriesFromFields( evaluator: PartialEvaluator): R3QueryMetadata[] { return fields.map(({member, decorators}) => { const decorator = decorators[0]; - const node = member.node || decorator.node; + const node = member.node || Decorator.nodeForError(decorator); // Throw in case of `@Input() @ContentChild('foo') foo: any`, which is not supported in Ivy if (member.decorators !.some(v => v.name === 'Input')) { @@ -465,7 +468,7 @@ export function queriesFromFields( 'Query decorator must go on a property-type member'); } return extractQueryMetadata( - decorator.node, decorator.name, decorator.args || [], member.name, reflector, evaluator); + node, decorator.name, decorator.args || [], member.name, reflector, evaluator); }); } diff --git a/packages/compiler-cli/src/ngtsc/annotations/src/injectable.ts b/packages/compiler-cli/src/ngtsc/annotations/src/injectable.ts index b2b6a4d58330f..c1ca25603b266 100644 --- a/packages/compiler-cli/src/ngtsc/annotations/src/injectable.ts +++ b/packages/compiler-cli/src/ngtsc/annotations/src/injectable.ts @@ -116,7 +116,8 @@ function extractInjectableMetadata( const typeArgumentCount = reflector.getGenericArityOfClass(clazz) || 0; if (decorator.args === null) { throw new FatalDiagnosticError( - ErrorCode.DECORATOR_NOT_CALLED, decorator.node, '@Injectable must be called'); + ErrorCode.DECORATOR_NOT_CALLED, Decorator.nodeForError(decorator), + '@Injectable must be called'); } if (decorator.args.length === 0) { return { @@ -202,7 +203,8 @@ function extractInjectableCtorDeps( strictCtorDeps: boolean) { if (decorator.args === null) { throw new FatalDiagnosticError( - ErrorCode.DECORATOR_NOT_CALLED, decorator.node, '@Injectable must be called'); + ErrorCode.DECORATOR_NOT_CALLED, Decorator.nodeForError(decorator), + '@Injectable must be called'); } let ctorDeps: R3DependencyMetadata[]|'invalid'|null = null; diff --git a/packages/compiler-cli/src/ngtsc/annotations/src/metadata.ts b/packages/compiler-cli/src/ngtsc/annotations/src/metadata.ts index 595a9957cf675..4c82fec50639b 100644 --- a/packages/compiler-cli/src/ngtsc/annotations/src/metadata.ts +++ b/packages/compiler-cli/src/ngtsc/annotations/src/metadata.ts @@ -122,6 +122,9 @@ function classMemberToMetadata( * Convert a reflected decorator to metadata. */ function decoratorToMetadata(decorator: Decorator): ts.ObjectLiteralExpression { + if (decorator.identifier === null) { + throw new Error('Illegal state: synthesized decorator cannot be emitted in class metadata.'); + } // Decorators have a type. const properties: ts.ObjectLiteralElementLike[] = [ ts.createPropertyAssignment('type', ts.getMutableClone(decorator.identifier)), diff --git a/packages/compiler-cli/src/ngtsc/annotations/src/ng_module.ts b/packages/compiler-cli/src/ngtsc/annotations/src/ng_module.ts index c40d354581d81..e8e3f48af0a41 100644 --- a/packages/compiler-cli/src/ngtsc/annotations/src/ng_module.ts +++ b/packages/compiler-cli/src/ngtsc/annotations/src/ng_module.ts @@ -67,7 +67,7 @@ export class NgModuleDecoratorHandler implements DecoratorHandler 1) { throw new FatalDiagnosticError( - ErrorCode.DECORATOR_ARITY_WRONG, decorator.node, + ErrorCode.DECORATOR_ARITY_WRONG, Decorator.nodeForError(decorator), `Incorrect number of arguments to @NgModule decorator`); } diff --git a/packages/compiler-cli/src/ngtsc/annotations/src/pipe.ts b/packages/compiler-cli/src/ngtsc/annotations/src/pipe.ts index d8668b439a29f..0857548a99850 100644 --- a/packages/compiler-cli/src/ngtsc/annotations/src/pipe.ts +++ b/packages/compiler-cli/src/ngtsc/annotations/src/pipe.ts @@ -53,11 +53,13 @@ export class PipeDecoratorHandler implements DecoratorHandler|'dynamic'|null { - if (!isNamedClassDeclaration(node)) { - // If the node isn't a ts.ClassDeclaration, consider any base class to be dynamic for now. - return reflector.hasBaseClass(node) ? 'dynamic' : null; - } - const baseExpression = reflector.getBaseClassExpression(node); if (baseExpression !== null) { const baseClass = evaluator.evaluate(baseExpression); - if (baseClass instanceof Reference && isNamedClassDeclaration(baseClass.node)) { + if (baseClass instanceof Reference && reflector.isClass(baseClass.node)) { return baseClass as Reference; } else { return 'dynamic'; 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. */ diff --git a/packages/compiler-cli/src/ngtsc/reflection/src/host.ts b/packages/compiler-cli/src/ngtsc/reflection/src/host.ts index 8593dfd7c1af4..a55f0a20f5000 100644 --- a/packages/compiler-cli/src/ngtsc/reflection/src/host.ts +++ b/packages/compiler-cli/src/ngtsc/reflection/src/host.ts @@ -9,9 +9,12 @@ import * as ts from 'typescript'; /** - * Metadata extracted from an instance of a decorator on another declaration. + * Metadata extracted from an instance of a decorator on another declaration, or synthesized from + * other information about a class. */ -export interface Decorator { +export type Decorator = ConcreteDecorator | SyntheticDecorator; + +export interface BaseDecorator { /** * Name by which the decorator was invoked in the user's code. * @@ -23,7 +26,7 @@ export interface Decorator { /** * Identifier which refers to the decorator in the user's code. */ - identifier: DecoratorIdentifier; + identifier: DecoratorIdentifier|null; /** * `Import` by which the decorator was brought into the module in which it was invoked, or `null` @@ -32,16 +35,54 @@ export interface Decorator { import : Import | null; /** - * TypeScript reference to the decorator itself. + * TypeScript reference to the decorator itself, or `null` if the decorator is synthesized (e.g. + * in ngcc). */ - node: ts.Node; + node: ts.Node|null; /** - * Arguments of the invocation of the decorator, if the decorator is invoked, or `null` otherwise. + * Arguments of the invocation of the decorator, if the decorator is invoked, or `null` + * otherwise. */ args: ts.Expression[]|null; } +/** + * Metadata extracted from an instance of a decorator on another declaration, which was actually + * present in a file. + * + * Concrete decorators always have an `identifier` and a `node`. + */ +export interface ConcreteDecorator extends BaseDecorator { + identifier: DecoratorIdentifier; + node: ts.Node; +} + +/** + * Synthetic decorators never have an `identifier` or a `node`, but know the node for which they + * were synthesized. + */ +export interface SyntheticDecorator extends BaseDecorator { + identifier: null; + node: null; + + /** + * The `ts.Node` for which this decorator was created. + */ + synthesizedFor: ts.Node; +} + +export const Decorator = { + nodeForError: (decorator: Decorator): ts.Node => { + if (decorator.node !== null) { + return decorator.node; + } else { + // TODO(alxhub): we can't rely on narrowing until TS 3.6 is in g3. + return (decorator as SyntheticDecorator).synthesizedFor; + } + }, +}; + /** * A decorator is identified by either a simple identifier (e.g. `Decorator`) or, in some cases, * a namespaced property access (e.g. `core.Decorator`). diff --git a/packages/compiler-cli/src/ngtsc/transform/src/api.ts b/packages/compiler-cli/src/ngtsc/transform/src/api.ts index 7f1db50c7c9ba..6c01f812b4a49 100644 --- a/packages/compiler-cli/src/ngtsc/transform/src/api.ts +++ b/packages/compiler-cli/src/ngtsc/transform/src/api.ts @@ -37,6 +37,27 @@ export enum HandlerPrecedence { WEAK, } +/** + * A set of options which can be passed to a `DecoratorHandler` by a consumer, to tailor the output + * of compilation beyond the decorators themselves. + */ +export enum HandlerFlags { + /** + * No flags set. + */ + NONE = 0x0, + + /** + * Indicates that this decorator is fully inherited from its parent at runtime. In addition to + * normally inherited aspects such as inputs and queries, full inheritance applies to every aspect + * of the component or directive, such as the template function itself. + * + * Its primary effect is to cause the `CopyDefinitionFeature` to be applied to the definition + * being compiled. See that class for more information. + */ + FULL_INHERITANCE = 0x00000001, +} + /** * Provides the interface between a decorator compiler from @angular/compiler and the Typescript @@ -75,7 +96,7 @@ export interface DecoratorHandler { * if successful, or an array of diagnostic messages if the analysis fails or the decorator * isn't valid. */ - analyze(node: ClassDeclaration, metadata: M): AnalysisOutput; + analyze(node: ClassDeclaration, metadata: M, handlerFlags?: HandlerFlags): AnalysisOutput; /** * Registers information about the decorator for the indexing phase in a diff --git a/packages/compiler/src/jit_compiler_facade.ts b/packages/compiler/src/jit_compiler_facade.ts index 8be8159abd013..c4a2c307abf49 100644 --- a/packages/compiler/src/jit_compiler_facade.ts +++ b/packages/compiler/src/jit_compiler_facade.ts @@ -271,6 +271,7 @@ function convertDirectiveFacadeToMetadata(facade: R3DirectiveMetadataFacade): R3 queries: facade.queries.map(convertToR3QueryMetadata), providers: facade.providers != null ? new WrappedNodeExpr(facade.providers) : null, viewQueries: facade.viewQueries.map(convertToR3QueryMetadata), + fullInheritance: false, }; } diff --git a/packages/compiler/src/render3/r3_identifiers.ts b/packages/compiler/src/render3/r3_identifiers.ts index 6b648511aafcf..286659d8b17c5 100644 --- a/packages/compiler/src/render3/r3_identifiers.ts +++ b/packages/compiler/src/render3/r3_identifiers.ts @@ -284,6 +284,9 @@ export class Identifiers { static InheritDefinitionFeature: o.ExternalReference = {name: 'ɵɵInheritDefinitionFeature', moduleName: CORE}; + static CopyDefinitionFeature: + o.ExternalReference = {name: 'ɵɵCopyDefinitionFeature', moduleName: CORE}; + static ProvidersFeature: o.ExternalReference = {name: 'ɵɵProvidersFeature', moduleName: CORE}; static listener: o.ExternalReference = {name: 'ɵɵlistener', moduleName: CORE}; diff --git a/packages/compiler/src/render3/view/api.ts b/packages/compiler/src/render3/view/api.ts index 44ad40ef1b53e..4db8c1d4f6fe6 100644 --- a/packages/compiler/src/render3/view/api.ts +++ b/packages/compiler/src/render3/view/api.ts @@ -90,6 +90,11 @@ export interface R3DirectiveMetadata { */ usesInheritance: boolean; + /** + * Whether or not the component or directive inherits its entire decorator from its base class. + */ + fullInheritance: boolean; + /** * Reference name under which to export the directive's type in a template, * if any. diff --git a/packages/compiler/src/render3/view/compiler.ts b/packages/compiler/src/render3/view/compiler.ts index 9769f31a5a1ab..97251967acb5b 100644 --- a/packages/compiler/src/render3/view/compiler.ts +++ b/packages/compiler/src/render3/view/compiler.ts @@ -104,6 +104,9 @@ function addFeatures( if (meta.usesInheritance) { features.push(o.importExpr(R3.InheritDefinitionFeature)); } + if (meta.fullInheritance) { + features.push(o.importExpr(R3.CopyDefinitionFeature)); + } if (meta.lifecycle.usesOnChanges) { features.push(o.importExpr(R3.NgOnChangesFeature).callFn(EMPTY_ARRAY)); } diff --git a/packages/core/src/core_render3_private_export.ts b/packages/core/src/core_render3_private_export.ts index eff2b5a2bcbe6..739d8af84f7f6 100644 --- a/packages/core/src/core_render3_private_export.ts +++ b/packages/core/src/core_render3_private_export.ts @@ -40,6 +40,7 @@ export { ɵɵsetNgModuleScope, ɵɵtemplateRefExtractor, ɵɵProvidersFeature, + ɵɵCopyDefinitionFeature, ɵɵInheritDefinitionFeature, ɵɵNgOnChangesFeature, LifecycleHooksFeature as ɵLifecycleHooksFeature, diff --git a/packages/core/src/render3/features/copy_definition_feature.ts b/packages/core/src/render3/features/copy_definition_feature.ts new file mode 100644 index 0000000000000..3d4bc0f2569c0 --- /dev/null +++ b/packages/core/src/render3/features/copy_definition_feature.ts @@ -0,0 +1,93 @@ +/** + * @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 {ComponentDef, DirectiveDef} from '../interfaces/definition'; +import {isComponentDef} from '../interfaces/type_checks'; + +import {getSuperType} from './inherit_definition_feature'; + +/** + * Fields which exist on either directive or component definitions, and need to be copied from + * parent to child classes by the `ɵɵCopyDefinitionFeature`. + */ +const COPY_DIRECTIVE_FIELDS: (keyof DirectiveDef)[] = [ + // The child class should use the providers of its parent. + 'providersResolver', + + // Not listed here are any fields which are handled by the `ɵɵInheritDefinitionFeature`, such + // as inputs, outputs, and host binding functions. +]; + +/** + * Fields which exist only on component definitions, and need to be copied from parent to child + * classes by the `ɵɵCopyDefinitionFeature`. + * + * The type here allows any field of `ComponentDef` which is not also a property of `DirectiveDef`, + * since those should go in `COPY_DIRECTIVE_FIELDS` above. + */ +const COPY_COMPONENT_FIELDS: Exclude, keyof DirectiveDef>[] = [ + // The child class should use the template function of its parent, including all template + // semantics. + 'template', + 'decls', + 'consts', + 'vars', + 'onPush', + 'ngContentSelectors', + + // The child class should use the CSS styles of its parent, including all styling semantics. + 'styles', + 'encapsulation', + + // The child class should be checked by the runtime in the same way as its parent. + 'schemas', +]; + +/** + * Copies the fields not handled by the `ɵɵInheritDefinitionFeature` from the supertype of a + * definition. + * + * This exists primarily to support ngcc migration of an existing View Engine pattern, where an + * entire decorator is inherited from a parent to a child class. When ngcc detects this case, it + * generates a skeleton definition on the child class, and applies this feature. + * + * The `ɵɵCopyDefinitionFeature` then copies any needed fields from the parent class' definition, + * including things like the component template function. + * + * @param definition The definition of a child class which inherits from a parent class with its + * own definition. + * + * @codeGenApi + */ +export function ɵɵCopyDefinitionFeature(definition: DirectiveDef| ComponentDef): void { + let superType = getSuperType(definition.type) !; + + let superDef: DirectiveDef|ComponentDef|undefined = undefined; + if (isComponentDef(definition)) { + // Don't use getComponentDef/getDirectiveDef. This logic relies on inheritance. + superDef = superType.ɵcmp !; + } else { + // Don't use getComponentDef/getDirectiveDef. This logic relies on inheritance. + superDef = superType.ɵdir !; + } + + // Needed because `definition` fields are readonly. + const defAny = (definition as any); + + // Copy over any fields that apply to either directives or components. + for (const field of COPY_DIRECTIVE_FIELDS) { + defAny[field] = superDef[field]; + } + + if (isComponentDef(superDef)) { + // Copy over any component-specific fields. + for (const field of COPY_COMPONENT_FIELDS) { + defAny[field] = superDef[field]; + } + } +} diff --git a/packages/core/src/render3/features/inherit_definition_feature.ts b/packages/core/src/render3/features/inherit_definition_feature.ts index 618b1fdb4f204..87c35e370b64c 100644 --- a/packages/core/src/render3/features/inherit_definition_feature.ts +++ b/packages/core/src/render3/features/inherit_definition_feature.ts @@ -14,7 +14,7 @@ import {isComponentDef} from '../interfaces/type_checks'; import {ɵɵNgOnChangesFeature} from './ng_onchanges_feature'; -function getSuperType(type: Type): Type& +export function getSuperType(type: Type): Type& {ɵcmp?: ComponentDef, ɵdir?: DirectiveDef} { return Object.getPrototypeOf(type.prototype).constructor; } diff --git a/packages/core/src/render3/index.ts b/packages/core/src/render3/index.ts index 18dd27c5d6af7..0f56d2fc30de3 100644 --- a/packages/core/src/render3/index.ts +++ b/packages/core/src/render3/index.ts @@ -7,6 +7,7 @@ */ import {LifecycleHooksFeature, renderComponent, whenRendered} from './component'; import {ɵɵdefineBase, ɵɵdefineComponent, ɵɵdefineDirective, ɵɵdefineNgModule, ɵɵdefinePipe, ɵɵsetComponentScope, ɵɵsetNgModuleScope} from './definition'; +import {ɵɵCopyDefinitionFeature} from './features/copy_definition_feature'; import {ɵɵInheritDefinitionFeature} from './features/inherit_definition_feature'; import {ɵɵNgOnChangesFeature} from './features/ng_onchanges_feature'; import {ɵɵProvidersFeature} from './features/providers_feature'; @@ -210,6 +211,7 @@ export { ɵɵDirectiveDefWithMeta, DirectiveType, ɵɵNgOnChangesFeature, + ɵɵCopyDefinitionFeature, ɵɵInheritDefinitionFeature, ɵɵProvidersFeature, PipeDef, diff --git a/packages/core/src/render3/jit/environment.ts b/packages/core/src/render3/jit/environment.ts index 8f60f5a4d8ab6..01b0f72c97fb9 100644 --- a/packages/core/src/render3/jit/environment.ts +++ b/packages/core/src/render3/jit/environment.ts @@ -46,6 +46,7 @@ export const angularCoreEnv: {[name: string]: Function} = 'ɵɵtemplateRefExtractor': r3.ɵɵtemplateRefExtractor, 'ɵɵNgOnChangesFeature': r3.ɵɵNgOnChangesFeature, 'ɵɵProvidersFeature': r3.ɵɵProvidersFeature, + 'ɵɵCopyDefinitionFeature': r3.ɵɵCopyDefinitionFeature, 'ɵɵInheritDefinitionFeature': r3.ɵɵInheritDefinitionFeature, 'ɵɵcontainer': r3.ɵɵcontainer, 'ɵɵnextContext': r3.ɵɵnextContext, diff --git a/packages/core/test/acceptance/copy_definition_feature_spec.ts b/packages/core/test/acceptance/copy_definition_feature_spec.ts new file mode 100644 index 0000000000000..ea4c220c70179 --- /dev/null +++ b/packages/core/test/acceptance/copy_definition_feature_spec.ts @@ -0,0 +1,86 @@ +/** + * @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 {Component, NgModule, ɵɵCopyDefinitionFeature as CopyDefinitionFeature, ɵɵInheritDefinitionFeature as InheritDefinitionFeature, ɵɵdefineComponent as defineComponent} from '@angular/core'; +import {TestBed} from '@angular/core/testing'; +import {onlyInIvy} from '@angular/private/testing'; + +describe('Ivy CopyDefinitionFeature', () => { + onlyInIvy('this feature is not required in View Engine') + .it('should copy the template function of a component definition from parent to child', + () => { + + // It would be nice if the base component could be JIT compiled. However, this creates + // a getter for ɵcmp which precludes adding a static definition of that field for the + // child class. + // TODO(alxhub): see if there's a cleaner way to do this. + class BaseComponent { + name !: string; + static ɵcmp = defineComponent({ + type: BaseComponent, + selectors: [['some-cmp']], + decls: 0, + vars: 0, + inputs: {name: 'name'}, + template: function BaseComponent_Template(rf, ctx) { ctx.rendered = true; }, + encapsulation: 2 + }); + static ɵfac = function BaseComponent_Factory(t: any) { + return new (t || BaseComponent)(); + }; + + rendered = false; + } + + class ChildComponent extends BaseComponent { + static ɵcmp = defineComponent({ + type: ChildComponent, + selectors: [['some-cmp']], + features: [InheritDefinitionFeature, CopyDefinitionFeature], + decls: 0, + vars: 0, + template: function ChildComponent_Template(rf, ctx) {}, + encapsulation: 2 + }); + static ɵfac = function ChildComponent_Factory(t: any) { + return new (t || ChildComponent)(); + }; + } + + @NgModule({ + declarations: [ChildComponent], + exports: [ChildComponent], + }) + class Module { + } + + @Component({ + selector: 'test-cmp', + template: '', + }) + class TestCmp { + } + + TestBed.configureTestingModule({ + declarations: [TestCmp], + imports: [Module], + }); + + const fixture = TestBed.createComponent(TestCmp); + + // The child component should have matched and been instantiated. + const child = fixture.debugElement.children[0].componentInstance as ChildComponent; + expect(child instanceof ChildComponent).toBe(true); + + // And the base class template function should've been called. + expect(child.rendered).toBe(true); + + // The input binding should have worked. + expect(child.name).toBe('Success!'); + }); +}); diff --git a/tools/public_api_guard/core/core.d.ts b/tools/public_api_guard/core/core.d.ts index 4a0795c2fa0e5..6bc18b230fb23 100644 --- a/tools/public_api_guard/core/core.d.ts +++ b/tools/public_api_guard/core/core.d.ts @@ -756,6 +756,8 @@ export declare function ɵɵcontainerRefreshStart(index: number): void; export declare function ɵɵcontentQuery(directiveIndex: number, predicate: Type | string[], descend: boolean, read?: any): void; +export declare function ɵɵCopyDefinitionFeature(definition: DirectiveDef | ComponentDef): void; + export declare const ɵɵdefaultStyleSanitizer: StyleSanitizeFn; export declare function ɵɵdefineBase(baseDefinition: {