feat(core): add ModuleWithProviders generic type migration (#33217)

Static methods that return a type of ModuleWithProviders currently
do not have to specify a type because the generic falls back to any.
This is problematic because the type of the actual module being
returned is not present in the type information.

Since Ivy uses d.ts files exclusively for downstream packages
(rather than metadata.json files, for example), we no longer have
the type of the actual module being created.

For this reason, a generic type should be added for
ModuleWithProviders that specifies the module type. This will be
required for all users in v10, but will only be necessary for
users of Ivy in v9.

PR Close #33217
This commit is contained in:
Adam Plumer 2019-10-17 01:40:36 -05:00 committed by Matias Niemelä
parent e5081bcf25
commit 56731f624a
10 changed files with 668 additions and 0 deletions

View File

@ -12,6 +12,7 @@ npm_package(
deps = [ deps = [
"//packages/core/schematics/migrations/dynamic-queries", "//packages/core/schematics/migrations/dynamic-queries",
"//packages/core/schematics/migrations/missing-injectable", "//packages/core/schematics/migrations/missing-injectable",
"//packages/core/schematics/migrations/module-with-providers",
"//packages/core/schematics/migrations/move-document", "//packages/core/schematics/migrations/move-document",
"//packages/core/schematics/migrations/postinstall-ngcc", "//packages/core/schematics/migrations/postinstall-ngcc",
"//packages/core/schematics/migrations/renderer-to-renderer2", "//packages/core/schematics/migrations/renderer-to-renderer2",

View File

@ -44,6 +44,11 @@
"version": "9-beta", "version": "9-beta",
"description": "Adds an ngcc call as a postinstall hook in package.json", "description": "Adds an ngcc call as a postinstall hook in package.json",
"factory": "./migrations/postinstall-ngcc/index" "factory": "./migrations/postinstall-ngcc/index"
},
"migration-v9-module-with-providers": {
"version": "9.0.0-beta",
"description": "Adds explicit typing to `ModuleWithProviders`",
"factory": "./migrations/module-with-providers/index"
} }
} }
} }

View File

@ -0,0 +1,20 @@
load("//tools:defaults.bzl", "ts_library")
ts_library(
name = "module-with-providers",
srcs = glob(["**/*.ts"]),
tsconfig = "//packages/core/schematics:tsconfig.json",
visibility = [
"//packages/core/schematics:__pkg__",
"//packages/core/schematics/test:__pkg__",
],
deps = [
"//packages/compiler-cli/src/ngtsc/imports",
"//packages/compiler-cli/src/ngtsc/partial_evaluator",
"//packages/compiler-cli/src/ngtsc/reflection",
"//packages/core/schematics/utils",
"@npm//@angular-devkit/schematics",
"@npm//@types/node",
"@npm//typescript",
],
)

View File

@ -0,0 +1,28 @@
## ModuleWithProviders migration
`ModuleWithProviders` type will not default to the `any` type for its generic in a future version of Angular.
This migration adds a generic to any `ModuleWithProvider` types found.
#### Before
```ts
import { NgModule, ModuleWithProviders } from '@angular/core';
@NgModule({})
export class MyModule {
static forRoot(): ModuleWithProviders {
ngModule: MyModule
}
}
```
#### After
```ts
import { NgModule, ModuleWithProviders } from '@angular/core';
@NgModule({})
export class MyModule {
static forRoot(): ModuleWithProviders<MyModule> {
ngModule: MyModule
}
}
```

View File

@ -0,0 +1,79 @@
/**
* @license
* Copyright Google Inc. All Rights Reserved.
*
* Use of this source code is governed by an MIT-style license that can be
* found in the LICENSE file at https://angular.io/license
*/
import * as ts from 'typescript';
import {NgDecorator, getAngularDecorators} from '../../utils/ng_decorators';
import {isModuleWithProvidersNotGeneric} from './util';
export interface ResolvedNgModule {
name: string;
node: ts.ClassDeclaration;
decorator: NgDecorator;
/**
* List of found static method declarations on the module which do not
* declare an explicit return type.
*/
staticMethodsWithoutType: ts.MethodDeclaration[];
}
/**
* Visitor that walks through specified TypeScript nodes and collects all
* found NgModule static methods without types and all ModuleWithProviders
* usages without generic types attached.
*/
export class Collector {
resolvedModules: ResolvedNgModule[] = [];
resolvedNonGenerics: ts.TypeReferenceNode[] = [];
constructor(public typeChecker: ts.TypeChecker) {}
visitNode(node: ts.Node) {
if (ts.isClassDeclaration(node)) {
this.visitClassDeclaration(node);
} else if (isModuleWithProvidersNotGeneric(this.typeChecker, node)) {
this.resolvedNonGenerics.push(node);
}
ts.forEachChild(node, n => this.visitNode(n));
}
private visitClassDeclaration(node: ts.ClassDeclaration) {
if (!node.decorators || !node.decorators.length) {
return;
}
const ngDecorators = getAngularDecorators(this.typeChecker, node.decorators);
const ngModuleDecorator = ngDecorators.find(({name}) => name === 'NgModule');
if (ngModuleDecorator) {
this._visitNgModuleClass(node, ngModuleDecorator);
}
}
private _visitNgModuleClass(node: ts.ClassDeclaration, decorator: NgDecorator) {
const decoratorCall = decorator.node.expression;
const metadata = decoratorCall.arguments[0];
if (!metadata || !ts.isObjectLiteralExpression(metadata)) {
return;
}
this.resolvedModules.push({
name: node.name ? node.name.text : 'default',
node,
decorator,
staticMethodsWithoutType: node.members.filter(isStaticMethodNoType),
});
}
}
function isStaticMethodNoType(node: ts.ClassElement): node is ts.MethodDeclaration {
return ts.isMethodDeclaration(node) && !!node.modifiers &&
node.modifiers.findIndex(m => m.kind === ts.SyntaxKind.StaticKeyword) > -1 && !node.type;
}

View File

@ -0,0 +1,101 @@
/**
* @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 {Rule, SchematicContext, SchematicsException, Tree, UpdateRecorder} from '@angular-devkit/schematics';
import {dirname, relative} from 'path';
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 {Collector} from './collector';
import {AnalysisFailure, ModuleWithProvidersTransform} from './transform';
/**
* Runs the ModuleWithProviders migration for all TypeScript projects in the current CLI workspace.
*/
export default function(): Rule {
return (tree: Tree, ctx: SchematicContext) => {
const {buildPaths, testPaths} = getProjectTsConfigPaths(tree);
const basePath = process.cwd();
const allPaths = [...buildPaths, ...testPaths];
const failures: string[] = [];
ctx.logger.info('------ ModuleWithProviders migration ------');
if (!allPaths.length) {
throw new SchematicsException(
'Could not find any tsconfig file. Cannot migrate ModuleWithProviders.');
}
for (const tsconfigPath of allPaths) {
failures.push(...runModuleWithProvidersMigration(tree, tsconfigPath, basePath));
}
if (failures.length) {
ctx.logger.info('Could not migrate all instances of ModuleWithProviders');
ctx.logger.info('Please manually fix the following failures:');
failures.forEach(message => ctx.logger.warn(`${message}`));
} else {
ctx.logger.info('Successfully migrated all found ModuleWithProviders.');
}
ctx.logger.info('----------------------------------------------');
};
}
function runModuleWithProvidersMigration(tree: Tree, tsconfigPath: string, basePath: string) {
const parsed = parseTsconfigFile(tsconfigPath, dirname(tsconfigPath));
const host = createMigrationCompilerHost(tree, parsed.options, basePath);
const failures: string[] = [];
const program = ts.createProgram(parsed.fileNames, parsed.options, host);
const typeChecker = program.getTypeChecker();
const collector = new Collector(typeChecker);
const sourceFiles = program.getSourceFiles().filter(
f => !f.isDeclarationFile && !program.isSourceFileFromExternalLibrary(f));
// Analyze source files by detecting all modules.
sourceFiles.forEach(sourceFile => collector.visitNode(sourceFile));
const {resolvedModules, resolvedNonGenerics} = collector;
const transformer = new ModuleWithProvidersTransform(typeChecker, getUpdateRecorder);
const updateRecorders = new Map<ts.SourceFile, UpdateRecorder>();
[...resolvedModules.reduce(
(failures, m) => failures.concat(transformer.migrateModule(m)), [] as AnalysisFailure[]),
...resolvedNonGenerics.reduce(
(failures, t) => failures.concat(transformer.migrateType(t)), [] as AnalysisFailure[])]
.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}`);
});
// Walk through each update recorder and commit the update. We need to commit the
// updates in batches per source file as there can be only one recorder per source
// file in order to avoid shift character offsets.
updateRecorders.forEach(recorder => tree.commitUpdate(recorder));
return failures;
/** Gets the update recorder for the specified source file. */
function getUpdateRecorder(sourceFile: ts.SourceFile): UpdateRecorder {
if (updateRecorders.has(sourceFile)) {
return updateRecorders.get(sourceFile) !;
}
const recorder = tree.beginUpdate(relative(basePath, sourceFile.fileName));
updateRecorders.set(sourceFile, recorder);
return recorder;
}
}

View File

@ -0,0 +1,167 @@
/**
* @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 {UpdateRecorder} from '@angular-devkit/schematics';
import {Reference} from '@angular/compiler-cli/src/ngtsc/imports';
import {DynamicValue, PartialEvaluator, ResolvedValue, ResolvedValueMap} from '@angular/compiler-cli/src/ngtsc/partial_evaluator';
import {TypeScriptReflectionHost} from '@angular/compiler-cli/src/ngtsc/reflection';
import * as ts from 'typescript';
import {ResolvedNgModule} from './collector';
import {createModuleWithProvidersType} from './util';
export interface AnalysisFailure {
node: ts.Node;
message: string;
}
const TODO_COMMENT = 'TODO: The following node requires a generic type for `ModuleWithProviders';
export class ModuleWithProvidersTransform {
private printer = ts.createPrinter();
private partialEvaluator: PartialEvaluator =
new PartialEvaluator(new TypeScriptReflectionHost(this.typeChecker), this.typeChecker);
constructor(
private typeChecker: ts.TypeChecker,
private getUpdateRecorder: (sf: ts.SourceFile) => UpdateRecorder) {}
/** Migrates a given NgModule by walking through the referenced providers and static methods. */
migrateModule(module: ResolvedNgModule): AnalysisFailure[] {
return module.staticMethodsWithoutType.map(this._migrateStaticNgModuleMethod.bind(this))
.filter(v => v) as AnalysisFailure[];
}
/** Migrates a ModuleWithProviders type definition that has no explicit generic type */
migrateType(type: ts.TypeReferenceNode): AnalysisFailure[] {
const parent = type.parent;
let moduleText: string|undefined;
if ((ts.isFunctionDeclaration(parent) || ts.isMethodDeclaration(parent)) && parent.body) {
const returnStatement = parent.body.statements.find(ts.isReturnStatement);
// No return type found, exit
if (!returnStatement || !returnStatement.expression) {
return [{node: parent, message: `Return type is not statically analyzable.`}];
}
moduleText = this._getNgModuleTypeOfExpression(returnStatement.expression);
} else if (ts.isPropertyDeclaration(parent) || ts.isVariableDeclaration(parent)) {
if (!parent.initializer) {
addTodoToNode(type, TODO_COMMENT);
this._updateNode(type, type);
return [{node: parent, message: `Unable to determine type for declaration.`}];
}
moduleText = this._getNgModuleTypeOfExpression(parent.initializer);
}
if (moduleText) {
this._addGenericToTypeReference(type, moduleText);
return [];
}
return [{node: parent, message: `Type is not statically analyzable.`}];
}
/** Add a given generic to a type reference node */
private _addGenericToTypeReference(node: ts.TypeReferenceNode, typeName: string) {
const newGenericExpr = createModuleWithProvidersType(typeName, node);
this._updateNode(node, newGenericExpr);
}
/**
* Migrates a given static method if its ModuleWithProviders does not provide
* a generic type.
*/
private _updateStaticMethodType(method: ts.MethodDeclaration, typeName: string) {
const newGenericExpr =
createModuleWithProvidersType(typeName, method.type as ts.TypeReferenceNode);
const newMethodDecl = ts.updateMethod(
method, method.decorators, method.modifiers, method.asteriskToken, method.name,
method.questionToken, method.typeParameters, method.parameters, newGenericExpr,
method.body);
this._updateNode(method, newMethodDecl);
}
/** Whether the resolved value map represents a ModuleWithProviders object */
isModuleWithProvidersType(value: ResolvedValueMap): boolean {
const ngModule = value.get('ngModule') !== undefined;
const providers = value.get('providers') !== undefined;
return ngModule && (value.size === 1 || (providers && value.size === 2));
}
/** Determine the generic type of a suspected ModuleWithProviders return type and add it
* explicitly */
private _migrateStaticNgModuleMethod(node: ts.MethodDeclaration): AnalysisFailure|null {
const returnStatement = node.body &&
node.body.statements.find(n => ts.isReturnStatement(n)) as ts.ReturnStatement | undefined;
// No return type found, exit
if (!returnStatement || !returnStatement.expression) {
return {node: node, message: `Return type is not statically analyzable.`};
}
const moduleText = this._getNgModuleTypeOfExpression(returnStatement.expression);
if (moduleText) {
this._updateStaticMethodType(node, moduleText);
return null;
}
return {node: node, message: `Method type is not statically analyzable.`};
}
/** Evaluate and return the ngModule type from an expression */
private _getNgModuleTypeOfExpression(expr: ts.Expression): string|undefined {
const evaluatedExpr = this.partialEvaluator.evaluate(expr);
return this._getTypeOfResolvedValue(evaluatedExpr);
}
/**
* Visits a given object literal expression to determine the ngModule type. If the expression
* cannot be resolved, add a TODO to alert the user.
*/
private _getTypeOfResolvedValue(value: ResolvedValue): string|undefined {
if (value instanceof Map && this.isModuleWithProvidersType(value)) {
const mapValue = value.get('ngModule') !;
if (mapValue instanceof Reference && ts.isClassDeclaration(mapValue.node) &&
mapValue.node.name) {
return mapValue.node.name.text;
} else if (mapValue instanceof DynamicValue) {
addTodoToNode(mapValue.node, TODO_COMMENT);
this._updateNode(mapValue.node, mapValue.node);
}
}
return undefined;
}
private _updateNode(node: ts.Node, newNode: ts.Node) {
const newText = this.printer.printNode(ts.EmitHint.Unspecified, newNode, node.getSourceFile());
const recorder = this.getUpdateRecorder(node.getSourceFile());
recorder.remove(node.getStart(), node.getWidth());
recorder.insertRight(node.getStart(), newText);
}
}
/**
* Adds a to-do to the given TypeScript node which alerts developers to fix
* potential issues identified by the migration.
*/
function addTodoToNode(node: ts.Node, text: string) {
ts.setSyntheticLeadingComments(node, [{
pos: -1,
end: -1,
hasTrailingNewLine: false,
kind: ts.SyntaxKind.MultiLineCommentTrivia,
text: ` ${text} `
}]);
}

View File

@ -0,0 +1,31 @@
/**
* @license
* Copyright Google Inc. All Rights Reserved.
*
* Use of this source code is governed by an MIT-style license that can be
* found in the LICENSE file at https://angular.io/license
*/
import * as ts from 'typescript';
import {getImportOfIdentifier} from '../../utils/typescript/imports';
/** Add a generic type to a type reference. */
export function createModuleWithProvidersType(
type: string, node?: ts.TypeReferenceNode): ts.TypeReferenceNode {
const typeNode = node || ts.createTypeReferenceNode('ModuleWithProviders', []);
const typeReferenceNode = ts.createTypeReferenceNode(ts.createIdentifier(type), []);
return ts.updateTypeReferenceNode(
typeNode, typeNode.typeName, ts.createNodeArray([typeReferenceNode]));
}
/** Determine whether a node is a ModuleWithProviders type reference node without a generic type */
export function isModuleWithProvidersNotGeneric(
typeChecker: ts.TypeChecker, node: ts.Node): node is ts.TypeReferenceNode {
if (!ts.isTypeReferenceNode(node) || !ts.isIdentifier(node.typeName)) {
return false;
}
const imp = getImportOfIdentifier(typeChecker, node.typeName);
return !!imp && imp.name === 'ModuleWithProviders' && imp.importModule === '@angular/core' &&
!node.typeArguments;
}

View File

@ -10,6 +10,7 @@ ts_library(
deps = [ deps = [
"//packages/core/schematics/migrations/dynamic-queries", "//packages/core/schematics/migrations/dynamic-queries",
"//packages/core/schematics/migrations/missing-injectable", "//packages/core/schematics/migrations/missing-injectable",
"//packages/core/schematics/migrations/module-with-providers",
"//packages/core/schematics/migrations/move-document", "//packages/core/schematics/migrations/move-document",
"//packages/core/schematics/migrations/postinstall-ngcc", "//packages/core/schematics/migrations/postinstall-ngcc",
"//packages/core/schematics/migrations/renderer-to-renderer2", "//packages/core/schematics/migrations/renderer-to-renderer2",

View File

@ -0,0 +1,235 @@
/**
* @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 {getSystemPath, normalize, virtualFs} from '@angular-devkit/core';
import {TempScopedNodeJsSyncHost} from '@angular-devkit/core/node/testing';
import {HostTree} from '@angular-devkit/schematics';
import {SchematicTestRunner, UnitTestTree} from '@angular-devkit/schematics/testing';
import * as shx from 'shelljs';
describe('ModuleWithProviders migration', () => {
let runner: SchematicTestRunner;
let host: TempScopedNodeJsSyncHost;
let tree: UnitTestTree;
let tmpDirPath: string;
let previousWorkingDir: string;
beforeEach(() => {
runner = new SchematicTestRunner('test', require.resolve('../migrations.json'));
host = new TempScopedNodeJsSyncHost();
tree = new UnitTestTree(new HostTree(host));
writeFile('/tsconfig.json', JSON.stringify({
compilerOptions: {
lib: ['es2015'],
}
}));
writeFile('/angular.json', JSON.stringify({
projects: {t: {architect: {build: {options: {tsConfig: './tsconfig.json'}}}}}
}));
previousWorkingDir = shx.pwd();
tmpDirPath = getSystemPath(host.root);
// Switch into the temporary directory path. This allows us to run
// the schematic against our custom unit test tree.
shx.cd(tmpDirPath);
});
afterEach(() => {
shx.cd(previousWorkingDir);
shx.rm('-r', tmpDirPath);
});
it('should add generic type for function return', async() => {
writeFile('/index.ts', `
import {NgModule, ModuleWithProviders} from '@angular/core';
@NgModule({})
export class BaseModule {}
export function getProvider() {
return {ngModule: BaseModule}
}
@NgModule({})
export class TestModule {
static forRoot(): ModuleWithProviders {
return getProvider();
}
}
`);
await runMigration();
expect(tree.readContent('/index.ts')).toContain(`ModuleWithProviders<BaseModule>`);
});
it('should add generic type for function return; external file', async() => {
writeFile('/module.ts', `
import {NgModule} from '@angular/core';
@NgModule({})
export class BaseModule {}
`);
writeFile('/index.ts', `
import {NgModule, ModuleWithProviders} from '@angular/core';
import {BaseModule} from './module';
export function getProvider() {
return {ngModule: BaseModule}
}
@NgModule({})
export class TestModule {
static forRoot(): ModuleWithProviders {
return getProvider();
}
}
`);
await runMigration();
expect(tree.readContent('/index.ts')).toContain(`ModuleWithProviders<BaseModule>`);
});
it('should add generic type for function return without explicit type', async() => {
writeFile('/index.ts', `
import {NgModule} from '@angular/core';
@NgModule({})
export class BaseModule {}
export function getProvider() {
return {ngModule: BaseModule}
}
@NgModule({})
export class TestModule {
static forRoot() {
return getProvider();
}
}
`);
await runMigration();
expect(tree.readContent('/index.ts')).toContain(`ModuleWithProviders<BaseModule>`);
});
it('should add generic type for const variable', async() => {
writeFile('/index.ts', `
import {ModuleWithProviders, NgModule} from '@angular/core';
@NgModule({})
export class BaseModule {}
export const myModuleWithProviders = {ngModule: BaseModule};
@NgModule({})
export class TestModule {
static forRoot(): ModuleWithProviders {
return myModuleWithProviders;
}
}
`);
await runMigration();
expect(tree.readContent('/index.ts')).toContain(`ModuleWithProviders<BaseModule>`);
});
it('should add generic type for const variable without explicit type', async() => {
writeFile('/index.ts', `
import {NgModule} from '@angular/core';
@NgModule({})
export class BaseModule {}
export const myModuleWithProviders = {ngModule: BaseModule};
@NgModule({})
export class TestModule {
static forRoot() {
return myModuleWithProviders;
}
}
`);
await runMigration();
expect(tree.readContent('/index.ts')).toContain(`ModuleWithProviders<BaseModule>`);
});
it('should not add generic type for const variable with invalid base object', async() => {
writeFile('/index.ts', `
import {NgModule} from '@angular/core';
@NgModule({})
export class BaseModule {}
export const myModuleWithProviders = {ngModule: BaseModule, otherKey: 'a'};
@NgModule({})
export class TestModule {
static forRoot() {
return myModuleWithProviders;
}
}
`);
await runMigration();
expect(tree.readContent('/index.ts')).not.toContain(`ModuleWithProviders<BaseModule>`);
});
it('should add generic type for const variables and functions with incomplete type', async() => {
writeFile('/index.ts', `
import {ModuleWithProviders, NgModule} from '@angular/core';
@NgModule({})
export class BaseModule {}
export const myModuleWithProviders: ModuleWithProviders = {ngModule: BaseModule};
export function mwpFunction(): ModuleWithProviders {
return myModuleWithProviders;
}
export class MwpClass {
mwp: ModuleWithProviders = myModuleWithProviders;
private _mwp: ModuleWithProviders = myModuleWithProviders;
getMwp(): ModuleWithProviders {
return myModuleWithProviders;
}
static initMwp(): ModuleWithProviders {
return myModuleWithProviders;
}
}
`);
await runMigration();
// Note the explicit space at the end here
expect(tree.readContent('/index.ts')).not.toContain(`ModuleWithProviders `);
});
it('should not add generic type for const variables without initialization', async() => {
writeFile('/index.ts', `
import {ModuleWithProviders} from '@angular/core';
export const myModuleWithProviders: ModuleWithProviders;
`);
await runMigration();
expect(tree.readContent('/index.ts')).toContain(`TODO`);
});
function writeFile(filePath: string, contents: string) {
host.sync.write(normalize(filePath), virtualFs.stringToFileBuffer(contents));
}
function runMigration() {
runner.runSchematicAsync('migration-v9-module-with-providers', {}, tree).toPromise();
}
});