Skip to content

Commit

Permalink
fix(core): missing-injectable migration should handle forwardRef (#33286
Browse files Browse the repository at this point in the history
)

Currently the migration is unable to migrate instances where
the provider definition uses `forwardRef`. Since this is a
common pattern, we should support that from within the migration.

The solution to the problem is adding a foreign function resolver
to the `PartialEvaluator`. This basically matches the usage of
the static evaluation that is used by the ngtsc annotations.

PR Close #33286
  • Loading branch information
devversion authored and AndrewKushnir committed Oct 25, 2019
1 parent 4b81bb5 commit eeecbf2
Show file tree
Hide file tree
Showing 3 changed files with 35 additions and 3 deletions.
Expand Up @@ -11,6 +11,7 @@ ts_library(
"//packages/core/schematics/test:__pkg__",
],
deps = [
"//packages/compiler-cli/src/ngtsc/annotations",
"//packages/compiler-cli/src/ngtsc/imports",
"//packages/compiler-cli/src/ngtsc/partial_evaluator",
"//packages/compiler-cli/src/ngtsc/reflection",
Expand Down
Expand Up @@ -6,6 +6,7 @@
* 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 {TypeScriptReflectionHost} from '@angular/compiler-cli/src/ngtsc/reflection';
Expand Down Expand Up @@ -67,7 +68,7 @@ export class MissingInjectableTransform {
return [];
}

const evaluatedExpr = this.partialEvaluator.evaluate(module.providersExpr);
const evaluatedExpr = this._evaluateExpression(module.providersExpr);

if (!Array.isArray(evaluatedExpr)) {
return [{
Expand All @@ -89,7 +90,7 @@ export class MissingInjectableTransform {

// Migrate "providers" on directives and components if defined.
if (directive.providersExpr) {
const evaluatedExpr = this.partialEvaluator.evaluate(directive.providersExpr);
const evaluatedExpr = this._evaluateExpression(directive.providersExpr);
if (!Array.isArray(evaluatedExpr)) {
return [
{node: directive.providersExpr, message: `Providers are not statically analyzable.`}
Expand All @@ -100,7 +101,7 @@ export class MissingInjectableTransform {

// Migrate "viewProviders" on components if defined.
if (directive.viewProvidersExpr) {
const evaluatedExpr = this.partialEvaluator.evaluate(directive.viewProvidersExpr);
const evaluatedExpr = this._evaluateExpression(directive.viewProvidersExpr);
if (!Array.isArray(evaluatedExpr)) {
return [
{node: directive.viewProvidersExpr, message: `Providers are not statically analyzable.`}
Expand Down Expand Up @@ -150,6 +151,14 @@ export class MissingInjectableTransform {
}
}

/**
* Evaluates the given TypeScript expression using the partial evaluator with
* the foreign function resolver for handling "forwardRef" calls.
*/
private _evaluateExpression(expr: ts.Expression): ResolvedValue {
return this.partialEvaluator.evaluate(expr, forwardRefResolver);
}

/**
* Visits the given resolved value of a provider. Providers can be nested in
* arrays and we need to recursively walk through the providers to be able to
Expand Down
22 changes: 22 additions & 0 deletions packages/core/schematics/test/missing_injectable_migration_spec.ts
Expand Up @@ -48,6 +48,10 @@ describe('Missing injectable migration', () => {
// Switch into the temporary directory path. This allows us to run
// the schematic against our custom unit test tree.
shx.cd(tmpDirPath);

writeFile('/node_modules/@angular/core/index.d.ts', `
export declare function forwardRef(fn: Function);
`);
});

afterEach(() => {
Expand Down Expand Up @@ -135,6 +139,24 @@ describe('Missing injectable migration', () => {
.toContain(`{ ${type}, Injectable } 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)}]})
export class TestClass {}
export class MyService {}
`);

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}, forwardRef, Injectable } from '@angular/core`);
});

it(`should not migrate object literal provider with "useValue" in ${type}`, async() => {
writeFile('/index.ts', `
import {${type}} from '@angular/core';
Expand Down

0 comments on commit eeecbf2

Please sign in to comment.