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
This commit is contained in:
Paul Gschwendtner 2019-10-05 23:41:37 +02:00 committed by Miško Hevery
parent 52483bf680
commit 5557dec120
8 changed files with 881 additions and 695 deletions

View File

@ -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,25 +26,25 @@ 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<ts.SourceFile, TslintUpdateRecorder>();
resolvedModules.forEach(module => {
transformer.migrateModule(module).forEach(({message, node}) => {
[...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));
failures.push(new RuleFailure(node.getSourceFile(), node.getStart(), 0, message, ruleName));
}
});
});
// Record the changes collected in the import manager and NgModule manager.
transformer.recordChanges();

View File

@ -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];

View File

@ -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));
}

View File

@ -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,26 +53,26 @@ 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<ts.SourceFile, UpdateRecorder>();
resolvedModules.forEach(module => {
transformer.migrateModule(module).forEach(({message, node}) => {
[...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.
transformer.recordChanges();

View File

@ -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);
}
}

View File

@ -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;
}

View File

@ -45,14 +45,48 @@ describe('Google3 missing injectable tslint rule', () => {
function getFile(fileName: string) { return readFileSync(join(tmpDir, fileName), 'utf8'); }
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',
() => {
writeFile('/index.ts', `
import {Component} from '@angular/core';
export class MyService {}
export class MySecondService {}
@Component({
providers: [MyService],
viewProviders: [MySecondService],
})
export class TestClass {}
`);
const result = runTSLint().getResult();
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`);
});
});
function createTests(
type: 'NgModule' | 'Directive' | 'Component', propName: 'providers' | 'viewProviders') {
it('should create proper failures for missing injectable providers', () => {
writeFile('index.ts', `
import { NgModule } from '@angular/core';
import { ${type} } from '@angular/core';
export class A {}
@NgModule({providers: [A]})
export class AppModule {}
@${type}({${propName}: [A]})
export class TestClass {}
`);
const linter = runTSLint(false);
@ -60,10 +94,11 @@ describe('Google3 missing injectable tslint rule', () => {
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});
.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: 13});
expect(failures[1].getStartPosition().getLineAndCharacter())
.toEqual({line: 1, character: 15});
});
it('should update provider classes which need to be migrated in Ivy', () => {
@ -87,7 +122,7 @@ describe('Google3 missing injectable tslint rule', () => {
export class MyServiceF {}
export class MyServiceG {}
@NgModule({providers: [
@${type}({${propName}: [
WithPipe,
[
WithDirective,
@ -101,7 +136,7 @@ describe('Google3 missing injectable tslint rule', () => {
{provide: MyServiceF, useFactory: () => null},
{provide: MyServiceG, useValue: null},
]})
export class MyModule {}
export class TestClass {}
`);
@ -121,41 +156,41 @@ describe('Google3 missing injectable tslint rule', () => {
expect(getFile('/index.ts')).toMatch(/MyServiceF {}\s+export class MyServiceG/);
});
it('should migrate provider once if referenced in multiple NgModule definitions', () => {
it(`should migrate provider once if referenced in multiple ${type} definitions`, () => {
writeFile('/index.ts', `
import {NgModule} from '@angular/core';
import {${type}} from '@angular/core';
export class ServiceA {}
@NgModule({providers: [ServiceA]})
export class MyModule {}
@${type}({${propName}: [ServiceA]})
export class TestClass {}
`);
writeFile('/second.ts', `
import {NgModule} from '@angular/core';
import {${type}} from '@angular/core';
import {ServiceA} from './index';
export class ServiceB {}
@NgModule({providers: [ServiceA, ServiceB]})
export class SecondModule {}
@${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')).toMatch(/{ NgModule, Injectable } from '@angular\/core/);
expect(getFile('/index.ts')).toContain(`{ ${type}, 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('/second.ts')).toContain(`{ ${type}, Injectable } from '@angular/core`);
});
it('should warn if a referenced provider could not be resolved', () => {
writeFile('/index.ts', `
import {NgModule} from '@angular/core';
import {${type}} from '@angular/core';
@NgModule({providers: [NotPresent]})
export class MyModule {}
@${type}({${propName}: [NotPresent]})
export class TestClass {}
`);
const linter = runTSLint();
@ -163,32 +198,34 @@ describe('Google3 missing injectable tslint rule', () => {
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});
expect(failures[0].getStartPosition().getLineAndCharacter())
.toEqual({line: 3, character: 14 + type.length + propName.length});
});
it('should warn if the module providers could not be resolved', () => {
it(`should warn if the "${propName}" value could not be resolved`, () => {
writeFile('/index.ts', `
import {NgModule} from '@angular/core';
import {${type}} from '@angular/core';
@NgModule({providers: NOT_ANALYZABLE)
export class MyModule {}
@${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 of module.*not statically analyzable./);
expect(failures[0].getStartPosition().getLineAndCharacter()).toEqual({line: 3, character: 28});
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 {NgModule} from '@angular/core';
import {${type}} from '@angular/core';
import {MyService, MySecondService} from './service';
@NgModule({providers: [MyService, MySecondService]})
export class MyModule {}
@${type}({${propName}: [MyService, MySecondService]})
export class TestClass {}
`);
writeFile('/service.ts', `export class MyService {}
@ -205,11 +242,11 @@ describe('Google3 missing injectable tslint rule', () => {
it('should remove @Inject decorator for providers which are migrated', () => {
writeFile('/index.ts', `
import {NgModule} from '@angular/core';
import {${type}} from '@angular/core';
import {MyService} from './service';
@NgModule({providers: [MyService]})
export class MyModule {}
@${type}({${propName}: [MyService]})
export class TestClass {}
`);
writeFile('/service.ts', `
@ -222,6 +259,10 @@ describe('Google3 missing injectable tslint rule', () => {
runTSLint();
expect(getFile('/service.ts')).toMatch(/core';\s+@Injectable\(\)\s+export class MyService/);
expect(getFile('/service.ts')).toMatch(/import { Inject, Injectable } from '@angular\/core';/);
expect(getFile('/service.ts'))
.toMatch(/import { Inject, Injectable } from '@angular\/core';/);
});
}
});

View File

@ -63,48 +63,86 @@ describe('Missing injectable migration', () => {
await runner.runSchematicAsync('migration-v9-missing-injectable', {}, tree).toPromise();
}
it('should migrate type provider in NgModule', async() => {
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 {NgModule} from '@angular/core';
import {Component} from '@angular/core';
export class MyService {}
export class MySecondService {}
@NgModule({providers: [MyService]})
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(/{ NgModule, Injectable } from '@angular\/core/);
expect(tree.readContent('/index.ts'))
.toMatch(/@Injectable\(\)\s+export class MySecondService/);
expect(tree.readContent('/index.ts'))
.toContain(`{ Component, Injectable } from '@angular/core`);
});
});
it('should migrate object literal provider in NgModule', async() => {
function createTests(
type: 'NgModule' | 'Directive' | 'Component', propName: 'providers' | 'viewProviders') {
it(`should migrate type provider in ${type}`, async() => {
writeFile('/index.ts', `
import {NgModule} from '@angular/core';
import {${type}} from '@angular/core';
export class MyService {}
@NgModule({providers: [{provide: MyService}]})
export class MyModule {}
@${type}({${propName}: [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(/{ NgModule, Injectable } from '@angular\/core/);
expect(tree.readContent('/index.ts'))
.toContain(`{ ${type}, Injectable } from '@angular/core`);
});
it('should not migrate object literal provider with "useValue" in NgModule', async() => {
it(`should migrate object literal provider in ${type}`, async() => {
writeFile('/index.ts', `
import {NgModule} from '@angular/core';
import {${type}} from '@angular/core';
export class MyService {}
@NgModule({providers: [{provide: MyService, useValue: null }]})
export class MyModule {}
@${type}({${propName}: [{provide: 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'))
.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();
@ -113,14 +151,14 @@ describe('Missing injectable migration', () => {
expect(tree.readContent('/index.ts')).not.toContain('@Injectable');
});
it('should not migrate object literal provider with "useFactory" in NgModule', async() => {
it(`should not migrate object literal provider with "useFactory" in ${type}`, async() => {
writeFile('/index.ts', `
import {NgModule} from '@angular/core';
import {${type}} from '@angular/core';
export class MyService {}
@NgModule({providers: [{provide: MyService, useFactory: () => null }]})
export class MyModule {}
@${type}({${propName}: [{provide: MyService, useFactory: () => null }]})
export class TestClass {}
`);
await runMigration();
@ -129,15 +167,15 @@ describe('Missing injectable migration', () => {
expect(tree.readContent('/index.ts')).not.toContain('@Injectable');
});
it('should migrate object literal provider with "useExisting" in NgModule', async() => {
it(`should migrate object literal provider with "useExisting" in ${type}`, async() => {
writeFile('/index.ts', `
import {NgModule} from '@angular/core';
import {${type}} from '@angular/core';
export class MyService {}
export class MyToken {}
@NgModule({providers: [{provide: MyToken, useExisting: MyService}]})
export class MyModule {}
@${type}({${propName}: [{provide: MyToken, useExisting: MyService}]})
export class TestClass {}
`);
await runMigration();
@ -145,18 +183,19 @@ describe('Missing injectable migration', () => {
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/);
expect(tree.readContent('/index.ts'))
.toContain(`{ ${type}, Injectable } from '@angular/core`);
});
it('should migrate object literal provider with "useClass" in NgModule', async() => {
it(`should migrate object literal provider with "useClass" in ${type}`, async() => {
writeFile('/index.ts', `
import {NgModule} from '@angular/core';
import {${type}} from '@angular/core';
export class MyService {}
export class MyToken {}
@NgModule({providers: [{provide: MyToken, useClass: MyService}]})
export class MyModule {}
@${type}({${propName}: [{provide: MyToken, useClass: MyService}]})
export class TestClass {}
`);
await runMigration();
@ -164,18 +203,19 @@ describe('Missing injectable migration', () => {
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/);
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, NgModule} from '@angular/core';
import {Injectable, ${type}} from '@angular/core';
@Injectable()
export class MyService {}
@NgModule({providers: [MyService]})
export class MyModule {}
@${type}({${propName}: [MyService]})
export class TestClass {}
`);
await runMigration();
@ -187,13 +227,13 @@ describe('Missing injectable migration', () => {
it('should not migrate provider which is already decorated with @Directive', async() => {
writeFile('/index.ts', `
import {Directive, NgModule} from '@angular/core';
import {Directive, ${type}} from '@angular/core';
@Directive()
export class MyService {}
@NgModule({providers: [MyService]})
export class MyModule {}
@${type}({${propName}: [MyService]})
export class TestClass {}
`);
await runMigration();
@ -204,13 +244,13 @@ describe('Missing injectable migration', () => {
it('should not migrate provider which is already decorated with @Component', async() => {
writeFile('/index.ts', `
import {Component, NgModule} from '@angular/core';
import {Component, ${type}} from '@angular/core';
@Component()
export class MyService {}
@NgModule({providers: [MyService]})
export class MyModule {}
@${type}({${propName}: [MyService]})
export class TestClass {}
`);
await runMigration();
@ -221,13 +261,13 @@ describe('Missing injectable migration', () => {
it('should not migrate provider which is already decorated with @Pipe', async() => {
writeFile('/index.ts', `
import {Pipe, NgModule} from '@angular/core';
import {Pipe, ${type}} from '@angular/core';
@Pipe()
export class MyService {}
@NgModule({providers: [MyService]})
export class MyModule {}
@${type}({${propName}: [MyService]})
export class TestClass {}
`);
await runMigration();
@ -236,15 +276,15 @@ describe('Missing injectable migration', () => {
expect(tree.readContent('/index.ts')).not.toContain('@Injectable');
});
it('should migrate multiple providers in same NgModule', async() => {
it(`should migrate multiple providers in same ${type}`, async() => {
writeFile('/index.ts', `
import {NgModule} from '@angular/core';
import {${type}} from '@angular/core';
export class ServiceA {}
export class ServiceB {}
@NgModule({providers: [ServiceA, ServiceB]})
export class MyModule {}
@${type}({${propName}: [ServiceA, ServiceB]})
export class TestClass {}
`);
await runMigration();
@ -252,24 +292,25 @@ describe('Missing injectable migration', () => {
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/);
expect(tree.readContent('/index.ts'))
.toContain(`{ ${type}, Injectable } from '@angular/core`);
});
it('should migrate multiple mixed providers in same NgModule', async() => {
it(`should migrate multiple mixed providers in same ${type}`, async() => {
writeFile('/index.ts', `
import {NgModule} from '@angular/core';
import {${type}} from '@angular/core';
export class ServiceA {}
export class ServiceB {}
export class ServiceC {}
@NgModule({
providers: [
@${type}({
${propName}: [
ServiceA,
{provide: ServiceB},
{provide: SomeToken, useClass: ServiceC},
]})
export class MyModule {}
export class TestClass {}
`);
await runMigration();
@ -278,27 +319,28 @@ describe('Missing injectable migration', () => {
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/);
expect(tree.readContent('/index.ts'))
.toContain(`{ ${type}, Injectable } from '@angular/core`);
});
it('should migrate multiple nested providers in same NgModule', async() => {
it(`should migrate multiple nested providers in same ${type}`, async() => {
writeFile('/index.ts', `
import {NgModule} from '@angular/core';
import {${type}} from '@angular/core';
export class ServiceA {}
export class ServiceB {}
export class ServiceC {}
@NgModule({
providers: [
@${type}({
${propName}: [
ServiceA,
[
{provide: ServiceB},
ServiceC,
],
]})
export class MyModule {}
export class TestClass {}
`);
await runMigration();
@ -307,22 +349,23 @@ describe('Missing injectable migration', () => {
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/);
expect(tree.readContent('/index.ts'))
.toContain(`{ ${type}, Injectable } from '@angular/core`);
});
it('should migrate providers referenced through identifier', async() => {
writeFile('/index.ts', `
import {NgModule} from '@angular/core';
import {${type}} from '@angular/core';
export class ServiceA {}
export class ServiceB {}
const PROVIDERS = [ServiceA, ServiceB];
@NgModule({
providers: PROVIDERS,
@${type}({
${propName}: PROVIDERS,
})
export class MyModule {}
export class TestClass {}
`);
await runMigration();
@ -330,12 +373,13 @@ describe('Missing injectable migration', () => {
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/);
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 {NgModule} from '@angular/core';
import {${type}} from '@angular/core';
export class ServiceA {}
export class ServiceB {}
@ -344,10 +388,10 @@ describe('Missing injectable migration', () => {
return [ServiceA, x]
}
@NgModule({
providers: createProviders(ServiceB),
@${type}({
${propName}: createProviders(ServiceB),
})
export class MyModule {}
export class TestClass {}
`);
await runMigration();
@ -355,22 +399,23 @@ describe('Missing injectable migration', () => {
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/);
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 {NgModule} from '@angular/core';
import {${type}} from '@angular/core';
export class ServiceA {}
export class ServiceB {}
const otherServices = [ServiceB];
@NgModule({
providers: [ServiceA, ...otherServices],
@${type}({
${propName}: [ServiceA, ...otherServices],
})
export class MyModule {}
export class TestClass {}
`);
await runMigration();
@ -378,27 +423,28 @@ describe('Missing injectable migration', () => {
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/);
expect(tree.readContent('/index.ts'))
.toContain(`{ ${type}, Injectable } from '@angular/core`);
});
it('should migrate provider once if referenced in multiple NgModule definitions', async() => {
it(`should migrate provider once if referenced in multiple ${type} definitions`, async() => {
writeFile('/index.ts', `
import {NgModule} from '@angular/core';
import {${type}} from '@angular/core';
export class ServiceA {}
@NgModule({providers: [ServiceA]})
export class MyModule {}
@${type}({${propName}: [ServiceA]})
export class TestClassA {}
`);
writeFile('/second.ts', `
import {NgModule} from '@angular/core';
import {${type}} from '@angular/core';
import {ServiceA} from './index';
export class ServiceB {}
@NgModule({providers: [ServiceA, ServiceB]})
export class SecondModule {}
@${type}({${propName}: [ServiceA, ServiceB]})
export class TestClassB {}
`);
await runMigration();
@ -406,18 +452,20 @@ describe('Missing injectable migration', () => {
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('/index.ts'))
.toContain(`{ ${type}, 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/);
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 {NgModule} from '@angular/core';
import {${type}} from '@angular/core';
import {MyService, MySecondService} from './service';
@NgModule({providers: [MyService, MySecondService]})
export class MyModule {}
@${type}({${propName}: [MyService, MySecondService]})
export class TestClass {}
`);
writeFile('/service.ts', `export class MyService {}
@ -431,7 +479,8 @@ describe('Missing injectable migration', () => {
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";/);
expect(tree.readContent('/service.ts'))
.toMatch(/import { Injectable } from "@angular\/core";/);
});
it('should re-use existing namespace import for importing @Injectable when migrating provider',
@ -447,11 +496,11 @@ describe('Missing injectable migration', () => {
`);
writeFile('/app.module.ts', `
import {NgModule} from '@angular/core';
import {${type}} from '@angular/core';
import {MyService} from './index';
@NgModule({providers: [MyService]})
export class MyModule {}
@${type}({${propName}: [MyService]})
export class TestClass {}
`);
await runMigration();
@ -461,40 +510,43 @@ describe('Missing injectable migration', () => {
.toMatch(/@core.Injectable\(\)\s+export class MyService/);
});
it('should warn if a referenced provider could not be resolved', async() => {
it('should warn if a referenced individual provider could not be resolved', async() => {
writeFile('/index.ts', `
import {NgModule} from '@angular/core';
import {${type}} from '@angular/core';
@NgModule({providers: [NotPresent]})
export class MyModule {}
@${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:30: Provider is not statically analyzable./);
expect(warnOutput[0]).toMatch(/\s+index\.ts@4:.+Provider is not statically analyzable./);
expect(warnOutput[0]).toContain(`4:${providerSourceTextColumn}:`);
});
it('should warn if the module providers could not be resolved', async() => {
it(`should warn if ${propName} value could not be resolved`, async() => {
writeFile('/index.ts', `
import {NgModule} from '@angular/core';
import {${type}} from '@angular/core';
@NgModule({providers: NOT_ANALYZABLE)
export class MyModule {}
@${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:29: Providers of module.*not statically analyzable./);
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 @NgModule is analyzed', async() => {
it(`should not throw if an empty @${type} is analyzed`, async() => {
writeFile('/index.ts', `
import {NgModule} from '@angular/core';
import {${type}} from '@angular/core';
@NgModule()
@${type}()
export class MyModule {}
`);
@ -507,13 +559,14 @@ describe('Missing injectable migration', () => {
expect(warnOutput.length).toBe(0);
});
it('should create new import for injectable after full end of last import statement', async() => {
it('should create new import for injectable after full end of last import statement',
async() => {
writeFile('/index.ts', `
import {NgModule} from '@angular/core';
import {${type}} from '@angular/core';
import {MyService} from './service';
@NgModule({providers: [MyService]})
export class MyModule {}
@${type}({${propName}: [MyService]})
export class TestClass {}
`);
writeFile('/service.ts', `
@ -526,18 +579,19 @@ describe('Missing injectable migration', () => {
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 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 {NgModule} from '@angular/core';
import {${type}} from '@angular/core';
import {MyService} from './service';
@NgModule({providers: [MyService]})
export class MyModule {}
@${type}({${propName}: [MyService]})
export class TestClass {}
`);
writeFile('/service.ts', `/* @license */
@ -554,11 +608,11 @@ describe('Missing injectable migration', () => {
it('should remove @Inject decorator for providers which are migrated', async() => {
writeFile('/index.ts', `
import {NgModule} from '@angular/core';
import {${type}} from '@angular/core';
import {MyService} from './service';
@NgModule({providers: [MyService]})
export class MyModule {}
@${type}({${propName}: [MyService]})
export class TestClass {}
`);
writeFile('/service.ts', `
@ -573,8 +627,10 @@ describe('Missing injectable migration', () => {
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';/);
});
}
});