diff --git a/packages/compiler-cli/ngcc/src/analysis/decoration_analyzer.ts b/packages/compiler-cli/ngcc/src/analysis/decoration_analyzer.ts index 410d0af0f8..1a36eb621c 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 b575300318..c100a11443 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 3854684eef..104eb7fbfc 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 0000000000..8c590a0887 --- /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 4113d43876..219c2ab474 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 f4f8981f1d..c0e52ef119 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, +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, + }; + }); + + 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 new decorator into an already existing `AnalyzedClass`', () => { + const analyzedClass: AnalyzedClass = { + name: 'MockClazz', + declaration: mockClazz, + matches: [], + decorators: null, }; - }, - }; - const mockMetadata: any = {}; - const mockEvaluator: any = {}; - const mockClazz: any = { - name: {text: 'MockClazz'}, - getSourceFile: () => { fileName: 'test-file.js'; }, - }; - const mockDecorator: any = {name: 'MockDecorator'}; + 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 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`, - ]); + 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 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', - ]); - }); + 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); - 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'); + 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 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'); - }); + describe('isInScope', () => { + it('should be true for nodes within the entry-point', () => { + const analyzedFiles: AnalyzedFile[] = []; + const host = new DefaultMigrationHost( + mockHost, mockMetadata, mockEvaluator, [], entryPointPath, analyzedFiles); - 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'); - }); + 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 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 be false for nodes outside the entry-point', () => { + const analyzedFiles: AnalyzedFile[] = []; + const host = new DefaultMigrationHost( + mockHost, mockMetadata, mockEvaluator, [], entryPointPath, analyzedFiles); - 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})); + 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 0000000000..707dfc1a7d --- /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 2790a74f86..3d5ff3cb64 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';