From 5c427c5276438c3581842553679e7f88216fa92d Mon Sep 17 00:00:00 2001 From: JoostK Date: Tue, 22 Oct 2019 20:32:38 +0200 Subject: [PATCH 1/7] fix(ngcc): prevent reflected decorators from being clobbered ngcc has an internal cache of computed decorator information for reflected classes, which could previously be mutated by consumers of the reflection host. With the ability to inject synthesized decorators, such decorators would inadvertently be added into the array of decorators that was owned by the internal cache of the reflection host, incorrectly resulting in synthesized decorators to be considered real decorators on a class. This commit fixes the issue by cloning the cached array before returning it. --- .../compiler-cli/ngcc/src/host/esm2015_host.ts | 12 ++++++++++-- .../ngcc/test/host/esm2015_host_spec.ts | 14 ++++++++++++++ 2 files changed, 24 insertions(+), 2 deletions(-) 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/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()', () => { From 77200880561c96a5aacc8ed08d8134c055e83641 Mon Sep 17 00:00:00 2001 From: JoostK Date: Sun, 20 Oct 2019 20:40:48 +0200 Subject: [PATCH 2/7] feat(ngcc): migrate services that are missing `@Injectable()` A class that is provided as Angular service is required to have an `@Injectable()` decorator so that the compiler generates its injectable definition for the runtime. Applications are automatically migrated using the "missing-injectable" schematic, however libraries built for older version of Angular may not yet satisfy this requirement. This commit ports the "missing-injectable" schematic to a migration that is ran when ngcc is processing a library. This ensures that any service that is provided from an NgModule or Directive/Component will have an `@Injectable()` decorator. --- .../ngcc/src/analysis/decoration_analyzer.ts | 3 +- .../ngcc/src/analysis/migration_host.ts | 26 +- .../ngcc/src/migrations/migration.ts | 16 + .../missing_injectable_migration.ts | 192 ++++++ .../compiler-cli/ngcc/src/migrations/utils.ts | 21 + .../ngcc/test/analysis/migration_host_spec.ts | 374 +++++++---- .../missing_injectable_migration_spec.ts | 592 ++++++++++++++++++ .../src/ngtsc/annotations/index.ts | 1 + 8 files changed, 1085 insertions(+), 140 deletions(-) create mode 100644 packages/compiler-cli/ngcc/src/migrations/missing_injectable_migration.ts create mode 100644 packages/compiler-cli/ngcc/test/migrations/missing_injectable_migration_spec.ts diff --git a/packages/compiler-cli/ngcc/src/analysis/decoration_analyzer.ts b/packages/compiler-cli/ngcc/src/analysis/decoration_analyzer.ts index 410d0af0f86ed..1a36eb621cb25 100644 --- a/packages/compiler-cli/ngcc/src/analysis/decoration_analyzer.ts +++ b/packages/compiler-cli/ngcc/src/analysis/decoration_analyzer.ts @@ -119,7 +119,8 @@ export class DecorationAnalyzer { .map(sourceFile => this.analyzeFile(sourceFile)) .filter(isDefined); const migrationHost = new DefaultMigrationHost( - this.reflectionHost, this.fullMetaReader, this.evaluator, this.handlers, analyzedFiles); + this.reflectionHost, this.fullMetaReader, this.evaluator, this.handlers, + this.bundle.entryPoint.path, analyzedFiles); analyzedFiles.forEach(analyzedFile => this.migrateFile(migrationHost, analyzedFile)); analyzedFiles.forEach(analyzedFile => this.resolveFile(analyzedFile)); const compiledFiles = analyzedFiles.map(analyzedFile => this.compileFile(analyzedFile)); diff --git a/packages/compiler-cli/ngcc/src/analysis/migration_host.ts b/packages/compiler-cli/ngcc/src/analysis/migration_host.ts index b575300318c6a..c100a11443b8a 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 {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,7 +27,7 @@ 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 { const classSymbol = this.reflectionHost.getClassSymbol(clazz) !; @@ -41,6 +44,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/migrations/migration.ts b/packages/compiler-cli/ngcc/src/migrations/migration.ts index 3854684eeff52..104eb7fbfc75b 100644 --- a/packages/compiler-cli/ngcc/src/migrations/migration.ts +++ b/packages/compiler-cli/ngcc/src/migrations/migration.ts @@ -42,4 +42,20 @@ export interface MigrationHost { * @param decorator the decorator to inject. */ injectSyntheticDecorator(clazz: ClassDeclaration, decorator: Decorator): 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/utils.ts b/packages/compiler-cli/ngcc/src/migrations/utils.ts index 4113d438766d4..219c2ab474f19 100644 --- a/packages/compiler-cli/ngcc/src/migrations/utils.ts +++ b/packages/compiler-cli/ngcc/src/migrations/utils.ts @@ -54,6 +54,27 @@ export function createDirectiveDecorator(clazz: ClassDeclaration): Decorator { }; } +/** + * Create an empty `Injectable` decorator that will be associated with the `clazz`. + */ +export function createInjectableDecorator(clazz: ClassDeclaration): Decorator { + const decoratorType = ts.createIdentifier('Injectable'); + const decoratorNode = ts.createObjectLiteral([ + ts.createPropertyAssignment('type', decoratorType), + ts.createPropertyAssignment('args', ts.createArrayLiteral([])), + ]); + + setParentPointers(clazz.getSourceFile(), decoratorNode); + + return { + name: 'Injectable', + identifier: decoratorType, + import: {name: 'Injectable', from: '@angular/core'}, + node: decoratorNode, + args: [], + }; +} + /** * Ensure that a tree of AST nodes have their parents wired up. */ 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/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/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'; From c8cfa3ec1f2ab73185aa3e40f80b08a348f73e1c Mon Sep 17 00:00:00 2001 From: JoostK Date: Sun, 20 Oct 2019 22:45:28 +0200 Subject: [PATCH 3/7] refactor(ivy): mark synthetic decorators explicitly In ngcc's migration system, synthetic decorators can be injected into a compilation to ensure that certain classes are compiled with Angular logic, where the original library code did not include the necessary decorators. Prior to this change, synthesized decorators would have a fake AST structure as associated node and a made-up identifier. In theory, this may introduce issues downstream: 1) a decorator's node is used for diagnostics, so it must have position information. Having fake AST nodes without a position is therefore a problem. Note that this is currently not a problem in practice, as injected synthesized decorators would not produce any diagnostics. 2) the decorator's identifier should refer to an imported symbol. Therefore, it is required that the symbol is actually imported. Moreover, bundle formats such as UMD and CommonJS use namespaces for imports, so a bare `ts.Identifier` would not be suitable to use as identifier. This was also not a problem in practice, as the identifier is only used in the `setClassMetadata` generated code, which is omitted for synthetically injected decorators. To remedy these potential issues, this commit makes a decorator's identifier optional and switches its node over from a fake AST structure to the class' name. --- .../compiler-cli/ngcc/src/migrations/utils.ts | 48 ++++++++--------- .../ngcc/src/rendering/renderer.ts | 3 ++ .../host/commonjs_host_import_helper_spec.ts | 2 +- .../host/esm2015_host_import_helper_spec.ts | 8 +-- .../test/host/esm5_host_import_helper_spec.ts | 8 +-- .../test/host/umd_host_import_helper_spec.ts | 2 +- .../commonjs_rendering_formatter_spec.ts | 12 ++--- .../esm5_rendering_formatter_spec.ts | 12 ++--- .../rendering/esm_rendering_formatter_spec.ts | 14 ++--- .../rendering/umd_rendering_formatter_spec.ts | 12 ++--- .../src/ngtsc/annotations/src/component.ts | 5 +- .../src/ngtsc/annotations/src/directive.ts | 6 +-- .../src/ngtsc/annotations/src/injectable.ts | 6 ++- .../src/ngtsc/annotations/src/metadata.ts | 3 ++ .../src/ngtsc/annotations/src/ng_module.ts | 2 +- .../src/ngtsc/annotations/src/pipe.ts | 6 ++- .../src/ngtsc/annotations/src/util.ts | 7 +-- .../src/ngtsc/reflection/src/host.ts | 53 ++++++++++++++++--- 18 files changed, 131 insertions(+), 78 deletions(-) diff --git a/packages/compiler-cli/ngcc/src/migrations/utils.ts b/packages/compiler-cli/ngcc/src/migrations/utils.ts index 219c2ab474f19..a305986de0772 100644 --- a/packages/compiler-cli/ngcc/src/migrations/utils.ts +++ b/packages/compiler-cli/ngcc/src/migrations/utils.ts @@ -37,20 +37,14 @@ export function createDirectiveDecorator(clazz: ClassDeclaration): Decorator { // 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); return { name: 'Directive', - identifier: decoratorType, + identifier: null, import: {name: 'Directive', from: '@angular/core'}, - node: decoratorNode, - args: [selectorArg], + node: null, + synthesizedFor: clazz.name, + args: [reifySourceFile(selectorArg)], }; } @@ -58,27 +52,33 @@ export function createDirectiveDecorator(clazz: ClassDeclaration): Decorator { * Create an empty `Injectable` decorator that will be associated with the `clazz`. */ export function createInjectableDecorator(clazz: ClassDeclaration): Decorator { - const decoratorType = ts.createIdentifier('Injectable'); - const decoratorNode = ts.createObjectLiteral([ - ts.createPropertyAssignment('type', decoratorType), - ts.createPropertyAssignment('args', ts.createArrayLiteral([])), - ]); - - setParentPointers(clazz.getSourceFile(), decoratorNode); - return { name: 'Injectable', - identifier: decoratorType, + identifier: null, import: {name: 'Injectable', from: '@angular/core'}, - node: decoratorNode, + node: null, + synthesizedFor: clazz.name, args: [], }; } +const EMPTY_SF = ts.createSourceFile('(empty)', '', ts.ScriptTarget.Latest); + /** - * Ensure that a tree of AST nodes have their parents wired up. + * 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/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/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/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/src/component.ts b/packages/compiler-cli/src/ngtsc/annotations/src/component.ts index 4578da8b48e02..23cbe879228ab 100644 --- a/packages/compiler-cli/src/ngtsc/annotations/src/component.ts +++ b/packages/compiler-cli/src/ngtsc/annotations/src/component.ts @@ -509,7 +509,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 +624,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..47be27966cd4a 100644 --- a/packages/compiler-cli/src/ngtsc/annotations/src/directive.ts +++ b/packages/compiler-cli/src/ngtsc/annotations/src/directive.ts @@ -121,7 +121,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]); @@ -447,7 +447,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 +465,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 { + 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`). From e28043dc31c680a790db12a7e5c2cc74bea121c4 Mon Sep 17 00:00:00 2001 From: JoostK Date: Sun, 20 Oct 2019 23:28:00 +0200 Subject: [PATCH 4/7] refactor(ngcc): rework undecorated parent migration Previously, the (currently disabled) undecorated parent migration in ngcc would produce errors when a base class could not be determined statically or when a class extends from a class in another package. This is not ideal, as it would cause the library to fail compilation without a workaround, whereas those problems are not guaranteed to cause issues. Additionally, inheritance chains were not handled. This commit reworks the migration to address these limitations. --- .../undecorated_parent_migration.ts | 65 +++++++----- .../compiler-cli/ngcc/src/migrations/utils.ts | 10 +- .../undecorated_parent_migration_spec.ts | 100 ++++++++++++++++-- .../src/ngtsc/diagnostics/src/code.ts | 11 -- 4 files changed, 134 insertions(+), 52 deletions(-) diff --git a/packages/compiler-cli/ngcc/src/migrations/undecorated_parent_migration.ts b/packages/compiler-cli/ngcc/src/migrations/undecorated_parent_migration.ts index 921242983fe6e..4b77e3eec4aa3 100644 --- a/packages/compiler-cli/ngcc/src/migrations/undecorated_parent_migration.ts +++ b/packages/compiler-cli/ngcc/src/migrations/undecorated_parent_migration.ts @@ -6,12 +6,14 @@ * found in the LICENSE file at https://angular.io/license */ import * as ts from 'typescript'; -import {ErrorCode, makeDiagnostic} from '../../../src/ngtsc/diagnostics'; + +import {Reference} from '../../../src/ngtsc/imports'; import {ClassDeclaration} from '../../../src/ngtsc/reflection'; -import {isRelativePath} from '../utils'; + import {Migration, MigrationHost} from './migration'; import {createDirectiveDecorator, hasConstructor, hasDirectiveDecorator, isClassDeclaration} from './utils'; + /** * Ensure that the parents of directives and components that have no constructor are also decorated * as a `Directive`. @@ -59,36 +61,49 @@ export class UndecoratedParentMigration implements Migration { } // Only interested in `clazz` if it inherits from a base class. - const baseClassExpr = host.reflectionHost.getBaseClassExpression(clazz); - if (baseClassExpr === null) { - return null; - } + let baseClazzRef = determineBaseClass(clazz, host); + while (baseClazzRef !== null) { + const baseClazz = baseClazzRef.node; - if (!ts.isIdentifier(baseClassExpr)) { - return makeDiagnostic( - ErrorCode.NGCC_MIGRATION_EXTERNAL_BASE_CLASS, baseClassExpr, - `${clazz.name.text} class has a dynamic base class ${baseClassExpr.getText()}, so it is not possible to migrate.`); - } + // Do not proceed if the base class already has a decorator, or is not in scope of the + // entry-point that is currently being compiled. + if (hasDirectiveDecorator(host, baseClazz) || !host.isInScope(baseClazz)) { + break; + } - const baseClazz = host.reflectionHost.getDeclarationOfIdentifier(baseClassExpr) !.node; - if (baseClazz === null || !isClassDeclaration(baseClazz)) { - return null; - } + // Inject an `@Directive()` decorator for the base class. + host.injectSyntheticDecorator(baseClazz, createDirectiveDecorator(baseClazz)); - // Only interested in this base class if it doesn't have a `Directive` or `Component` decorator. - if (hasDirectiveDecorator(host, baseClazz)) { - return null; - } + // If the base class has a constructor, there's no need to continue walking up the + // inheritance chain. The injected decorator ensures that a factory is generated that does + // not delegate to the base class. + if (hasConstructor(host, baseClazz)) { + break; + } - const importInfo = host.reflectionHost.getImportOfIdentifier(baseClassExpr); - if (importInfo !== null && !isRelativePath(importInfo.from)) { - return makeDiagnostic( - ErrorCode.NGCC_MIGRATION_EXTERNAL_BASE_CLASS, baseClassExpr, - 'The base class was imported from an external entry-point so we cannot add a directive to it.'); + // Continue with another level of class inheritance. + baseClazzRef = determineBaseClass(baseClazz, host); } - host.injectSyntheticDecorator(baseClazz, createDirectiveDecorator(baseClazz)); + return null; + } +} +/** + * Computes a reference to the base class, or `null` if the class has no base class or if it could + * not be statically determined. + */ +function determineBaseClass( + clazz: ClassDeclaration, host: MigrationHost): Reference|null { + const baseClassExpr = host.reflectionHost.getBaseClassExpression(clazz); + if (baseClassExpr === null) { return null; } + + const baseClass = host.evaluator.evaluate(baseClassExpr); + if (!(baseClass instanceof Reference) || !isClassDeclaration(baseClass.node as ts.Declaration)) { + return null; + } + + return baseClass as Reference; } diff --git a/packages/compiler-cli/ngcc/src/migrations/utils.ts b/packages/compiler-cli/ngcc/src/migrations/utils.ts index a305986de0772..e988dde29bc86 100644 --- a/packages/compiler-cli/ngcc/src/migrations/utils.ts +++ b/packages/compiler-cli/ngcc/src/migrations/utils.ts @@ -19,7 +19,8 @@ export function isClassDeclaration(clazz: ts.Declaration): clazz is ClassDeclara * Returns true if the `clazz` is decorated as a `Directive` or `Component`. */ export function hasDirectiveDecorator(host: MigrationHost, clazz: ClassDeclaration): boolean { - return host.metadata.getDirectiveMetadata(new Reference(clazz)) !== null; + const ref = new Reference(clazz); + return host.metadata.getDirectiveMetadata(ref) !== null; } /** @@ -33,18 +34,13 @@ export function hasConstructor(host: MigrationHost, clazz: ClassDeclaration): bo * Create an empty `Directive` decorator that will be associated with the `clazz`. */ export function createDirectiveDecorator(clazz: ClassDeclaration): Decorator { - const selectorArg = ts.createObjectLiteral([ - // TODO: At the moment ngtsc does not accept a directive with no selector - ts.createPropertyAssignment('selector', ts.createStringLiteral('NGCC_DUMMY')), - ]); - return { name: 'Directive', identifier: null, import: {name: 'Directive', from: '@angular/core'}, node: null, synthesizedFor: clazz.name, - args: [reifySourceFile(selectorArg)], + args: [], }; } diff --git a/packages/compiler-cli/ngcc/test/migrations/undecorated_parent_migration_spec.ts b/packages/compiler-cli/ngcc/test/migrations/undecorated_parent_migration_spec.ts index 065ea13612b83..74536c1b9085a 100644 --- a/packages/compiler-cli/ngcc/test/migrations/undecorated_parent_migration_spec.ts +++ b/packages/compiler-cli/ngcc/test/migrations/undecorated_parent_migration_spec.ts @@ -26,19 +26,20 @@ runInEachFileSystem(() => { }); it('should ignore undecorated classes', () => { - const {program, analysis} = setUpAndAnalyzeProgram([{ + const {program, analysis, errors} = setUpAndAnalyzeProgram([{ name: INDEX_FILENAME, contents: ` export class DerivedClass extends BaseClass {} export class BaseClass {} ` }]); + expect(errors).toEqual([]); const file = analysis.get(program.getSourceFile(INDEX_FILENAME) !); expect(file).toBeUndefined(); }); it('should ignore an undecorated base class if the derived class has a constructor', () => { - const {program, analysis} = setUpAndAnalyzeProgram([{ + const {program, analysis, errors} = setUpAndAnalyzeProgram([{ name: INDEX_FILENAME, contents: ` import {Directive, ViewContainerRef} from '@angular/core'; @@ -54,6 +55,7 @@ runInEachFileSystem(() => { export class BaseClass {} ` }]); + expect(errors).toEqual([]); const file = analysis.get(program.getSourceFile(INDEX_FILENAME) !) !; expect(file.compiledClasses.find(c => c.name === 'DerivedClass')).toBeDefined(); expect(file.compiledClasses.find(c => c.name === 'BaseClass')).toBeUndefined(); @@ -61,7 +63,7 @@ runInEachFileSystem(() => { it('should add a decorator to an undecorated base class if the derived class is a Directive with no constructor', () => { - const {program, analysis} = setUpAndAnalyzeProgram([{ + const {program, analysis, errors} = setUpAndAnalyzeProgram([{ name: INDEX_FILENAME, contents: ` import {Directive, ViewContainerRef} from '@angular/core'; @@ -78,20 +80,96 @@ runInEachFileSystem(() => { ]; ` }]); + expect(errors).toEqual([]); const file = analysis.get(program.getSourceFile(INDEX_FILENAME) !) !; expect(file.compiledClasses.find(c => c.name === 'DerivedClass')).toBeDefined(); const baseClass = file.compiledClasses.find(c => c.name === 'BaseClass') !; expect(baseClass.decorators !.length).toEqual(1); const decorator = baseClass.decorators ![0]; expect(decorator.name).toEqual('Directive'); + expect(decorator.identifier).toBeNull('The decorator must be synthesized'); expect(decorator.import).toEqual({from: '@angular/core', name: 'Directive'}); - expect(decorator.args !.length).toEqual(1); + expect(decorator.args !.length).toEqual(0); + }); + + it('should not add a decorator to a base class that is already decorated', () => { + const {program, analysis, errors} = setUpAndAnalyzeProgram([{ + name: INDEX_FILENAME, + contents: ` + import {Directive, ViewContainerRef} from '@angular/core'; + export class DerivedClass extends BaseClass { + } + DerivedClass.decorators = [ + { type: Directive, args: [{ selector: '[dir]' }] } + ]; + export class BaseClass { + constructor(private vcr: ViewContainerRef) {} + } + BaseClass.decorators = [ + { type: Directive, args: [] } + ]; + BaseClass.ctorParameters = () => [ + { type: ViewContainerRef, } + ]; + ` + }]); + expect(errors).toEqual([]); + const file = analysis.get(program.getSourceFile(INDEX_FILENAME) !) !; + expect(file.compiledClasses.find(c => c.name === 'DerivedClass')).toBeDefined(); + const baseClass = file.compiledClasses.find(c => c.name === 'BaseClass') !; + expect(baseClass.decorators !.length).toEqual(1); + const decorator = baseClass.decorators ![0]; + expect(decorator.name).toEqual('Directive'); + expect(decorator.identifier).not.toBeNull('The decorator must not be synthesized'); + }); + + it('should add decorators to all classes in an inheritance chain until a constructor is found', + () => { + const {program, analysis, errors} = setUpAndAnalyzeProgram([{ + name: INDEX_FILENAME, + contents: ` + import {Directive, ViewContainerRef} from '@angular/core'; + export class DerivedClass extends IntermediateClass { + } + DerivedClass.decorators = [ + { type: Directive, args: [{ selector: '[dir]' }] } + ]; + export class IntermediateClass extends BaseClass {} + export class BaseClass extends RealBaseClass { + constructor(private vcr: ViewContainerRef) {} + } + BaseClass.ctorParameters = () => [ + { type: ViewContainerRef, } + ]; + export class RealBaseClass {} + ` + }]); + expect(errors).toEqual([]); + const file = analysis.get(program.getSourceFile(INDEX_FILENAME) !) !; + expect(file.compiledClasses.find(c => c.name === 'DerivedClass')).toBeDefined(); + expect(file.compiledClasses.find(c => c.name === 'RealBaseClass')).toBeUndefined(); + + const intermediateClass = file.compiledClasses.find(c => c.name === 'IntermediateClass') !; + expect(intermediateClass.decorators !.length).toEqual(1); + const intermediateDecorator = intermediateClass.decorators ![0]; + expect(intermediateDecorator.name).toEqual('Directive'); + expect(intermediateDecorator.identifier).toBeNull('The decorator must be synthesized'); + expect(intermediateDecorator.import).toEqual({from: '@angular/core', name: 'Directive'}); + expect(intermediateDecorator.args !.length).toEqual(0); + + const baseClass = file.compiledClasses.find(c => c.name === 'BaseClass') !; + expect(baseClass.decorators !.length).toEqual(1); + const baseDecorator = baseClass.decorators ![0]; + expect(baseDecorator.name).toEqual('Directive'); + expect(baseDecorator.identifier).toBeNull('The decorator must be synthesized'); + expect(baseDecorator.import).toEqual({from: '@angular/core', name: 'Directive'}); + expect(baseDecorator.args !.length).toEqual(0); }); it('should handle the base class being in a different file (same package) as the derived class', () => { const BASE_FILENAME = _('/node_modules/test-package/base.js'); - const {program, analysis} = setUpAndAnalyzeProgram([ + const {program, analysis, errors} = setUpAndAnalyzeProgram([ { name: INDEX_FILENAME, contents: ` @@ -116,18 +194,20 @@ runInEachFileSystem(() => { ` } ]); + expect(errors).toEqual([]); const file = analysis.get(program.getSourceFile(BASE_FILENAME) !) !; const baseClass = file.compiledClasses.find(c => c.name === 'BaseClass') !; expect(baseClass.decorators !.length).toEqual(1); const decorator = baseClass.decorators ![0]; expect(decorator.name).toEqual('Directive'); + expect(decorator.identifier).toBeNull('The decorator must be synthesized'); expect(decorator.import).toEqual({from: '@angular/core', name: 'Directive'}); - expect(decorator.args !.length).toEqual(1); + expect(decorator.args !.length).toEqual(0); }); - it('should error if the base class being is a different package from the derived class', () => { + it('should skip the base class if it is in a different package from the derived class', () => { const BASE_FILENAME = _('/node_modules/other-package/index.js'); - const {errors} = setUpAndAnalyzeProgram([ + const {program, analysis, errors} = setUpAndAnalyzeProgram([ { name: INDEX_FILENAME, contents: ` @@ -152,7 +232,9 @@ runInEachFileSystem(() => { ` } ]); - expect(errors.length).toEqual(1); + expect(errors).toEqual([]); + const file = analysis.get(program.getSourceFile(BASE_FILENAME) !); + expect(file).toBeUndefined(); }); }); diff --git a/packages/compiler-cli/src/ngtsc/diagnostics/src/code.ts b/packages/compiler-cli/src/ngtsc/diagnostics/src/code.ts index c3528a9a9c46a..cbad3b5f84b8d 100644 --- a/packages/compiler-cli/src/ngtsc/diagnostics/src/code.ts +++ b/packages/compiler-cli/src/ngtsc/diagnostics/src/code.ts @@ -75,17 +75,6 @@ export enum ErrorCode { */ NGCC_MIGRATION_DECORATOR_INJECTION_ERROR = 7001, - /** - * Raised when ngcc tries to decorate a base class that was imported from outside the package. - */ - NGCC_MIGRATION_EXTERNAL_BASE_CLASS = 7002, - - /** - * Raised when ngcc tries to migrate a class that is extended from a dynamic base class - * expression. - */ - NGCC_MIGRATION_DYNAMIC_BASE_CLASS = 7003, - /** * An element name failed validation against the DOM schema. */ From 1af6a29946fad32c67c539138561f0e5e7c57c09 Mon Sep 17 00:00:00 2001 From: JoostK Date: Sun, 20 Oct 2019 23:36:04 +0200 Subject: [PATCH 5/7] feat(ngcc): enable migrations to apply schematics to libraries When upgrading an Angular application to a new version using the Angular CLI, built-in schematics are being run to update user code from deprecated patterns to the new way of working. For libraries that have been built for older versions of Angular however, such schematics have not been executed which means that deprecated code patterns may still be present, potentially resulting in incorrect behavior. Some of the logic of schematics has been ported over to ngcc migrations, which are automatically run on libraries. These migrations achieve the same goal of the regular schematics, but operating on published library sources instead of used code. --- .../ngcc/src/analysis/decoration_analyzer.ts | 49 +++++++++++-------- .../test/analysis/decoration_analyzer_spec.ts | 4 +- 2 files changed, 31 insertions(+), 22 deletions(-) diff --git a/packages/compiler-cli/ngcc/src/analysis/decoration_analyzer.ts b/packages/compiler-cli/ngcc/src/analysis/decoration_analyzer.ts index 1a36eb621cb25..36d3c512ce742 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,9 +19,12 @@ 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 {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'; @@ -100,7 +104,7 @@ export class DecorationAnalyzer { this.reflectionHost, this.evaluator, this.metaRegistry, NOOP_DEFAULT_IMPORT_RECORDER, this.isCore), ]; - migrations: Migration[] = []; + migrations: Migration[] = [new UndecoratedParentMigration(), new MissingInjectableMigration()]; constructor( private fs: FileSystem, private bundle: EntryPointBundle, @@ -118,10 +122,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, - this.bundle.entryPoint.path, 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( @@ -147,21 +150,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/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', ]); }); From cc29841bef27c79ab6f270bd7bd8eec212edabb2 Mon Sep 17 00:00:00 2001 From: Alex Rickabaugh Date: Wed, 23 Oct 2019 11:56:30 -0700 Subject: [PATCH 6/7] feat(ivy): add a runtime feature to copy cmp/dir definitions This commit adds CopyDefinitionFeature, which supports the case where an entire decorator (@Component or @Directive) is inherited from parent to child. The existing inheritance feature, InheritDefinitionFeature, supports merging of parent and child definitions when both were originally present. This merges things like inputs, outputs, host bindings, etc. CopyDefinitionFeature, on the other hand, compensates for a definition that was missing entirely on the child class, by copying fields that aren't ordinarily inherited (like the template function itself). This feature is intended to only be used as part of ngcc code generation. --- .../compiler/src/render3/r3_identifiers.ts | 3 + .../core/src/core_render3_private_export.ts | 1 + .../features/copy_definition_feature.ts | 93 +++++++++++++++++++ .../features/inherit_definition_feature.ts | 2 +- packages/core/src/render3/index.ts | 2 + packages/core/src/render3/jit/environment.ts | 1 + .../copy_definition_feature_spec.ts | 86 +++++++++++++++++ tools/public_api_guard/core/core.d.ts | 2 + 8 files changed, 189 insertions(+), 1 deletion(-) create mode 100644 packages/core/src/render3/features/copy_definition_feature.ts create mode 100644 packages/core/test/acceptance/copy_definition_feature_spec.ts 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/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: { From f1b630213430e106bd59cbd4884fe2c1c62fbd67 Mon Sep 17 00:00:00 2001 From: Alex Rickabaugh Date: Wed, 23 Oct 2019 12:00:49 -0700 Subject: [PATCH 7/7] feat(ngcc): add a migration for undecorated child classes In Angular View Engine, there are two kinds of decorator inheritance: 1) both the parent and child classes have decorators This case is supported by InheritDefinitionFeature, which merges some fields of the definitions (such as the inputs or queries). 2) only the parent class has a decorator If the child class is missing a decorator, the compiler effectively behaves as if the parent class' decorator is applied to the child class as well. This is the "undecorated child" scenario, and this commit adds a migration to ngcc to support this pattern in Ivy. This migration has 2 phases. First, the NgModules of the application are scanned for classes in 'declarations' which are missing decorators, but whose base classes do have decorators. These classes are the undecorated children. This scan is performed recursively, so even if a declared class has a base class that itself inherits a decorator, this case is handled. Next, a synthetic decorator (either @Component or @Directive) is created on the child class. This decorator copies some critical information such as 'selector' and 'exportAs', as well as supports any decorated fields (@Input, etc). A flag is passed to the decorator compiler which causes a special feature `CopyDefinitionFeature` to be included on the compiled definition. This feature copies at runtime the remaining aspects of the parent definition which `InheritDefinitionFeature` does not handle, completing the "full" inheritance of the child class' decorator from its parent class. --- .../ngcc/src/analysis/decoration_analyzer.ts | 8 +- .../ngcc/src/analysis/migration_host.ts | 7 +- .../compiler-cli/ngcc/src/analysis/util.ts | 6 +- .../ngcc/src/migrations/migration.ts | 6 +- .../migrations/undecorated_child_migration.ts | 75 ++++++++++ .../compiler-cli/ngcc/src/migrations/utils.ts | 60 +++++++- packages/compiler-cli/ngcc/test/BUILD.bazel | 1 + .../ngcc/test/integration/ngcc_spec.ts | 130 ++++++++++++++++++ .../ngcc/test/integration/util.ts | 125 +++++++++++++++++ .../src/ngtsc/annotations/src/component.ts | 7 +- .../src/ngtsc/annotations/src/directive.ts | 11 +- .../src/ngtsc/annotations/src/util.ts | 7 +- .../src/ngtsc/transform/src/api.ts | 23 +++- packages/compiler/src/jit_compiler_facade.ts | 1 + packages/compiler/src/render3/view/api.ts | 5 + .../compiler/src/render3/view/compiler.ts | 3 + 16 files changed, 451 insertions(+), 24 deletions(-) create mode 100644 packages/compiler-cli/ngcc/src/migrations/undecorated_child_migration.ts create mode 100644 packages/compiler-cli/ngcc/test/integration/util.ts diff --git a/packages/compiler-cli/ngcc/src/analysis/decoration_analyzer.ts b/packages/compiler-cli/ngcc/src/analysis/decoration_analyzer.ts index 36d3c512ce742..e2cc4086b6d05 100644 --- a/packages/compiler-cli/ngcc/src/analysis/decoration_analyzer.ts +++ b/packages/compiler-cli/ngcc/src/analysis/decoration_analyzer.ts @@ -21,6 +21,7 @@ import {CompileResult, DecoratorHandler, DetectResult, HandlerPrecedence} from ' import {NgccClassSymbol, NgccReflectionHost} from '../host/ngcc_host'; import {Migration} from '../migrations/migration'; import {MissingInjectableMigration} from '../migrations/missing_injectable_migration'; +import {UndecoratedChildMigration} from '../migrations/undecorated_child_migration'; import {UndecoratedParentMigration} from '../migrations/undecorated_parent_migration'; import {EntryPointBundle} from '../packages/entry_point_bundle'; import {isDefined} from '../utils'; @@ -29,6 +30,7 @@ import {DefaultMigrationHost} from './migration_host'; import {AnalyzedClass, AnalyzedFile, CompiledClass, CompiledFile, DecorationAnalyses} from './types'; import {analyzeDecorators, isWithinPackage} from './util'; + /** * Simple class that resolves and loads files directly from the filesystem. */ @@ -104,7 +106,11 @@ export class DecorationAnalyzer { this.reflectionHost, this.evaluator, this.metaRegistry, NOOP_DEFAULT_IMPORT_RECORDER, this.isCore), ]; - migrations: Migration[] = [new UndecoratedParentMigration(), new MissingInjectableMigration()]; + migrations: Migration[] = [ + new UndecoratedParentMigration(), + new UndecoratedChildMigration(), + new MissingInjectableMigration(), + ]; constructor( private fs: FileSystem, private bundle: EntryPointBundle, diff --git a/packages/compiler-cli/ngcc/src/analysis/migration_host.ts b/packages/compiler-cli/ngcc/src/analysis/migration_host.ts index c100a11443b8a..4ff258e44b661 100644 --- a/packages/compiler-cli/ngcc/src/analysis/migration_host.ts +++ b/packages/compiler-cli/ngcc/src/analysis/migration_host.ts @@ -12,7 +12,7 @@ import {AbsoluteFsPath} from '../../../src/ngtsc/file_system'; import {MetadataReader} from '../../../src/ngtsc/metadata'; import {PartialEvaluator} from '../../../src/ngtsc/partial_evaluator'; import {ClassDeclaration, Decorator} from '../../../src/ngtsc/reflection'; -import {DecoratorHandler} from '../../../src/ngtsc/transform'; +import {DecoratorHandler, HandlerFlags} from '../../../src/ngtsc/transform'; import {NgccReflectionHost} from '../host/ngcc_host'; import {MigrationHost} from '../migrations/migration'; @@ -29,9 +29,10 @@ export class DefaultMigrationHost implements MigrationHost { readonly evaluator: PartialEvaluator, private handlers: DecoratorHandler[], 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; } 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/migrations/migration.ts b/packages/compiler-cli/ngcc/src/migrations/migration.ts index 104eb7fbfc75b..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,7 +44,8 @@ export interface MigrationHost { * @param clazz the class to receive the new decorator. * @param decorator the decorator to inject. */ - injectSyntheticDecorator(clazz: ClassDeclaration, decorator: Decorator): void; + injectSyntheticDecorator(clazz: ClassDeclaration, decorator: Decorator, flags?: HandlerFlags): + void; /** * Retrieves all decorators that are associated with the class, including synthetic decorators 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/utils.ts b/packages/compiler-cli/ngcc/src/migrations/utils.ts index e988dde29bc86..5524661a022a5 100644 --- a/packages/compiler-cli/ngcc/src/migrations/utils.ts +++ b/packages/compiler-cli/ngcc/src/migrations/utils.ts @@ -23,6 +23,14 @@ export function hasDirectiveDecorator(host: MigrationHost, clazz: ClassDeclarati 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; +} + /** * Returns true if the `clazz` has its own constructor function. */ @@ -33,14 +41,53 @@ export function hasConstructor(host: MigrationHost, clazz: ClassDeclaration): bo /** * Create an empty `Directive` decorator that will be associated with the `clazz`. */ -export function createDirectiveDecorator(clazz: ClassDeclaration): Decorator { +export function createDirectiveDecorator( + clazz: ClassDeclaration, + metadata?: {selector: string | 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: null, import: {name: 'Directive', from: '@angular/core'}, 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: [], + args: [ + reifySourceFile(ts.createObjectLiteral(metaArgs)), + ], }; } @@ -58,6 +105,15 @@ export function createInjectableDecorator(clazz: ClassDeclaration): Decorator { }; } +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); /** 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/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/src/ngtsc/annotations/src/component.ts b/packages/compiler-cli/src/ngtsc/annotations/src/component.ts index 23cbe879228ab..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 diff --git a/packages/compiler-cli/src/ngtsc/annotations/src/directive.ts b/packages/compiler-cli/src/ngtsc/annotations/src/directive.ts index 47be27966cd4a..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[], @@ -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 diff --git a/packages/compiler-cli/src/ngtsc/annotations/src/util.ts b/packages/compiler-cli/src/ngtsc/annotations/src/util.ts index 466ba9b7bd903..170965cb97970 100644 --- a/packages/compiler-cli/src/ngtsc/annotations/src/util.ts +++ b/packages/compiler-cli/src/ngtsc/annotations/src/util.ts @@ -313,15 +313,10 @@ export function isWrappedTsNodeExpr(expr: Expression): expr is WrappedNodeExpr|'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/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/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)); }