Sitelet https://github.com/angular/angular/commit/7849fdd
Skip to content

Commit 7849fdd

Browse files
crisbetoAndrewKushnir
crisbeto
authored and
AndrewKushnir
committed
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(/sitelet?url=https%3A%2F%2Fgithub.com%2Fangular%2Fangular%2Fcommit%2F%27%2F%27%2C%2520%257BskipLocationChange%3A%2520false%2C%2520fragment%3A%2520%27foo%27%257D); ``` **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
1 parent 97adc27 commit 7849fdd

19 files changed

Lines changed: 1053 additions & 73 deletions

‎packages/core/schematics/BUILD.bazel‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,7 @@ pkg_npm(
1414
"//packages/core/schematics/migrations/missing-injectable",
1515
"//packages/core/schematics/migrations/module-with-providers",
1616
"//packages/core/schematics/migrations/move-document",
17+
"//packages/core/schematics/migrations/navigation-extras-omissions",
1718
"//packages/core/schematics/migrations/renderer-to-renderer2",
1819
"//packages/core/schematics/migrations/static-queries",
1920
"//packages/core/schematics/migrations/template-var-assignment",

‎packages/core/schematics/migrations.json‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -44,6 +44,11 @@
4444
"version": "10.0.0-beta",
4545
"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",
4646
"factory": "./migrations/undecorated-classes-with-decorated-fields/index"
47+
},
48+
"migration-v11-navigation-extras-omissions": {
49+
"version": "11.0.0-beta",
50+
"description": "NavigationExtras omissions migration. In version 11, some unsupported properties were omitted from the `extras` parameter of the `Router.navigateByUrl` and `Router.createUrlTree` methods.",
51+
"factory": "./migrations/navigation-extras-omissions/index"
4752
}
4853
}
4954
}

‎packages/core/schematics/migrations/google3/BUILD.bazel‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@ ts_library(
99
"//packages/core/schematics/migrations/dynamic-queries",
1010
"//packages/core/schematics/migrations/missing-injectable",
1111
"//packages/core/schematics/migrations/missing-injectable/google3",
12+
"//packages/core/schematics/migrations/navigation-extras-omissions",
1213
"//packages/core/schematics/migrations/renderer-to-renderer2",
1314
"//packages/core/schematics/migrations/static-queries",
1415
"//packages/core/schematics/migrations/template-var-assignment",

‎packages/core/schematics/migrations/google3/navigationExtrasOmissionsRule.ts‎

Lines changed: 38 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,38 @@
1+
/**
2+
* @license
3+
* Copyright Google LLC All Rights Reserved.
4+
*
5+
* Use of this source code is governed by an MIT-style license that can be
6+
* found in the LICENSE file at https://angular.io/license
7+
*/
8+
9+
import {Replacement, RuleFailure, Rules} from 'tslint';
10+
import * as ts from 'typescript';
11+
import {findLiteralsToMigrate, migrateLiteral} from '../../migrations/navigation-extras-omissions/util';
12+
13+
14+
/** TSLint rule that migrates `navigateByUrl` and `createUrlTree` calls to an updated signature. */
15+
export class Rule extends Rules.TypedRule {
16+
applyWithProgram(sourceFile: ts.SourceFile, program: ts.Program): RuleFailure[] {
17+
const failures: RuleFailure[] = [];
18+
const typeChecker = program.getTypeChecker();
19+
const printer = ts.createPrinter();
20+
const literalsToMigrate = findLiteralsToMigrate(sourceFile, typeChecker);
21+
22+
literalsToMigrate.forEach((instances, methodName) => instances.forEach(instance => {
23+
const migratedNode = migrateLiteral(methodName, instance);
24+
25+
if (migratedNode !== instance) {
26+
failures.push(new RuleFailure(
27+
sourceFile, instance.getStart(), instance.getEnd(),
28+
'Object used in navigateByUrl or createUrlTree call contains unsupported properties.',
29+
this.ruleName,
30+
new Replacement(
31+
instance.getStart(), instance.getWidth(),
32+
printer.printNode(ts.EmitHint.Unspecified, migratedNode, sourceFile))));
33+
}
34+
}));
35+
36+
return failures;
37+
}
38+
}

‎packages/core/schematics/migrations/google3/rendererToRenderer2Rule.ts‎

Lines changed: 12 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -8,10 +8,11 @@
88

99
import {Replacement, RuleFailure, Rules} from 'tslint';
1010
import * as ts from 'typescript';
11+
import {getImportSpecifier} from '../../utils/typescript/imports';
1112

1213
import {getHelper, HelperFunction} from '../renderer-to-renderer2/helpers';
1314
import {migrateExpression, replaceImport} from '../renderer-to-renderer2/migration';
14-
import {findCoreImport, findRendererReferences} from '../renderer-to-renderer2/util';
15+
import {findRendererReferences, getNamedImports} from '../renderer-to-renderer2/util';
1516

1617
/**
1718
* TSLint rule that migrates from `Renderer` to `Renderer2`. More information on how it works:
@@ -22,18 +23,21 @@ export class Rule extends Rules.TypedRule {
2223
const typeChecker = program.getTypeChecker();
2324
const printer = ts.createPrinter();
2425
const failures: RuleFailure[] = [];
25-
const rendererImport = findCoreImport(sourceFile, 'Renderer');
26+
const rendererImportSpecifier = getImportSpecifier(sourceFile, '@angular/core', 'Renderer');
27+
const rendererImport =
28+
rendererImportSpecifier ? getNamedImports(rendererImportSpecifier) : null;
2629

2730
// If there are no imports for the `Renderer`, we can exit early.
28-
if (!rendererImport) {
31+
if (!rendererImportSpecifier || !rendererImport) {
2932
return failures;
3033
}
3134

3235
const {typedNodes, methodCalls, forwardRefs} =
33-
findRendererReferences(sourceFile, typeChecker, rendererImport);
36+
findRendererReferences(sourceFile, typeChecker, rendererImportSpecifier);
3437
const helpersToAdd = new Set<HelperFunction>();
3538

36-
failures.push(this._getNamedImportsFailure(rendererImport, sourceFile, printer));
39+
failures.push(
40+
this._getNamedImportsFailure(rendererImport, rendererImportSpecifier, sourceFile, printer));
3741
typedNodes.forEach(node => failures.push(this._getTypedNodeFailure(node, sourceFile)));
3842
forwardRefs.forEach(node => failures.push(this._getIdentifierNodeFailure(node, sourceFile)));
3943

@@ -61,9 +65,10 @@ export class Rule extends Rules.TypedRule {
6165

6266
/** Gets a failure for an import of the Renderer. */
6367
private _getNamedImportsFailure(
64-
node: ts.NamedImports, sourceFile: ts.SourceFile, printer: ts.Printer): RuleFailure {
68+
node: ts.NamedImports, importSpecifier: ts.ImportSpecifier, sourceFile: ts.SourceFile,
69+
printer: ts.Printer): RuleFailure {
6570
const replacementText = printer.printNode(
66-
ts.EmitHint.Unspecified, replaceImport(node, 'Renderer', 'Renderer2'), sourceFile);
71+
ts.EmitHint.Unspecified, replaceImport(node, importSpecifier, 'Renderer2'), sourceFile);
6772

6873
return new RuleFailure(
6974
sourceFile, node.getStart(), node.getEnd(),

‎packages/core/schematics/migrations/navigation-extras-omissions/BUILD.bazel‎

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,18 @@
1+
load("//tools:defaults.bzl", "ts_library")
2+
3+
ts_library(
4+
name = "navigation-extras-omissions",
5+
srcs = glob(["**/*.ts"]),
6+
tsconfig = "//packages/core/schematics:tsconfig.json",
7+
visibility = [
8+
"//packages/core/schematics:__pkg__",
9+
"//packages/core/schematics/migrations/google3:__pkg__",
10+
"//packages/core/schematics/test:__pkg__",
11+
],
12+
deps = [
13+
"//packages/core/schematics/utils",
14+
"@npm//@angular-devkit/schematics",
15+
"@npm//@types/node",
16+
"@npm//typescript",
17+
],
18+
)

‎packages/core/schematics/migrations/navigation-extras-omissions/README.md‎

Lines changed: 35 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,35 @@
1+
## Router.navigateByUrl and Router.createUrlTree extras migration
2+
3+
Previously the `extras` parameter of `Router.navigateByUrl` and `Router.createUrlTree` accepted the
4+
full `NavigationExtras` object, even though only a subset of properties was supported. This
5+
migration removes the unsupported properties from the relevant method call sites.
6+
7+
#### Before
8+
```ts
9+
import { Component } from '@angular/core';
10+
import { Router } from '@angular/router';
11+
12+
@Component({})
13+
export class MyComponent {
14+
constructor(private _router: Router) {}
15+
16+
goHome() {
17+
this._router.navigateByUrl('/', {skipLocationChange: false, fragment: 'foo'});
18+
}
19+
}
20+
```
21+
22+
#### After
23+
```ts
24+
import { Component } from '@angular/core';
25+
import { Router } from '@angular/router';
26+
27+
@Component({})
28+
export class MyComponent {
29+
constructor(private _router: Router) {}
30+
31+
goHome() {
32+
this._router.navigateByUrl('/', { /* Removed unsupported properties by Angular migration: fragment. */ skipLocationChange: false });
33+
}
34+
}
35+
```

‎packages/core/schematics/migrations/navigation-extras-omissions/index.ts‎

Lines changed: 61 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,61 @@
1+
/**
2+
* @license
3+
* Copyright Google LLC All Rights Reserved.
4+
*
5+
* Use of this source code is governed by an MIT-style license that can be
6+
* found in the LICENSE file at https://angular.io/license
7+
*/
8+
9+
import {Rule, SchematicsException, Tree} from '@angular-devkit/schematics';
10+
import {relative} from 'path';
11+
import * as ts from 'typescript';
12+
13+
import {getProjectTsConfigPaths} from '../../utils/project_tsconfig_paths';
14+
import {createMigrationProgram} from '../../utils/typescript/compiler_host';
15+
import {findLiteralsToMigrate, migrateLiteral} from './util';
16+
17+
18+
/** Migration that switches `Router.navigateByUrl` and `Router.createUrlTree` to a new signature. */
19+
export default function(): Rule {
20+
return (tree: Tree) => {
21+
const {buildPaths, testPaths} = getProjectTsConfigPaths(tree);
22+
const basePath = process.cwd();
23+
const allPaths = [...buildPaths, ...testPaths];
24+
25+
if (!allPaths.length) {
26+
throw new SchematicsException(
27+
'Could not find any tsconfig file. Cannot migrate ' +
28+
'Router.navigateByUrl and Router.createUrlTree calls.');
29+
}
30+
31+
for (const tsconfigPath of allPaths) {
32+
runNavigationExtrasOmissionsMigration(tree, tsconfigPath, basePath);
33+
}
34+
};
35+
}
36+
37+
function runNavigationExtrasOmissionsMigration(tree: Tree, tsconfigPath: string, basePath: string) {
38+
const {program} = createMigrationProgram(tree, tsconfigPath, basePath);
39+
const typeChecker = program.getTypeChecker();
40+
const printer = ts.createPrinter();
41+
const sourceFiles = program.getSourceFiles().filter(
42+
f => !f.isDeclarationFile && !program.isSourceFileFromExternalLibrary(f));
43+
44+
sourceFiles.forEach(sourceFile => {
45+
const literalsToMigrate = findLiteralsToMigrate(sourceFile, typeChecker);
46+
const update = tree.beginUpdate(relative(basePath, sourceFile.fileName));
47+
48+
literalsToMigrate.forEach((instances, methodName) => instances.forEach(instance => {
49+
const migratedNode = migrateLiteral(methodName, instance);
50+
51+
if (migratedNode !== instance) {
52+
update.remove(instance.getStart(), instance.getWidth());
53+
update.insertRight(
54+
instance.getStart(),
55+
printer.printNode(ts.EmitHint.Unspecified, migratedNode, sourceFile));
56+
}
57+
}));
58+
59+
tree.commitUpdate(update);
60+
});
61+
}

‎packages/core/schematics/migrations/navigation-extras-omissions/util.ts‎

Lines changed: 125 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,125 @@
1+
/**
2+
* @license
3+
* Copyright Google LLC All Rights Reserved.
4+
*
5+
* Use of this source code is governed by an MIT-style license that can be
6+
* found in the LICENSE file at https://angular.io/license
7+
*/
8+
9+
import * as ts from 'typescript';
10+
11+
import {getImportSpecifier} from '../../utils/typescript/imports';
12+
import {isReferenceToImport} from '../../utils/typescript/symbol';
13+
14+
/**
15+
* Configures the methods that the migration should be looking for
16+
* and the properties from `NavigationExtras` that should be preserved.
17+
*/
18+
const methodConfig = new Map<string, Set<string>>([
19+
['navigateByUrl', new Set<string>(['skipLocationChange', 'replaceUrl', 'state'])],
20+
[
21+
'createUrlTree', new Set<string>([
22+
'relativeTo', 'queryParams', 'fragment', 'preserveQueryParams', 'queryParamsHandling',
23+
'preserveFragment'
24+
])
25+
]
26+
]);
27+
28+
export function migrateLiteral(
29+
methodName: string, node: ts.ObjectLiteralExpression): ts.ObjectLiteralExpression {
30+
const allowedProperties = methodConfig.get(methodName);
31+
32+
if (!allowedProperties) {
33+
throw Error(`Attempting to migrate unconfigured method called ${methodName}.`);
34+
}
35+
36+
const propertiesToKeep: ts.ObjectLiteralElementLike[] = [];
37+
const removedPropertyNames: string[] = [];
38+
39+
node.properties.forEach(property => {
40+
// Only look for regular and shorthand property assignments since resolving things
41+
// like spread operators becomes too complicated for this migration.
42+
if ((ts.isPropertyAssignment(property) || ts.isShorthandPropertyAssignment(property)) &&
43+
(ts.isStringLiteralLike(property.name) || ts.isNumericLiteral(property.name) ||
44+
ts.isIdentifier(property.name))) {
45+
if (allowedProperties.has(property.name.text)) {
46+
propertiesToKeep.push(property);
47+
} else {
48+
removedPropertyNames.push(property.name.text);
49+
}
50+
} else {
51+
propertiesToKeep.push(property);
52+
}
53+
});
54+
55+
// Don't modify the node if there's nothing to remove.
56+
if (removedPropertyNames.length === 0) {
57+
return node;
58+
}
59+
60+
// Note that the trailing/leading spaces are necessary so the comment looks good.
61+
const removalComment =
62+
` Removed unsupported properties by Angular migration: ${removedPropertyNames.join(', ')}. `;
63+
64+
if (propertiesToKeep.length > 0) {
65+
propertiesToKeep[0] = addUniqueLeadingComment(propertiesToKeep[0], removalComment);
66+
return ts.createObjectLiteral(propertiesToKeep);
67+
} else {
68+
return addUniqueLeadingComment(ts.createObjectLiteral(propertiesToKeep), removalComment);
69+
}
70+
}
71+
72+
export function findLiteralsToMigrate(sourceFile: ts.SourceFile, typeChecker: ts.TypeChecker) {
73+
const results = new Map<string, Set<ts.ObjectLiteralExpression>>(
74+
Array.from(methodConfig.keys(), key => [key, new Set()]));
75+
const routerImport = getImportSpecifier(sourceFile, '@angular/router', 'Router');
76+
const seenLiterals = new Map<ts.ObjectLiteralExpression, string>();
77+
78+
if (routerImport) {
79+
sourceFile.forEachChild(function visitNode(node: ts.Node) {
80+
// Look for calls that look like `foo.<method to migrate>` with more than one parameter.
81+
if (ts.isCallExpression(node) && node.arguments.length > 1 &&
82+
ts.isPropertyAccessExpression(node.expression) && ts.isIdentifier(node.expression.name) &&
83+
methodConfig.has(node.expression.name.text)) {
84+
// Check whether the type of the object on which the
85+
// function is called refers to the Router import.
86+
if (isReferenceToImport(typeChecker, node.expression.expression, routerImport)) {
87+
const methodName = node.expression.name.text;
88+
const parameterDeclaration =
89+
typeChecker.getTypeAtLocation(node.arguments[1]).getSymbol()?.valueDeclaration;
90+
91+
// Find the source of the object literal.
92+
if (parameterDeclaration && ts.isObjectLiteralExpression(parameterDeclaration)) {
93+
if (!seenLiterals.has(parameterDeclaration)) {
94+
results.get(methodName)!.add(parameterDeclaration);
95+
seenLiterals.set(parameterDeclaration, methodName);
96+
// If the same literal has been passed into multiple different methods, we can't
97+
// migrate it, because the supported properties are different. When we detect such
98+
// a case, we drop it from the results so that it gets ignored. If it's used multiple
99+
// times for the same method, it can still be migrated.
100+
} else if (seenLiterals.get(parameterDeclaration) !== methodName) {
101+
results.forEach(literals => literals.delete(parameterDeclaration));
102+
}
103+
}
104+
}
105+
} else {
106+
node.forEachChild(visitNode);
107+
}
108+
});
109+
}
110+
111+
return results;
112+
}
113+
114+
/** Adds a leading comment to a node, if the node doesn't have such a comment already. */
115+
function addUniqueLeadingComment<T extends ts.Node>(node: T, comment: string): T {
116+
const existingComments = ts.getSyntheticLeadingComments(node);
117+
118+
// This logic is primarily to ensure that we don't add the same comment multiple
119+
// times when tslint runs over the same file again with outdated information.
120+
if (!existingComments || existingComments.every(c => c.text !== comment)) {
121+
return ts.addSyntheticLeadingComment(node, ts.SyntaxKind.MultiLineCommentTrivia, comment);
122+
}
123+
124+
return node;
125+
}

0 commit comments

Comments
 (0)