From 9f2ae5d6ff487df080d670a1adef0cc01878c92c Mon Sep 17 00:00:00 2001 From: Paul Gschwendtner Date: Mon, 10 Jun 2019 19:08:54 +0200 Subject: [PATCH] feat(ivy): introduce missing-injectable migration for google3 (#30956) Introduces a new migration schematic for adding the "@Injectable()" decorator to provider classes which are currently not migrated. Previously in ViewEngine, classes which are declared as providers sometimes don't require the "@Injectable()" decorator (e.g. https://stackblitz.com/edit/angular-hpo7gw) With Ivy, provider classes need to be explicitly decorated with the "@Injectable()" decorator if they are declared as providers of a given module. This commit introduces a migration schematic which automatically adds the explicit decorator to places where the decorator is currently missing. The migration logic is designed in a CLI devkit and TSlint agnostic way so that we can also have this migration run as part of a public CLI migration w/ `ng update`. This will be handled as part of a follow-up to reiterate on console output etc. Resolves FW-1371 PR Close #30956 --- .../migrations/missing-injectable/BUILD.bazel | 21 + .../missing-injectable/google3/BUILD.bazel | 13 + .../google3/noMissingInjectableRule.ts | 64 ++ .../google3/tslint_update_recorder.ts | 58 ++ .../missing-injectable/import_manager.ts | 254 ++++++++ .../migrations/missing-injectable/index.ts | 131 ++++ .../missing-injectable/module_collector.ts | 68 ++ .../missing-injectable/transform.ts | 130 ++++ .../missing-injectable/update_recorder.ts | 22 + packages/core/schematics/test/BUILD.bazel | 2 + .../google3/missing_injectable_rule_spec.ts | 227 +++++++ .../test/missing_injectable_migration_spec.ts | 580 ++++++++++++++++++ .../core/schematics/test/test-migrations.json | 4 + 13 files changed, 1574 insertions(+) create mode 100644 packages/core/schematics/migrations/missing-injectable/BUILD.bazel create mode 100644 packages/core/schematics/migrations/missing-injectable/google3/BUILD.bazel create mode 100644 packages/core/schematics/migrations/missing-injectable/google3/noMissingInjectableRule.ts create mode 100644 packages/core/schematics/migrations/missing-injectable/google3/tslint_update_recorder.ts create mode 100644 packages/core/schematics/migrations/missing-injectable/import_manager.ts create mode 100644 packages/core/schematics/migrations/missing-injectable/index.ts create mode 100644 packages/core/schematics/migrations/missing-injectable/module_collector.ts create mode 100644 packages/core/schematics/migrations/missing-injectable/transform.ts create mode 100644 packages/core/schematics/migrations/missing-injectable/update_recorder.ts create mode 100644 packages/core/schematics/test/google3/missing_injectable_rule_spec.ts create mode 100644 packages/core/schematics/test/missing_injectable_migration_spec.ts diff --git a/packages/core/schematics/migrations/missing-injectable/BUILD.bazel b/packages/core/schematics/migrations/missing-injectable/BUILD.bazel new file mode 100644 index 0000000000..f0dcc07ad2 --- /dev/null +++ b/packages/core/schematics/migrations/missing-injectable/BUILD.bazel @@ -0,0 +1,21 @@ +load("//tools:defaults.bzl", "ts_library") + +ts_library( + name = "missing-injectable", + srcs = glob(["**/*.ts"]), + tsconfig = "//packages/core/schematics:tsconfig.json", + visibility = [ + "//packages/core/schematics:__pkg__", + "//packages/core/schematics/migrations/missing-injectable/google3:__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", + ], +) diff --git a/packages/core/schematics/migrations/missing-injectable/google3/BUILD.bazel b/packages/core/schematics/migrations/missing-injectable/google3/BUILD.bazel new file mode 100644 index 0000000000..37beacee1f --- /dev/null +++ b/packages/core/schematics/migrations/missing-injectable/google3/BUILD.bazel @@ -0,0 +1,13 @@ +load("//tools:defaults.bzl", "ts_library") + +ts_library( + name = "google3", + srcs = glob(["**/*.ts"]), + tsconfig = "//packages/core/schematics:tsconfig.json", + visibility = ["//packages/core/schematics/test:__pkg__"], + deps = [ + "//packages/core/schematics/migrations/missing-injectable", + "@npm//tslint", + "@npm//typescript", + ], +) diff --git a/packages/core/schematics/migrations/missing-injectable/google3/noMissingInjectableRule.ts b/packages/core/schematics/migrations/missing-injectable/google3/noMissingInjectableRule.ts new file mode 100644 index 0000000000..1f596f6335 --- /dev/null +++ b/packages/core/schematics/migrations/missing-injectable/google3/noMissingInjectableRule.ts @@ -0,0 +1,64 @@ +/** + * @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 {RuleFailure, Rules} from 'tslint'; +import * as ts from 'typescript'; +import {NgModuleCollector} from '../module_collector'; +import {MissingInjectableTransform} from '../transform'; +import {TslintUpdateRecorder} from './tslint_update_recorder'; + +/** + * TSLint rule that flags classes which are declared as providers in NgModules but + * aren't decorated with any Angular decorator. + */ +export class Rule extends Rules.TypedRule { + applyWithProgram(sourceFile: ts.SourceFile, program: ts.Program): RuleFailure[] { + const ruleName = this.ruleName; + const typeChecker = program.getTypeChecker(); + const sourceFiles = program.getSourceFiles().filter( + s => !s.isDeclarationFile && !program.isSourceFileFromExternalLibrary(s)); + const moduleCollector = new NgModuleCollector(typeChecker); + const failures: RuleFailure[] = []; + + // Analyze source files by detecting all NgModule definitions. + sourceFiles.forEach(sourceFile => moduleCollector.visitNode(sourceFile)); + + const {resolvedModules} = moduleCollector; + const transformer = new MissingInjectableTransform(typeChecker, getUpdateRecorder); + const updateRecorders = new Map(); + + resolvedModules.forEach(module => { + transformer.migrateModule(module).forEach(({message, node}) => { + // Only report failures for the current source file that is visited. + if (node.getSourceFile() === sourceFile) { + failures.push( + new RuleFailure(node.getSourceFile(), node.getStart(), 0, message, ruleName)); + } + }); + }); + + // Record the changes collected in the import manager and NgModule manager. + transformer.recordChanges(); + + if (updateRecorders.has(sourceFile)) { + failures.push(...updateRecorders.get(sourceFile) !.failures); + } + + return failures; + + /** Gets the update recorder for the specified source file. */ + function getUpdateRecorder(sourceFile: ts.SourceFile): TslintUpdateRecorder { + if (updateRecorders.has(sourceFile)) { + return updateRecorders.get(sourceFile) !; + } + const recorder = new TslintUpdateRecorder(ruleName, sourceFile); + updateRecorders.set(sourceFile, recorder); + return recorder; + } + } +} 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 new file mode 100644 index 0000000000..d3c2d0b982 --- /dev/null +++ b/packages/core/schematics/migrations/missing-injectable/google3/tslint_update_recorder.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 {Replacement, RuleFailure} from 'tslint'; +import * as ts from 'typescript'; + +import {UpdateRecorder} from '../update_recorder'; + +export class TslintUpdateRecorder implements UpdateRecorder { + failures: RuleFailure[] = []; + + constructor(private ruleName: string, private sourceFile: ts.SourceFile) {} + + addClassDecorator(node: ts.ClassDeclaration, decoratorText: string, moduleName: 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}".`, + this.ruleName, Replacement.appendText(node.getStart(), `${decoratorText}\n`))); + } + + addNewImport(start: number, importText: string) { + this.failures.push(new RuleFailure( + this.sourceFile, start, 0, `Source file needs to have import: "${importText}"`, + this.ruleName, Replacement.appendText(start, importText))); + } + + updateExistingImport(namedBindings: ts.NamedImports, newNamedBindings: string): void { + const fix = [ + Replacement.deleteText(namedBindings.getStart(), namedBindings.getWidth()), + Replacement.appendText(namedBindings.getStart(), newNamedBindings), + ]; + this.failures.push(new RuleFailure( + this.sourceFile, namedBindings.getStart(), namedBindings.getEnd(), + `Import needs to be updated to import symbols: "${newNamedBindings}"`, this.ruleName, fix)); + } + + replaceDecorator(decorator: ts.Node, newText: string, moduleName: string): void { + const fix = [ + Replacement.deleteText(decorator.getStart(), decorator.getWidth()), + Replacement.appendText(decorator.getStart(), newText), + ]; + 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}"`, + this.ruleName, fix)); + } + + commitUpdate() {} +} diff --git a/packages/core/schematics/migrations/missing-injectable/import_manager.ts b/packages/core/schematics/migrations/missing-injectable/import_manager.ts new file mode 100644 index 0000000000..8bc46eca1f --- /dev/null +++ b/packages/core/schematics/migrations/missing-injectable/import_manager.ts @@ -0,0 +1,254 @@ +/** + * @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 {dirname, resolve} from 'path'; +import * as ts from 'typescript'; +import {UpdateRecorder} from './update_recorder'; + +/** + * Import manager that can be used to add TypeScript imports to given source + * files. The manager ensures that multiple transformations are applied properly + * without shifted offsets and that similar existing import declarations are re-used. + */ +export class ImportManager { + /** Map of import declarations that need to be updated to include the given symbols. */ + private updatedImports = + new Map(); + /** Map of source-files and their previously used identifier names. */ + private usedIdentifierNames = new Map(); + /** + * Array of previously resolved symbol imports. Cache can be re-used to return + * the same identifier without checking the source-file again. + */ + private importCache: { + sourceFile: ts.SourceFile, + symbolName: string|null, + moduleName: string, + identifier: ts.Identifier + }[] = []; + + constructor( + private getUpdateRecorder: (sf: ts.SourceFile) => UpdateRecorder, + private printer: ts.Printer) {} + + /** + * Adds an import to the given source-file and returns the TypeScript + * identifier that can be used to access the newly imported symbol. + */ + addImportToSourceFile( + sourceFile: ts.SourceFile, symbolName: string|null, moduleName: string, + typeImport = false): ts.Expression { + const sourceDir = dirname(sourceFile.fileName); + let importStartIndex = 0; + let existingImport: ts.ImportDeclaration|null = null; + + // In case the given import has been already generated previously, we just return + // the previous generated identifier in order to avoid duplicate generated imports. + const cachedImport = this.importCache.find( + c => c.sourceFile === sourceFile && c.symbolName === symbolName && + c.moduleName === moduleName); + if (cachedImport) { + return cachedImport.identifier; + } + + // Walk through all source-file top-level statements and search for import declarations + // that already match the specified "moduleName" and can be updated to import the + // given symbol. If no matching import can be found, the last import in the source-file + // will be used as starting point for a new import that will be generated. + for (let i = sourceFile.statements.length - 1; i >= 0; i--) { + const statement = sourceFile.statements[i]; + + if (!ts.isImportDeclaration(statement) || !ts.isStringLiteral(statement.moduleSpecifier) || + !statement.importClause) { + continue; + } + + if (importStartIndex === 0) { + importStartIndex = this._getEndPositionOfNode(statement); + } + + const moduleSpecifier = statement.moduleSpecifier.text; + + if (moduleSpecifier.startsWith('.') && + resolve(sourceDir, moduleSpecifier) !== resolve(sourceDir, moduleName) || + moduleSpecifier !== moduleName) { + continue; + } + + if (statement.importClause.namedBindings) { + const namedBindings = statement.importClause.namedBindings; + + // In case a "Type" symbol is imported, we can't use namespace imports + // because these only export symbols available at runtime (no types) + if (ts.isNamespaceImport(namedBindings) && !typeImport) { + return ts.createPropertyAccess( + ts.createIdentifier(namedBindings.name.text), + ts.createIdentifier(symbolName || 'default')); + } else if (ts.isNamedImports(namedBindings) && symbolName) { + const existingElement = namedBindings.elements.find( + e => + e.propertyName ? e.propertyName.text === symbolName : e.name.text === symbolName); + + if (existingElement) { + return ts.createIdentifier(existingElement.name.text); + } + + // In case the symbol could not be found in an existing import, we + // keep track of the import declaration as it can be updated to include + // the specified symbol name without having to create a new import. + existingImport = statement; + } + } else if (statement.importClause.name && !symbolName) { + return ts.createIdentifier(statement.importClause.name.text); + } + } + + if (existingImport) { + const propertyIdentifier = ts.createIdentifier(symbolName !); + const generatedUniqueIdentifier = this._getUniqueIdentifier(sourceFile, symbolName !); + const needsGeneratedUniqueName = generatedUniqueIdentifier.text !== symbolName; + const importName = needsGeneratedUniqueName ? generatedUniqueIdentifier : propertyIdentifier; + + // Since it can happen that multiple classes need to be imported within the + // specified source file and we want to add the identifiers to the existing + // import declaration, we need to keep track of the updated import declarations. + // We can't directly update the import declaration for each identifier as this + // would throw off the recorder offsets. We need to keep track of the new identifiers + // for the import and perform the import transformation as batches per source-file. + this.updatedImports.set( + existingImport, (this.updatedImports.get(existingImport) || []).concat({ + propertyName: needsGeneratedUniqueName ? propertyIdentifier : undefined, + importName: importName, + })); + + // Keep track of all updated imports so that we don't generate duplicate + // similar imports as these can't be statically analyzed in the source-file yet. + this.importCache.push({sourceFile, moduleName, symbolName, identifier: importName}); + + return importName; + } + + let identifier: ts.Identifier|null = null; + let newImport: ts.ImportDeclaration|null = null; + + if (symbolName) { + const propertyIdentifier = ts.createIdentifier(symbolName); + const generatedUniqueIdentifier = this._getUniqueIdentifier(sourceFile, symbolName); + const needsGeneratedUniqueName = generatedUniqueIdentifier.text !== symbolName; + identifier = needsGeneratedUniqueName ? generatedUniqueIdentifier : propertyIdentifier; + + newImport = ts.createImportDeclaration( + undefined, undefined, + ts.createImportClause( + undefined, + ts.createNamedImports([ts.createImportSpecifier( + needsGeneratedUniqueName ? propertyIdentifier : undefined, identifier)])), + ts.createStringLiteral(moduleName)); + } else { + identifier = this._getUniqueIdentifier(sourceFile, 'defaultExport'); + newImport = ts.createImportDeclaration( + undefined, undefined, ts.createImportClause(identifier, undefined), + ts.createStringLiteral(moduleName)); + } + + const newImportText = this.printer.printNode(ts.EmitHint.Unspecified, newImport, sourceFile); + // If the import is generated at the start of the source file, we want to add + // a new-line after the import. Otherwise if the import is generated after an + // existing import, we need to prepend a new-line so that the import is not on + // the same line as the existing import anchor. + this.getUpdateRecorder(sourceFile) + .addNewImport( + importStartIndex, importStartIndex === 0 ? `${newImportText}\n` : `\n${newImportText}`); + + // Keep track of all generated imports so that we don't generate duplicate + // similar imports as these can't be statically analyzed in the source-file yet. + this.importCache.push({sourceFile, symbolName, moduleName, identifier}); + + return identifier; + } + + /** + * Stores the collected import changes within the appropriate update recorders. The + * updated imports can only be updated *once* per source-file because previous updates + * could otherwise shift the source-file offsets. + */ + recordChanges() { + this.updatedImports.forEach((expressions, importDecl) => { + const sourceFile = importDecl.getSourceFile(); + const recorder = this.getUpdateRecorder(sourceFile); + const namedBindings = importDecl.importClause !.namedBindings as ts.NamedImports; + const newNamedBindings = ts.updateNamedImports( + namedBindings, + namedBindings.elements.concat(expressions.map( + ({propertyName, importName}) => ts.createImportSpecifier(propertyName, importName)))); + + const newNamedBindingsText = + this.printer.printNode(ts.EmitHint.Unspecified, newNamedBindings, sourceFile); + recorder.updateExistingImport(namedBindings, newNamedBindingsText); + }); + } + + /** Gets an unique identifier with a base name for the given source file. */ + private _getUniqueIdentifier(sourceFile: ts.SourceFile, baseName: string): ts.Identifier { + if (this.isUniqueIdentifierName(sourceFile, baseName)) { + this._recordUsedIdentifier(sourceFile, baseName); + return ts.createIdentifier(baseName); + } + + let name = null; + let counter = 1; + do { + name = `${baseName}_${counter++}`; + } while (!this.isUniqueIdentifierName(sourceFile, name)); + + this._recordUsedIdentifier(sourceFile, name !); + return ts.createIdentifier(name !); + } + + /** + * Checks whether the specified identifier name is used within the given + * source file. + */ + private isUniqueIdentifierName(sourceFile: ts.SourceFile, name: string) { + if (this.usedIdentifierNames.has(sourceFile) && + this.usedIdentifierNames.get(sourceFile) !.indexOf(name) !== -1) { + return false; + } + + // Walk through the source file and search for an identifier matching + // the given name. In that case, it's not guaranteed that this name + // is unique in the given declaration scope and we just return false. + const nodeQueue: ts.Node[] = [sourceFile]; + while (nodeQueue.length) { + const node = nodeQueue.shift() !; + if (ts.isIdentifier(node) && node.text === name) { + return false; + } + nodeQueue.push(...node.getChildren()); + } + return true; + } + + private _recordUsedIdentifier(sourceFile: ts.SourceFile, identifierName: string) { + this.usedIdentifierNames.set( + sourceFile, (this.usedIdentifierNames.get(sourceFile) || []).concat(identifierName)); + } + + /** + * Determines the full end of a given node. By default the end position of a node is + * before all trailing comments. This could mean that generated imports shift comments. + */ + private _getEndPositionOfNode(node: ts.Node) { + const nodeEndPos = node.getEnd(); + const commentRanges = ts.getTrailingCommentRanges(node.getSourceFile().text, nodeEndPos); + if (!commentRanges || !commentRanges.length) { + return nodeEndPos; + } + return commentRanges[commentRanges.length - 1] !.end; + } +} diff --git a/packages/core/schematics/migrations/missing-injectable/index.ts b/packages/core/schematics/migrations/missing-injectable/index.ts new file mode 100644 index 0000000000..76a7cd1537 --- /dev/null +++ b/packages/core/schematics/migrations/missing-injectable/index.ts @@ -0,0 +1,131 @@ +/** + * @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} from '@angular-devkit/schematics'; +import {dirname, relative} from 'path'; +import * as ts from 'typescript'; + +import {getProjectTsConfigPaths} from '../../utils/project_tsconfig_paths'; +import {parseTsconfigFile} from '../../utils/typescript/parse_tsconfig'; + +import {NgModuleCollector} from './module_collector'; +import {MissingInjectableTransform} from './transform'; +import {UpdateRecorder} from './update_recorder'; + +/** Entry point for the V9 "missing @Injectable" schematic. */ +export default function(): Rule { + return (tree: Tree, ctx: SchematicContext) => { + const {buildPaths, testPaths} = getProjectTsConfigPaths(tree); + const basePath = process.cwd(); + const failures: string[] = []; + + ctx.logger.info('------ Missing @Injectable migration ------'); + if (!buildPaths.length && !testPaths.length) { + throw new SchematicsException( + 'Could not find any tsconfig file. Cannot add the "@Injectable" decorator to providers ' + + 'which don\'t have that decorator set.'); + } + + for (const tsconfigPath of [...buildPaths, ...testPaths]) { + failures.push(...runMissingInjectableMigration(tree, tsconfigPath, basePath)); + } + + if (failures.length) { + ctx.logger.info('Could not migrate all providers automatically. Please'); + ctx.logger.info('manually migrate the following instances:'); + failures.forEach(message => ctx.logger.warn(`⮑ ${message}`)); + } else { + ctx.logger.info('Successfully migrated all undecorated providers.'); + } + ctx.logger.info('-------------------------------------------'); + }; +} + +function runMissingInjectableMigration( + tree: Tree, tsconfigPath: string, basePath: string): string[] { + const parsed = parseTsconfigFile(tsconfigPath, dirname(tsconfigPath)); + const host = ts.createCompilerHost(parsed.options, true); + const failures: string[] = []; + + // We need to overwrite the host "readFile" method, as we want the TypeScript + // program to be based on the file contents in the virtual file tree. + host.readFile = fileName => { + const buffer = tree.read(relative(basePath, fileName)); + // Strip BOM because TypeScript respects this character and it ultimately + // results in shifted offsets since the CLI UpdateRecorder tries to + // automatically account for the BOM character. + // https://github.com/angular/angular-cli/issues/14558 + return buffer ? buffer.toString().replace(/^\uFEFF/, '') : undefined; + }; + + const program = ts.createProgram(parsed.fileNames, parsed.options, host); + const typeChecker = program.getTypeChecker(); + const moduleCollector = new NgModuleCollector(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)); + + const {resolvedModules} = moduleCollector; + const transformer = new MissingInjectableTransform(typeChecker, getUpdateRecorder); + const updateRecorders = new Map(); + + resolvedModules.forEach(module => { + transformer.migrateModule(module).forEach(({message, node}) => { + const nodeSourceFile = node.getSourceFile(); + const relativeFilePath = relative(basePath, nodeSourceFile.fileName); + const {line, character} = + ts.getLineAndCharacterOfPosition(node.getSourceFile(), node.getStart()); + failures.push(`${relativeFilePath}@${line + 1}:${character + 1}: ${message}`); + }); + }); + + // Record the changes collected in the import manager and transformer. + transformer.recordChanges(); + + // 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 => recorder.commitUpdate()); + + 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 treeRecorder = tree.beginUpdate(relative(basePath, sourceFile.fileName)); + const recorder: UpdateRecorder = { + addClassDecorator(node: ts.ClassDeclaration, text: string) { + // New imports should be inserted at the left while decorators should be inserted + // at the right in order to ensure that imports are inserted before the decorator + // if the start position of import and decorator is the source file start. + treeRecorder.insertRight(node.getStart(), `${text}\n`); + }, + replaceDecorator(decorator: ts.Decorator, newText: string) { + treeRecorder.remove(decorator.getStart(), decorator.getWidth()); + treeRecorder.insertRight(decorator.getStart(), newText); + }, + addNewImport(start: number, importText: string) { + // New imports should be inserted at the left while decorators should be inserted + // at the right in order to ensure that imports are inserted before the decorator + // if the start position of import and decorator is the source file start. + treeRecorder.insertLeft(start, importText); + }, + updateExistingImport(namedBindings: ts.NamedImports, newNamedBindings: string) { + treeRecorder.remove(namedBindings.getStart(), namedBindings.getWidth()); + treeRecorder.insertRight(namedBindings.getStart(), newNamedBindings); + }, + commitUpdate() { tree.commitUpdate(treeRecorder); } + }; + updateRecorders.set(sourceFile, recorder); + return recorder; + } +} diff --git a/packages/core/schematics/migrations/missing-injectable/module_collector.ts b/packages/core/schematics/migrations/missing-injectable/module_collector.ts new file mode 100644 index 0000000000..ba6644ed8d --- /dev/null +++ b/packages/core/schematics/migrations/missing-injectable/module_collector.ts @@ -0,0 +1,68 @@ +/** + * @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 {getPropertyNameText} from '../../utils/typescript/property_name'; + +export interface ResolvedNgModule { + name: string; + node: ts.ClassDeclaration; + decorator: NgDecorator; + providersExpr: ts.Expression|null; +} + +/** + * Visitor that walks through specified TypeScript nodes and collects all + * found NgModule definitions. + */ +export class NgModuleCollector { + resolvedModules: ResolvedNgModule[] = []; + + constructor(public typeChecker: ts.TypeChecker) {} + + visitNode(node: ts.Node) { + if (ts.isClassDeclaration(node)) { + this.visitClassDeclaration(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; + } + + const providersNode = metadata.properties.filter(ts.isPropertyAssignment) + .find(p => getPropertyNameText(p.name) === 'providers'); + this.resolvedModules.push({ + name: node.name ? node.name.text : 'default', + node, + decorator, + providersExpr: providersNode !== undefined ? providersNode.initializer : null, + }); + } +} diff --git a/packages/core/schematics/migrations/missing-injectable/transform.ts b/packages/core/schematics/migrations/missing-injectable/transform.ts new file mode 100644 index 0000000000..eb940067e2 --- /dev/null +++ b/packages/core/schematics/migrations/missing-injectable/transform.ts @@ -0,0 +1,130 @@ +/** + * @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 {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'; +import * as ts from 'typescript'; + +import {getAngularDecorators} from '../../utils/ng_decorators'; + +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']; + +export interface AnalysisFailure { + node: ts.Node; + message: string; +} + +export class MissingInjectableTransform { + private printer = ts.createPrinter(); + private importManager = new ImportManager(this.getUpdateRecorder, this.printer); + private partialEvaluator: PartialEvaluator; + + /** Set of provider class declarations which were already checked or migrated. */ + private visitedProviderClasses = new Set(); + + constructor( + private typeChecker: ts.TypeChecker, + private getUpdateRecorder: (sf: ts.SourceFile) => UpdateRecorder) { + this.partialEvaluator = + new PartialEvaluator(new TypeScriptReflectionHost(typeChecker), typeChecker); + } + + recordChanges() { this.importManager.recordChanges(); } + + /** Migrates a given NgModule by walking through the referenced providers. */ + migrateModule(module: ResolvedNgModule): AnalysisFailure[] { + if (module.providersExpr === null) { + return []; + } + + const evaluatedExpr = this.partialEvaluator.evaluate(module.providersExpr); + + if (!Array.isArray(evaluatedExpr)) { + return [{ + node: module.providersExpr, + message: 'Providers of module are not statically analyzable.' + }]; + } + + return this._visitProviderResolvedValue(evaluatedExpr, module); + } + + /** + * Migrates a given provider class if it is not decorated with + * any Angular decorator. + */ + migrateProviderClass(node: ts.ClassDeclaration, module: ResolvedNgModule) { + if (this.visitedProviderClasses.has(node)) { + return; + } + this.visitedProviderClasses.add(node); + + const sourceFile = node.getSourceFile(); + const ngDecorators = + node.decorators ? getAngularDecorators(this.typeChecker, node.decorators) : null; + + if (ngDecorators !== null && + ngDecorators.some(d => NO_MIGRATE_DECORATORS.indexOf(d.name) !== -1)) { + return; + } + + const updateRecorder = this.getUpdateRecorder(sourceFile); + const importExpr = + this.importManager.addImportToSourceFile(sourceFile, 'Injectable', '@angular/core'); + const newDecoratorExpr = ts.createDecorator(ts.createCall(importExpr, undefined, undefined)); + const newDecoratorText = + this.printer.printNode(ts.EmitHint.Unspecified, newDecoratorExpr, sourceFile); + + + // In case the class is already decorated with "@Inject(..)", we replace the "@Inject" + // decorator with "@Injectable()" since using "@Inject(..)" on a class is a noop and + // most likely was meant to be "@Injectable()". + const existingInjectDecorator = + ngDecorators !== null ? ngDecorators.find(d => d.name === 'Inject') : null; + if (existingInjectDecorator) { + updateRecorder.replaceDecorator(existingInjectDecorator.node, newDecoratorText, module.name); + } else { + updateRecorder.addClassDecorator(node, newDecoratorText, module.name); + } + } + + /** + * 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 + * migrate all referenced provider classes. e.g. "providers: [[A, [B]]]". + */ + private _visitProviderResolvedValue(value: ResolvedValue, module: ResolvedNgModule): + AnalysisFailure[] { + 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')) { + return []; + } + if (value.has('useExisting')) { + return this._visitProviderResolvedValue(value.get('useExisting') !, module); + } else if (value.has('useClass')) { + 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)), [ + ] as AnalysisFailure[]); + } else if (value instanceof DynamicValue) { + return [{node: value.node, message: `Provider is not statically analyzable.`}]; + } + return []; + } +} diff --git a/packages/core/schematics/migrations/missing-injectable/update_recorder.ts b/packages/core/schematics/migrations/missing-injectable/update_recorder.ts new file mode 100644 index 0000000000..cac8551020 --- /dev/null +++ b/packages/core/schematics/migrations/missing-injectable/update_recorder.ts @@ -0,0 +1,22 @@ +/** + * @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'; + +/** + * Update recorder interface that is used to transform source files in a non-colliding + * way. Also this indirection makes it possible to re-use logic for both TSLint rules + * and CLI devkit schematic updates. + */ +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; + commitUpdate(): void; +} diff --git a/packages/core/schematics/test/BUILD.bazel b/packages/core/schematics/test/BUILD.bazel index e63051f3a1..4699ef653e 100644 --- a/packages/core/schematics/test/BUILD.bazel +++ b/packages/core/schematics/test/BUILD.bazel @@ -11,6 +11,8 @@ ts_library( deps = [ "//packages/core/schematics/migrations/injectable-pipe", "//packages/core/schematics/migrations/injectable-pipe/google3", + "//packages/core/schematics/migrations/missing-injectable", + "//packages/core/schematics/migrations/missing-injectable/google3", "//packages/core/schematics/migrations/move-document", "//packages/core/schematics/migrations/renderer-to-renderer2", "//packages/core/schematics/migrations/renderer-to-renderer2/google3", diff --git a/packages/core/schematics/test/google3/missing_injectable_rule_spec.ts b/packages/core/schematics/test/google3/missing_injectable_rule_spec.ts new file mode 100644 index 0000000000..085710bf76 --- /dev/null +++ b/packages/core/schematics/test/google3/missing_injectable_rule_spec.ts @@ -0,0 +1,227 @@ +/** + * @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 {readFileSync, writeFileSync} from 'fs'; +import {dirname, join} from 'path'; +import * as shx from 'shelljs'; +import {Configuration, Linter} from 'tslint'; + +describe('Google3 missing injectable tslint rule', () => { + const rulesDirectory = dirname( + require.resolve('../../migrations/missing-injectable/google3/noMissingInjectableRule')); + + let tmpDir: string; + + beforeEach(() => { + tmpDir = join(process.env['TEST_TMPDIR'] !, 'google3-test'); + shx.mkdir('-p', tmpDir); + + writeFile('tsconfig.json', JSON.stringify({compilerOptions: {module: 'es2015'}})); + }); + + afterEach(() => shx.rm('-r', tmpDir)); + + function runTSLint(fix = true) { + const program = Linter.createProgram(join(tmpDir, 'tsconfig.json')); + const linter = new Linter({fix, rulesDirectory: [rulesDirectory]}, program); + const config = Configuration.parseConfigFile( + {rules: {'no-missing-injectable': true}, linterOptions: {typeCheck: true}}); + + program.getRootFileNames().forEach(fileName => { + linter.lint(fileName, program.getSourceFile(fileName) !.getFullText(), config); + }); + + return linter; + } + + function writeFile(fileName: string, content: string) { + writeFileSync(join(tmpDir, fileName), content); + } + + function getFile(fileName: string) { return readFileSync(join(tmpDir, fileName), 'utf8'); } + + it('should create proper failures for missing injectable providers', () => { + writeFile('index.ts', ` + import { NgModule } from '@angular/core'; + + export class A {} + + @NgModule({providers: [A]}) + export class AppModule {} + `); + + const linter = runTSLint(false); + const failures = linter.getResult().failures; + + expect(failures.length).toBe(2); + expect(failures[0].getFailure()) + .toMatch(/Class needs to be decorated with "@Injectable\(\)".*provided by "AppModule"/); + expect(failures[0].getStartPosition().getLineAndCharacter()).toEqual({line: 3, character: 6}); + expect(failures[1].getFailure()).toMatch(/Import needs to be updated to import.*Injectable/); + expect(failures[1].getStartPosition().getLineAndCharacter()).toEqual({line: 1, character: 13}); + }); + + it('should update provider classes which need to be migrated in Ivy', () => { + writeFile('/index.ts', ` + import {Pipe, Directive, Component, NgModule} from '@angular/core'; + + @Pipe() + export class WithPipe {} + + @Directive() + export class WithDirective {} + + @Component() + export class WithComponent {} + + export class MyServiceA {} + export class MyServiceB {} + export class MyServiceC {} + export class MyServiceD {} + export class MyServiceE {} + export class MyServiceF {} + export class MyServiceG {} + + @NgModule({providers: [ + WithPipe, + [ + WithDirective, + WithComponent, + MyServiceA, + ] + MyServiceB, + {provide: MyServiceC}, + {provide: null, useClass: MyServiceD}, + {provide: null, useExisting: MyServiceE}, + {provide: MyServiceF, useFactory: () => null}, + {provide: MyServiceG, useValue: null}, + ]}) + export class MyModule {} + `); + + + runTSLint(); + + expect(getFile('/index.ts')).toMatch(/'@angular\/core';\s+@Pipe\(\)\s+export class WithPipe/); + expect(getFile('/index.ts')) + .toMatch(/WithPipe {}\s+@Directive\(\)\s+export class WithDirective/); + expect(getFile('/index.ts')) + .toMatch(/WithDirective {}\s+@Component\(\)\s+export class WithComponent/); + expect(getFile('/index.ts')).toMatch(/@Injectable\(\)\s+export class MyServiceA/); + expect(getFile('/index.ts')).toMatch(/@Injectable\(\)\s+export class MyServiceB/); + expect(getFile('/index.ts')).toMatch(/@Injectable\(\)\s+export class MyServiceC/); + expect(getFile('/index.ts')).toMatch(/@Injectable\(\)\s+export class MyServiceD/); + expect(getFile('/index.ts')).toMatch(/@Injectable\(\)\s+export class MyServiceE/); + expect(getFile('/index.ts')).toMatch(/MyServiceE {}\s+export class MyServiceF/); + expect(getFile('/index.ts')).toMatch(/MyServiceF {}\s+export class MyServiceG/); + }); + + it('should migrate provider once if referenced in multiple NgModule definitions', () => { + writeFile('/index.ts', ` + import {NgModule} from '@angular/core'; + + export class ServiceA {} + + @NgModule({providers: [ServiceA]}) + export class MyModule {} + `); + + writeFile('/second.ts', ` + import {NgModule} from '@angular/core'; + import {ServiceA} from './index'; + + export class ServiceB {} + + @NgModule({providers: [ServiceA, ServiceB]}) + export class SecondModule {} + `); + + runTSLint(); + + expect(getFile('/index.ts')) + .toMatch(/@angular\/core';\s+@Injectable\(\)\s+export class ServiceA/); + expect(getFile('/index.ts')).toMatch(/{ NgModule, Injectable } from '@angular\/core/); + expect(getFile('/second.ts')).toMatch(/@Injectable\(\)\s+export class ServiceB/); + expect(getFile('/second.ts')).toMatch(/{ NgModule, Injectable } from '@angular\/core/); + }); + + it('should warn if a referenced provider could not be resolved', () => { + writeFile('/index.ts', ` + import {NgModule} from '@angular/core'; + + @NgModule({providers: [NotPresent]}) + export class MyModule {} + `); + + const linter = runTSLint(); + const failures = linter.getResult().failures; + + expect(failures.length).toBe(1); + expect(failures[0].getFailure()).toMatch(/Provider is not statically analyzable./); + expect(failures[0].getStartPosition().getLineAndCharacter()).toEqual({line: 3, character: 29}); + }); + + it('should warn if the module providers could not be resolved', () => { + writeFile('/index.ts', ` + import {NgModule} from '@angular/core'; + + @NgModule({providers: NOT_ANALYZABLE) + export class MyModule {} + `); + + const linter = runTSLint(); + const failures = linter.getResult().failures; + + expect(failures.length).toBe(1); + expect(failures[0].getFailure()).toMatch(/Providers of module.*not statically analyzable./); + expect(failures[0].getStartPosition().getLineAndCharacter()).toEqual({line: 3, character: 28}); + }); + + it('should create new import for @Injectable when migrating provider', () => { + writeFile('/index.ts', ` + import {NgModule} from '@angular/core'; + import {MyService, MySecondService} from './service'; + + @NgModule({providers: [MyService, MySecondService]}) + export class MyModule {} + `); + + writeFile('/service.ts', `export class MyService {} + + export class MySecondService {} + `); + + runTSLint(); + + expect(getFile('/service.ts')).toMatch(/@Injectable\(\)\s+export class MyService/); + expect(getFile('/service.ts')).toMatch(/@Injectable\(\)\s+export class MySecondService/); + expect(getFile('/service.ts')).toMatch(/import { Injectable } from "@angular\/core";/); + }); + + it('should remove @Inject decorator for providers which are migrated', () => { + writeFile('/index.ts', ` + import {NgModule} from '@angular/core'; + import {MyService} from './service'; + + @NgModule({providers: [MyService]}) + export class MyModule {} + `); + + writeFile('/service.ts', ` + import {Inject} from '@angular/core'; + + @Inject() + export class MyService {} + `); + + runTSLint(); + + expect(getFile('/service.ts')).toMatch(/core';\s+@Injectable\(\)\s+export class MyService/); + expect(getFile('/service.ts')).toMatch(/import { Inject, Injectable } from '@angular\/core';/); + }); +}); diff --git a/packages/core/schematics/test/missing_injectable_migration_spec.ts b/packages/core/schematics/test/missing_injectable_migration_spec.ts new file mode 100644 index 0000000000..d53e7d562e --- /dev/null +++ b/packages/core/schematics/test/missing_injectable_migration_spec.ts @@ -0,0 +1,580 @@ +/** + * @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('Missing injectable migration', () => { + let runner: SchematicTestRunner; + let host: TempScopedNodeJsSyncHost; + let tree: UnitTestTree; + let tmpDirPath: string; + let previousWorkingDir: string; + let warnOutput: string[]; + + beforeEach(() => { + runner = new SchematicTestRunner('test', require.resolve('./test-migrations.json')); + host = new TempScopedNodeJsSyncHost(); + tree = new UnitTestTree(new HostTree(host)); + + writeFile('/tsconfig.json', JSON.stringify({ + compilerOptions: { + experimentalDecorators: true, + lib: ['es2015'], + } + })); + writeFile('/angular.json', JSON.stringify({ + projects: {t: {architect: {build: {options: {tsConfig: './tsconfig.json'}}}}} + })); + + warnOutput = []; + runner.logger.subscribe(logEntry => { + if (logEntry.level === 'warn') { + warnOutput.push(logEntry.message); + } + }); + + 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); + }); + + function writeFile(filePath: string, contents: string) { + host.sync.write(normalize(filePath), virtualFs.stringToFileBuffer(contents)); + } + + async function runMigration() { + await runner.runSchematicAsync('migration-missing-injectable', {}, tree).toPromise(); + } + + it('should migrate type provider in NgModule', async() => { + writeFile('/index.ts', ` + import {NgModule} from '@angular/core'; + + export class MyService {} + + @NgModule({providers: [MyService]}) + export class MyModule {} + `); + + await runMigration(); + + expect(warnOutput.length).toBe(0); + expect(tree.readContent('/index.ts')).toMatch(/@Injectable\(\)\s+export class MyService/); + expect(tree.readContent('/index.ts')).toMatch(/{ NgModule, Injectable } from '@angular\/core/); + }); + + it('should migrate object literal provider in NgModule', async() => { + writeFile('/index.ts', ` + import {NgModule} from '@angular/core'; + + export class MyService {} + + @NgModule({providers: [{provide: MyService}]}) + export class MyModule {} + `); + + await runMigration(); + + expect(warnOutput.length).toBe(0); + expect(tree.readContent('/index.ts')).toMatch(/@Injectable\(\)\s+export class MyService/); + expect(tree.readContent('/index.ts')).toMatch(/{ NgModule, Injectable } from '@angular\/core/); + }); + + it('should not migrate object literal provider with "useValue" in NgModule', async() => { + writeFile('/index.ts', ` + import {NgModule} from '@angular/core'; + + export class MyService {} + + @NgModule({providers: [{provide: MyService, useValue: null }]}) + export class MyModule {} + `); + + await runMigration(); + + expect(warnOutput.length).toBe(0); + expect(tree.readContent('/index.ts')).not.toContain('@Injectable'); + }); + + it('should not migrate object literal provider with "useFactory" in NgModule', async() => { + writeFile('/index.ts', ` + import {NgModule} from '@angular/core'; + + export class MyService {} + + @NgModule({providers: [{provide: MyService, useFactory: () => null }]}) + export class MyModule {} + `); + + await runMigration(); + + expect(warnOutput.length).toBe(0); + expect(tree.readContent('/index.ts')).not.toContain('@Injectable'); + }); + + it('should migrate object literal provider with "useExisting" in NgModule', async() => { + writeFile('/index.ts', ` + import {NgModule} from '@angular/core'; + + export class MyService {} + export class MyToken {} + + @NgModule({providers: [{provide: MyToken, useExisting: MyService}]}) + export class MyModule {} + `); + + await runMigration(); + + expect(warnOutput.length).toBe(0); + expect(tree.readContent('/index.ts')).toMatch(/@Injectable\(\)\s+export class MyService/); + expect(tree.readContent('/index.ts')).toMatch(/MyService {}\s+export class MyToken/); + expect(tree.readContent('/index.ts')).toMatch(/{ NgModule, Injectable } from '@angular\/core/); + }); + + it('should migrate object literal provider with "useClass" in NgModule', async() => { + writeFile('/index.ts', ` + import {NgModule} from '@angular/core'; + + export class MyService {} + export class MyToken {} + + @NgModule({providers: [{provide: MyToken, useClass: MyService}]}) + export class MyModule {} + `); + + await runMigration(); + + expect(warnOutput.length).toBe(0); + expect(tree.readContent('/index.ts')).toMatch(/@Injectable\(\)\s+export class MyService/); + expect(tree.readContent('/index.ts')).toMatch(/MyService {}\s+export class MyToken/); + expect(tree.readContent('/index.ts')).toMatch(/{ NgModule, Injectable } from '@angular\/core/); + }); + + it('should not migrate provider which is already decorated with @Injectable', async() => { + writeFile('/index.ts', ` + import {Injectable, NgModule} from '@angular/core'; + + @Injectable() + export class MyService {} + + @NgModule({providers: [MyService]}) + export class MyModule {} + `); + + await runMigration(); + + expect(warnOutput.length).toBe(0); + expect(tree.readContent('/index.ts')) + .toMatch(/@angular\/core';\s+@Injectable\(\)\s+export class MyService/); + }); + + it('should not migrate provider which is already decorated with @Directive', async() => { + writeFile('/index.ts', ` + import {Directive, NgModule} from '@angular/core'; + + @Directive() + export class MyService {} + + @NgModule({providers: [MyService]}) + export class MyModule {} + `); + + await runMigration(); + + expect(warnOutput.length).toBe(0); + expect(tree.readContent('/index.ts')).not.toContain('@Injectable'); + }); + + it('should not migrate provider which is already decorated with @Component', async() => { + writeFile('/index.ts', ` + import {Component, NgModule} from '@angular/core'; + + @Component() + export class MyService {} + + @NgModule({providers: [MyService]}) + export class MyModule {} + `); + + await runMigration(); + + expect(warnOutput.length).toBe(0); + expect(tree.readContent('/index.ts')).not.toContain('@Injectable'); + }); + + it('should not migrate provider which is already decorated with @Pipe', async() => { + writeFile('/index.ts', ` + import {Pipe, NgModule} from '@angular/core'; + + @Pipe() + export class MyService {} + + @NgModule({providers: [MyService]}) + export class MyModule {} + `); + + await runMigration(); + + expect(warnOutput.length).toBe(0); + expect(tree.readContent('/index.ts')).not.toContain('@Injectable'); + }); + + it('should migrate multiple providers in same NgModule', async() => { + writeFile('/index.ts', ` + import {NgModule} from '@angular/core'; + + export class ServiceA {} + export class ServiceB {} + + @NgModule({providers: [ServiceA, ServiceB]}) + export class MyModule {} + `); + + await runMigration(); + + expect(warnOutput.length).toBe(0); + expect(tree.readContent('/index.ts')).toMatch(/@Injectable\(\)\s+export class ServiceA/); + expect(tree.readContent('/index.ts')).toMatch(/@Injectable\(\)\s+export class ServiceB/); + expect(tree.readContent('/index.ts')).toMatch(/{ NgModule, Injectable } from '@angular\/core/); + }); + + it('should migrate multiple mixed providers in same NgModule', async() => { + writeFile('/index.ts', ` + import {NgModule} from '@angular/core'; + + export class ServiceA {} + export class ServiceB {} + export class ServiceC {} + + @NgModule({ + providers: [ + ServiceA, + {provide: ServiceB}, + {provide: SomeToken, useClass: ServiceC}, + ]}) + export class MyModule {} + `); + + await runMigration(); + + expect(warnOutput.length).toBe(0); + expect(tree.readContent('/index.ts')).toMatch(/@Injectable\(\)\s+export class ServiceA/); + expect(tree.readContent('/index.ts')).toMatch(/@Injectable\(\)\s+export class ServiceB/); + expect(tree.readContent('/index.ts')).toMatch(/@Injectable\(\)\s+export class ServiceC/); + expect(tree.readContent('/index.ts')).toMatch(/{ NgModule, Injectable } from '@angular\/core/); + }); + + + it('should migrate multiple nested providers in same NgModule', async() => { + writeFile('/index.ts', ` + import {NgModule} from '@angular/core'; + + export class ServiceA {} + export class ServiceB {} + export class ServiceC {} + + @NgModule({ + providers: [ + ServiceA, + [ + {provide: ServiceB}, + ServiceC, + ], + ]}) + export class MyModule {} + `); + + await runMigration(); + + expect(warnOutput.length).toBe(0); + expect(tree.readContent('/index.ts')).toMatch(/@Injectable\(\)\s+export class ServiceA/); + expect(tree.readContent('/index.ts')).toMatch(/@Injectable\(\)\s+export class ServiceB/); + expect(tree.readContent('/index.ts')).toMatch(/@Injectable\(\)\s+export class ServiceC/); + expect(tree.readContent('/index.ts')).toMatch(/{ NgModule, Injectable } from '@angular\/core/); + }); + + it('should migrate providers referenced through identifier', async() => { + writeFile('/index.ts', ` + import {NgModule} from '@angular/core'; + + export class ServiceA {} + export class ServiceB {} + + const PROVIDERS = [ServiceA, ServiceB]; + + @NgModule({ + providers: PROVIDERS, + }) + export class MyModule {} + `); + + await runMigration(); + + expect(warnOutput.length).toBe(0); + expect(tree.readContent('/index.ts')).toMatch(/@Injectable\(\)\s+export class ServiceA/); + expect(tree.readContent('/index.ts')).toMatch(/@Injectable\(\)\s+export class ServiceB/); + expect(tree.readContent('/index.ts')).toMatch(/{ NgModule, Injectable } from '@angular\/core/); + }); + + it('should migrate providers created through static analyzable function call', async() => { + writeFile('/index.ts', ` + import {NgModule} from '@angular/core'; + + export class ServiceA {} + export class ServiceB {} + + export function createProviders(x: any) { + return [ServiceA, x] + } + + @NgModule({ + providers: createProviders(ServiceB), + }) + export class MyModule {} + `); + + await runMigration(); + + expect(warnOutput.length).toBe(0); + expect(tree.readContent('/index.ts')).toMatch(/@Injectable\(\)\s+export class ServiceA/); + expect(tree.readContent('/index.ts')).toMatch(/@Injectable\(\)\s+export class ServiceB/); + expect(tree.readContent('/index.ts')).toMatch(/{ NgModule, Injectable } from '@angular\/core/); + }); + + it('should migrate providers which are computed through spread operator', async() => { + writeFile('/index.ts', ` + import {NgModule} from '@angular/core'; + + export class ServiceA {} + export class ServiceB {} + + const otherServices = [ServiceB]; + + @NgModule({ + providers: [ServiceA, ...otherServices], + }) + export class MyModule {} + `); + + await runMigration(); + + expect(warnOutput.length).toBe(0); + expect(tree.readContent('/index.ts')).toMatch(/@Injectable\(\)\s+export class ServiceA/); + expect(tree.readContent('/index.ts')).toMatch(/@Injectable\(\)\s+export class ServiceB/); + expect(tree.readContent('/index.ts')).toMatch(/{ NgModule, Injectable } from '@angular\/core/); + }); + + it('should migrate provider once if referenced in multiple NgModule definitions', async() => { + writeFile('/index.ts', ` + import {NgModule} from '@angular/core'; + + export class ServiceA {} + + @NgModule({providers: [ServiceA]}) + export class MyModule {} + `); + + writeFile('/second.ts', ` + import {NgModule} from '@angular/core'; + import {ServiceA} from './index'; + + export class ServiceB {} + + @NgModule({providers: [ServiceA, ServiceB]}) + export class SecondModule {} + `); + + await runMigration(); + + expect(warnOutput.length).toBe(0); + expect(tree.readContent('/index.ts')) + .toMatch(/@angular\/core';\s+@Injectable\(\)\s+export class ServiceA/); + expect(tree.readContent('/index.ts')).toMatch(/{ NgModule, Injectable } from '@angular\/core/); + expect(tree.readContent('/second.ts')).toMatch(/@Injectable\(\)\s+export class ServiceB/); + expect(tree.readContent('/second.ts')).toMatch(/{ NgModule, Injectable } from '@angular\/core/); + }); + + it('should create new import for @Injectable when migrating provider', async() => { + writeFile('/index.ts', ` + import {NgModule} from '@angular/core'; + import {MyService, MySecondService} from './service'; + + @NgModule({providers: [MyService, MySecondService]}) + export class MyModule {} + `); + + writeFile('/service.ts', `export class MyService {} + + export class MySecondService {} + `); + + await runMigration(); + + expect(warnOutput.length).toBe(0); + expect(tree.readContent('/service.ts')).toMatch(/@Injectable\(\)\s+export class MyService/); + expect(tree.readContent('/service.ts')) + .toMatch(/@Injectable\(\)\s+export class MySecondService/); + expect(tree.readContent('/service.ts')).toMatch(/import { Injectable } from "@angular\/core";/); + }); + + it('should re-use existing namespace import for importing @Injectable when migrating provider', + async() => { + writeFile('/index.ts', ` + import * as core from '@angular/core'; + + export class MyService { + constructor() { + console.log(core.isDevMode()); + } + } + `); + + writeFile('/app.module.ts', ` + import {NgModule} from '@angular/core'; + import {MyService} from './index'; + + @NgModule({providers: [MyService]}) + export class MyModule {} + `); + + await runMigration(); + + expect(warnOutput.length).toBe(0); + expect(tree.readContent('/index.ts')) + .toMatch(/@core.Injectable\(\)\s+export class MyService/); + }); + + it('should warn if a referenced provider could not be resolved', async() => { + writeFile('/index.ts', ` + import {NgModule} from '@angular/core'; + + @NgModule({providers: [NotPresent]}) + export class MyModule {} + `); + + await runMigration(); + + expect(warnOutput.length).toBe(1); + expect(warnOutput[0]).toMatch(/\s+index\.ts@4:30: Provider is not statically analyzable./); + }); + + it('should warn if the module providers could not be resolved', async() => { + writeFile('/index.ts', ` + import {NgModule} from '@angular/core'; + + @NgModule({providers: NOT_ANALYZABLE) + export class MyModule {} + `); + + await runMigration(); + + expect(warnOutput.length).toBe(1); + expect(warnOutput[0]) + .toMatch(/\s+index\.ts@4:29: Providers of module.*not statically analyzable./); + }); + + it('should not throw if an empty @NgModule is analyzed', async() => { + writeFile('/index.ts', ` + import {NgModule} from '@angular/core'; + + @NgModule() + export class MyModule {} + `); + + try { + await runMigration(); + } catch (e) { + fail(e); + } + + expect(warnOutput.length).toBe(0); + }); + + it('should create new import for injectable after full end of last import statement', async() => { + writeFile('/index.ts', ` + import {NgModule} from '@angular/core'; + import {MyService} from './service'; + + @NgModule({providers: [MyService]}) + export class MyModule {} + `); + + writeFile('/service.ts', ` + import * as a from 'a'; + import * as a from 'b'; // some comment + + export class MyService {} + `); + + await runMigration(); + + expect(warnOutput.length).toBe(0); + expect(tree.readContent('/service.ts')).toMatch(/@Injectable\(\)\s+export class MyService/); + expect(tree.readContent('/service.ts')) + .toMatch(/'b'; \/\/ some comment\s+import { Injectable } from "@angular\/core";/); + }); + + it('should create new import at source file start with trailing new-line', async() => { + writeFile('/index.ts', ` + import {NgModule} from '@angular/core'; + import {MyService} from './service'; + + @NgModule({providers: [MyService]}) + export class MyModule {} + `); + + writeFile('/service.ts', `/* @license */ + export class MyService {} + `); + + await runMigration(); + + expect(warnOutput.length).toBe(0); + expect(tree.readContent('/service.ts')) + .toMatch( + /^import { Injectable } from "@angular\/core";\s+\/\* @license \*\/\s+@Injectable\(\)\s+export class MyService/); + }); + + it('should remove @Inject decorator for providers which are migrated', async() => { + writeFile('/index.ts', ` + import {NgModule} from '@angular/core'; + import {MyService} from './service'; + + @NgModule({providers: [MyService]}) + export class MyModule {} + `); + + writeFile('/service.ts', ` + import {Inject} from '@angular/core'; + + @Inject() + export class MyService {} + `); + + await runMigration(); + + expect(warnOutput.length).toBe(0); + expect(tree.readContent('/service.ts')) + .toMatch(/core';\s+@Injectable\(\)\s+export class MyService/); + expect(tree.readContent('/service.ts')) + .toMatch(/import { Inject, Injectable } from '@angular\/core';/); + }); + +}); diff --git a/packages/core/schematics/test/test-migrations.json b/packages/core/schematics/test/test-migrations.json index cdb97750a4..59f354d7e3 100644 --- a/packages/core/schematics/test/test-migrations.json +++ b/packages/core/schematics/test/test-migrations.json @@ -5,6 +5,10 @@ "migration-injectable-pipe": { "description": "Migrates all Pipe classes so that they have an Injectable annotation", "factory": "../migrations/injectable-pipe/index" + }, + "migration-missing-injectable": { + "description": "Migrates all declared undecorated providers with the @Injectable decorator", + "factory": "../migrations/missing-injectable/index" } } }