diff --git a/packages/core/schematics/BUILD.bazel b/packages/core/schematics/BUILD.bazel index 6f99228d7af7a6..66c250a589dfea 100644 --- a/packages/core/schematics/BUILD.bazel +++ b/packages/core/schematics/BUILD.bazel @@ -23,6 +23,7 @@ pkg_npm( "//packages/core/schematics/migrations/navigation-extras-omissions", "//packages/core/schematics/migrations/relative-link-resolution", "//packages/core/schematics/migrations/renderer-to-renderer2", + "//packages/core/schematics/migrations/router-link-empty-expression", "//packages/core/schematics/migrations/router-preserve-query-params", "//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 11101b01f252c2..fce8147718f5f5 100644 --- a/packages/core/schematics/migrations.json +++ b/packages/core/schematics/migrations.json @@ -99,6 +99,11 @@ "version": "12.0.2", "description": "Automatically migrates shadow-piercing selector from `/deep/` to the recommended alternative `::ng-deep`.", "factory": "./migrations/deep-shadow-piercing-selector/index" + }, + "migration-v13-router-link-empty-expression": { + "version": "13.0.0-beta", + "description": "Migrates `[routerLink]=\"\"` in templates to `[routerLink]=\"[]\"` because these links are likely intented to route to the current page with updated fragment/query params.", + "factory": "./migrations/router-link-empty-expression/index" } } -} +} \ No newline at end of file diff --git a/packages/core/schematics/migrations/router-link-empty-expression/BUILD.bazel b/packages/core/schematics/migrations/router-link-empty-expression/BUILD.bazel new file mode 100644 index 00000000000000..0d5091642f83d8 --- /dev/null +++ b/packages/core/schematics/migrations/router-link-empty-expression/BUILD.bazel @@ -0,0 +1,20 @@ +load("//tools:defaults.bzl", "ts_library") + +ts_library( + name = "router-link-empty-expression", + 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/compiler", + "//packages/core/schematics/utils", + "@npm//@angular-devkit/core", + "@npm//@angular-devkit/schematics", + "@npm//@types/node", + "@npm//typescript", + ], +) diff --git a/packages/core/schematics/migrations/router-link-empty-expression/README.md b/packages/core/schematics/migrations/router-link-empty-expression/README.md new file mode 100644 index 00000000000000..f877a333f436db --- /dev/null +++ b/packages/core/schematics/migrations/router-link-empty-expression/README.md @@ -0,0 +1,28 @@ +## RouterLink `null` and `undefined` inputs + +The previous behavior of `RouterLink` for `null` and `undefined` inputs was to treat +the input the same as `[]` or `''`. This creates several unresolvable issues with +correctly disabling the links because `commands = []` does not behave the same +as disabling a link. Instead, it navigates to the current page, but will also +clear any fragment and/or query params. + +The new behavior of the `routerLink` input will be to completely disable navigation +for `null` and `undefined` inputs. For HTML Anchor elements, this will also mean +removing the `href` attribute. + +```html + +``` + +In the example from above, there is no value provided to the `routerLink` input. +This button would previously navigate to the current page and update the fragment to "section_2". +The updated behavio is to disable this link because the input +for `routerLink` is `undefined`. + +If the intent for the link is to link to the current page rather than disable navigation, +the template should be updated to one of the following options: +```html + + + +``` \ No newline at end of file diff --git a/packages/core/schematics/migrations/router-link-empty-expression/analyze_template.ts b/packages/core/schematics/migrations/router-link-empty-expression/analyze_template.ts new file mode 100644 index 00000000000000..68aa1cf23e24a6 --- /dev/null +++ b/packages/core/schematics/migrations/router-link-empty-expression/analyze_template.ts @@ -0,0 +1,29 @@ +/** + * @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 {BoundAttribute, visitAll} from '@angular/compiler/src/render3/r3_ast'; + +import {ResolvedTemplate} from '../../utils/ng_component_template'; +import {parseHtmlGracefully} from '../../utils/parse_html'; + +import {RouterLinkEmptyExprVisitor} from './angular/html_routerlink_empty_expr_visitor'; + +export function analyzeResolvedTemplate(template: ResolvedTemplate): BoundAttribute[]|null { + const templateNodes = parseHtmlGracefully(template.content, template.filePath); + + if (!templateNodes) { + return null; + } + + const visitor = new RouterLinkEmptyExprVisitor(); + + // Analyze the Angular Render3 HTML AST and collect all template variable assignments. + visitAll(visitor, templateNodes); + + return visitor.emptyRouterLinkExpressions; +} diff --git a/packages/core/schematics/migrations/router-link-empty-expression/angular/html_routerlink_empty_expr_visitor.ts b/packages/core/schematics/migrations/router-link-empty-expression/angular/html_routerlink_empty_expr_visitor.ts new file mode 100644 index 00000000000000..bc92940cb8a22b --- /dev/null +++ b/packages/core/schematics/migrations/router-link-empty-expression/angular/html_routerlink_empty_expr_visitor.ts @@ -0,0 +1,35 @@ +/** + * @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 {ASTWithSource, EmptyExpr} from '@angular/compiler'; +import {BoundAttribute, Element, NullVisitor, Template, visitAll} from '@angular/compiler/src/render3/r3_ast'; + +/** + * HTML AST visitor that traverses the Render3 HTML AST in order to find all + * undefined routerLink asssignment ([routerLink]=""). + */ +export class RouterLinkEmptyExprVisitor extends NullVisitor { + readonly emptyRouterLinkExpressions: BoundAttribute[] = []; + + override visitElement(element: Element): void { + visitAll(this, element.inputs); + visitAll(this, element.children); + } + + override visitTemplate(t: Template): void { + visitAll(this, t.inputs); + visitAll(this, t.children); + } + + override visitBoundAttribute(node: BoundAttribute) { + if (node.name === 'routerLink' && node.value instanceof ASTWithSource && + node.value.ast instanceof EmptyExpr) { + this.emptyRouterLinkExpressions.push(node); + } + } +} diff --git a/packages/core/schematics/migrations/router-link-empty-expression/index.ts b/packages/core/schematics/migrations/router-link-empty-expression/index.ts new file mode 100644 index 00000000000000..5652644182d253 --- /dev/null +++ b/packages/core/schematics/migrations/router-link-empty-expression/index.ts @@ -0,0 +1,157 @@ +/** + * @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 {logging, normalize} from '@angular-devkit/core'; +import {Rule, SchematicContext, SchematicsException, Tree} from '@angular-devkit/schematics'; +import {EmptyExpr, TmplAstBoundAttribute} from '@angular/compiler'; +import {relative} from 'path'; + +import {NgComponentTemplateVisitor, ResolvedTemplate} from '../../utils/ng_component_template'; +import {getProjectTsConfigPaths} from '../../utils/project_tsconfig_paths'; +import {canMigrateFile, createMigrationProgram} from '../../utils/typescript/compiler_host'; + +import {analyzeResolvedTemplate} from './analyze_template'; + +type Logger = logging.LoggerApi; + +const README_URL = + 'https://github.com/angular/angular/blob/master/packages/core/schematics/migrations/router-link-empty-expression/README.md'; + +interface FixedTemplate { + originalTemplate: ResolvedTemplate; + newContent: string; + emptyRouterlinkExpressions: TmplAstBoundAttribute[]; +} + +/** Entry point for the RouterLink empty expression migration. */ +export default function(): Rule { + return (tree: Tree, context: SchematicContext) => { + const {buildPaths, testPaths} = getProjectTsConfigPaths(tree); + const basePath = process.cwd(); + + if (!buildPaths.length && !testPaths.length) { + throw new SchematicsException( + 'Could not find any tsconfig file. Cannot check templates for empty routerLinks.'); + } + + for (const tsconfigPath of [...buildPaths, ...testPaths]) { + runEmptyRouterLinkExpressionMigration(tree, tsconfigPath, basePath, context.logger); + } + }; +} + +/** + * Runs the routerLink migration, changing routerLink="" to routerLink="[]" and notifying developers + * which templates received updates. + */ +function runEmptyRouterLinkExpressionMigration( + tree: Tree, tsconfigPath: string, basePath: string, logger: Logger) { + const {program} = createMigrationProgram(tree, tsconfigPath, basePath); + const typeChecker = program.getTypeChecker(); + const templateVisitor = new NgComponentTemplateVisitor(typeChecker); + const sourceFiles = + program.getSourceFiles().filter(sourceFile => canMigrateFile(basePath, sourceFile, program)); + + // Analyze source files by detecting HTML templates. + sourceFiles.forEach(sourceFile => templateVisitor.visitNode(sourceFile)); + + const {resolvedTemplates} = templateVisitor; + fixEmptyRouterlinks(resolvedTemplates, tree, logger); +} + +function fixEmptyRouterlinks(resolvedTemplates: ResolvedTemplate[], tree: Tree, logger: Logger) { + const basePath = process.cwd(); + const collectedFixes: string[] = []; + const fixesByFile = getFixesByFile(resolvedTemplates); + + for (const [absFilePath, fixes] of fixesByFile) { + const treeFilePath = relative(normalize(basePath), normalize(absFilePath)); + const originalFileContent = tree.read(treeFilePath)?.toString(); + if (originalFileContent === undefined) { + logger.error( + `Failed to read file containing template; cannot apply fixes for empty routerLink expressions in ${ + treeFilePath}.`); + continue; + } + + const updater = tree.beginUpdate(treeFilePath); + for (const fix of fixes) { + const displayFilePath = normalize(relative(basePath, fix.originalTemplate.filePath)); + updater.remove(fix.originalTemplate.start, fix.originalTemplate.content.length); + updater.insertLeft(fix.originalTemplate.start, fix.newContent); + + for (const n of fix.emptyRouterlinkExpressions) { + const {line, character} = + fix.originalTemplate.getCharacterAndLineOfPosition(n.sourceSpan.start.offset); + collectedFixes.push(`${displayFilePath}@${line + 1}:${character + 1}`); + } + tree.commitUpdate(updater); + } + } + + if (collectedFixes.length > 0) { + logger.info('---- RouterLink empty assignment schematic ----'); + logger.info('The behavior of empty/`undefined` inputs for `routerLink` has changed'); + logger.info('from linking to the current page to instead completely disable the link.'); + logger.info(`Read more about this change here: ${README_URL}`); + logger.info(''); + logger.info('The following empty `routerLink` inputs were found and fixed:'); + collectedFixes.forEach(fix => logger.warn(`⮑ ${fix}`)); + } +} + +/** + * Returns fixes for nodes in templates which contain empty routerLink assignments, grouped by file. + */ +function getFixesByFile(templates: ResolvedTemplate[]): Map { + const fixesByFile = new Map(); + for (const template of templates) { + const templateFix = fixEmptyRouterlinksInTemplate(template); + if (templateFix === null) { + continue; + } + + const file = template.filePath; + if (fixesByFile.has(file)) { + if (template.inline) { + // External templates may be referenced multiple times in the project + // (e.g. if shared between components), but we only want to record them + // once. On the other hand, an inline template resides in a TS file that + // may contain multiple inline templates. + fixesByFile.get(file)!.push(templateFix); + } + } else { + fixesByFile.set(file, [templateFix]); + } + } + + return fixesByFile; +} + +function fixEmptyRouterlinksInTemplate(template: ResolvedTemplate): FixedTemplate|null { + const emptyRouterlinkExpressions = analyzeResolvedTemplate(template); + + if (!emptyRouterlinkExpressions) { + return null; + } + + // Sort backwards so string replacements do not conflict + emptyRouterlinkExpressions.sort((a, b) => b.value.sourceSpan.start - a.value.sourceSpan.start); + let newContent = template.content; + for (const expr of emptyRouterlinkExpressions) { + if (expr.valueSpan) { + newContent = newContent.substr(0, expr.value.sourceSpan.start) + '[]' + + newContent.substr(expr.value.sourceSpan.start); + } else { + newContent = newContent.substr(0, expr.sourceSpan.end.offset) + '="[]"' + + newContent.substr(expr.sourceSpan.end.offset); + } + } + + return {originalTemplate: template, newContent, emptyRouterlinkExpressions}; +} diff --git a/packages/core/schematics/test/BUILD.bazel b/packages/core/schematics/test/BUILD.bazel index 0904e31590c2a9..30f0b69f4f984c 100644 --- a/packages/core/schematics/test/BUILD.bazel +++ b/packages/core/schematics/test/BUILD.bazel @@ -21,6 +21,7 @@ ts_library( "//packages/core/schematics/migrations/navigation-extras-omissions", "//packages/core/schematics/migrations/relative-link-resolution", "//packages/core/schematics/migrations/renderer-to-renderer2", + "//packages/core/schematics/migrations/router-link-empty-expression", "//packages/core/schematics/migrations/router-preserve-query-params", "//packages/core/schematics/migrations/static-queries", "//packages/core/schematics/migrations/template-var-assignment", diff --git a/packages/core/schematics/test/routerlink_empty_expr_migration_spec.ts b/packages/core/schematics/test/routerlink_empty_expr_migration_spec.ts new file mode 100644 index 00000000000000..fa011c3c6ff68a --- /dev/null +++ b/packages/core/schematics/test/routerlink_empty_expr_migration_spec.ts @@ -0,0 +1,202 @@ +/** + * @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('routerlink emptyExpr assignment 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('../migrations.json')); + host = new TempScopedNodeJsSyncHost(); + tree = new UnitTestTree(new HostTree(host)); + + writeFile('/tsconfig.json', JSON.stringify({ + compilerOptions: { + lib: ['es2015'], + }, + })); + writeFile('/angular.json', JSON.stringify({ + projects: {t: {architect: {build: {options: {tsConfig: './tsconfig.json'}}}}} + })); + + 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)); + } + + function runMigration() { + return runner.runSchematicAsync('migration-v13-router-link-empty-expression', {}, tree) + .toPromise(); + } + + it('should warn for emptyExpr assignment in inline template', async () => { + writeFile('/index.ts', ` + import {Component} from '@angular/core'; + + @Component({ + template: '
', + }) + export class MyComp {} + `); + + await runMigration(); + expect(warnOutput.length).toBe(1); + expect(warnOutput[0]).toMatch(/^⮑ {3}index.ts@5:25/); + + const content = tree.readContent('/index.ts'); + expect(content).toContain(`
`); + }); + + it('should warn for EmptyExpr assignment in external templatae', async () => { + writeFile('/index.ts', ` + import {Component} from '@angular/core'; + + @Component({ + templateUrl: './tmpl.html', + }) + export class MyComp {} + `); + + writeFile('/tmpl.html', ` +
+ +
+ `); + + await runMigration(); + + expect(warnOutput.length).toBe(1); + expect(warnOutput).toMatch(/^⮑ {3}tmpl.html@3:20/); + + const content = tree.readContent('/tmpl.html'); + expect(content).toContain(``); + }); + + it('should warn for `[routerLink]`', async () => { + writeFile('/index.ts', ` + import {Component} from '@angular/core'; + + @Component({ + templateUrl: './tmpl.html', + }) + export class MyComp {} + `); + + writeFile('/tmpl.html', ` +
+ +
+ `); + + await runMigration(); + + const content = tree.readContent('/tmpl.html'); + expect(content).toContain(``); + }); + + it('should work for many instances in a single template', async () => { + writeFile('/index.ts', ` + import {Component} from '@angular/core'; + + @Component({ + templateUrl: './tmpl.html', + }) + export class MyComp {} + `); + + writeFile('/tmpl.html', ` + + + `); + + await runMigration(); + const content = tree.readContent('/tmpl.html'); + + expect(content).toContain( + ` `); + expect(content).toContain( + ` `); + }); + + it('should work for many references to one template', async () => { + writeFile('/index.ts', ` + import {Component} from '@angular/core'; + + @Component({ + templateUrl: './tmpl.html', + }) + export class MyComp {} + + @Component({ + templateUrl: './tmpl.html', + }) + export class MyComp2 {} + `); + + writeFile('/tmpl.html', ``); + + await runMigration(); + const content = tree.readContent('/tmpl.html'); + + expect(content).toContain(``); + }); + + it('does not migrate empty attribute expression because it is equivalent to empty string, not undefined', + async () => { + writeFile('/index.ts', ` + import {Component} from '@angular/core'; + + @Component({ + templateUrl: './tmpl.html', + }) + export class MyComp {} + + @Component({ + templateUrl: './tmpl.html', + }) + export class MyComp2 {} + `); + + const contents = ``; + writeFile('/tmpl.html', contents); + + await runMigration(); + const content = tree.readContent('/tmpl.html'); + + expect(content).toEqual(contents); + }); +});