From 7849fdde09be38b09c939a4ac033861daf52ca4e Mon Sep 17 00:00:00 2001 From: Kristiyan Kostadinov Date: Sat, 12 Sep 2020 14:33:06 +0200 Subject: [PATCH] feat(router): add migration to update calls to navigateByUrl and createUrlTree with invalid parameters (#38825) In #38227 the signatures of `navigateByUrl` and `createUrlTree` were updated to exclude unsupported properties from their `extras` parameter. This migration looks for the relevant method calls that pass in an `extras` parameter and drops the unsupported properties. **Before:** ``` this._router.navigateByUrl('/', {skipLocationChange: false, fragment: 'foo'}); ``` **After:** ``` this._router.navigateByUrl('/', { /* Removed unsupported properties by Angular migration: fragment. */ skipLocationChange: false }); ``` These changes also move the method call detection logic out of the `Renderer2` migration and into a common place so that it can be reused in other migrations. PR Close #38825 --- packages/core/schematics/BUILD.bazel | 1 + packages/core/schematics/migrations.json | 5 + .../schematics/migrations/google3/BUILD.bazel | 1 + .../google3/navigationExtrasOmissionsRule.ts | 38 ++ .../google3/rendererToRenderer2Rule.ts | 19 +- .../navigation-extras-omissions/BUILD.bazel | 18 + .../navigation-extras-omissions/README.md | 35 ++ .../navigation-extras-omissions/index.ts | 61 ++++ .../navigation-extras-omissions/util.ts | 125 +++++++ .../migrations/renderer-to-renderer2/index.ts | 15 +- .../renderer-to-renderer2/migration.ts | 19 +- .../migrations/renderer-to-renderer2/util.ts | 68 +--- packages/core/schematics/test/BUILD.bazel | 1 + .../navigation_extras_omissions_spec.ts | 334 ++++++++++++++++++ .../google3/renderer_to_renderer2_spec.ts | 20 ++ ...igation_extras_omissions_migration_spec.ts | 302 ++++++++++++++++ .../renderer_to_renderer2_migration_spec.ts | 15 + .../schematics/utils/typescript/imports.ts | 40 +++ .../schematics/utils/typescript/symbol.ts | 9 + 19 files changed, 1053 insertions(+), 73 deletions(-) create mode 100644 packages/core/schematics/migrations/google3/navigationExtrasOmissionsRule.ts create mode 100644 packages/core/schematics/migrations/navigation-extras-omissions/BUILD.bazel create mode 100644 packages/core/schematics/migrations/navigation-extras-omissions/README.md create mode 100644 packages/core/schematics/migrations/navigation-extras-omissions/index.ts create mode 100644 packages/core/schematics/migrations/navigation-extras-omissions/util.ts create mode 100644 packages/core/schematics/test/google3/navigation_extras_omissions_spec.ts create mode 100644 packages/core/schematics/test/navigation_extras_omissions_migration_spec.ts diff --git a/packages/core/schematics/BUILD.bazel b/packages/core/schematics/BUILD.bazel index 3630353908..4595724e09 100644 --- a/packages/core/schematics/BUILD.bazel +++ b/packages/core/schematics/BUILD.bazel @@ -14,6 +14,7 @@ pkg_npm( "//packages/core/schematics/migrations/missing-injectable", "//packages/core/schematics/migrations/module-with-providers", "//packages/core/schematics/migrations/move-document", + "//packages/core/schematics/migrations/navigation-extras-omissions", "//packages/core/schematics/migrations/renderer-to-renderer2", "//packages/core/schematics/migrations/static-queries", "//packages/core/schematics/migrations/template-var-assignment", diff --git a/packages/core/schematics/migrations.json b/packages/core/schematics/migrations.json index e4a15c2d0c..ce899486eb 100644 --- a/packages/core/schematics/migrations.json +++ b/packages/core/schematics/migrations.json @@ -44,6 +44,11 @@ "version": "10.0.0-beta", "description": "Undecorated classes with Angular features migration. In version 10, classes that use Angular features and do not have an Angular decorator are no longer supported. Read more about this here: https://v10.angular.io/guide/migration-undecorated-classes", "factory": "./migrations/undecorated-classes-with-decorated-fields/index" + }, + "migration-v11-navigation-extras-omissions": { + "version": "11.0.0-beta", + "description": "NavigationExtras omissions migration. In version 11, some unsupported properties were omitted from the `extras` parameter of the `Router.navigateByUrl` and `Router.createUrlTree` methods.", + "factory": "./migrations/navigation-extras-omissions/index" } } } diff --git a/packages/core/schematics/migrations/google3/BUILD.bazel b/packages/core/schematics/migrations/google3/BUILD.bazel index 1a68dcf1d0..6093886f1d 100644 --- a/packages/core/schematics/migrations/google3/BUILD.bazel +++ b/packages/core/schematics/migrations/google3/BUILD.bazel @@ -9,6 +9,7 @@ ts_library( "//packages/core/schematics/migrations/dynamic-queries", "//packages/core/schematics/migrations/missing-injectable", "//packages/core/schematics/migrations/missing-injectable/google3", + "//packages/core/schematics/migrations/navigation-extras-omissions", "//packages/core/schematics/migrations/renderer-to-renderer2", "//packages/core/schematics/migrations/static-queries", "//packages/core/schematics/migrations/template-var-assignment", diff --git a/packages/core/schematics/migrations/google3/navigationExtrasOmissionsRule.ts b/packages/core/schematics/migrations/google3/navigationExtrasOmissionsRule.ts new file mode 100644 index 0000000000..7b3492b0e7 --- /dev/null +++ b/packages/core/schematics/migrations/google3/navigationExtrasOmissionsRule.ts @@ -0,0 +1,38 @@ +/** + * @license + * Copyright Google LLC 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, Rules} from 'tslint'; +import * as ts from 'typescript'; +import {findLiteralsToMigrate, migrateLiteral} from '../../migrations/navigation-extras-omissions/util'; + + +/** TSLint rule that migrates `navigateByUrl` and `createUrlTree` calls to an updated signature. */ +export class Rule extends Rules.TypedRule { + applyWithProgram(sourceFile: ts.SourceFile, program: ts.Program): RuleFailure[] { + const failures: RuleFailure[] = []; + const typeChecker = program.getTypeChecker(); + const printer = ts.createPrinter(); + const literalsToMigrate = findLiteralsToMigrate(sourceFile, typeChecker); + + literalsToMigrate.forEach((instances, methodName) => instances.forEach(instance => { + const migratedNode = migrateLiteral(methodName, instance); + + if (migratedNode !== instance) { + failures.push(new RuleFailure( + sourceFile, instance.getStart(), instance.getEnd(), + 'Object used in navigateByUrl or createUrlTree call contains unsupported properties.', + this.ruleName, + new Replacement( + instance.getStart(), instance.getWidth(), + printer.printNode(ts.EmitHint.Unspecified, migratedNode, sourceFile)))); + } + })); + + return failures; + } +} diff --git a/packages/core/schematics/migrations/google3/rendererToRenderer2Rule.ts b/packages/core/schematics/migrations/google3/rendererToRenderer2Rule.ts index 161e251afb..4ec020fbf5 100644 --- a/packages/core/schematics/migrations/google3/rendererToRenderer2Rule.ts +++ b/packages/core/schematics/migrations/google3/rendererToRenderer2Rule.ts @@ -8,10 +8,11 @@ import {Replacement, RuleFailure, Rules} from 'tslint'; import * as ts from 'typescript'; +import {getImportSpecifier} from '../../utils/typescript/imports'; import {getHelper, HelperFunction} from '../renderer-to-renderer2/helpers'; import {migrateExpression, replaceImport} from '../renderer-to-renderer2/migration'; -import {findCoreImport, findRendererReferences} from '../renderer-to-renderer2/util'; +import {findRendererReferences, getNamedImports} from '../renderer-to-renderer2/util'; /** * TSLint rule that migrates from `Renderer` to `Renderer2`. More information on how it works: @@ -22,18 +23,21 @@ export class Rule extends Rules.TypedRule { const typeChecker = program.getTypeChecker(); const printer = ts.createPrinter(); const failures: RuleFailure[] = []; - const rendererImport = findCoreImport(sourceFile, 'Renderer'); + const rendererImportSpecifier = getImportSpecifier(sourceFile, '@angular/core', 'Renderer'); + const rendererImport = + rendererImportSpecifier ? getNamedImports(rendererImportSpecifier) : null; // If there are no imports for the `Renderer`, we can exit early. - if (!rendererImport) { + if (!rendererImportSpecifier || !rendererImport) { return failures; } const {typedNodes, methodCalls, forwardRefs} = - findRendererReferences(sourceFile, typeChecker, rendererImport); + findRendererReferences(sourceFile, typeChecker, rendererImportSpecifier); const helpersToAdd = new Set(); - failures.push(this._getNamedImportsFailure(rendererImport, sourceFile, printer)); + failures.push( + this._getNamedImportsFailure(rendererImport, rendererImportSpecifier, sourceFile, printer)); typedNodes.forEach(node => failures.push(this._getTypedNodeFailure(node, sourceFile))); forwardRefs.forEach(node => failures.push(this._getIdentifierNodeFailure(node, sourceFile))); @@ -61,9 +65,10 @@ export class Rule extends Rules.TypedRule { /** Gets a failure for an import of the Renderer. */ private _getNamedImportsFailure( - node: ts.NamedImports, sourceFile: ts.SourceFile, printer: ts.Printer): RuleFailure { + node: ts.NamedImports, importSpecifier: ts.ImportSpecifier, sourceFile: ts.SourceFile, + printer: ts.Printer): RuleFailure { const replacementText = printer.printNode( - ts.EmitHint.Unspecified, replaceImport(node, 'Renderer', 'Renderer2'), sourceFile); + ts.EmitHint.Unspecified, replaceImport(node, importSpecifier, 'Renderer2'), sourceFile); return new RuleFailure( sourceFile, node.getStart(), node.getEnd(), diff --git a/packages/core/schematics/migrations/navigation-extras-omissions/BUILD.bazel b/packages/core/schematics/migrations/navigation-extras-omissions/BUILD.bazel new file mode 100644 index 0000000000..dc793c9702 --- /dev/null +++ b/packages/core/schematics/migrations/navigation-extras-omissions/BUILD.bazel @@ -0,0 +1,18 @@ +load("//tools:defaults.bzl", "ts_library") + +ts_library( + name = "navigation-extras-omissions", + srcs = glob(["**/*.ts"]), + tsconfig = "//packages/core/schematics:tsconfig.json", + visibility = [ + "//packages/core/schematics:__pkg__", + "//packages/core/schematics/migrations/google3:__pkg__", + "//packages/core/schematics/test:__pkg__", + ], + deps = [ + "//packages/core/schematics/utils", + "@npm//@angular-devkit/schematics", + "@npm//@types/node", + "@npm//typescript", + ], +) diff --git a/packages/core/schematics/migrations/navigation-extras-omissions/README.md b/packages/core/schematics/migrations/navigation-extras-omissions/README.md new file mode 100644 index 0000000000..e8236f03ad --- /dev/null +++ b/packages/core/schematics/migrations/navigation-extras-omissions/README.md @@ -0,0 +1,35 @@ +## Router.navigateByUrl and Router.createUrlTree extras migration + +Previously the `extras` parameter of `Router.navigateByUrl` and `Router.createUrlTree` accepted the +full `NavigationExtras` object, even though only a subset of properties was supported. This +migration removes the unsupported properties from the relevant method call sites. + +#### Before +```ts +import { Component } from '@angular/core'; +import { Router } from '@angular/router'; + +@Component({}) +export class MyComponent { + constructor(private _router: Router) {} + + goHome() { + this._router.navigateByUrl('/', {skipLocationChange: false, fragment: 'foo'}); + } +} +``` + +#### After +```ts +import { Component } from '@angular/core'; +import { Router } from '@angular/router'; + +@Component({}) +export class MyComponent { + constructor(private _router: Router) {} + + goHome() { + this._router.navigateByUrl('/', { /* Removed unsupported properties by Angular migration: fragment. */ skipLocationChange: false }); + } +} +``` diff --git a/packages/core/schematics/migrations/navigation-extras-omissions/index.ts b/packages/core/schematics/migrations/navigation-extras-omissions/index.ts new file mode 100644 index 0000000000..c8e7fa8032 --- /dev/null +++ b/packages/core/schematics/migrations/navigation-extras-omissions/index.ts @@ -0,0 +1,61 @@ +/** + * @license + * Copyright Google LLC 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, SchematicsException, Tree} from '@angular-devkit/schematics'; +import {relative} from 'path'; +import * as ts from 'typescript'; + +import {getProjectTsConfigPaths} from '../../utils/project_tsconfig_paths'; +import {createMigrationProgram} from '../../utils/typescript/compiler_host'; +import {findLiteralsToMigrate, migrateLiteral} from './util'; + + +/** Migration that switches `Router.navigateByUrl` and `Router.createUrlTree` to a new signature. */ +export default function(): Rule { + return (tree: Tree) => { + const {buildPaths, testPaths} = getProjectTsConfigPaths(tree); + const basePath = process.cwd(); + const allPaths = [...buildPaths, ...testPaths]; + + if (!allPaths.length) { + throw new SchematicsException( + 'Could not find any tsconfig file. Cannot migrate ' + + 'Router.navigateByUrl and Router.createUrlTree calls.'); + } + + for (const tsconfigPath of allPaths) { + runNavigationExtrasOmissionsMigration(tree, tsconfigPath, basePath); + } + }; +} + +function runNavigationExtrasOmissionsMigration(tree: Tree, tsconfigPath: string, basePath: string) { + const {program} = createMigrationProgram(tree, tsconfigPath, basePath); + const typeChecker = program.getTypeChecker(); + const printer = ts.createPrinter(); + const sourceFiles = program.getSourceFiles().filter( + f => !f.isDeclarationFile && !program.isSourceFileFromExternalLibrary(f)); + + sourceFiles.forEach(sourceFile => { + const literalsToMigrate = findLiteralsToMigrate(sourceFile, typeChecker); + const update = tree.beginUpdate(relative(basePath, sourceFile.fileName)); + + literalsToMigrate.forEach((instances, methodName) => instances.forEach(instance => { + const migratedNode = migrateLiteral(methodName, instance); + + if (migratedNode !== instance) { + update.remove(instance.getStart(), instance.getWidth()); + update.insertRight( + instance.getStart(), + printer.printNode(ts.EmitHint.Unspecified, migratedNode, sourceFile)); + } + })); + + tree.commitUpdate(update); + }); +} diff --git a/packages/core/schematics/migrations/navigation-extras-omissions/util.ts b/packages/core/schematics/migrations/navigation-extras-omissions/util.ts new file mode 100644 index 0000000000..cdf8c3c4c9 --- /dev/null +++ b/packages/core/schematics/migrations/navigation-extras-omissions/util.ts @@ -0,0 +1,125 @@ +/** + * @license + * Copyright Google LLC 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 {getImportSpecifier} from '../../utils/typescript/imports'; +import {isReferenceToImport} from '../../utils/typescript/symbol'; + +/** + * Configures the methods that the migration should be looking for + * and the properties from `NavigationExtras` that should be preserved. + */ +const methodConfig = new Map>([ + ['navigateByUrl', new Set(['skipLocationChange', 'replaceUrl', 'state'])], + [ + 'createUrlTree', new Set([ + 'relativeTo', 'queryParams', 'fragment', 'preserveQueryParams', 'queryParamsHandling', + 'preserveFragment' + ]) + ] +]); + +export function migrateLiteral( + methodName: string, node: ts.ObjectLiteralExpression): ts.ObjectLiteralExpression { + const allowedProperties = methodConfig.get(methodName); + + if (!allowedProperties) { + throw Error(`Attempting to migrate unconfigured method called ${methodName}.`); + } + + const propertiesToKeep: ts.ObjectLiteralElementLike[] = []; + const removedPropertyNames: string[] = []; + + node.properties.forEach(property => { + // Only look for regular and shorthand property assignments since resolving things + // like spread operators becomes too complicated for this migration. + if ((ts.isPropertyAssignment(property) || ts.isShorthandPropertyAssignment(property)) && + (ts.isStringLiteralLike(property.name) || ts.isNumericLiteral(property.name) || + ts.isIdentifier(property.name))) { + if (allowedProperties.has(property.name.text)) { + propertiesToKeep.push(property); + } else { + removedPropertyNames.push(property.name.text); + } + } else { + propertiesToKeep.push(property); + } + }); + + // Don't modify the node if there's nothing to remove. + if (removedPropertyNames.length === 0) { + return node; + } + + // Note that the trailing/leading spaces are necessary so the comment looks good. + const removalComment = + ` Removed unsupported properties by Angular migration: ${removedPropertyNames.join(', ')}. `; + + if (propertiesToKeep.length > 0) { + propertiesToKeep[0] = addUniqueLeadingComment(propertiesToKeep[0], removalComment); + return ts.createObjectLiteral(propertiesToKeep); + } else { + return addUniqueLeadingComment(ts.createObjectLiteral(propertiesToKeep), removalComment); + } +} + +export function findLiteralsToMigrate(sourceFile: ts.SourceFile, typeChecker: ts.TypeChecker) { + const results = new Map>( + Array.from(methodConfig.keys(), key => [key, new Set()])); + const routerImport = getImportSpecifier(sourceFile, '@angular/router', 'Router'); + const seenLiterals = new Map(); + + if (routerImport) { + sourceFile.forEachChild(function visitNode(node: ts.Node) { + // Look for calls that look like `foo.` with more than one parameter. + if (ts.isCallExpression(node) && node.arguments.length > 1 && + ts.isPropertyAccessExpression(node.expression) && ts.isIdentifier(node.expression.name) && + methodConfig.has(node.expression.name.text)) { + // Check whether the type of the object on which the + // function is called refers to the Router import. + if (isReferenceToImport(typeChecker, node.expression.expression, routerImport)) { + const methodName = node.expression.name.text; + const parameterDeclaration = + typeChecker.getTypeAtLocation(node.arguments[1]).getSymbol()?.valueDeclaration; + + // Find the source of the object literal. + if (parameterDeclaration && ts.isObjectLiteralExpression(parameterDeclaration)) { + if (!seenLiterals.has(parameterDeclaration)) { + results.get(methodName)!.add(parameterDeclaration); + seenLiterals.set(parameterDeclaration, methodName); + // If the same literal has been passed into multiple different methods, we can't + // migrate it, because the supported properties are different. When we detect such + // a case, we drop it from the results so that it gets ignored. If it's used multiple + // times for the same method, it can still be migrated. + } else if (seenLiterals.get(parameterDeclaration) !== methodName) { + results.forEach(literals => literals.delete(parameterDeclaration)); + } + } + } + } else { + node.forEachChild(visitNode); + } + }); + } + + return results; +} + +/** Adds a leading comment to a node, if the node doesn't have such a comment already. */ +function addUniqueLeadingComment(node: T, comment: string): T { + const existingComments = ts.getSyntheticLeadingComments(node); + + // This logic is primarily to ensure that we don't add the same comment multiple + // times when tslint runs over the same file again with outdated information. + if (!existingComments || existingComments.every(c => c.text !== comment)) { + return ts.addSyntheticLeadingComment(node, ts.SyntaxKind.MultiLineCommentTrivia, comment); + } + + return node; +} diff --git a/packages/core/schematics/migrations/renderer-to-renderer2/index.ts b/packages/core/schematics/migrations/renderer-to-renderer2/index.ts index 06fee368ce..b40d044b04 100644 --- a/packages/core/schematics/migrations/renderer-to-renderer2/index.ts +++ b/packages/core/schematics/migrations/renderer-to-renderer2/index.ts @@ -12,10 +12,11 @@ import * as ts from 'typescript'; import {getProjectTsConfigPaths} from '../../utils/project_tsconfig_paths'; import {createMigrationProgram} from '../../utils/typescript/compiler_host'; +import {getImportSpecifier} from '../../utils/typescript/imports'; import {getHelper, HelperFunction} from './helpers'; import {migrateExpression, replaceImport} from './migration'; -import {findCoreImport, findRendererReferences} from './util'; +import {findRendererReferences, getNamedImports} from './util'; const MODULE_AUGMENTATION_FILENAME = 'ɵɵRENDERER_MIGRATION_CORE_AUGMENTATION.d.ts'; @@ -61,15 +62,17 @@ function runRendererToRenderer2Migration(tree: Tree, tsconfigPath: string, baseP f => !f.isDeclarationFile && !program.isSourceFileFromExternalLibrary(f)); sourceFiles.forEach(sourceFile => { - const rendererImport = findCoreImport(sourceFile, 'Renderer'); + const rendererImportSpecifier = getImportSpecifier(sourceFile, '@angular/core', 'Renderer'); + const rendererImport = + rendererImportSpecifier ? getNamedImports(rendererImportSpecifier) : null; // If there are no imports for the `Renderer`, we can exit early. - if (!rendererImport) { + if (!rendererImportSpecifier || !rendererImport) { return; } const {typedNodes, methodCalls, forwardRefs} = - findRendererReferences(sourceFile, typeChecker, rendererImport); + findRendererReferences(sourceFile, typeChecker, rendererImportSpecifier); const update = tree.beginUpdate(relative(basePath, sourceFile.fileName)); const helpersToAdd = new Set(); @@ -78,8 +81,8 @@ function runRendererToRenderer2Migration(tree: Tree, tsconfigPath: string, baseP update.insertRight( rendererImport.getStart(), printer.printNode( - ts.EmitHint.Unspecified, replaceImport(rendererImport, 'Renderer', 'Renderer2'), - sourceFile)); + ts.EmitHint.Unspecified, + replaceImport(rendererImport, rendererImportSpecifier, 'Renderer2'), sourceFile)); // Change the method parameter and property types to `Renderer2`. typedNodes.forEach(node => { diff --git a/packages/core/schematics/migrations/renderer-to-renderer2/migration.ts b/packages/core/schematics/migrations/renderer-to-renderer2/migration.ts index 2d7875c029..49587c4b83 100644 --- a/packages/core/schematics/migrations/renderer-to-renderer2/migration.ts +++ b/packages/core/schematics/migrations/renderer-to-renderer2/migration.ts @@ -9,31 +9,28 @@ import * as ts from 'typescript'; import {HelperFunction} from './helpers'; -import {findImportSpecifier} from './util'; /** A call expression that is based on a property access. */ type PropertyAccessCallExpression = ts.CallExpression&{expression: ts.PropertyAccessExpression}; /** Replaces an import inside an import statement with a different one. */ -export function replaceImport(node: ts.NamedImports, oldImport: string, newImport: string) { - const isAlreadyImported = findImportSpecifier(node.elements, newImport); +export function replaceImport( + node: ts.NamedImports, existingImport: ts.ImportSpecifier, newImportName: string) { + const isAlreadyImported = node.elements.find(element => { + const {name, propertyName} = element; + return propertyName ? propertyName.text === newImportName : name.text === newImportName; + }); if (isAlreadyImported) { return node; } - const existingImport = findImportSpecifier(node.elements, oldImport); - - if (!existingImport) { - throw new Error(`Could not find an import to replace using ${oldImport}.`); - } - return ts.updateNamedImports(node, [ ...node.elements.filter(current => current !== existingImport), // Create a new import while trying to preserve the alias of the old one. ts.createImportSpecifier( - existingImport.propertyName ? ts.createIdentifier(newImport) : undefined, - existingImport.propertyName ? existingImport.name : ts.createIdentifier(newImport)) + existingImport.propertyName ? ts.createIdentifier(newImportName) : undefined, + existingImport.propertyName ? existingImport.name : ts.createIdentifier(newImportName)) ]); } diff --git a/packages/core/schematics/migrations/renderer-to-renderer2/util.ts b/packages/core/schematics/migrations/renderer-to-renderer2/util.ts index 347f6ef1dd..6d22c4fa01 100644 --- a/packages/core/schematics/migrations/renderer-to-renderer2/util.ts +++ b/packages/core/schematics/migrations/renderer-to-renderer2/util.ts @@ -8,30 +8,32 @@ import * as ts from 'typescript'; +import {getImportSpecifier} from '../../utils/typescript/imports'; +import {isReferenceToImport} from '../../utils/typescript/symbol'; + /** * Finds typed nodes (e.g. function parameters or class properties) that are referencing the old * `Renderer`, as well as calls to the `Renderer` methods. */ export function findRendererReferences( - sourceFile: ts.SourceFile, typeChecker: ts.TypeChecker, rendererImport: ts.NamedImports) { + sourceFile: ts.SourceFile, typeChecker: ts.TypeChecker, + rendererImportSpecifier: ts.ImportSpecifier) { const typedNodes = new Set(); const methodCalls = new Set(); const forwardRefs = new Set(); - const importSpecifier = findImportSpecifier(rendererImport.elements, 'Renderer'); - const forwardRefImport = findCoreImport(sourceFile, 'forwardRef'); - const forwardRefSpecifier = - forwardRefImport ? findImportSpecifier(forwardRefImport.elements, 'forwardRef') : null; + const forwardRefSpecifier = getImportSpecifier(sourceFile, '@angular/core', 'forwardRef'); ts.forEachChild(sourceFile, function visitNode(node: ts.Node) { if ((ts.isParameter(node) || ts.isPropertyDeclaration(node)) && - isReferenceToImport(typeChecker, node.name, importSpecifier)) { + isReferenceToImport(typeChecker, node.name, rendererImportSpecifier)) { typedNodes.add(node); } else if ( - ts.isAsExpression(node) && isReferenceToImport(typeChecker, node.type, importSpecifier)) { + ts.isAsExpression(node) && + isReferenceToImport(typeChecker, node.type, rendererImportSpecifier)) { typedNodes.add(node); } else if (ts.isCallExpression(node)) { if (ts.isPropertyAccessExpression(node.expression) && - isReferenceToImport(typeChecker, node.expression.expression, importSpecifier)) { + isReferenceToImport(typeChecker, node.expression.expression, rendererImportSpecifier)) { methodCalls.add(node); } else if ( // If we're dealing with a forwardRef that's returning a Renderer. @@ -39,7 +41,7 @@ export function findRendererReferences( isReferenceToImport(typeChecker, node.expression, forwardRefSpecifier) && node.arguments.length) { const rendererIdentifier = - findRendererIdentifierInForwardRef(typeChecker, node, importSpecifier); + findRendererIdentifierInForwardRef(typeChecker, node, rendererImportSpecifier); if (rendererIdentifier) { forwardRefs.add(rendererIdentifier); } @@ -52,59 +54,27 @@ export function findRendererReferences( return {typedNodes, methodCalls, forwardRefs}; } -/** Finds the import from @angular/core that has a symbol with a particular name. */ -export function findCoreImport(sourceFile: ts.SourceFile, symbolName: string): ts.NamedImports| - null { - // Only look through the top-level imports. - for (const node of sourceFile.statements) { - if (!ts.isImportDeclaration(node) || !ts.isStringLiteral(node.moduleSpecifier) || - node.moduleSpecifier.text !== '@angular/core') { - continue; - } +/** Gets the closest `NamedImports` to an `ImportSpecifier`. */ +export function getNamedImports(specifier: ts.ImportSpecifier): ts.NamedImports|null { + let current: ts.Node = specifier; - const namedBindings = node.importClause && node.importClause.namedBindings; - - if (!namedBindings || !ts.isNamedImports(namedBindings)) { - continue; - } - - if (findImportSpecifier(namedBindings.elements, symbolName)) { - return namedBindings; + while (current && !ts.isSourceFile(current)) { + if (ts.isNamedImports(current)) { + return current; } + current = current.parent; } return null; } -/** Finds an import specifier with a particular name, accounting for aliases. */ -export function findImportSpecifier( - elements: ts.NodeArray, importName: string) { - return elements.find(element => { - const {name, propertyName} = element; - return propertyName ? propertyName.text === importName : name.text === importName; - }) || - null; -} - -/** Checks whether a node is referring to an import spcifier. */ -function isReferenceToImport( - typeChecker: ts.TypeChecker, node: ts.Node, importSpecifier: ts.ImportSpecifier|null): boolean { - if (importSpecifier) { - const nodeSymbol = typeChecker.getTypeAtLocation(node).getSymbol(); - const importSymbol = typeChecker.getTypeAtLocation(importSpecifier).getSymbol(); - return !!(nodeSymbol && importSymbol) && - nodeSymbol.valueDeclaration === importSymbol.valueDeclaration; - } - return false; -} - /** Finds the identifier referring to the `Renderer` inside a `forwardRef` call expression. */ function findRendererIdentifierInForwardRef( typeChecker: ts.TypeChecker, node: ts.CallExpression, rendererImport: ts.ImportSpecifier|null): ts.Identifier|null { const firstArg = node.arguments[0]; - if (ts.isArrowFunction(firstArg)) { + if (ts.isArrowFunction(firstArg) && rendererImport) { // Check if the function is `forwardRef(() => Renderer)`. if (ts.isIdentifier(firstArg.body) && isReferenceToImport(typeChecker, firstArg.body, rendererImport)) { diff --git a/packages/core/schematics/test/BUILD.bazel b/packages/core/schematics/test/BUILD.bazel index 1a232cd29f..2030fef2d6 100644 --- a/packages/core/schematics/test/BUILD.bazel +++ b/packages/core/schematics/test/BUILD.bazel @@ -12,6 +12,7 @@ ts_library( "//packages/core/schematics/migrations/missing-injectable", "//packages/core/schematics/migrations/module-with-providers", "//packages/core/schematics/migrations/move-document", + "//packages/core/schematics/migrations/navigation-extras-omissions", "//packages/core/schematics/migrations/renderer-to-renderer2", "//packages/core/schematics/migrations/static-queries", "//packages/core/schematics/migrations/template-var-assignment", diff --git a/packages/core/schematics/test/google3/navigation_extras_omissions_spec.ts b/packages/core/schematics/test/google3/navigation_extras_omissions_spec.ts new file mode 100644 index 0000000000..42325de7c0 --- /dev/null +++ b/packages/core/schematics/test/google3/navigation_extras_omissions_spec.ts @@ -0,0 +1,334 @@ +/** + * @license + * Copyright Google LLC 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 NavigationExtras omissions TSLint rule', () => { + const rulesDirectory = + dirname(require.resolve('../../migrations/google3/navigationExtrasOmissionsRule')); + + let tmpDir: string; + + beforeEach(() => { + tmpDir = join(process.env['TEST_TMPDIR']!, 'google3-test'); + shx.mkdir('-p', tmpDir); + + // We need to declare the Angular symbols we're testing for, otherwise type checking won't work. + writeFile('router.d.ts', ` + export declare class Router { + navigateByUrl(url: string, extras?: any); + createUrlTree(commands: any[], extras?: any); + } + `); + + writeFile('tsconfig.json', JSON.stringify({ + compilerOptions: { + module: 'es2015', + baseUrl: './', + paths: { + '@angular/router': ['router.d.ts'], + } + }, + })); + }); + + afterEach(() => shx.rm('-r', tmpDir)); + + function runTSLint(fix: boolean) { + const program = Linter.createProgram(join(tmpDir, 'tsconfig.json')); + const linter = new Linter({fix, rulesDirectory: [rulesDirectory]}, program); + const config = Configuration.parseConfigFile({rules: {'navigation-extras-omissions': 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 flag objects with invalid properties used inside the relevant method calls', () => { + writeFile('/index.ts', ` + import {Router} from '@angular/router'; + + class Navigator { + constructor(private _router: Router) {} + + goHome() { + this._router.navigateByUrl('/', {fragment: 'foo'}); + } + + createTree() { + return this._router.createUrlTree(['/'], {state: {}}); + } + + goAway() { + this._router.navigateByUrl('/away'); + } + } + `); + + const linter = runTSLint(false); + const failures = linter.getResult().failures.map(failure => failure.getFailure()); + + expect(failures.length).toBe(2); + expect(failures[0]) + .toMatch( + /Object used in navigateByUrl or createUrlTree call contains unsupported properties/); + expect(failures[1]) + .toMatch( + /Object used in navigateByUrl or createUrlTree call contains unsupported properties/); + }); + + it('should not change calls with a single argument', () => { + writeFile('/index.ts', ` + import {Router} from '@angular/router'; + + class Navigator { + constructor(private _router: Router) {} + + goHome() { + this._router.navigateByUrl('/'); + } + } + + function createTree(router: Router) { + return router.createUrlTree(['/']); + } + `); + + runTSLint(true); + + const content = getFile('/index.ts'); + expect(content).toContain(`this._router.navigateByUrl('/');`); + expect(content).toContain(`return router.createUrlTree(['/']);`); + }); + + it('should not change calls with an empty object literal', () => { + writeFile('/index.ts', ` + import {Router} from '@angular/router'; + + class Navigator { + constructor(private _router: Router) {} + + goHome() { + this._router.navigateByUrl('/', {}); + } + } + + function createTree(router: Router) { + return router.createUrlTree(['/'], {}); + } + `); + + runTSLint(true); + + const content = getFile('/index.ts'); + expect(content).toContain(`this._router.navigateByUrl('/', {});`); + expect(content).toContain(`return router.createUrlTree(['/'], {});`); + }); + + it('should not change objects that are used in multiple different methods', () => { + writeFile('/index.ts', ` + import {Router} from '@angular/router'; + + const config = {replaceUrl: true, fragment: 'foo', state: {}}; + + class Navigator { + constructor(private _router: Router) {} + + goHome() { + this._router.navigateByUrl('/', config); + return this._router.createUrlTree(['/'], config); + } + } + `); + + runTSLint(true); + + const content = getFile('/index.ts'); + expect(content).toContain(`const config = {replaceUrl: true, fragment: 'foo', state: {}};`); + }); + + it('should preserve calls if the router does not come from @angular/router', () => { + writeFile('/index.ts', ` + import {Router} from '@custom/router'; + + function createTree(router: Router) { + return router.createUrlTree(['/'], {foo: 1, bar: 2}); + } + `); + + runTSLint(true); + + const content = getFile('/index.ts'); + expect(content).toContain(`return router.createUrlTree(['/'], {foo: 1, bar: 2});`); + }); + + it('should change invalid navigateByUrl calls', () => { + writeFile('/index.ts', ` + import {Router} from '@angular/router'; + + class Navigator { + constructor(private _router: Router) {} + + goHome() { + this._router.navigateByUrl('/', {preserveFragment: false, skipLocationChange: false, fragment: 'foo'}); + } + } + `); + + runTSLint(true); + + const content = getFile('/index.ts'); + expect(content).toContain( + `this._router.navigateByUrl('/', { /* Removed unsupported properties by Angular migration: preserveFragment, fragment. */ skipLocationChange: false });`); + }); + + it('should change invalid navigateByUrl calls', () => { + writeFile('/index.ts', ` + import {Router} from '@angular/router'; + + function createTree(router: Router) { + return router.createUrlTree(['/'], {replaceUrl: true, preserveFragment: true, state: {}}); + } + `); + + runTSLint(true); + + const content = getFile('/index.ts'); + expect(content).toContain( + `return router.createUrlTree(['/'], { /* Removed unsupported properties by Angular migration: replaceUrl, state. */ preserveFragment: true });`); + }); + + it('should set the comment outside the object if all properties were removed', () => { + writeFile('/index.ts', ` + import {Router} from '@angular/router'; + + function navigate(router: Router) { + router.navigateByUrl('/', {fragment: 'foo'}); + } + `); + + runTSLint(true); + + const content = getFile('/index.ts'); + expect(content).toContain( + `router.navigateByUrl('/', /* Removed unsupported properties by Angular migration: fragment. */ {});`); + }); + + it('should migrate object literals defined as variables', () => { + writeFile('/index.ts', ` + import {Router} from '@angular/router'; + + const config = {skipLocationChange: false, fragment: 'foo'}; + const proxy = config; + + function navigate(router: Router) { + router.navigateByUrl('/', proxy); + } + `); + + runTSLint(true); + + const content = getFile('/index.ts'); + expect(content).toContain( + `const config = { /* Removed unsupported properties by Angular migration: fragment. */ skipLocationChange: false };`); + expect(content).toContain(`const proxy = config;`); + expect(content).toContain(`router.navigateByUrl('/', proxy);`); + }); + + it('should pick up calls where the router is returned by a function', () => { + writeFile('/index.ts', ` + import {Router} from '@angular/router'; + + function navigate(router: Router) { + getRouter().navigateByUrl('/', {fragment: 'foo'}); + } + + function getRouter() { + return {} as Router; + } + `); + + runTSLint(true); + + const content = getFile('/index.ts'); + expect(content).toContain( + `getRouter().navigateByUrl('/', /* Removed unsupported properties by Angular migration: fragment. */ {});`); + }); + + it('should pick up calls where the router is aliased', () => { + writeFile('/index.ts', ` + import {Router as AliasedRouter} from '@angular/router'; + + function navigate(router: AliasedRouter) { + router.navigateByUrl('/', {fragment: 'foo'}); + } + `); + + runTSLint(true); + + const content = getFile('/index.ts'); + expect(content).toContain( + `router.navigateByUrl('/', /* Removed unsupported properties by Angular migration: fragment. */ {});`); + }); + + it('should preserve object spread assignments', () => { + writeFile('/index.ts', ` + import {Router} from '@angular/router'; + + function navigate(router: Router) { + const overrides = {foo: 1}; + router.navigateByUrl('/', {replaceUrl: true, fragment: 'foo', ...overrides}); + } + `); + + runTSLint(true); + + const content = getFile('/index.ts'); + expect(content).toContain( + `router.navigateByUrl('/', { /* Removed unsupported properties by Angular migration: fragment. */ replaceUrl: true, ...overrides });`); + }); + + it('should migrate objects that are used in multiple calls of the same method', () => { + writeFile('/index.ts', ` + import {Router} from '@angular/router'; + + const config = {skipLocationChange: false, fragment: 'foo'}; + + class Navigator { + constructor(private _router: Router) {} + + goHome() { + this._router.navigateByUrl('/', config); + } + + goFish() { + this._router.navigateByUrl('/fish', config); + } + } + `); + + runTSLint(true); + + const content = getFile('/index.ts'); + expect(content).toContain( + `const config = { /* Removed unsupported properties by Angular migration: fragment. */ skipLocationChange: false };`); + }); +}); diff --git a/packages/core/schematics/test/google3/renderer_to_renderer2_spec.ts b/packages/core/schematics/test/google3/renderer_to_renderer2_spec.ts index 8810fe5871..c71620cf5f 100644 --- a/packages/core/schematics/test/google3/renderer_to_renderer2_spec.ts +++ b/packages/core/schematics/test/google3/renderer_to_renderer2_spec.ts @@ -105,6 +105,26 @@ describe('Google3 Renderer to Renderer2 TSLint rule', () => { expect(content).toContain('(renderer: Renderer2)'); }); + it('should not change Renderer imports if Renderer2 is already imported', () => { + writeFile('/index.ts', ` + import { Renderer, Component, Renderer2 } from '@angular/core'; + + @Component({template: ''}) + export class MyComp { + public renderer: Renderer; + + constructor(renderer: Renderer) { + this.renderer = renderer; + } + } + `); + + runTSLint(true); + const content = getFile('index.ts'); + + expect(content).toContain(`import { Renderer, Component, Renderer2 } from '@angular/core';`); + }); + it('should change Renderer inside single-line forwardRefs to Renderer2', () => { writeFile('/index.ts', ` import { Renderer, Component, forwardRef, Inject } from '@angular/core'; diff --git a/packages/core/schematics/test/navigation_extras_omissions_migration_spec.ts b/packages/core/schematics/test/navigation_extras_omissions_migration_spec.ts new file mode 100644 index 0000000000..3133a8cf68 --- /dev/null +++ b/packages/core/schematics/test/navigation_extras_omissions_migration_spec.ts @@ -0,0 +1,302 @@ +/** + * @license + * Copyright Google LLC 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('NavigationExtras omissions 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'], + strictNullChecks: true, + }, + })); + writeFile('/angular.json', JSON.stringify({ + projects: {t: {architect: {build: {options: {tsConfig: './tsconfig.json'}}}}} + })); + // We need to declare the Angular symbols we're testing for, otherwise type checking won't work. + writeFile('/node_modules/@angular/router/index.d.ts', ` + export declare class Router { + navigateByUrl(url: string, extras?: any); + createUrlTree(commands: any[], extras?: any); + } + `); + + 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 not change calls with a single argument', async () => { + writeFile('/index.ts', ` + import {Router} from '@angular/router'; + + class Navigator { + constructor(private _router: Router) {} + + goHome() { + this._router.navigateByUrl('/'); + } + } + + function createTree(router: Router) { + return router.createUrlTree(['/']); + } + `); + + await runMigration(); + + const content = tree.readContent('/index.ts'); + expect(content).toContain(`this._router.navigateByUrl('/');`); + expect(content).toContain(`return router.createUrlTree(['/']);`); + }); + + it('should not change calls with an empty object literal', async () => { + writeFile('/index.ts', ` + import {Router} from '@angular/router'; + + class Navigator { + constructor(private _router: Router) {} + + goHome() { + this._router.navigateByUrl('/', {}); + } + } + + function createTree(router: Router) { + return router.createUrlTree(['/'], {}); + } + `); + + await runMigration(); + + const content = tree.readContent('/index.ts'); + expect(content).toContain(`this._router.navigateByUrl('/', {});`); + expect(content).toContain(`return router.createUrlTree(['/'], {});`); + }); + + it('should not change objects that are used in multiple different methods', async () => { + writeFile('/index.ts', ` + import {Router} from '@angular/router'; + + const config = {replaceUrl: true, fragment: 'foo', state: {}}; + + class Navigator { + constructor(private _router: Router) {} + + goHome() { + this._router.navigateByUrl('/', config); + return this._router.createUrlTree(['/'], config); + } + } + `); + + await runMigration(); + + const content = tree.readContent('/index.ts'); + expect(content).toContain(`const config = {replaceUrl: true, fragment: 'foo', state: {}};`); + }); + + it('should preserve calls if the router does not come from @angular/router', async () => { + writeFile('/index.ts', ` + import {Router} from '@custom/router'; + + function createTree(router: Router) { + return router.createUrlTree(['/'], {foo: 1, bar: 2}); + } + `); + + await runMigration(); + + const content = tree.readContent('/index.ts'); + expect(content).toContain(`return router.createUrlTree(['/'], {foo: 1, bar: 2});`); + }); + + it('should change invalid navigateByUrl calls', async () => { + writeFile('/index.ts', ` + import {Router} from '@angular/router'; + + class Navigator { + constructor(private _router: Router) {} + + goHome() { + this._router.navigateByUrl('/', {preserveFragment: false, skipLocationChange: false, fragment: 'foo'}); + } + } + `); + + await runMigration(); + + const content = tree.readContent('/index.ts'); + expect(content).toContain( + `this._router.navigateByUrl('/', { /* Removed unsupported properties by Angular migration: preserveFragment, fragment. */ skipLocationChange: false });`); + }); + + it('should change invalid navigateByUrl calls', async () => { + writeFile('/index.ts', ` + import {Router} from '@angular/router'; + + function createTree(router: Router) { + return router.createUrlTree(['/'], {replaceUrl: true, preserveFragment: true, state: {}}); + } + `); + + await runMigration(); + + const content = tree.readContent('/index.ts'); + expect(content).toContain( + `return router.createUrlTree(['/'], { /* Removed unsupported properties by Angular migration: replaceUrl, state. */ preserveFragment: true });`); + }); + + it('should set the comment outside the object if all properties were removed', async () => { + writeFile('/index.ts', ` + import {Router} from '@angular/router'; + + function navigate(router: Router) { + router.navigateByUrl('/', {fragment: 'foo'}); + } + `); + + await runMigration(); + + const content = tree.readContent('/index.ts'); + expect(content).toContain( + `router.navigateByUrl('/', /* Removed unsupported properties by Angular migration: fragment. */ {});`); + }); + + it('should migrate object literals defined as variables', async () => { + writeFile('/index.ts', ` + import {Router} from '@angular/router'; + + const config = {skipLocationChange: false, fragment: 'foo'}; + const proxy = config; + + function navigate(router: Router) { + router.navigateByUrl('/', proxy); + } + `); + + await runMigration(); + + const content = tree.readContent('/index.ts'); + expect(content).toContain( + `const config = { /* Removed unsupported properties by Angular migration: fragment. */ skipLocationChange: false };`); + expect(content).toContain(`const proxy = config;`); + expect(content).toContain(`router.navigateByUrl('/', proxy);`); + }); + + it('should pick up calls where the router is returned by a function', async () => { + writeFile('/index.ts', ` + import {Router} from '@angular/router'; + + function navigate(router: Router) { + getRouter().navigateByUrl('/', {fragment: 'foo'}); + } + + function getRouter() { + return {} as Router; + } + `); + + await runMigration(); + + const content = tree.readContent('/index.ts'); + expect(content).toContain( + `getRouter().navigateByUrl('/', /* Removed unsupported properties by Angular migration: fragment. */ {});`); + }); + + it('should pick up calls where the router is aliased', async () => { + writeFile('/index.ts', ` + import {Router as AliasedRouter} from '@angular/router'; + + function navigate(router: AliasedRouter) { + router.navigateByUrl('/', {fragment: 'foo'}); + } + `); + + await runMigration(); + + const content = tree.readContent('/index.ts'); + expect(content).toContain( + `router.navigateByUrl('/', /* Removed unsupported properties by Angular migration: fragment. */ {});`); + }); + + it('should preserve object spread assignments', async () => { + writeFile('/index.ts', ` + import {Router} from '@angular/router'; + + function navigate(router: Router) { + const overrides = {foo: 1}; + router.navigateByUrl('/', {replaceUrl: true, fragment: 'foo', ...overrides}); + } + `); + + await runMigration(); + + const content = tree.readContent('/index.ts'); + expect(content).toContain( + `router.navigateByUrl('/', { /* Removed unsupported properties by Angular migration: fragment. */ replaceUrl: true, ...overrides });`); + }); + + it('should migrate objects that are used in multiple calls of the same method', async () => { + writeFile('/index.ts', ` + import {Router} from '@angular/router'; + + const config = {skipLocationChange: false, fragment: 'foo'}; + + class Navigator { + constructor(private _router: Router) {} + + goHome() { + this._router.navigateByUrl('/', config); + } + + goFish() { + this._router.navigateByUrl('/fish', config); + } + } + `); + + await runMigration(); + + const content = tree.readContent('/index.ts'); + expect(content).toContain( + `const config = { /* Removed unsupported properties by Angular migration: fragment. */ skipLocationChange: false };`); + }); + + function writeFile(filePath: string, contents: string) { + host.sync.write(normalize(filePath), virtualFs.stringToFileBuffer(contents)); + } + + function runMigration() { + return runner.runSchematicAsync('migration-v11-navigation-extras-omissions', {}, tree) + .toPromise(); + } +}); diff --git a/packages/core/schematics/test/renderer_to_renderer2_migration_spec.ts b/packages/core/schematics/test/renderer_to_renderer2_migration_spec.ts index e335a6ecdb..6312105868 100644 --- a/packages/core/schematics/test/renderer_to_renderer2_migration_spec.ts +++ b/packages/core/schematics/test/renderer_to_renderer2_migration_spec.ts @@ -99,6 +99,21 @@ describe('Renderer to Renderer2 migration', () => { expect(content).toContain(`import { Component } from '@angular/core';`); expect(content).toContain(`import { Renderer } from './my-renderer';`); }); + + it('should not change imports if Renderer2 was already imported', async () => { + writeFile('/index.ts', ` + import { Renderer, Component, Renderer2 } from '@angular/core'; + + @Component({template: ''}) + export class MyComp { + constructor(renderer: Renderer) {} + } + `); + + await runMigration(); + expect(tree.readContent('/index.ts')) + .toContain(`import { Renderer, Component, Renderer2 } from '@angular/core';`); + }); }); describe('type renaming', () => { diff --git a/packages/core/schematics/utils/typescript/imports.ts b/packages/core/schematics/utils/typescript/imports.ts index 1bfe7a85ce..152c21d292 100644 --- a/packages/core/schematics/utils/typescript/imports.ts +++ b/packages/core/schematics/utils/typescript/imports.ts @@ -42,3 +42,43 @@ export function getImportOfIdentifier(typeChecker: ts.TypeChecker, node: ts.Iden node: importDecl }; } + + +/** + * Gets a top-level import specifier with a specific name that is imported from a particular module. + * E.g. given a file that looks like: + * + * ``` + * import { Component, Directive } from '@angular/core'; + * import { Foo } from './foo'; + * ``` + * + * Calling `getImportSpecifier(sourceFile, '@angular/core', 'Directive')` will yield the node + * referring to `Directive` in the top import. + * + * @param sourceFile File in which to look for imports. + * @param moduleName Name of the import's module. + * @param specifierName Original name of the specifier to look for. Aliases will be resolved to + * their original name. + */ +export function getImportSpecifier( + sourceFile: ts.SourceFile, moduleName: string, specifierName: string): ts.ImportSpecifier|null { + for (const node of sourceFile.statements) { + if (ts.isImportDeclaration(node) && ts.isStringLiteral(node.moduleSpecifier) && + node.moduleSpecifier.text === moduleName) { + const namedBindings = node.importClause && node.importClause.namedBindings; + if (namedBindings && ts.isNamedImports(namedBindings)) { + const match = namedBindings.elements.find(element => { + const {name, propertyName} = element; + return propertyName ? propertyName.text === specifierName : name.text === specifierName; + }); + + if (match) { + return match; + } + } + } + } + + return null; +} diff --git a/packages/core/schematics/utils/typescript/symbol.ts b/packages/core/schematics/utils/typescript/symbol.ts index 5585c51e16..9d4bbffd1f 100644 --- a/packages/core/schematics/utils/typescript/symbol.ts +++ b/packages/core/schematics/utils/typescript/symbol.ts @@ -18,3 +18,12 @@ export function getValueSymbolOfDeclaration(node: ts.Node, typeChecker: ts.TypeC return symbol; } + +/** Checks whether a node is referring to a specific import specifier. */ +export function isReferenceToImport( + typeChecker: ts.TypeChecker, node: ts.Node, importSpecifier: ts.ImportSpecifier): boolean { + const nodeSymbol = typeChecker.getTypeAtLocation(node).getSymbol(); + const importSymbol = typeChecker.getTypeAtLocation(importSpecifier).getSymbol(); + return !!(nodeSymbol && importSymbol) && + nodeSymbol.valueDeclaration === importSymbol.valueDeclaration; +}