From 5557dec120e3e81f5078089cb7821187fae501e0 Mon Sep 17 00:00:00 2001 From: Paul Gschwendtner Date: Sat, 5 Oct 2019 23:41:37 +0200 Subject: [PATCH] refactor(core): missing-injectable migration should respect providers of directives and components (#33011) Currenly the `missing-injectable` migration only migrates providers referenced from `@NgModule` definitions. The schematic currently does not cover the migration for providers referenced in `@Directive` or `@Component` definitions. We need to handle the following keys for directives/components: - `@Directive` -> `providers` - `@Component` -> `providers` and `viewProviders`. This commit ensures that the migration handles providers for these definitions. PR Close #33011 --- .../google3/noMissingInjectableRule.ts | 32 +- ...e_collector.ts => definition_collector.ts} | 40 +- .../google3/tslint_update_recorder.ts | 8 +- .../migrations/missing-injectable/index.ts | 26 +- .../missing-injectable/transform.ts | 59 +- .../missing-injectable/update_recorder.ts | 4 +- .../google3/missing_injectable_rule_spec.ts | 363 +++--- .../test/missing_injectable_migration_spec.ts | 1044 +++++++++-------- 8 files changed, 881 insertions(+), 695 deletions(-) rename packages/core/schematics/migrations/missing-injectable/{module_collector.ts => definition_collector.ts} (56%) diff --git a/packages/core/schematics/migrations/google3/noMissingInjectableRule.ts b/packages/core/schematics/migrations/google3/noMissingInjectableRule.ts index 453ab55dc3..5fa6511c9c 100644 --- a/packages/core/schematics/migrations/google3/noMissingInjectableRule.ts +++ b/packages/core/schematics/migrations/google3/noMissingInjectableRule.ts @@ -9,14 +9,16 @@ import {RuleFailure, Rules} from 'tslint'; import * as ts from 'typescript'; +import {NgDefinitionCollector} from '../missing-injectable/definition_collector'; import {TslintUpdateRecorder} from '../missing-injectable/google3/tslint_update_recorder'; -import {NgModuleCollector} from '../missing-injectable/module_collector'; import {MissingInjectableTransform} from '../missing-injectable/transform'; + /** - * TSLint rule that flags classes which are declared as providers in NgModules but - * aren't decorated with any Angular decorator. + * TSLint rule that flags classes which are declared as providers in "NgModule", + * "Directive" or "Component" definitions while not being decorated with any + * Angular decorator (e.g. "@Injectable"). */ export class Rule extends Rules.TypedRule { applyWithProgram(sourceFile: ts.SourceFile, program: ts.Program): RuleFailure[] { @@ -24,24 +26,24 @@ export class Rule extends Rules.TypedRule { const typeChecker = program.getTypeChecker(); const sourceFiles = program.getSourceFiles().filter( s => !s.isDeclarationFile && !program.isSourceFileFromExternalLibrary(s)); - const moduleCollector = new NgModuleCollector(typeChecker); + const definitionCollector = new NgDefinitionCollector(typeChecker); const failures: RuleFailure[] = []; - // Analyze source files by detecting all NgModule definitions. - sourceFiles.forEach(sourceFile => moduleCollector.visitNode(sourceFile)); + // Analyze source files by detecting all "NgModule", "Directive" or + // "Component" definitions. + sourceFiles.forEach(sourceFile => definitionCollector.visitNode(sourceFile)); - const {resolvedModules} = moduleCollector; + const {resolvedModules, resolvedDirectives} = definitionCollector; const transformer = new MissingInjectableTransform(typeChecker, getUpdateRecorder); const updateRecorders = new Map(); - resolvedModules.forEach(module => { - transformer.migrateModule(module).forEach(({message, node}) => { - // Only report failures for the current source file that is visited. - if (node.getSourceFile() === sourceFile) { - failures.push( - new RuleFailure(node.getSourceFile(), node.getStart(), 0, message, ruleName)); - } - }); + [...transformer.migrateModules(resolvedModules), + ...transformer.migrateDirectives(resolvedDirectives), + ].forEach(({message, node}) => { + // Only report failures for the current source file that is visited. + if (node.getSourceFile() === sourceFile) { + failures.push(new RuleFailure(node.getSourceFile(), node.getStart(), 0, message, ruleName)); + } }); // Record the changes collected in the import manager and NgModule manager. diff --git a/packages/core/schematics/migrations/missing-injectable/module_collector.ts b/packages/core/schematics/migrations/missing-injectable/definition_collector.ts similarity index 56% rename from packages/core/schematics/migrations/missing-injectable/module_collector.ts rename to packages/core/schematics/migrations/missing-injectable/definition_collector.ts index ba6644ed8d..b2d5cee32e 100644 --- a/packages/core/schematics/migrations/missing-injectable/module_collector.ts +++ b/packages/core/schematics/migrations/missing-injectable/definition_collector.ts @@ -18,12 +18,21 @@ export interface ResolvedNgModule { providersExpr: ts.Expression|null; } +export interface ResolvedDirective { + name: string; + node: ts.ClassDeclaration; + decorator: NgDecorator; + providersExpr: ts.Expression|null; + viewProvidersExpr: ts.Expression|null; +} + /** * Visitor that walks through specified TypeScript nodes and collects all - * found NgModule definitions. + * found NgModule, Directive or Component definitions. */ -export class NgModuleCollector { +export class NgDefinitionCollector { resolvedModules: ResolvedNgModule[] = []; + resolvedDirectives: ResolvedDirective[] = []; constructor(public typeChecker: ts.TypeChecker) {} @@ -41,13 +50,40 @@ export class NgModuleCollector { } const ngDecorators = getAngularDecorators(this.typeChecker, node.decorators); + const directiveDecorator = + ngDecorators.find(({name}) => name === 'Component' || name == 'Directive'); const ngModuleDecorator = ngDecorators.find(({name}) => name === 'NgModule'); if (ngModuleDecorator) { this._visitNgModuleClass(node, ngModuleDecorator); + } else if (directiveDecorator) { + this._visitDirectiveClass(node, directiveDecorator); } } + private _visitDirectiveClass(node: ts.ClassDeclaration, decorator: NgDecorator) { + const decoratorCall = decorator.node.expression; + const metadata = decoratorCall.arguments[0]; + + if (!metadata || !ts.isObjectLiteralExpression(metadata)) { + return; + } + + const providersNode = metadata.properties.filter(ts.isPropertyAssignment) + .find(p => getPropertyNameText(p.name) === 'providers'); + + const viewProvidersNode = metadata.properties.filter(ts.isPropertyAssignment) + .find(p => getPropertyNameText(p.name) === 'viewProviders'); + + this.resolvedDirectives.push({ + name: node.name ? node.name.text : 'default', + node, + decorator, + providersExpr: providersNode !== undefined ? providersNode.initializer : null, + viewProvidersExpr: viewProvidersNode !== undefined ? viewProvidersNode.initializer : null, + }); + } + private _visitNgModuleClass(node: ts.ClassDeclaration, decorator: NgDecorator) { const decoratorCall = decorator.node.expression; const metadata = decoratorCall.arguments[0]; diff --git a/packages/core/schematics/migrations/missing-injectable/google3/tslint_update_recorder.ts b/packages/core/schematics/migrations/missing-injectable/google3/tslint_update_recorder.ts index d3c2d0b982..13b7e80961 100644 --- a/packages/core/schematics/migrations/missing-injectable/google3/tslint_update_recorder.ts +++ b/packages/core/schematics/migrations/missing-injectable/google3/tslint_update_recorder.ts @@ -16,13 +16,13 @@ export class TslintUpdateRecorder implements UpdateRecorder { constructor(private ruleName: string, private sourceFile: ts.SourceFile) {} - addClassDecorator(node: ts.ClassDeclaration, decoratorText: string, moduleName: string) { + addClassDecorator(node: ts.ClassDeclaration, decoratorText: string, className: string) { // Adding a decorator should be the last replacement. Replacements/rule failures // are handled in reverse and in case a decorator and import are inserted at // the start of the file, the class decorator should come after the import. this.failures.unshift(new RuleFailure( this.sourceFile, node.getStart(), 0, `Class needs to be decorated with ` + - `"${decoratorText}" because it has been provided by "${moduleName}".`, + `"${decoratorText}" because it has been provided by "${className}".`, this.ruleName, Replacement.appendText(node.getStart(), `${decoratorText}\n`))); } @@ -42,7 +42,7 @@ export class TslintUpdateRecorder implements UpdateRecorder { `Import needs to be updated to import symbols: "${newNamedBindings}"`, this.ruleName, fix)); } - replaceDecorator(decorator: ts.Node, newText: string, moduleName: string): void { + replaceDecorator(decorator: ts.Node, newText: string, className: string): void { const fix = [ Replacement.deleteText(decorator.getStart(), decorator.getWidth()), Replacement.appendText(decorator.getStart(), newText), @@ -50,7 +50,7 @@ export class TslintUpdateRecorder implements UpdateRecorder { this.failures.push(new RuleFailure( this.sourceFile, decorator.getStart(), decorator.getEnd(), `Decorator needs to be replaced with "${newText}" because it has been provided ` + - `by "${moduleName}"`, + `by "${className}"`, this.ruleName, fix)); } diff --git a/packages/core/schematics/migrations/missing-injectable/index.ts b/packages/core/schematics/migrations/missing-injectable/index.ts index 6c21d43689..0e9e02b442 100644 --- a/packages/core/schematics/migrations/missing-injectable/index.ts +++ b/packages/core/schematics/migrations/missing-injectable/index.ts @@ -12,7 +12,7 @@ import * as ts from 'typescript'; import {getProjectTsConfigPaths} from '../../utils/project_tsconfig_paths'; import {createMigrationCompilerHost} from '../../utils/typescript/compiler_host'; import {parseTsconfigFile} from '../../utils/typescript/parse_tsconfig'; -import {NgModuleCollector} from './module_collector'; +import {NgDefinitionCollector} from './definition_collector'; import {MissingInjectableTransform} from './transform'; import {UpdateRecorder} from './update_recorder'; @@ -53,25 +53,25 @@ function runMissingInjectableMigration( const program = ts.createProgram(parsed.fileNames, parsed.options, host); const typeChecker = program.getTypeChecker(); - const moduleCollector = new NgModuleCollector(typeChecker); + const definitionCollector = new NgDefinitionCollector(typeChecker); const sourceFiles = program.getSourceFiles().filter( f => !f.isDeclarationFile && !program.isSourceFileFromExternalLibrary(f)); - // Analyze source files by detecting all modules. - sourceFiles.forEach(sourceFile => moduleCollector.visitNode(sourceFile)); + // Analyze source files by detecting all modules, directives and components. + sourceFiles.forEach(sourceFile => definitionCollector.visitNode(sourceFile)); - const {resolvedModules} = moduleCollector; + const {resolvedModules, resolvedDirectives} = definitionCollector; const transformer = new MissingInjectableTransform(typeChecker, getUpdateRecorder); const updateRecorders = new Map(); - resolvedModules.forEach(module => { - transformer.migrateModule(module).forEach(({message, node}) => { - const nodeSourceFile = node.getSourceFile(); - const relativeFilePath = relative(basePath, nodeSourceFile.fileName); - const {line, character} = - ts.getLineAndCharacterOfPosition(node.getSourceFile(), node.getStart()); - failures.push(`${relativeFilePath}@${line + 1}:${character + 1}: ${message}`); - }); + [...transformer.migrateModules(resolvedModules), + ...transformer.migrateDirectives(resolvedDirectives), + ].forEach(({message, node}) => { + const nodeSourceFile = node.getSourceFile(); + const relativeFilePath = relative(basePath, nodeSourceFile.fileName); + const {line, character} = + ts.getLineAndCharacterOfPosition(node.getSourceFile(), node.getStart()); + failures.push(`${relativeFilePath}@${line + 1}:${character + 1}: ${message}`); }); // Record the changes collected in the import manager and transformer. diff --git a/packages/core/schematics/migrations/missing-injectable/transform.ts b/packages/core/schematics/migrations/missing-injectable/transform.ts index eb940067e2..543ee428d3 100644 --- a/packages/core/schematics/migrations/missing-injectable/transform.ts +++ b/packages/core/schematics/migrations/missing-injectable/transform.ts @@ -13,10 +13,11 @@ import * as ts from 'typescript'; import {getAngularDecorators} from '../../utils/ng_decorators'; +import {ResolvedDirective, ResolvedNgModule} from './definition_collector'; import {ImportManager} from './import_manager'; -import {ResolvedNgModule} from './module_collector'; import {UpdateRecorder} from './update_recorder'; + /** Name of decorators which imply that a given class does not need to be migrated. */ const NO_MIGRATE_DECORATORS = ['Injectable', 'Directive', 'Component', 'Pipe']; @@ -42,6 +43,24 @@ export class MissingInjectableTransform { recordChanges() { this.importManager.recordChanges(); } + /** + * Migrates all specified NgModule's by walking through referenced providers + * and decorating them with "@Injectable" if needed. + */ + migrateModules(modules: ResolvedNgModule[]): AnalysisFailure[] { + return modules.reduce( + (failures, node) => failures.concat(this.migrateModule(node)), [] as AnalysisFailure[]); + } + + /** + * Migrates all specified directives by walking through referenced providers + * and decorating them with "@Injectable" if needed. + */ + migrateDirectives(directives: ResolvedDirective[]): AnalysisFailure[] { + return directives.reduce( + (failures, node) => failures.concat(this.migrateDirective(node)), [] as AnalysisFailure[]); + } + /** Migrates a given NgModule by walking through the referenced providers. */ migrateModule(module: ResolvedNgModule): AnalysisFailure[] { if (module.providersExpr === null) { @@ -60,11 +79,43 @@ export class MissingInjectableTransform { return this._visitProviderResolvedValue(evaluatedExpr, module); } + + /** + * Migrates a given directive by walking through defined providers. This method + * also handles components with "viewProviders" defined. + */ + migrateDirective(directive: ResolvedDirective): AnalysisFailure[] { + const failures: AnalysisFailure[] = []; + + // Migrate "providers" on directives and components if defined. + if (directive.providersExpr) { + const evaluatedExpr = this.partialEvaluator.evaluate(directive.providersExpr); + if (!Array.isArray(evaluatedExpr)) { + return [ + {node: directive.providersExpr, message: `Providers are not statically analyzable.`} + ]; + } + failures.push(...this._visitProviderResolvedValue(evaluatedExpr, directive)); + } + + // Migrate "viewProviders" on components if defined. + if (directive.viewProvidersExpr) { + const evaluatedExpr = this.partialEvaluator.evaluate(directive.viewProvidersExpr); + if (!Array.isArray(evaluatedExpr)) { + return [ + {node: directive.viewProvidersExpr, message: `Providers are not statically analyzable.`} + ]; + } + failures.push(...this._visitProviderResolvedValue(evaluatedExpr, directive)); + } + return failures; + } + /** * Migrates a given provider class if it is not decorated with * any Angular decorator. */ - migrateProviderClass(node: ts.ClassDeclaration, module: ResolvedNgModule) { + migrateProviderClass(node: ts.ClassDeclaration, context: ResolvedNgModule|ResolvedDirective) { if (this.visitedProviderClasses.has(node)) { return; } @@ -93,9 +144,9 @@ export class MissingInjectableTransform { const existingInjectDecorator = ngDecorators !== null ? ngDecorators.find(d => d.name === 'Inject') : null; if (existingInjectDecorator) { - updateRecorder.replaceDecorator(existingInjectDecorator.node, newDecoratorText, module.name); + updateRecorder.replaceDecorator(existingInjectDecorator.node, newDecoratorText, context.name); } else { - updateRecorder.addClassDecorator(node, newDecoratorText, module.name); + updateRecorder.addClassDecorator(node, newDecoratorText, context.name); } } diff --git a/packages/core/schematics/migrations/missing-injectable/update_recorder.ts b/packages/core/schematics/migrations/missing-injectable/update_recorder.ts index cac8551020..ca9215d897 100644 --- a/packages/core/schematics/migrations/missing-injectable/update_recorder.ts +++ b/packages/core/schematics/migrations/missing-injectable/update_recorder.ts @@ -16,7 +16,7 @@ import * as ts from 'typescript'; export interface UpdateRecorder { addNewImport(start: number, importText: string): void; updateExistingImport(namedBindings: ts.NamedImports, newNamedBindings: string): void; - addClassDecorator(node: ts.ClassDeclaration, text: string, moduleName: string): void; - replaceDecorator(node: ts.Decorator, newText: string, moduleName: string): void; + addClassDecorator(node: ts.ClassDeclaration, text: string, className: string): void; + replaceDecorator(node: ts.Decorator, newText: string, className: string): void; commitUpdate(): void; } diff --git a/packages/core/schematics/test/google3/missing_injectable_rule_spec.ts b/packages/core/schematics/test/google3/missing_injectable_rule_spec.ts index f5cac83730..a3fc1afc30 100644 --- a/packages/core/schematics/test/google3/missing_injectable_rule_spec.ts +++ b/packages/core/schematics/test/google3/missing_injectable_rule_spec.ts @@ -45,183 +45,224 @@ describe('Google3 missing injectable tslint rule', () => { function getFile(fileName: string) { return readFileSync(join(tmpDir, fileName), 'utf8'); } - it('should create proper failures for missing injectable providers', () => { - writeFile('index.ts', ` - import { NgModule } from '@angular/core'; + describe('NgModule', () => createTests('NgModule', 'providers')); + describe('Directive', () => createTests('Directive', 'providers')); - export class A {} + describe('Component', () => { + createTests('Component', 'providers'); + createTests('Component', 'viewProviders'); - @NgModule({providers: [A]}) - export class AppModule {} - `); + it('should migrate all providers defined in "viewProviders" and "providers" in the ' + + 'same component', + () => { + writeFile('/index.ts', ` + import {Component} from '@angular/core'; + + export class MyService {} + export class MySecondService {} + + @Component({ + providers: [MyService], + viewProviders: [MySecondService], + }) + export class TestClass {} + `); - const linter = runTSLint(false); - const failures = linter.getResult().failures; + const result = runTSLint().getResult(); - expect(failures.length).toBe(2); - expect(failures[0].getFailure()) - .toMatch(/Class needs to be decorated with "@Injectable\(\)".*provided by "AppModule"/); - expect(failures[0].getStartPosition().getLineAndCharacter()).toEqual({line: 3, character: 6}); - expect(failures[1].getFailure()).toMatch(/Import needs to be updated to import.*Injectable/); - expect(failures[1].getStartPosition().getLineAndCharacter()).toEqual({line: 1, character: 13}); + expect(result.errorCount).toBe(0); + expect(getFile('/index.ts')).toMatch(/@Injectable\(\)\s+export class MyService/); + expect(getFile('/index.ts')).toMatch(/@Injectable\(\)\s+export class MySecondService/); + expect(getFile('/index.ts')).toContain(`{ Component, Injectable } from '@angular/core`); + }); }); - it('should update provider classes which need to be migrated in Ivy', () => { - writeFile('/index.ts', ` - import {Pipe, Directive, Component, NgModule} from '@angular/core'; - - @Pipe() - export class WithPipe {} + function createTests( + type: 'NgModule' | 'Directive' | 'Component', propName: 'providers' | 'viewProviders') { + it('should create proper failures for missing injectable providers', () => { + writeFile('index.ts', ` + import { ${type} } from '@angular/core'; + + export class A {} + + @${type}({${propName}: [A]}) + export class TestClass {} + `); + + const linter = runTSLint(false); + const failures = linter.getResult().failures; + + expect(failures.length).toBe(2); + expect(failures[0].getFailure()) + .toMatch(/Class needs to be decorated with "@Injectable\(\)".*provided by "TestClass"/); + expect(failures[0].getStartPosition().getLineAndCharacter()).toEqual({line: 3, character: 8}); + expect(failures[1].getFailure()).toMatch(/Import needs to be updated to import.*Injectable/); + expect(failures[1].getStartPosition().getLineAndCharacter()) + .toEqual({line: 1, character: 15}); + }); + + it('should update provider classes which need to be migrated in Ivy', () => { + writeFile('/index.ts', ` + import {Pipe, Directive, Component, NgModule} from '@angular/core'; - @Directive() - export class WithDirective {} + @Pipe() + export class WithPipe {} + + @Directive() + export class WithDirective {} + + @Component() + export class WithComponent {} + + export class MyServiceA {} + export class MyServiceB {} + export class MyServiceC {} + export class MyServiceD {} + export class MyServiceE {} + export class MyServiceF {} + export class MyServiceG {} + + @${type}({${propName}: [ + WithPipe, + [ + WithDirective, + WithComponent, + MyServiceA, + ] + MyServiceB, + {provide: MyServiceC}, + {provide: null, useClass: MyServiceD}, + {provide: null, useExisting: MyServiceE}, + {provide: MyServiceF, useFactory: () => null}, + {provide: MyServiceG, useValue: null}, + ]}) + export class TestClass {} + `); + + + runTSLint(); + + expect(getFile('/index.ts')).toMatch(/'@angular\/core';\s+@Pipe\(\)\s+export class WithPipe/); + expect(getFile('/index.ts')) + .toMatch(/WithPipe {}\s+@Directive\(\)\s+export class WithDirective/); + expect(getFile('/index.ts')) + .toMatch(/WithDirective {}\s+@Component\(\)\s+export class WithComponent/); + expect(getFile('/index.ts')).toMatch(/@Injectable\(\)\s+export class MyServiceA/); + expect(getFile('/index.ts')).toMatch(/@Injectable\(\)\s+export class MyServiceB/); + expect(getFile('/index.ts')).toMatch(/@Injectable\(\)\s+export class MyServiceC/); + expect(getFile('/index.ts')).toMatch(/@Injectable\(\)\s+export class MyServiceD/); + expect(getFile('/index.ts')).toMatch(/@Injectable\(\)\s+export class MyServiceE/); + expect(getFile('/index.ts')).toMatch(/MyServiceE {}\s+export class MyServiceF/); + expect(getFile('/index.ts')).toMatch(/MyServiceF {}\s+export class MyServiceG/); + }); + + it(`should migrate provider once if referenced in multiple ${type} definitions`, () => { + writeFile('/index.ts', ` + import {${type}} from '@angular/core'; - @Component() - export class WithComponent {} + export class ServiceA {} + + @${type}({${propName}: [ServiceA]}) + export class TestClass {} + `); + + writeFile('/second.ts', ` + import {${type}} from '@angular/core'; + import {ServiceA} from './index'; + + export class ServiceB {} + + @${type}({${propName}: [ServiceA, ServiceB]}) + export class TestClass2 {} + `); + + runTSLint(); + + expect(getFile('/index.ts')) + .toMatch(/@angular\/core';\s+@Injectable\(\)\s+export class ServiceA/); + expect(getFile('/index.ts')).toContain(`{ ${type}, Injectable } from '@angular/core`); + expect(getFile('/second.ts')).toMatch(/@Injectable\(\)\s+export class ServiceB/); + expect(getFile('/second.ts')).toContain(`{ ${type}, Injectable } from '@angular/core`); + }); + + it('should warn if a referenced provider could not be resolved', () => { + writeFile('/index.ts', ` + import {${type}} from '@angular/core'; + + @${type}({${propName}: [NotPresent]}) + export class TestClass {} + `); + + const linter = runTSLint(); + const failures = linter.getResult().failures; + + expect(failures.length).toBe(1); + expect(failures[0].getFailure()).toMatch(/Provider is not statically analyzable./); + expect(failures[0].getStartPosition().getLineAndCharacter()) + .toEqual({line: 3, character: 14 + type.length + propName.length}); + }); + + it(`should warn if the "${propName}" value could not be resolved`, () => { + writeFile('/index.ts', ` + import {${type}} from '@angular/core'; + + @${type}({${propName}: NOT_ANALYZABLE) + export class TestClass {} + `); + + const linter = runTSLint(); + const failures = linter.getResult().failures; + + expect(failures.length).toBe(1); + expect(failures[0].getFailure()).toMatch(/Providers.*not statically analyzable./); + expect(failures[0].getStartPosition().getLineAndCharacter()) + .toEqual({line: 3, character: 13 + type.length + propName.length}); + }); + + it('should create new import for @Injectable when migrating provider', () => { + writeFile('/index.ts', ` + import {${type}} from '@angular/core'; + import {MyService, MySecondService} from './service'; + + @${type}({${propName}: [MyService, MySecondService]}) + export class TestClass {} + `); + + writeFile('/service.ts', `export class MyService {} - export class MyServiceA {} - export class MyServiceB {} - export class MyServiceC {} - export class MyServiceD {} - export class MyServiceE {} - export class MyServiceF {} - export class MyServiceG {} - - @NgModule({providers: [ - WithPipe, - [ - WithDirective, - WithComponent, - MyServiceA, - ] - MyServiceB, - {provide: MyServiceC}, - {provide: null, useClass: MyServiceD}, - {provide: null, useExisting: MyServiceE}, - {provide: MyServiceF, useFactory: () => null}, - {provide: MyServiceG, useValue: null}, - ]}) - export class MyModule {} - `); + export class MySecondService {} + `); + runTSLint(); - runTSLint(); + expect(getFile('/service.ts')).toMatch(/@Injectable\(\)\s+export class MyService/); + expect(getFile('/service.ts')).toMatch(/@Injectable\(\)\s+export class MySecondService/); + expect(getFile('/service.ts')).toMatch(/import { Injectable } from "@angular\/core";/); + }); - expect(getFile('/index.ts')).toMatch(/'@angular\/core';\s+@Pipe\(\)\s+export class WithPipe/); - expect(getFile('/index.ts')) - .toMatch(/WithPipe {}\s+@Directive\(\)\s+export class WithDirective/); - expect(getFile('/index.ts')) - .toMatch(/WithDirective {}\s+@Component\(\)\s+export class WithComponent/); - expect(getFile('/index.ts')).toMatch(/@Injectable\(\)\s+export class MyServiceA/); - expect(getFile('/index.ts')).toMatch(/@Injectable\(\)\s+export class MyServiceB/); - expect(getFile('/index.ts')).toMatch(/@Injectable\(\)\s+export class MyServiceC/); - expect(getFile('/index.ts')).toMatch(/@Injectable\(\)\s+export class MyServiceD/); - expect(getFile('/index.ts')).toMatch(/@Injectable\(\)\s+export class MyServiceE/); - expect(getFile('/index.ts')).toMatch(/MyServiceE {}\s+export class MyServiceF/); - expect(getFile('/index.ts')).toMatch(/MyServiceF {}\s+export class MyServiceG/); - }); + it('should remove @Inject decorator for providers which are migrated', () => { + writeFile('/index.ts', ` + import {${type}} from '@angular/core'; + import {MyService} from './service'; + + @${type}({${propName}: [MyService]}) + export class TestClass {} + `); - it('should migrate provider once if referenced in multiple NgModule definitions', () => { - writeFile('/index.ts', ` - import {NgModule} from '@angular/core'; - - export class ServiceA {} - - @NgModule({providers: [ServiceA]}) - export class MyModule {} - `); - - writeFile('/second.ts', ` - import {NgModule} from '@angular/core'; - import {ServiceA} from './index'; + writeFile('/service.ts', ` + import {Inject} from '@angular/core'; - export class ServiceB {} - - @NgModule({providers: [ServiceA, ServiceB]}) - export class SecondModule {} - `); + @Inject() + export class MyService {} + `); - runTSLint(); + runTSLint(); - expect(getFile('/index.ts')) - .toMatch(/@angular\/core';\s+@Injectable\(\)\s+export class ServiceA/); - expect(getFile('/index.ts')).toMatch(/{ NgModule, Injectable } from '@angular\/core/); - expect(getFile('/second.ts')).toMatch(/@Injectable\(\)\s+export class ServiceB/); - expect(getFile('/second.ts')).toMatch(/{ NgModule, Injectable } from '@angular\/core/); - }); + expect(getFile('/service.ts')).toMatch(/core';\s+@Injectable\(\)\s+export class MyService/); + expect(getFile('/service.ts')) + .toMatch(/import { Inject, Injectable } from '@angular\/core';/); + }); + } - it('should warn if a referenced provider could not be resolved', () => { - writeFile('/index.ts', ` - import {NgModule} from '@angular/core'; - - @NgModule({providers: [NotPresent]}) - export class MyModule {} - `); - const linter = runTSLint(); - const failures = linter.getResult().failures; - - expect(failures.length).toBe(1); - expect(failures[0].getFailure()).toMatch(/Provider is not statically analyzable./); - expect(failures[0].getStartPosition().getLineAndCharacter()).toEqual({line: 3, character: 29}); - }); - - it('should warn if the module providers could not be resolved', () => { - writeFile('/index.ts', ` - import {NgModule} from '@angular/core'; - - @NgModule({providers: NOT_ANALYZABLE) - export class MyModule {} - `); - - const linter = runTSLint(); - const failures = linter.getResult().failures; - - expect(failures.length).toBe(1); - expect(failures[0].getFailure()).toMatch(/Providers of module.*not statically analyzable./); - expect(failures[0].getStartPosition().getLineAndCharacter()).toEqual({line: 3, character: 28}); - }); - - it('should create new import for @Injectable when migrating provider', () => { - writeFile('/index.ts', ` - import {NgModule} from '@angular/core'; - import {MyService, MySecondService} from './service'; - - @NgModule({providers: [MyService, MySecondService]}) - export class MyModule {} - `); - - writeFile('/service.ts', `export class MyService {} - - export class MySecondService {} - `); - - runTSLint(); - - expect(getFile('/service.ts')).toMatch(/@Injectable\(\)\s+export class MyService/); - expect(getFile('/service.ts')).toMatch(/@Injectable\(\)\s+export class MySecondService/); - expect(getFile('/service.ts')).toMatch(/import { Injectable } from "@angular\/core";/); - }); - - it('should remove @Inject decorator for providers which are migrated', () => { - writeFile('/index.ts', ` - import {NgModule} from '@angular/core'; - import {MyService} from './service'; - - @NgModule({providers: [MyService]}) - export class MyModule {} - `); - - writeFile('/service.ts', ` - import {Inject} from '@angular/core'; - - @Inject() - export class MyService {} - `); - - runTSLint(); - - expect(getFile('/service.ts')).toMatch(/core';\s+@Injectable\(\)\s+export class MyService/); - expect(getFile('/service.ts')).toMatch(/import { Inject, Injectable } from '@angular\/core';/); - }); }); diff --git a/packages/core/schematics/test/missing_injectable_migration_spec.ts b/packages/core/schematics/test/missing_injectable_migration_spec.ts index 00ef1f98a8..cca3a0cc36 100644 --- a/packages/core/schematics/test/missing_injectable_migration_spec.ts +++ b/packages/core/schematics/test/missing_injectable_migration_spec.ts @@ -63,518 +63,574 @@ describe('Missing injectable migration', () => { await runner.runSchematicAsync('migration-v9-missing-injectable', {}, tree).toPromise(); } - it('should migrate type provider in NgModule', async() => { - writeFile('/index.ts', ` - import {NgModule} from '@angular/core'; - - export class MyService {} - - @NgModule({providers: [MyService]}) - export class MyModule {} - `); - - await runMigration(); - - expect(warnOutput.length).toBe(0); - expect(tree.readContent('/index.ts')).toMatch(/@Injectable\(\)\s+export class MyService/); - expect(tree.readContent('/index.ts')).toMatch(/{ NgModule, Injectable } from '@angular\/core/); - }); - - it('should migrate object literal provider in NgModule', async() => { - writeFile('/index.ts', ` - import {NgModule} from '@angular/core'; - - export class MyService {} - - @NgModule({providers: [{provide: MyService}]}) - export class MyModule {} - `); - - await runMigration(); - - expect(warnOutput.length).toBe(0); - expect(tree.readContent('/index.ts')).toMatch(/@Injectable\(\)\s+export class MyService/); - expect(tree.readContent('/index.ts')).toMatch(/{ NgModule, Injectable } from '@angular\/core/); - }); - - it('should not migrate object literal provider with "useValue" in NgModule', async() => { - writeFile('/index.ts', ` - import {NgModule} from '@angular/core'; - - export class MyService {} - - @NgModule({providers: [{provide: MyService, useValue: null }]}) - export class MyModule {} - `); - - await runMigration(); - - expect(warnOutput.length).toBe(0); - expect(tree.readContent('/index.ts')).not.toContain('@Injectable'); - }); - - it('should not migrate object literal provider with "useFactory" in NgModule', async() => { - writeFile('/index.ts', ` - import {NgModule} from '@angular/core'; - - export class MyService {} - - @NgModule({providers: [{provide: MyService, useFactory: () => null }]}) - export class MyModule {} - `); - - await runMigration(); - - expect(warnOutput.length).toBe(0); - expect(tree.readContent('/index.ts')).not.toContain('@Injectable'); - }); - - it('should migrate object literal provider with "useExisting" in NgModule', async() => { - writeFile('/index.ts', ` - import {NgModule} from '@angular/core'; - - export class MyService {} - export class MyToken {} - - @NgModule({providers: [{provide: MyToken, useExisting: MyService}]}) - export class MyModule {} - `); - - await runMigration(); - - expect(warnOutput.length).toBe(0); - expect(tree.readContent('/index.ts')).toMatch(/@Injectable\(\)\s+export class MyService/); - expect(tree.readContent('/index.ts')).toMatch(/MyService {}\s+export class MyToken/); - expect(tree.readContent('/index.ts')).toMatch(/{ NgModule, Injectable } from '@angular\/core/); - }); - - it('should migrate object literal provider with "useClass" in NgModule', async() => { - writeFile('/index.ts', ` - import {NgModule} from '@angular/core'; - - export class MyService {} - export class MyToken {} - - @NgModule({providers: [{provide: MyToken, useClass: MyService}]}) - export class MyModule {} - `); - - await runMigration(); - - expect(warnOutput.length).toBe(0); - expect(tree.readContent('/index.ts')).toMatch(/@Injectable\(\)\s+export class MyService/); - expect(tree.readContent('/index.ts')).toMatch(/MyService {}\s+export class MyToken/); - expect(tree.readContent('/index.ts')).toMatch(/{ NgModule, Injectable } from '@angular\/core/); - }); - - it('should not migrate provider which is already decorated with @Injectable', async() => { - writeFile('/index.ts', ` - import {Injectable, NgModule} from '@angular/core'; - - @Injectable() - export class MyService {} - - @NgModule({providers: [MyService]}) - export class MyModule {} - `); - - await runMigration(); - - expect(warnOutput.length).toBe(0); - expect(tree.readContent('/index.ts')) - .toMatch(/@angular\/core';\s+@Injectable\(\)\s+export class MyService/); - }); - - it('should not migrate provider which is already decorated with @Directive', async() => { - writeFile('/index.ts', ` - import {Directive, NgModule} from '@angular/core'; - - @Directive() - export class MyService {} - - @NgModule({providers: [MyService]}) - export class MyModule {} - `); - - await runMigration(); - - expect(warnOutput.length).toBe(0); - expect(tree.readContent('/index.ts')).not.toContain('@Injectable'); - }); - - it('should not migrate provider which is already decorated with @Component', async() => { - writeFile('/index.ts', ` - import {Component, NgModule} from '@angular/core'; - - @Component() - export class MyService {} - - @NgModule({providers: [MyService]}) - export class MyModule {} - `); - - await runMigration(); - - expect(warnOutput.length).toBe(0); - expect(tree.readContent('/index.ts')).not.toContain('@Injectable'); - }); - - it('should not migrate provider which is already decorated with @Pipe', async() => { - writeFile('/index.ts', ` - import {Pipe, NgModule} from '@angular/core'; - - @Pipe() - export class MyService {} - - @NgModule({providers: [MyService]}) - export class MyModule {} - `); - - await runMigration(); - - expect(warnOutput.length).toBe(0); - expect(tree.readContent('/index.ts')).not.toContain('@Injectable'); - }); - - it('should migrate multiple providers in same NgModule', async() => { - writeFile('/index.ts', ` - import {NgModule} from '@angular/core'; - - export class ServiceA {} - export class ServiceB {} - - @NgModule({providers: [ServiceA, ServiceB]}) - export class MyModule {} - `); - - await runMigration(); - - expect(warnOutput.length).toBe(0); - expect(tree.readContent('/index.ts')).toMatch(/@Injectable\(\)\s+export class ServiceA/); - expect(tree.readContent('/index.ts')).toMatch(/@Injectable\(\)\s+export class ServiceB/); - expect(tree.readContent('/index.ts')).toMatch(/{ NgModule, Injectable } from '@angular\/core/); - }); - - it('should migrate multiple mixed providers in same NgModule', async() => { - writeFile('/index.ts', ` - import {NgModule} from '@angular/core'; - - export class ServiceA {} - export class ServiceB {} - export class ServiceC {} - - @NgModule({ - providers: [ - ServiceA, - {provide: ServiceB}, - {provide: SomeToken, useClass: ServiceC}, - ]}) - export class MyModule {} - `); - - await runMigration(); - - expect(warnOutput.length).toBe(0); - expect(tree.readContent('/index.ts')).toMatch(/@Injectable\(\)\s+export class ServiceA/); - expect(tree.readContent('/index.ts')).toMatch(/@Injectable\(\)\s+export class ServiceB/); - expect(tree.readContent('/index.ts')).toMatch(/@Injectable\(\)\s+export class ServiceC/); - expect(tree.readContent('/index.ts')).toMatch(/{ NgModule, Injectable } from '@angular\/core/); - }); - - - it('should migrate multiple nested providers in same NgModule', async() => { - writeFile('/index.ts', ` - import {NgModule} from '@angular/core'; - - export class ServiceA {} - export class ServiceB {} - export class ServiceC {} - - @NgModule({ - providers: [ - ServiceA, - [ - {provide: ServiceB}, - ServiceC, - ], - ]}) - export class MyModule {} - `); - - await runMigration(); - - expect(warnOutput.length).toBe(0); - expect(tree.readContent('/index.ts')).toMatch(/@Injectable\(\)\s+export class ServiceA/); - expect(tree.readContent('/index.ts')).toMatch(/@Injectable\(\)\s+export class ServiceB/); - expect(tree.readContent('/index.ts')).toMatch(/@Injectable\(\)\s+export class ServiceC/); - expect(tree.readContent('/index.ts')).toMatch(/{ NgModule, Injectable } from '@angular\/core/); - }); - - it('should migrate providers referenced through identifier', async() => { - writeFile('/index.ts', ` - import {NgModule} from '@angular/core'; - - export class ServiceA {} - export class ServiceB {} - - const PROVIDERS = [ServiceA, ServiceB]; - - @NgModule({ - providers: PROVIDERS, - }) - export class MyModule {} - `); - - await runMigration(); - - expect(warnOutput.length).toBe(0); - expect(tree.readContent('/index.ts')).toMatch(/@Injectable\(\)\s+export class ServiceA/); - expect(tree.readContent('/index.ts')).toMatch(/@Injectable\(\)\s+export class ServiceB/); - expect(tree.readContent('/index.ts')).toMatch(/{ NgModule, Injectable } from '@angular\/core/); - }); - - it('should migrate providers created through static analyzable function call', async() => { - writeFile('/index.ts', ` - import {NgModule} from '@angular/core'; - - export class ServiceA {} - export class ServiceB {} - - export function createProviders(x: any) { - return [ServiceA, x] - } - - @NgModule({ - providers: createProviders(ServiceB), - }) - export class MyModule {} - `); - - await runMigration(); - - expect(warnOutput.length).toBe(0); - expect(tree.readContent('/index.ts')).toMatch(/@Injectable\(\)\s+export class ServiceA/); - expect(tree.readContent('/index.ts')).toMatch(/@Injectable\(\)\s+export class ServiceB/); - expect(tree.readContent('/index.ts')).toMatch(/{ NgModule, Injectable } from '@angular\/core/); - }); - - it('should migrate providers which are computed through spread operator', async() => { - writeFile('/index.ts', ` - import {NgModule} from '@angular/core'; - - export class ServiceA {} - export class ServiceB {} - - const otherServices = [ServiceB]; - - @NgModule({ - providers: [ServiceA, ...otherServices], - }) - export class MyModule {} - `); - - await runMigration(); - - expect(warnOutput.length).toBe(0); - expect(tree.readContent('/index.ts')).toMatch(/@Injectable\(\)\s+export class ServiceA/); - expect(tree.readContent('/index.ts')).toMatch(/@Injectable\(\)\s+export class ServiceB/); - expect(tree.readContent('/index.ts')).toMatch(/{ NgModule, Injectable } from '@angular\/core/); - }); - - it('should migrate provider once if referenced in multiple NgModule definitions', async() => { - writeFile('/index.ts', ` - import {NgModule} from '@angular/core'; - - export class ServiceA {} - - @NgModule({providers: [ServiceA]}) - export class MyModule {} - `); - - writeFile('/second.ts', ` - import {NgModule} from '@angular/core'; - import {ServiceA} from './index'; - - export class ServiceB {} - - @NgModule({providers: [ServiceA, ServiceB]}) - export class SecondModule {} - `); - - await runMigration(); - - expect(warnOutput.length).toBe(0); - expect(tree.readContent('/index.ts')) - .toMatch(/@angular\/core';\s+@Injectable\(\)\s+export class ServiceA/); - expect(tree.readContent('/index.ts')).toMatch(/{ NgModule, Injectable } from '@angular\/core/); - expect(tree.readContent('/second.ts')).toMatch(/@Injectable\(\)\s+export class ServiceB/); - expect(tree.readContent('/second.ts')).toMatch(/{ NgModule, Injectable } from '@angular\/core/); - }); - - it('should create new import for @Injectable when migrating provider', async() => { - writeFile('/index.ts', ` - import {NgModule} from '@angular/core'; - import {MyService, MySecondService} from './service'; - - @NgModule({providers: [MyService, MySecondService]}) - export class MyModule {} - `); - - writeFile('/service.ts', `export class MyService {} - - export class MySecondService {} - `); - - await runMigration(); - - expect(warnOutput.length).toBe(0); - expect(tree.readContent('/service.ts')).toMatch(/@Injectable\(\)\s+export class MyService/); - expect(tree.readContent('/service.ts')) - .toMatch(/@Injectable\(\)\s+export class MySecondService/); - expect(tree.readContent('/service.ts')).toMatch(/import { Injectable } from "@angular\/core";/); - }); - - it('should re-use existing namespace import for importing @Injectable when migrating provider', - async() => { - writeFile('/index.ts', ` - import * as core from '@angular/core'; - - export class MyService { - constructor() { - console.log(core.isDevMode()); - } - } - `); - - writeFile('/app.module.ts', ` - import {NgModule} from '@angular/core'; - import {MyService} from './index'; - - @NgModule({providers: [MyService]}) - export class MyModule {} - `); - - await runMigration(); - - expect(warnOutput.length).toBe(0); - expect(tree.readContent('/index.ts')) - .toMatch(/@core.Injectable\(\)\s+export class MyService/); - }); - - it('should warn if a referenced provider could not be resolved', async() => { - writeFile('/index.ts', ` - import {NgModule} from '@angular/core'; - - @NgModule({providers: [NotPresent]}) - export class MyModule {} - `); - - await runMigration(); - - expect(warnOutput.length).toBe(1); - expect(warnOutput[0]).toMatch(/\s+index\.ts@4:30: Provider is not statically analyzable./); - }); - - it('should warn if the module providers could not be resolved', async() => { - writeFile('/index.ts', ` - import {NgModule} from '@angular/core'; - - @NgModule({providers: NOT_ANALYZABLE) - export class MyModule {} - `); - - await runMigration(); - - expect(warnOutput.length).toBe(1); - expect(warnOutput[0]) - .toMatch(/\s+index\.ts@4:29: Providers of module.*not statically analyzable./); - }); - - it('should not throw if an empty @NgModule is analyzed', async() => { - writeFile('/index.ts', ` - import {NgModule} from '@angular/core'; + describe('NgModule', () => createTests('NgModule', 'providers')); + describe('Directive', () => createTests('Directive', 'providers')); + + describe('Component', () => { + createTests('Component', 'providers'); + createTests('Component', 'viewProviders'); + + it('should migrate all providers defined in "viewProviders" and "providers" in the ' + + 'same component', + async() => { + writeFile('/index.ts', ` + import {Component} from '@angular/core'; + + export class MyService {} + export class MySecondService {} - @NgModule() - export class MyModule {} - `); + @Component({ + providers: [MyService], + viewProviders: [MySecondService], + }) + export class TestClass {} + `); + + await runMigration(); + + expect(warnOutput.length).toBe(0); + expect(tree.readContent('/index.ts')).toMatch(/@Injectable\(\)\s+export class MyService/); + expect(tree.readContent('/index.ts')) + .toMatch(/@Injectable\(\)\s+export class MySecondService/); + expect(tree.readContent('/index.ts')) + .toContain(`{ Component, Injectable } from '@angular/core`); + }); + }); + + function createTests( + type: 'NgModule' | 'Directive' | 'Component', propName: 'providers' | 'viewProviders') { + it(`should migrate type provider in ${type}`, async() => { + writeFile('/index.ts', ` + import {${type}} from '@angular/core'; + + export class MyService {} + + @${type}({${propName}: [MyService]}) + export class TestClass {} + `); - try { await runMigration(); - } catch (e) { - fail(e); - } - expect(warnOutput.length).toBe(0); - }); + expect(warnOutput.length).toBe(0); + expect(tree.readContent('/index.ts')).toMatch(/@Injectable\(\)\s+export class MyService/); + expect(tree.readContent('/index.ts')) + .toContain(`{ ${type}, Injectable } from '@angular/core`); + }); - it('should create new import for injectable after full end of last import statement', async() => { - writeFile('/index.ts', ` - import {NgModule} from '@angular/core'; - import {MyService} from './service'; - - @NgModule({providers: [MyService]}) - export class MyModule {} - `); + it(`should migrate object literal provider in ${type}`, async() => { + writeFile('/index.ts', ` + import {${type}} from '@angular/core'; + + export class MyService {} + + @${type}({${propName}: [{provide: MyService}]}) + export class TestClass {} + `); - writeFile('/service.ts', ` - import * as a from 'a'; - import * as a from 'b'; // some comment + await runMigration(); + + expect(warnOutput.length).toBe(0); + expect(tree.readContent('/index.ts')).toMatch(/@Injectable\(\)\s+export class MyService/); + expect(tree.readContent('/index.ts')) + .toContain(`{ ${type}, Injectable } from '@angular/core`); + }); + + it(`should not migrate object literal provider with "useValue" in ${type}`, async() => { + writeFile('/index.ts', ` + import {${type}} from '@angular/core'; + + export class MyService {} + + @${type}({${propName}: [{provide: MyService, useValue: null }]}) + export class TestClass {} + `); + + await runMigration(); + + expect(warnOutput.length).toBe(0); + expect(tree.readContent('/index.ts')).not.toContain('@Injectable'); + }); + + it(`should not migrate object literal provider with "useFactory" in ${type}`, async() => { + writeFile('/index.ts', ` + import {${type}} from '@angular/core'; + + export class MyService {} + + @${type}({${propName}: [{provide: MyService, useFactory: () => null }]}) + export class TestClass {} + `); + + await runMigration(); + + expect(warnOutput.length).toBe(0); + expect(tree.readContent('/index.ts')).not.toContain('@Injectable'); + }); + + it(`should migrate object literal provider with "useExisting" in ${type}`, async() => { + writeFile('/index.ts', ` + import {${type}} from '@angular/core'; + + export class MyService {} + export class MyToken {} + + @${type}({${propName}: [{provide: MyToken, useExisting: MyService}]}) + export class TestClass {} + `); + + await runMigration(); + + expect(warnOutput.length).toBe(0); + expect(tree.readContent('/index.ts')).toMatch(/@Injectable\(\)\s+export class MyService/); + expect(tree.readContent('/index.ts')).toMatch(/MyService {}\s+export class MyToken/); + expect(tree.readContent('/index.ts')) + .toContain(`{ ${type}, Injectable } from '@angular/core`); + }); + + it(`should migrate object literal provider with "useClass" in ${type}`, async() => { + writeFile('/index.ts', ` + import {${type}} from '@angular/core'; + + export class MyService {} + export class MyToken {} + + @${type}({${propName}: [{provide: MyToken, useClass: MyService}]}) + export class TestClass {} + `); + + await runMigration(); + + expect(warnOutput.length).toBe(0); + expect(tree.readContent('/index.ts')).toMatch(/@Injectable\(\)\s+export class MyService/); + expect(tree.readContent('/index.ts')).toMatch(/MyService {}\s+export class MyToken/); + expect(tree.readContent('/index.ts')) + .toContain(`{ ${type}, Injectable } from '@angular/core`); + }); + + it('should not migrate provider which is already decorated with @Injectable', async() => { + writeFile('/index.ts', ` + import {Injectable, ${type}} from '@angular/core'; + + @Injectable() + export class MyService {} + + @${type}({${propName}: [MyService]}) + export class TestClass {} + `); + + await runMigration(); + + expect(warnOutput.length).toBe(0); + expect(tree.readContent('/index.ts')) + .toMatch(/@angular\/core';\s+@Injectable\(\)\s+export class MyService/); + }); + + it('should not migrate provider which is already decorated with @Directive', async() => { + writeFile('/index.ts', ` + import {Directive, ${type}} from '@angular/core'; + + @Directive() + export class MyService {} + + @${type}({${propName}: [MyService]}) + export class TestClass {} + `); + + await runMigration(); + + expect(warnOutput.length).toBe(0); + expect(tree.readContent('/index.ts')).not.toContain('@Injectable'); + }); + + it('should not migrate provider which is already decorated with @Component', async() => { + writeFile('/index.ts', ` + import {Component, ${type}} from '@angular/core'; + + @Component() + export class MyService {} + + @${type}({${propName}: [MyService]}) + export class TestClass {} + `); + + await runMigration(); + + expect(warnOutput.length).toBe(0); + expect(tree.readContent('/index.ts')).not.toContain('@Injectable'); + }); + + it('should not migrate provider which is already decorated with @Pipe', async() => { + writeFile('/index.ts', ` + import {Pipe, ${type}} from '@angular/core'; + + @Pipe() + export class MyService {} + + @${type}({${propName}: [MyService]}) + export class TestClass {} + `); + + await runMigration(); + + expect(warnOutput.length).toBe(0); + expect(tree.readContent('/index.ts')).not.toContain('@Injectable'); + }); + + it(`should migrate multiple providers in same ${type}`, async() => { + writeFile('/index.ts', ` + import {${type}} from '@angular/core'; - export class MyService {} + export class ServiceA {} + export class ServiceB {} + + @${type}({${propName}: [ServiceA, ServiceB]}) + export class TestClass {} `); - await runMigration(); + await runMigration(); - expect(warnOutput.length).toBe(0); - expect(tree.readContent('/service.ts')).toMatch(/@Injectable\(\)\s+export class MyService/); - expect(tree.readContent('/service.ts')) - .toMatch(/'b'; \/\/ some comment\s+import { Injectable } from "@angular\/core";/); - }); + expect(warnOutput.length).toBe(0); + expect(tree.readContent('/index.ts')).toMatch(/@Injectable\(\)\s+export class ServiceA/); + expect(tree.readContent('/index.ts')).toMatch(/@Injectable\(\)\s+export class ServiceB/); + expect(tree.readContent('/index.ts')) + .toContain(`{ ${type}, Injectable } from '@angular/core`); + }); - it('should create new import at source file start with trailing new-line', async() => { - writeFile('/index.ts', ` - import {NgModule} from '@angular/core'; - import {MyService} from './service'; - - @NgModule({providers: [MyService]}) - export class MyModule {} - `); + it(`should migrate multiple mixed providers in same ${type}`, async() => { + writeFile('/index.ts', ` + import {${type}} from '@angular/core'; + + export class ServiceA {} + export class ServiceB {} + export class ServiceC {} + + @${type}({ + ${propName}: [ + ServiceA, + {provide: ServiceB}, + {provide: SomeToken, useClass: ServiceC}, + ]}) + export class TestClass {} + `); - writeFile('/service.ts', `/* @license */ - export class MyService {} - `); + await runMigration(); - await runMigration(); + expect(warnOutput.length).toBe(0); + expect(tree.readContent('/index.ts')).toMatch(/@Injectable\(\)\s+export class ServiceA/); + expect(tree.readContent('/index.ts')).toMatch(/@Injectable\(\)\s+export class ServiceB/); + expect(tree.readContent('/index.ts')).toMatch(/@Injectable\(\)\s+export class ServiceC/); + expect(tree.readContent('/index.ts')) + .toContain(`{ ${type}, Injectable } from '@angular/core`); + }); - expect(warnOutput.length).toBe(0); - expect(tree.readContent('/service.ts')) - .toMatch( - /^import { Injectable } from "@angular\/core";\s+\/\* @license \*\/\s+@Injectable\(\)\s+export class MyService/); - }); - it('should remove @Inject decorator for providers which are migrated', async() => { - writeFile('/index.ts', ` - import {NgModule} from '@angular/core'; - import {MyService} from './service'; - - @NgModule({providers: [MyService]}) - export class MyModule {} - `); + it(`should migrate multiple nested providers in same ${type}`, async() => { + writeFile('/index.ts', ` + import {${type}} from '@angular/core'; + + export class ServiceA {} + export class ServiceB {} + export class ServiceC {} + + @${type}({ + ${propName}: [ + ServiceA, + [ + {provide: ServiceB}, + ServiceC, + ], + ]}) + export class TestClass {} + `); - writeFile('/service.ts', ` - import {Inject} from '@angular/core'; + await runMigration(); + + expect(warnOutput.length).toBe(0); + expect(tree.readContent('/index.ts')).toMatch(/@Injectable\(\)\s+export class ServiceA/); + expect(tree.readContent('/index.ts')).toMatch(/@Injectable\(\)\s+export class ServiceB/); + expect(tree.readContent('/index.ts')).toMatch(/@Injectable\(\)\s+export class ServiceC/); + expect(tree.readContent('/index.ts')) + .toContain(`{ ${type}, Injectable } from '@angular/core`); + }); + + it('should migrate providers referenced through identifier', async() => { + writeFile('/index.ts', ` + import {${type}} from '@angular/core'; + + export class ServiceA {} + export class ServiceB {} + + const PROVIDERS = [ServiceA, ServiceB]; + + @${type}({ + ${propName}: PROVIDERS, + }) + export class TestClass {} + `); + + await runMigration(); + + expect(warnOutput.length).toBe(0); + expect(tree.readContent('/index.ts')).toMatch(/@Injectable\(\)\s+export class ServiceA/); + expect(tree.readContent('/index.ts')).toMatch(/@Injectable\(\)\s+export class ServiceB/); + expect(tree.readContent('/index.ts')) + .toContain(`{ ${type}, Injectable } from '@angular/core`); + }); + + it('should migrate providers created through static analyzable function call', async() => { + writeFile('/index.ts', ` + import {${type}} from '@angular/core'; + + export class ServiceA {} + export class ServiceB {} + + export function createProviders(x: any) { + return [ServiceA, x] + } + + @${type}({ + ${propName}: createProviders(ServiceB), + }) + export class TestClass {} + `); + + await runMigration(); + + expect(warnOutput.length).toBe(0); + expect(tree.readContent('/index.ts')).toMatch(/@Injectable\(\)\s+export class ServiceA/); + expect(tree.readContent('/index.ts')).toMatch(/@Injectable\(\)\s+export class ServiceB/); + expect(tree.readContent('/index.ts')) + .toContain(`{ ${type}, Injectable } from '@angular/core`); + }); + + it('should migrate providers which are computed through spread operator', async() => { + writeFile('/index.ts', ` + import {${type}} from '@angular/core'; + + export class ServiceA {} + export class ServiceB {} + + const otherServices = [ServiceB]; + + @${type}({ + ${propName}: [ServiceA, ...otherServices], + }) + export class TestClass {} + `); + + await runMigration(); + + expect(warnOutput.length).toBe(0); + expect(tree.readContent('/index.ts')).toMatch(/@Injectable\(\)\s+export class ServiceA/); + expect(tree.readContent('/index.ts')).toMatch(/@Injectable\(\)\s+export class ServiceB/); + expect(tree.readContent('/index.ts')) + .toContain(`{ ${type}, Injectable } from '@angular/core`); + }); + + it(`should migrate provider once if referenced in multiple ${type} definitions`, async() => { + writeFile('/index.ts', ` + import {${type}} from '@angular/core'; + + export class ServiceA {} + + @${type}({${propName}: [ServiceA]}) + export class TestClassA {} + `); + + writeFile('/second.ts', ` + import {${type}} from '@angular/core'; + import {ServiceA} from './index'; + + export class ServiceB {} + + @${type}({${propName}: [ServiceA, ServiceB]}) + export class TestClassB {} + `); + + await runMigration(); + + expect(warnOutput.length).toBe(0); + expect(tree.readContent('/index.ts')) + .toMatch(/@angular\/core';\s+@Injectable\(\)\s+export class ServiceA/); + expect(tree.readContent('/index.ts')) + .toContain(`{ ${type}, Injectable } from '@angular/core`); + expect(tree.readContent('/second.ts')).toMatch(/@Injectable\(\)\s+export class ServiceB/); + expect(tree.readContent('/second.ts')) + .toContain(`{ ${type}, Injectable } from '@angular/core`); + }); + + it('should create new import for @Injectable when migrating provider', async() => { + writeFile('/index.ts', ` + import {${type}} from '@angular/core'; + import {MyService, MySecondService} from './service'; + + @${type}({${propName}: [MyService, MySecondService]}) + export class TestClass {} + `); + + writeFile('/service.ts', `export class MyService {} - @Inject() - export class MyService {} - `); + export class MySecondService {} + `); - await runMigration(); + await runMigration(); - expect(warnOutput.length).toBe(0); - expect(tree.readContent('/service.ts')) - .toMatch(/core';\s+@Injectable\(\)\s+export class MyService/); - expect(tree.readContent('/service.ts')) - .toMatch(/import { Inject, Injectable } from '@angular\/core';/); - }); + expect(warnOutput.length).toBe(0); + expect(tree.readContent('/service.ts')).toMatch(/@Injectable\(\)\s+export class MyService/); + expect(tree.readContent('/service.ts')) + .toMatch(/@Injectable\(\)\s+export class MySecondService/); + expect(tree.readContent('/service.ts')) + .toMatch(/import { Injectable } from "@angular\/core";/); + }); + it('should re-use existing namespace import for importing @Injectable when migrating provider', + async() => { + writeFile('/index.ts', ` + import * as core from '@angular/core'; + + export class MyService { + constructor() { + console.log(core.isDevMode()); + } + } + `); + + writeFile('/app.module.ts', ` + import {${type}} from '@angular/core'; + import {MyService} from './index'; + + @${type}({${propName}: [MyService]}) + export class TestClass {} + `); + + await runMigration(); + + expect(warnOutput.length).toBe(0); + expect(tree.readContent('/index.ts')) + .toMatch(/@core.Injectable\(\)\s+export class MyService/); + }); + + it('should warn if a referenced individual provider could not be resolved', async() => { + writeFile('/index.ts', ` + import {${type}} from '@angular/core'; + + @${type}({${propName}: [NotPresent]}) + export class TestClass {} + `); + + await runMigration(); + + const providerSourceTextColumn = 15 + type.length + propName.length; + expect(warnOutput.length).toBe(1); + expect(warnOutput[0]).toMatch(/\s+index\.ts@4:.+Provider is not statically analyzable./); + expect(warnOutput[0]).toContain(`4:${providerSourceTextColumn}:`); + }); + + it(`should warn if ${propName} value could not be resolved`, async() => { + writeFile('/index.ts', ` + import {${type}} from '@angular/core'; + + @${type}({${propName}: NOT_ANALYZABLE) + export class TestClass {} + `); + + await runMigration(); + + const propValueSourceTextColumn = 14 + type.length + propName.length; + expect(warnOutput.length).toBe(1); + expect(warnOutput[0]).toMatch(/\s+index\.ts@4:.+Providers.*not statically analyzable./); + expect(warnOutput[0]).toContain(`4:${propValueSourceTextColumn}:`); + }); + + it(`should not throw if an empty @${type} is analyzed`, async() => { + writeFile('/index.ts', ` + import {${type}} from '@angular/core'; + + @${type}() + export class MyModule {} + `); + + try { + await runMigration(); + } catch (e) { + fail(e); + } + + expect(warnOutput.length).toBe(0); + }); + + it('should create new import for injectable after full end of last import statement', + async() => { + writeFile('/index.ts', ` + import {${type}} from '@angular/core'; + import {MyService} from './service'; + + @${type}({${propName}: [MyService]}) + export class TestClass {} + `); + + writeFile('/service.ts', ` + import * as a from 'a'; + import * as a from 'b'; // some comment + + export class MyService {} + `); + + await runMigration(); + + expect(warnOutput.length).toBe(0); + expect(tree.readContent('/service.ts')) + .toMatch(/@Injectable\(\)\s+export class MyService/); + expect(tree.readContent('/service.ts')) + .toMatch(/'b'; \/\/ some comment\s+import { Injectable } from "@angular\/core";/); + }); + + it('should create new import at source file start with trailing new-line', async() => { + writeFile('/index.ts', ` + import {${type}} from '@angular/core'; + import {MyService} from './service'; + + @${type}({${propName}: [MyService]}) + export class TestClass {} + `); + + writeFile('/service.ts', `/* @license */ + export class MyService {} + `); + + await runMigration(); + + expect(warnOutput.length).toBe(0); + expect(tree.readContent('/service.ts')) + .toMatch( + /^import { Injectable } from "@angular\/core";\s+\/\* @license \*\/\s+@Injectable\(\)\s+export class MyService/); + }); + + it('should remove @Inject decorator for providers which are migrated', async() => { + writeFile('/index.ts', ` + import {${type}} from '@angular/core'; + import {MyService} from './service'; + + @${type}({${propName}: [MyService]}) + export class TestClass {} + `); + + writeFile('/service.ts', ` + import {Inject} from '@angular/core'; + + @Inject() + export class MyService {} + `); + + await runMigration(); + + expect(warnOutput.length).toBe(0); + expect(tree.readContent('/service.ts')) + .toMatch(/core';\s+@Injectable\(\)\s+export class MyService/); + // "inject" import will be preserved. We don't want to bother with detecting + // if the import is unused or not. We leave this up to the developers. + expect(tree.readContent('/service.ts')) + .toMatch(/import { Inject, Injectable } from '@angular\/core';/); + }); + } });