From 93ef27f20ff84140a3a5d69cf8c6e2aa9fb51752 Mon Sep 17 00:00:00 2001 From: Paul Gschwendtner Date: Thu, 17 Oct 2019 11:24:43 +0200 Subject: [PATCH] refactor: ensure hammer migration properly sets up gesture config app module Currently the HammerJS v9 migration relies on a few utilities from `@schematics/angular`. Apparently these are not reliable enough for our migration use-case, so we cannot use them. This came up when testing the migration with more possible scenarios in the docs app. Currently the migration is unable to set up the gesture config (if hammer is used) in the app module if the bootstrap module call imports it through multiple layers of files. We can fix this, make the logic more robust and make the code less verbose by simply using the type checker to resolve the file that contains the app module references in a bootstrap call (that way we do not need to deal with import resolution). --- src/cdk/schematics/update-tool/index.ts | 4 +- .../schematics/update-tool/migration-rule.ts | 2 +- .../test-cases/v9/hammer-migration-v9.spec.ts | 48 ++++++++++++++ .../hammer-gestures-v9/find-main-module.ts | 30 +++++++++ .../hammer-gestures-rule.ts | 66 ++++++++++++------- 5 files changed, 122 insertions(+), 28 deletions(-) create mode 100644 src/material/schematics/ng-update/upgrade-rules/hammer-gestures-v9/find-main-module.ts diff --git a/src/cdk/schematics/update-tool/index.ts b/src/cdk/schematics/update-tool/index.ts index 79783ce929c0..6b6d2684687d 100644 --- a/src/cdk/schematics/update-tool/index.ts +++ b/src/cdk/schematics/update-tool/index.ts @@ -122,8 +122,8 @@ export function runMigrationRules( if (ruleFailures.length) { ruleFailures.forEach(({filePath, message, position}) => { const normalizedFilePath = normalize(getProjectRelativePath(filePath)); - const lineAndCharacter = `${position.line + 1}:${position.character + 1}`; - logger.warn(`${normalizedFilePath}@${lineAndCharacter} - ${message}`); + const lineAndCharacter = position ? `@${position.line + 1}:${position.character + 1}` : ''; + logger.warn(`${normalizedFilePath}${lineAndCharacter} - ${message}`); }); } diff --git a/src/cdk/schematics/update-tool/migration-rule.ts b/src/cdk/schematics/update-tool/migration-rule.ts index 301b70ba0355..7399354fdbbc 100644 --- a/src/cdk/schematics/update-tool/migration-rule.ts +++ b/src/cdk/schematics/update-tool/migration-rule.ts @@ -16,7 +16,7 @@ import {LineAndCharacter} from './utils/line-mappings'; export interface MigrationFailure { filePath: string; message: string; - position: LineAndCharacter; + position?: LineAndCharacter; } export class MigrationRule { diff --git a/src/material/schematics/ng-update/test-cases/v9/hammer-migration-v9.spec.ts b/src/material/schematics/ng-update/test-cases/v9/hammer-migration-v9.spec.ts index 15411105356c..bb081d3a32b1 100644 --- a/src/material/schematics/ng-update/test-cases/v9/hammer-migration-v9.spec.ts +++ b/src/material/schematics/ng-update/test-cases/v9/hammer-migration-v9.spec.ts @@ -536,6 +536,54 @@ describe('v9 HammerJS removal', () => { export class AppModule { }`); }); + it('should add gesture config provider to app module if module is referenced through ' + + 're-exports in bootstrap', async () => { + writeFile('/projects/cdk-testing/src/app/app.component.html', ` + + `); + + writeFile('/projects/cdk-testing/src/main.ts', ` + import 'hammerjs'; + import { enableProdMode } from '@angular/core'; + import { platformBrowserDynamic } from '@angular/platform-browser-dynamic'; + + import { AppModule } from './app/'; + import { environment } from './environments/environment'; + + if (environment.production) { + enableProdMode(); + } + + platformBrowserDynamic().bootstrapModule(AppModule) + .catch(err => console.error(err)); + `); + + writeFile('/projects/cdk-testing/src/app/index.ts', `export * from './app.module';`); + + await runMigration(); + + expect(tree.readContent('/projects/cdk-testing/src/main.ts')).toContain(`import 'hammerjs';`); + expect(tree.exists('/projects/cdk-testing/src/gesture-config.ts')).toBe(true); + expect(tree.readContent('/projects/cdk-testing/src/app/app.module.ts')).toContain(dedent`\ + import { BrowserModule, HAMMER_GESTURE_CONFIG } from '@angular/platform-browser'; + import { NgModule } from '@angular/core'; + + import { AppComponent } from './app.component'; + import { GestureConfig } from "../gesture-config"; + + @NgModule({ + declarations: [ + AppComponent + ], + imports: [ + BrowserModule + ], + providers: [{ provide: HAMMER_GESTURE_CONFIG, useClass: GestureConfig }], + bootstrap: [AppComponent] + }) + export class AppModule { }`); + }); + it('should not add gesture config provider multiple times if already provided', async () => { writeFile('/projects/cdk-testing/src/app/app.component.html', ` diff --git a/src/material/schematics/ng-update/upgrade-rules/hammer-gestures-v9/find-main-module.ts b/src/material/schematics/ng-update/upgrade-rules/hammer-gestures-v9/find-main-module.ts new file mode 100644 index 000000000000..dfe8d7ef5b7f --- /dev/null +++ b/src/material/schematics/ng-update/upgrade-rules/hammer-gestures-v9/find-main-module.ts @@ -0,0 +1,30 @@ +/** + * @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'; + +/** + * Finds the main Angular module within the specified source file. The first module + * that is part of the "bootstrapModule" expression is returned. + */ +export function findMainModuleExpression(mainSourceFile: ts.SourceFile): ts.Expression|null { + let foundModule: ts.Expression|null = null; + const visitNode = (node: ts.Node) => { + if (ts.isCallExpression(node) && node.arguments.length && + ts.isPropertyAccessExpression(node.expression) && ts.isIdentifier(node.expression.name) && + node.expression.name.text === 'bootstrapModule') { + foundModule = node.arguments[0]!; + } else { + ts.forEachChild(node, visitNode); + } + }; + + ts.forEachChild(mainSourceFile, visitNode); + + return foundModule; +} diff --git a/src/material/schematics/ng-update/upgrade-rules/hammer-gestures-v9/hammer-gestures-rule.ts b/src/material/schematics/ng-update/upgrade-rules/hammer-gestures-v9/hammer-gestures-rule.ts index 1301c255ac90..e3472ddc7e4a 100644 --- a/src/material/schematics/ng-update/upgrade-rules/hammer-gestures-v9/hammer-gestures-rule.ts +++ b/src/material/schematics/ng-update/upgrade-rules/hammer-gestures-v9/hammer-gestures-rule.ts @@ -28,7 +28,6 @@ import { } from '@schematics/angular/utility/ast-utils'; import {InsertChange} from '@schematics/angular/utility/change'; import {getWorkspace} from '@schematics/angular/utility/config'; -import {getAppModulePath} from '@schematics/angular/utility/ng-ast-utils'; import {WorkspaceProject} from '@schematics/angular/utility/workspace-models'; import chalk from 'chalk'; import {readFileSync} from 'fs'; @@ -37,6 +36,7 @@ import * as ts from 'typescript'; import {getProjectFromProgram} from './cli-workspace'; import {findHammerScriptImportElements} from './find-hammer-script-tags'; +import {findMainModuleExpression} from './find-main-module'; import {isHammerJsUsedInTemplate} from './hammer-template-check'; import {getImportOfIdentifier, Import} from './identifier-imports'; import {ImportManager} from './import-manager'; @@ -53,6 +53,9 @@ const HAMMER_MODULE_SPECIFIER = 'hammerjs'; const CANNOT_REMOVE_REFERENCE_ERROR = `Cannot remove reference to "GestureConfig". Please remove manually.`; +const CANNOT_SETUP_APP_MODULE_ERROR = `Could not setup HammerJS gesture in module. Please ` + + `manually ensure that the Hammer gesture config is set up.`; + interface IdentifierReference { node: ts.Identifier; importData: Import; @@ -198,21 +201,8 @@ export class HammerGesturesRule extends MigrationRule { this._gestureConfigReferences.forEach( i => this._replaceGestureConfigReference(i, gestureConfigPath)); - const appModulePath = getAppModulePath(this.tree, getProjectMainFile(project)); - const sourceFile = this.program.getSourceFile(join(this.basePath, appModulePath)); - - if (!sourceFile) { - this.failures.push({ - filePath: appModulePath, - message: `Could not setup HammerJS gesture in module. Please manually ensure that ` + - `the Hammer gesture config is set up.`, - position: {character: 0, line: 0} - }); - return; - } - - // Setup the gesture config provider in the project app module if not done. - this._setupGestureConfigProviderIfNeeded(sourceFile, appModulePath, gestureConfigPath); + // Setup the gesture config provider in the project app module if not done already. + this._setupGestureConfigInAppModule(project, gestureConfigPath); } /** @@ -531,12 +521,38 @@ export class HammerGesturesRule extends MigrationRule { }); } - /** - * Sets up the Hammer gesture config provider in the given app module - * if needed. - */ - private _setupGestureConfigProviderIfNeeded( - sourceFile: ts.SourceFile, appModulePath: string, configPath: string) { + /** Sets up the Hammer gesture config provider in the app module if needed. */ + private _setupGestureConfigInAppModule(project: WorkspaceProject, configPath: string) { + const mainFilePath = join(this.basePath, getProjectMainFile(project)); + const mainFile = this.program.getSourceFile(mainFilePath); + if (!mainFile) { + this.failures.push({ + filePath: mainFilePath, + message: CANNOT_SETUP_APP_MODULE_ERROR, + }); + return; + } + + const appModuleExpr = findMainModuleExpression(mainFile); + if (!appModuleExpr) { + this.failures.push({ + filePath: mainFilePath, + message: CANNOT_SETUP_APP_MODULE_ERROR, + }); + return; + } + + const appModuleSymbol = this._getDeclarationSymbolOfNode(unwrapExpression(appModuleExpr)); + if (!appModuleSymbol || !appModuleSymbol.valueDeclaration) { + this.failures.push({ + filePath: mainFilePath, + message: CANNOT_SETUP_APP_MODULE_ERROR, + }); + return; + } + + const sourceFile = appModuleSymbol.valueDeclaration.getSourceFile(); + const relativePath = relative(this.basePath, sourceFile.fileName); const hammerConfigTokenExpr = this._importManager.addImportToSourceFile( sourceFile, HAMMER_CONFIG_TOKEN_NAME, HAMMER_CONFIG_TOKEN_MODULE); const gestureConfigExpr = this._importManager.addImportToSourceFile( @@ -574,8 +590,8 @@ export class HammerGesturesRule extends MigrationRule { return; } - const changeActions = addSymbolToNgModuleMetadata( - sourceFile, appModulePath, 'providers', this._printNode(newProviderNode, sourceFile), null); + const changeActions = addSymbolToNgModuleMetadata(sourceFile, relativePath, 'providers', + this._printNode(newProviderNode, sourceFile), null); changeActions.forEach(change => { if (change instanceof InsertChange) { @@ -668,7 +684,7 @@ export class HammerGesturesRule extends MigrationRule { } context.logger.info(chalk.yellow( - ' ⚠ The HammerJS v9 migration for Angular components is not able to migrate tests. ' + + '⚠ The HammerJS v9 migration for Angular components is not able to migrate tests. ' + 'Please manually clean up tests in your project if they rely on HammerJS.')); // Clean global state once the workspace has been migrated. This is technically