Skip to content

Commit 951ea63

Browse files
devversionmmalerba
authored andcommitted
refactor: ensure hammer migration properly sets up gesture config app module (#17429)
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).
1 parent e202531 commit 951ea63

File tree

5 files changed

+122
-28
lines changed

5 files changed

+122
-28
lines changed

src/cdk/schematics/update-tool/index.ts

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -122,8 +122,8 @@ export function runMigrationRules<T>(
122122
if (ruleFailures.length) {
123123
ruleFailures.forEach(({filePath, message, position}) => {
124124
const normalizedFilePath = normalize(getProjectRelativePath(filePath));
125-
const lineAndCharacter = `${position.line + 1}:${position.character + 1}`;
126-
logger.warn(`${normalizedFilePath}@${lineAndCharacter} - ${message}`);
125+
const lineAndCharacter = position ? `@${position.line + 1}:${position.character + 1}` : '';
126+
logger.warn(`${normalizedFilePath}${lineAndCharacter} - ${message}`);
127127
});
128128
}
129129

src/cdk/schematics/update-tool/migration-rule.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -16,7 +16,7 @@ import {LineAndCharacter} from './utils/line-mappings';
1616
export interface MigrationFailure {
1717
filePath: string;
1818
message: string;
19-
position: LineAndCharacter;
19+
position?: LineAndCharacter;
2020
}
2121

2222
export class MigrationRule<T> {

src/material/schematics/ng-update/test-cases/v9/hammer-migration-v9.spec.ts

Lines changed: 48 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -536,6 +536,54 @@ describe('v9 HammerJS removal', () => {
536536
export class AppModule { }`);
537537
});
538538

539+
it('should add gesture config provider to app module if module is referenced through ' +
540+
're-exports in bootstrap', async () => {
541+
writeFile('/projects/cdk-testing/src/app/app.component.html', `
542+
<span (pinch)="onPinch($event)"></span>
543+
`);
544+
545+
writeFile('/projects/cdk-testing/src/main.ts', `
546+
import 'hammerjs';
547+
import { enableProdMode } from '@angular/core';
548+
import { platformBrowserDynamic } from '@angular/platform-browser-dynamic';
549+
550+
import { AppModule } from './app/';
551+
import { environment } from './environments/environment';
552+
553+
if (environment.production) {
554+
enableProdMode();
555+
}
556+
557+
platformBrowserDynamic().bootstrapModule(AppModule)
558+
.catch(err => console.error(err));
559+
`);
560+
561+
writeFile('/projects/cdk-testing/src/app/index.ts', `export * from './app.module';`);
562+
563+
await runMigration();
564+
565+
expect(tree.readContent('/projects/cdk-testing/src/main.ts')).toContain(`import 'hammerjs';`);
566+
expect(tree.exists('/projects/cdk-testing/src/gesture-config.ts')).toBe(true);
567+
expect(tree.readContent('/projects/cdk-testing/src/app/app.module.ts')).toContain(dedent`\
568+
import { BrowserModule, HAMMER_GESTURE_CONFIG } from '@angular/platform-browser';
569+
import { NgModule } from '@angular/core';
570+
571+
import { AppComponent } from './app.component';
572+
import { GestureConfig } from "../gesture-config";
573+
574+
@NgModule({
575+
declarations: [
576+
AppComponent
577+
],
578+
imports: [
579+
BrowserModule
580+
],
581+
providers: [{ provide: HAMMER_GESTURE_CONFIG, useClass: GestureConfig }],
582+
bootstrap: [AppComponent]
583+
})
584+
export class AppModule { }`);
585+
});
586+
539587
it('should not add gesture config provider multiple times if already provided', async () => {
540588
writeFile('/projects/cdk-testing/src/app/app.component.html', `
541589
<span (pinch)="onPinch($event)"></span>
Lines changed: 30 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,30 @@
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+
/**
12+
* Finds the main Angular module within the specified source file. The first module
13+
* that is part of the "bootstrapModule" expression is returned.
14+
*/
15+
export function findMainModuleExpression(mainSourceFile: ts.SourceFile): ts.Expression|null {
16+
let foundModule: ts.Expression|null = null;
17+
const visitNode = (node: ts.Node) => {
18+
if (ts.isCallExpression(node) && node.arguments.length &&
19+
ts.isPropertyAccessExpression(node.expression) && ts.isIdentifier(node.expression.name) &&
20+
node.expression.name.text === 'bootstrapModule') {
21+
foundModule = node.arguments[0]!;
22+
} else {
23+
ts.forEachChild(node, visitNode);
24+
}
25+
};
26+
27+
ts.forEachChild(mainSourceFile, visitNode);
28+
29+
return foundModule;
30+
}

src/material/schematics/ng-update/upgrade-rules/hammer-gestures-v9/hammer-gestures-rule.ts

Lines changed: 41 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -28,7 +28,6 @@ import {
2828
} from '@schematics/angular/utility/ast-utils';
2929
import {InsertChange} from '@schematics/angular/utility/change';
3030
import {getWorkspace} from '@schematics/angular/utility/config';
31-
import {getAppModulePath} from '@schematics/angular/utility/ng-ast-utils';
3231
import {WorkspaceProject} from '@schematics/angular/utility/workspace-models';
3332
import chalk from 'chalk';
3433
import {readFileSync} from 'fs';
@@ -37,6 +36,7 @@ import * as ts from 'typescript';
3736

3837
import {getProjectFromProgram} from './cli-workspace';
3938
import {findHammerScriptImportElements} from './find-hammer-script-tags';
39+
import {findMainModuleExpression} from './find-main-module';
4040
import {isHammerJsUsedInTemplate} from './hammer-template-check';
4141
import {getImportOfIdentifier, Import} from './identifier-imports';
4242
import {ImportManager} from './import-manager';
@@ -53,6 +53,9 @@ const HAMMER_MODULE_SPECIFIER = 'hammerjs';
5353
const CANNOT_REMOVE_REFERENCE_ERROR =
5454
`Cannot remove reference to "GestureConfig". Please remove manually.`;
5555

56+
const CANNOT_SETUP_APP_MODULE_ERROR = `Could not setup HammerJS gesture in module. Please ` +
57+
`manually ensure that the Hammer gesture config is set up.`;
58+
5659
interface IdentifierReference {
5760
node: ts.Identifier;
5861
importData: Import;
@@ -198,21 +201,8 @@ export class HammerGesturesRule extends MigrationRule<null> {
198201
this._gestureConfigReferences.forEach(
199202
i => this._replaceGestureConfigReference(i, gestureConfigPath));
200203

201-
const appModulePath = getAppModulePath(this.tree, getProjectMainFile(project));
202-
const sourceFile = this.program.getSourceFile(join(this.basePath, appModulePath));
203-
204-
if (!sourceFile) {
205-
this.failures.push({
206-
filePath: appModulePath,
207-
message: `Could not setup HammerJS gesture in module. Please manually ensure that ` +
208-
`the Hammer gesture config is set up.`,
209-
position: {character: 0, line: 0}
210-
});
211-
return;
212-
}
213-
214-
// Setup the gesture config provider in the project app module if not done.
215-
this._setupGestureConfigProviderIfNeeded(sourceFile, appModulePath, gestureConfigPath);
204+
// Setup the gesture config provider in the project app module if not done already.
205+
this._setupGestureConfigInAppModule(project, gestureConfigPath);
216206
}
217207

218208
/**
@@ -531,12 +521,38 @@ export class HammerGesturesRule extends MigrationRule<null> {
531521
});
532522
}
533523

534-
/**
535-
* Sets up the Hammer gesture config provider in the given app module
536-
* if needed.
537-
*/
538-
private _setupGestureConfigProviderIfNeeded(
539-
sourceFile: ts.SourceFile, appModulePath: string, configPath: string) {
524+
/** Sets up the Hammer gesture config provider in the app module if needed. */
525+
private _setupGestureConfigInAppModule(project: WorkspaceProject, configPath: string) {
526+
const mainFilePath = join(this.basePath, getProjectMainFile(project));
527+
const mainFile = this.program.getSourceFile(mainFilePath);
528+
if (!mainFile) {
529+
this.failures.push({
530+
filePath: mainFilePath,
531+
message: CANNOT_SETUP_APP_MODULE_ERROR,
532+
});
533+
return;
534+
}
535+
536+
const appModuleExpr = findMainModuleExpression(mainFile);
537+
if (!appModuleExpr) {
538+
this.failures.push({
539+
filePath: mainFilePath,
540+
message: CANNOT_SETUP_APP_MODULE_ERROR,
541+
});
542+
return;
543+
}
544+
545+
const appModuleSymbol = this._getDeclarationSymbolOfNode(unwrapExpression(appModuleExpr));
546+
if (!appModuleSymbol || !appModuleSymbol.valueDeclaration) {
547+
this.failures.push({
548+
filePath: mainFilePath,
549+
message: CANNOT_SETUP_APP_MODULE_ERROR,
550+
});
551+
return;
552+
}
553+
554+
const sourceFile = appModuleSymbol.valueDeclaration.getSourceFile();
555+
const relativePath = relative(this.basePath, sourceFile.fileName);
540556
const hammerConfigTokenExpr = this._importManager.addImportToSourceFile(
541557
sourceFile, HAMMER_CONFIG_TOKEN_NAME, HAMMER_CONFIG_TOKEN_MODULE);
542558
const gestureConfigExpr = this._importManager.addImportToSourceFile(
@@ -574,8 +590,8 @@ export class HammerGesturesRule extends MigrationRule<null> {
574590
return;
575591
}
576592

577-
const changeActions = addSymbolToNgModuleMetadata(
578-
sourceFile, appModulePath, 'providers', this._printNode(newProviderNode, sourceFile), null);
593+
const changeActions = addSymbolToNgModuleMetadata(sourceFile, relativePath, 'providers',
594+
this._printNode(newProviderNode, sourceFile), null);
579595

580596
changeActions.forEach(change => {
581597
if (change instanceof InsertChange) {
@@ -668,7 +684,7 @@ export class HammerGesturesRule extends MigrationRule<null> {
668684
}
669685

670686
context.logger.info(chalk.yellow(
671-
' The HammerJS v9 migration for Angular components is not able to migrate tests. ' +
687+
' The HammerJS v9 migration for Angular components is not able to migrate tests. ' +
672688
'Please manually clean up tests in your project if they rely on HammerJS.'));
673689

674690
// Clean global state once the workspace has been migrated. This is technically

0 commit comments

Comments
 (0)