From 15fefdbb8d750c59dbddb0f69f1cd1edee1742c3 Mon Sep 17 00:00:00 2001 From: Paul Gschwendtner Date: Thu, 14 Nov 2019 10:16:07 +0100 Subject: [PATCH] feat(core): missing-injectable migration should migrate empty object literal providers (#33709) In View Engine, providers which neither used `useValue`, `useClass`, `useFactory` or `useExisting`, were interpreted differently. e.g. ``` {provide: X} -> {provide: X, useValue: undefined}, // this is how it works in View Engine {provide: X} -> {provide: X, useClass: X}, // this is how it works in Ivy ``` The missing-injectable migration should migrate such providers to the explicit `useValue` provider. This ensures that there is no unexpected behavioral change when updating to v9. PR Close #33709 --- .../migration-tests/providers-test-module.ts | 7 +- .../providers-test-module_expected.ts | 8 +- .../partial_evaluator/src/interpreter.ts | 2 +- .../google3/tslint_update_recorder.ts | 8 ++ .../migrations/missing-injectable/index.ts | 4 + .../missing-injectable/providers_evaluator.ts | 58 ++++++++++ .../missing-injectable/transform.ts | 82 +++++++++----- .../missing-injectable/update_recorder.ts | 1 + .../google3/missing_injectable_rule_spec.ts | 6 +- .../test/missing_injectable_migration_spec.ts | 101 +++++++++++++++--- 10 files changed, 229 insertions(+), 48 deletions(-) create mode 100644 packages/core/schematics/migrations/missing-injectable/providers_evaluator.ts diff --git a/integration/ng_update_migrations/src/app/migration-tests/providers-test-module.ts b/integration/ng_update_migrations/src/app/migration-tests/providers-test-module.ts index 392072d6d7..c56cb801d9 100644 --- a/integration/ng_update_migrations/src/app/migration-tests/providers-test-module.ts +++ b/integration/ng_update_migrations/src/app/migration-tests/providers-test-module.ts @@ -34,9 +34,9 @@ class BaseProviderCase { constructor(zone: NgZone) {} } -export class ProvideCase extends BaseProviderCase {} +export class EmptyProviderLiteralCase {} -export class ProviderClass {} +export class ProviderClass extends BaseProviderCase {} export class DontNeedCase {} @@ -46,7 +46,7 @@ export class DirectiveCase {} @NgModule({ providers: [ TypeCase, - {provide: ProvideCase}, + {provide: EmptyProviderLiteralCase}, {provide: DontNeedCase, useValue: 0}, {provide: DontNeedCase, useFactory: () => null}, {provide: DontNeedCase, useExisting: TypeCase}, @@ -56,4 +56,3 @@ export class DirectiveCase {} declarations: [ProvidersTestDirective, ProvidersTestComponent], }) export class ProvidersTestModule {} - diff --git a/integration/ng_update_migrations/src/app/migration-tests/providers-test-module_expected.ts b/integration/ng_update_migrations/src/app/migration-tests/providers-test-module_expected.ts index 6daf48c87f..0d6d4ac2c5 100644 --- a/integration/ng_update_migrations/src/app/migration-tests/providers-test-module_expected.ts +++ b/integration/ng_update_migrations/src/app/migration-tests/providers-test-module_expected.ts @@ -41,11 +41,10 @@ class BaseProviderCase { constructor(zone: NgZone) {} } -@Injectable() -export class ProvideCase extends BaseProviderCase {} +export class EmptyProviderLiteralCase {} @Injectable() -export class ProviderClass {} +export class ProviderClass extends BaseProviderCase {} export class DontNeedCase {} @@ -55,7 +54,7 @@ export class DirectiveCase {} @NgModule({ providers: [ TypeCase, - {provide: ProvideCase}, + { provide: EmptyProviderLiteralCase, useValue: undefined }, {provide: DontNeedCase, useValue: 0}, {provide: DontNeedCase, useFactory: () => null}, {provide: DontNeedCase, useExisting: TypeCase}, @@ -65,4 +64,3 @@ export class DirectiveCase {} declarations: [ProvidersTestDirective, ProvidersTestComponent], }) export class ProvidersTestModule {} - diff --git a/packages/compiler-cli/src/ngtsc/partial_evaluator/src/interpreter.ts b/packages/compiler-cli/src/ngtsc/partial_evaluator/src/interpreter.ts index 61c4653984..73e960df56 100644 --- a/packages/compiler-cli/src/ngtsc/partial_evaluator/src/interpreter.ts +++ b/packages/compiler-cli/src/ngtsc/partial_evaluator/src/interpreter.ts @@ -160,7 +160,7 @@ export class StaticInterpreter { return array; } - private visitObjectLiteralExpression(node: ts.ObjectLiteralExpression, context: Context): + protected visitObjectLiteralExpression(node: ts.ObjectLiteralExpression, context: Context): ResolvedValue { const map: ResolvedValueMap = new Map(); for (let i = 0; i < node.properties.length; i++) { 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 13b7e80961..0f3da184da 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 @@ -54,5 +54,13 @@ export class TslintUpdateRecorder implements UpdateRecorder { this.ruleName, fix)); } + + updateObjectLiteral(node: ts.ObjectLiteralExpression, newText: string): void { + this.failures.push(new RuleFailure( + this.sourceFile, node.getStart(), node.getEnd(), + `Object literal needs to be updated to: ${newText}`, this.ruleName, + Replacement.replaceFromTo(node.getStart(), node.getEnd(), newText))); + } + commitUpdate() {} } diff --git a/packages/core/schematics/migrations/missing-injectable/index.ts b/packages/core/schematics/migrations/missing-injectable/index.ts index 6e2dbfb33a..dcdad8eaea 100644 --- a/packages/core/schematics/migrations/missing-injectable/index.ts +++ b/packages/core/schematics/migrations/missing-injectable/index.ts @@ -107,6 +107,10 @@ function runMissingInjectableMigration( treeRecorder.remove(namedBindings.getStart(), namedBindings.getWidth()); treeRecorder.insertRight(namedBindings.getStart(), newNamedBindings); }, + updateObjectLiteral(node: ts.ObjectLiteralExpression, newText: string) { + treeRecorder.remove(node.getStart(), node.getWidth()); + treeRecorder.insertRight(node.getStart(), newText); + }, commitUpdate() { tree.commitUpdate(treeRecorder); } }; updateRecorders.set(sourceFile, recorder); diff --git a/packages/core/schematics/migrations/missing-injectable/providers_evaluator.ts b/packages/core/schematics/migrations/missing-injectable/providers_evaluator.ts new file mode 100644 index 0000000000..5558f41616 --- /dev/null +++ b/packages/core/schematics/migrations/missing-injectable/providers_evaluator.ts @@ -0,0 +1,58 @@ +/** + * @license + * Copyright Google Inc. All Rights Reserved. + * + * Use of this source code is governed by an MIT-style license that can be + * found in the LICENSE file at https://angular.io/license + */ + +import {forwardRefResolver} from '@angular/compiler-cli/src/ngtsc/annotations'; +import {ResolvedValue} from '@angular/compiler-cli/src/ngtsc/partial_evaluator'; +import {StaticInterpreter} from '@angular/compiler-cli/src/ngtsc/partial_evaluator/src/interpreter'; +import * as ts from 'typescript'; + +export interface ProviderLiteral { + node: ts.ObjectLiteralExpression; + resolvedValue: ResolvedValue; +} + +/** + * Providers evaluator that extends the ngtsc static interpreter. This is necessary because + * the static interpreter by default only exposes the resolved value, but we are also interested + * in the TypeScript nodes that declare providers. It would be possible to manually traverse the + * AST to collect these nodes, but that would mean that we need to re-implement the static + * interpreter in order to handle all possible scenarios. (e.g. spread operator, function calls, + * callee scope). This can be avoided by simply extending the static interpreter and intercepting + * the "visitObjectLiteralExpression" method. + */ +export class ProvidersEvaluator extends StaticInterpreter { + private _providerLiterals: ProviderLiteral[] = []; + + visitObjectLiteralExpression(node: ts.ObjectLiteralExpression, context: any) { + const resolvedValue = + super.visitObjectLiteralExpression(node, {...context, insideProviderDef: true}); + // do not collect nested object literals. e.g. a provider could use a + // spread assignment (which resolves to another object literal). In that + // case the referenced object literal is not a provider object literal. + if (!context.insideProviderDef) { + this._providerLiterals.push({node, resolvedValue}); + } + return resolvedValue; + } + + /** + * Evaluates the given expression and returns its statically resolved value + * and a list of object literals which define Angular providers. + */ + evaluate(expr: ts.Expression) { + this._providerLiterals = []; + const resolvedValue = this.visit(expr, { + originatingFile: expr.getSourceFile(), + absoluteModuleName: null, + resolutionContext: expr.getSourceFile().fileName, + scope: new Map(), + foreignFunctionResolver: forwardRefResolver + }); + return {resolvedValue, literals: this._providerLiterals}; + } +} diff --git a/packages/core/schematics/migrations/missing-injectable/transform.ts b/packages/core/schematics/migrations/missing-injectable/transform.ts index e124184906..87d3afa797 100644 --- a/packages/core/schematics/migrations/missing-injectable/transform.ts +++ b/packages/core/schematics/migrations/missing-injectable/transform.ts @@ -6,9 +6,8 @@ * found in the LICENSE file at https://angular.io/license */ -import {forwardRefResolver} from '@angular/compiler-cli/src/ngtsc/annotations/src/util'; import {Reference} from '@angular/compiler-cli/src/ngtsc/imports'; -import {DynamicValue, PartialEvaluator, ResolvedValue} from '@angular/compiler-cli/src/ngtsc/partial_evaluator'; +import {DynamicValue, ResolvedValue} from '@angular/compiler-cli/src/ngtsc/partial_evaluator'; import {TypeScriptReflectionHost} from '@angular/compiler-cli/src/ngtsc/reflection'; import * as ts from 'typescript'; @@ -16,6 +15,7 @@ import {getAngularDecorators} from '../../utils/ng_decorators'; import {ResolvedDirective, ResolvedNgModule} from './definition_collector'; import {ImportManager} from './import_manager'; +import {ProviderLiteral, ProvidersEvaluator} from './providers_evaluator'; import {UpdateRecorder} from './update_recorder'; @@ -30,16 +30,19 @@ export interface AnalysisFailure { export class MissingInjectableTransform { private printer = ts.createPrinter(); private importManager = new ImportManager(this.getUpdateRecorder, this.printer); - private partialEvaluator: PartialEvaluator; + private providersEvaluator: ProvidersEvaluator; /** Set of provider class declarations which were already checked or migrated. */ private visitedProviderClasses = new Set(); + /** Set of provider object literals which were already checked or migrated. */ + private visitedProviderLiterals = new Set(); + constructor( private typeChecker: ts.TypeChecker, private getUpdateRecorder: (sf: ts.SourceFile) => UpdateRecorder) { - this.partialEvaluator = - new PartialEvaluator(new TypeScriptReflectionHost(typeChecker), typeChecker); + this.providersEvaluator = + new ProvidersEvaluator(new TypeScriptReflectionHost(typeChecker), typeChecker); } recordChanges() { this.importManager.recordChanges(); } @@ -68,16 +71,17 @@ export class MissingInjectableTransform { return []; } - const evaluatedExpr = this._evaluateExpression(module.providersExpr); + const {resolvedValue, literals} = this.providersEvaluator.evaluate(module.providersExpr); + this._migrateLiteralProviders(literals); - if (!Array.isArray(evaluatedExpr)) { + if (!Array.isArray(resolvedValue)) { return [{ node: module.providersExpr, message: 'Providers of module are not statically analyzable.' }]; } - return this._visitProviderResolvedValue(evaluatedExpr, module); + return this._visitProviderResolvedValue(resolvedValue, module); } @@ -90,24 +94,27 @@ export class MissingInjectableTransform { // Migrate "providers" on directives and components if defined. if (directive.providersExpr) { - const evaluatedExpr = this._evaluateExpression(directive.providersExpr); - if (!Array.isArray(evaluatedExpr)) { + const {resolvedValue, literals} = this.providersEvaluator.evaluate(directive.providersExpr); + this._migrateLiteralProviders(literals); + if (!Array.isArray(resolvedValue)) { return [ {node: directive.providersExpr, message: `Providers are not statically analyzable.`} ]; } - failures.push(...this._visitProviderResolvedValue(evaluatedExpr, directive)); + failures.push(...this._visitProviderResolvedValue(resolvedValue, directive)); } // Migrate "viewProviders" on components if defined. if (directive.viewProvidersExpr) { - const evaluatedExpr = this._evaluateExpression(directive.viewProvidersExpr); - if (!Array.isArray(evaluatedExpr)) { + const {resolvedValue, literals} = + this.providersEvaluator.evaluate(directive.viewProvidersExpr); + this._migrateLiteralProviders(literals); + if (!Array.isArray(resolvedValue)) { return [ {node: directive.viewProvidersExpr, message: `Providers are not statically analyzable.`} ]; } - failures.push(...this._visitProviderResolvedValue(evaluatedExpr, directive)); + failures.push(...this._visitProviderResolvedValue(resolvedValue, directive)); } return failures; } @@ -160,11 +167,39 @@ export class MissingInjectableTransform { } /** - * Evaluates the given TypeScript expression using the partial evaluator with - * the foreign function resolver for handling "forwardRef" calls. + * Migrates object literal providers which do not use "useValue", "useClass", + * "useExisting" or "useFactory". These providers behave differently in Ivy. e.g. + * + * ```ts + * {provide: X} -> {provide: X, useValue: undefined} // this is how it behaves in VE + * {provide: X} -> {provide: X, useClass: X} // this is how it behaves in Ivy + * ``` + * + * To ensure forward compatibility, we migrate these empty object literal providers + * to explicitly use `useValue: undefined`. */ - private _evaluateExpression(expr: ts.Expression): ResolvedValue { - return this.partialEvaluator.evaluate(expr, forwardRefResolver); + private _migrateLiteralProviders(literals: ProviderLiteral[]) { + for (let {node, resolvedValue} of literals) { + if (this.visitedProviderLiterals.has(node)) { + continue; + } + this.visitedProviderLiterals.add(node); + + if (!resolvedValue || !(resolvedValue instanceof Map) || !resolvedValue.has('provide') || + resolvedValue.has('useClass') || resolvedValue.has('useValue') || + resolvedValue.has('useExisting') || resolvedValue.has('useFactory')) { + continue; + } + + const sourceFile = node.getSourceFile(); + const newObjectLiteral = ts.updateObjectLiteral( + node, node.properties.concat( + ts.createPropertyAssignment('useValue', ts.createIdentifier('undefined')))); + + this.getUpdateRecorder(sourceFile) + .updateObjectLiteral( + node, this.printer.printNode(ts.EmitHint.Unspecified, newObjectLiteral, sourceFile)); + } } /** @@ -177,14 +212,11 @@ export class MissingInjectableTransform { if (value instanceof Reference && ts.isClassDeclaration(value.node)) { this.migrateProviderClass(value.node, module); } else if (value instanceof Map) { - if (!value.has('provide') || value.has('useValue') || value.has('useFactory') || - value.has('useExisting')) { - return []; - } - if (value.has('useClass')) { + // If a "ClassProvider" has the "deps" property set, then we do not need to + // decorate the class. This is because the class is instantiated through the + // specified "deps" and the class does not need a factory definition. + if (value.has('provide') && value.has('useClass') && value.get('deps') == null) { return this._visitProviderResolvedValue(value.get('useClass') !, module); - } else { - return this._visitProviderResolvedValue(value.get('provide') !, module); } } else if (Array.isArray(value)) { return value.reduce((res, v) => res.concat(this._visitProviderResolvedValue(v, module)), [ diff --git a/packages/core/schematics/migrations/missing-injectable/update_recorder.ts b/packages/core/schematics/migrations/missing-injectable/update_recorder.ts index ca9215d897..80fc670f15 100644 --- a/packages/core/schematics/migrations/missing-injectable/update_recorder.ts +++ b/packages/core/schematics/migrations/missing-injectable/update_recorder.ts @@ -18,5 +18,6 @@ export interface UpdateRecorder { updateExistingImport(namedBindings: ts.NamedImports, newNamedBindings: string): void; addClassDecorator(node: ts.ClassDeclaration, text: string, className: string): void; replaceDecorator(node: ts.Decorator, newText: string, className: string): void; + updateObjectLiteral(node: ts.ObjectLiteralExpression, newText: 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 c1be822baa..2bbe3536b4 100644 --- a/packages/core/schematics/test/google3/missing_injectable_rule_spec.ts +++ b/packages/core/schematics/test/google3/missing_injectable_rule_spec.ts @@ -121,6 +121,7 @@ describe('Google3 missing injectable tslint rule', () => { export class MyServiceE {} export class MyServiceF {} export class MyServiceG {} + export class MyServiceH {} @${type}({${propName}: [ WithPipe, @@ -135,6 +136,7 @@ describe('Google3 missing injectable tslint rule', () => { {provide: null, useExisting: MyServiceE}, {provide: MyServiceF, useFactory: () => null}, {provide: MyServiceG, useValue: null}, + {provide: MyServiceH, deps: []}, ]}) export class TestClass {} `); @@ -149,11 +151,13 @@ describe('Google3 missing injectable tslint rule', () => { .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(/MyServiceB {}\s+export class MyServiceC/); expect(getFile('/index.ts')).toMatch(/@Injectable\(\)\s+export class MyServiceD/); expect(getFile('/index.ts')).toMatch(/MyServiceD {}\s+export class MyServiceE/); expect(getFile('/index.ts')).toMatch(/MyServiceE {}\s+export class MyServiceF/); expect(getFile('/index.ts')).toMatch(/MyServiceF {}\s+export class MyServiceG/); + expect(getFile('/index.ts')).toMatch(/MyServiceG {}\s+export class MyServiceH/); + expect(getFile('/index.ts')).toContain(`{ provide: MyServiceC, useValue: undefined },`); }); it(`should migrate provider once if referenced in multiple ${type} definitions`, () => { diff --git a/packages/core/schematics/test/missing_injectable_migration_spec.ts b/packages/core/schematics/test/missing_injectable_migration_spec.ts index f9526300c2..3aab813149 100644 --- a/packages/core/schematics/test/missing_injectable_migration_spec.ts +++ b/packages/core/schematics/test/missing_injectable_migration_spec.ts @@ -121,7 +121,7 @@ describe('Missing injectable migration', () => { .toContain(`{ ${type}, Injectable } from '@angular/core`); }); - it(`should migrate object literal provider in ${type}`, async() => { + it(`should migrate object literal provider in ${type} to explicit value provider`, async() => { writeFile('/index.ts', ` import {${type}} from '@angular/core'; @@ -134,16 +134,17 @@ describe('Missing injectable migration', () => { await runMigration(); expect(warnOutput.length).toBe(0); - expect(tree.readContent('/index.ts')).toMatch(/@Injectable\(\)\s+export class MyService/); + expect(tree.readContent('/index.ts')).toMatch(/@angular\/core';\s+export class MyService/); expect(tree.readContent('/index.ts')) - .toContain(`{ ${type}, Injectable } from '@angular/core`); + .toContain(`${propName}: [{ provide: MyService, useValue: undefined }]`); + expect(tree.readContent('/index.ts')).toContain(`{${type}} from '@angular/core`); }); it(`should migrate object literal provider with forwardRef in ${type}`, async() => { writeFile('/index.ts', ` import {${type}, forwardRef} from '@angular/core'; - @${type}({${propName}: [{provide: forwardRef(() => MyService)}]}) + @${type}({${propName}: [forwardRef(() => MyService)]}) export class TestClass {} export class MyService {} @@ -173,6 +174,22 @@ describe('Missing injectable migration', () => { expect(tree.readContent('/index.ts')).not.toContain('@Injectable'); }); + it(`should not migrate provider with "useClass" and "deps" in ${type}`, async() => { + writeFile('/index.ts', ` + import {${type}} from '@angular/core'; + + export class MyService {} + + @${type}({${propName}: [{provide: MyService, deps: []}]}) + 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'; @@ -251,7 +268,7 @@ describe('Missing injectable migration', () => { import {MyService} from './index'; export @${type}({ - ${propName}: [{provide: MyService}], + ${propName}: [{provide: MyService, useClass: MyService}], }) export class OtherClass {} `); @@ -374,10 +391,12 @@ 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(/ServiceA {}\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(tree.readContent('/index.ts')) + .toContain(`{ provide: ServiceB, useValue: undefined },`); }); it(`should migrate multiple nested providers in same ${type}`, async() => { @@ -387,13 +406,15 @@ describe('Missing injectable migration', () => { export class ServiceA {} export class ServiceB {} export class ServiceC {} + export class ServiceD {} @${type}({ ${propName}: [ ServiceA, [ - {provide: ServiceB}, + {provide: ServiceB, useClass: ServiceB}, ServiceC, + {provide: ServiceD}, ], ]}) export class TestClass {} @@ -405,8 +426,11 @@ 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(/ServiceC {}\s+export class ServiceD/); expect(tree.readContent('/index.ts')) .toContain(`{ ${type}, Injectable } from '@angular/core`); + expect(tree.readContent('/index.ts')) + .toContain(`{ provide: ServiceD, useValue: undefined },`); }); it('should migrate providers referenced through identifier', async() => { @@ -415,8 +439,15 @@ describe('Missing injectable migration', () => { export class ServiceA {} export class ServiceB {} + export class ServiceC {} - const PROVIDERS = [ServiceA, ServiceB]; + const PROVIDERS = [ + ServiceA, + ServiceB, + // trailing comma is by intention to check if do not create + // an invalid object literal. + {provide: ServiceC, }, + ]; @${type}({ ${propName}: PROVIDERS, @@ -429,8 +460,11 @@ 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(/ServiceB {}\s+export class ServiceC/); expect(tree.readContent('/index.ts')) .toContain(`{ ${type}, Injectable } from '@angular/core`); + expect(tree.readContent('/index.ts')) + .toContain(`{ provide: ServiceC, useValue: undefined },`); }); it('should migrate providers created through static analyzable function call', async() => { @@ -439,13 +473,14 @@ describe('Missing injectable migration', () => { export class ServiceA {} export class ServiceB {} + export class ServiceC {} - export function createProviders(x: any) { - return [ServiceA, x] + export function createProviders(x: any, b: any) { + return [ServiceA, x, b] } @${type}({ - ${propName}: createProviders(ServiceB), + ${propName}: createProviders(ServiceB, {provide: ServiceC}), }) export class TestClass {} `); @@ -455,8 +490,11 @@ 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(/ServiceB {}\s+export class ServiceC/); expect(tree.readContent('/index.ts')) .toContain(`{ ${type}, Injectable } from '@angular/core`); + expect(tree.readContent('/index.ts')) + .toContain(`ServiceB, { provide: ServiceC, useValue: undefined }),`); }); it('should migrate providers which are computed through spread operator', async() => { @@ -465,8 +503,9 @@ describe('Missing injectable migration', () => { export class ServiceA {} export class ServiceB {} + export class ServiceC {} - const otherServices = [ServiceB]; + const otherServices = [ServiceB, {provide: ServiceC}]; @${type}({ ${propName}: [ServiceA, ...otherServices], @@ -479,8 +518,11 @@ 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(/ServiceB {}\s+export class ServiceC/); expect(tree.readContent('/index.ts')) .toContain(`{ ${type}, Injectable } from '@angular/core`); + expect(tree.readContent('/index.ts')) + .toContain(`ServiceB, { provide: ServiceC, useValue: undefined }];`); }); it(`should migrate provider once if referenced in multiple ${type} definitions`, async() => { @@ -515,6 +557,41 @@ describe('Missing injectable migration', () => { .toContain(`{ ${type}, Injectable } from '@angular/core`); }); + it(`should only migrate empty object provider literal once if referenced multiple times ` + + `in ${type} definitions`, + async() => { + writeFile('/provider.ts', ` + export class MyService {} + + export const PROVIDER = {provide: MyService}; + `); + + writeFile('/index.ts', ` + import {${type}} from '@angular/core'; + import {PROVIDER} from './provider'; + + @${type}({${propName}: [PROVIDER]}) + export class TestClassA {} + `); + + writeFile('/second.ts', ` + import {${type}} from '@angular/core'; + import {PROVIDER} from './provider'; + + export class ServiceB {} + + @${type}({${propName}: [PROVIDER, ServiceB]}) + export class TestClassB {} + `); + + await runMigration(); + + expect(warnOutput.length).toBe(0); + expect(tree.readContent('/provider.ts')).toMatch(/^\s+export class MyService {}/); + expect(tree.readContent('/provider.ts')) + .toContain(`const PROVIDER = { provide: MyService, useValue: undefined };`); + }); + it('should create new import for @Injectable when migrating provider', async() => { writeFile('/index.ts', ` import {${type}} from '@angular/core';