Skip to content

fix(ng-update): hammer v9 migration should not unnecessarily set up gestures #17713

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Nov 15, 2019
Merged

fix(ng-update): hammer v9 migration should not unnecessarily set up gestures #17713

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
d8d7ea2
fix(ng-update): hammer v9 migration should not unnecessarily set up g…
devversion Nov 15, 2019
7675e99
Fix comment wording
devversion Nov 15, 2019
676a13d
fixup! Fix comment wording
devversion Nov 15, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/cdk/schematics/update-tool/utils/imports.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ export function getImportOfIdentifier(typeChecker: ts.TypeChecker, node: ts.Iden
null {
const symbol = typeChecker.getSymbolAtLocation(node);

if (!symbol || !symbol.declarations.length) {
if (!symbol || !symbol.declarations || !symbol.declarations.length) {
return null;
}

Expand Down
253 changes: 185 additions & 68 deletions src/material/schematics/ng-update/test-cases/v9/hammer-migration-v9.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,9 +59,12 @@ describe('v9 HammerJS removal', () => {
import 'hammerjs';
`);

await runMigration();
const {logOutput} = await runMigration();

expect(tree.readContent('/projects/cdk-testing/src/main.ts')).not.toContain('hammerjs');
expect(logOutput).toContain(
'General notice: 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.');
});

it('should remove empty named import to load hammerjs', async () => {
Expand Down Expand Up @@ -310,9 +313,13 @@ describe('v9 HammerJS removal', () => {
}
`);

await runMigration();
const {logOutput} = await runMigration();

expect(tree.readContent('/projects/cdk-testing/src/main.ts')).toContain(`import 'hammerjs';`);
expect(logOutput).toContain(
'General notice: 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 the ' +
'deprecated Angular Material gesture config.');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we link them to the gesture config API or docs in case they haven't touched that part of their app in years or no one who set that up is still on the team?

If you Google "Angular Material gesture config", you mostly just see docs for Angular's HammerGestureConfig and the Angular Material Getting Started Guide. So that won't be too helpful to them if we can't provide them some additional context/reference here.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This message will be only printed if they explicitly reference the GestureConfig from Angular Material. I think we can make the assumption that it is clear enough in that case.

I'm hesitant to adding any link since our docs page is likely to change in the future and we don't even have docs for material/core IIRC.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be nice to at least mention the GestureConfig class explicitly in that case as some people may not immediately know that "gesture config" maps directly to that class.

});

it('should ignore global reference to Hammer if not resolved to known types', async () => {
Expand Down Expand Up @@ -741,70 +748,180 @@ describe('v9 HammerJS removal', () => {
.toBe('0.0.0');
});

it('should not remove hammerjs if no usage could be detected but custom gesture config is set up',
async () => {
appendContent('/projects/cdk-testing/src/main.ts', `
import 'hammerjs';
`);

writeFile('/projects/cdk-testing/src/test.component.ts', dedent`
import {HAMMER_GESTURE_CONFIG} from '@angular/platform-browser';
import {NgModule} from '@angular/core';
import {CustomGestureConfig} from "../gesture-config";

@NgModule({
providers: [
{
provide: HAMMER_GESTURE_CONFIG,
useClass: CustomGestureConfig
},
],
})
export class TestModule {}
`);

writeFile('/projects/cdk-testing/src/sub.component.ts', dedent`
import {HAMMER_GESTURE_CONFIG} from '@angular/platform-browser';
import {NgModule} from '@angular/core';
import {GestureConfig} from '@angular/material/core';

@NgModule({
providers: [
{
provide: HAMMER_GESTURE_CONFIG,
useClass: GestureConfig
},
],
})
export class SubModule {}
`);

const {logOutput} = await runMigration();

expect(logOutput).toContain(`unable to perform the full migration for this target, but ` +
`removed all references to the deprecated Angular Material gesture config.`);
expect(tree.readContent('/projects/cdk-testing/src/main.ts')).toContain('hammerjs');
expect(tree.readContent('/projects/cdk-testing/src/test.component.ts')).toContain(dedent`
import {HAMMER_GESTURE_CONFIG} from '@angular/platform-browser';
import {NgModule} from '@angular/core';
import {CustomGestureConfig} from "../gesture-config";

@NgModule({
providers: [
{
provide: HAMMER_GESTURE_CONFIG,
useClass: CustomGestureConfig
},
],
})
export class TestModule {}`);
expect(tree.readContent('/projects/cdk-testing/src/sub.component.ts')).toContain(dedent`
import {NgModule} from '@angular/core';

@NgModule({
providers: [
],
})
export class SubModule {}`);
});
describe('with custom gesture config', () => {
beforeEach(() => {
addPackageToPackageJson(tree, 'hammerjs', '0.0.0');
appendContent('/projects/cdk-testing/src/main.ts', `import 'hammerjs';`);
});

it('should not setup copied gesture config if hammer is used in template', async () => {
writeFile('/projects/cdk-testing/src/test.component.ts', dedent`
import {HAMMER_GESTURE_CONFIG} from '@angular/platform-browser';
import {NgModule} from '@angular/core';
import {CustomGestureConfig} from "../gesture-config";

@NgModule({
providers: [
{
provide: HAMMER_GESTURE_CONFIG,
useClass: CustomGestureConfig
},
],
})
export class TestModule {}
`);

writeFile('/projects/cdk-testing/src/app/app.component.html', `
<span (longpress)="onPress()"></span>
`);

await runMigration();

expect(tree.exists('/projects/cdk-testing/src/gesture-config.ts')).toBe(false);
expect(tree.readContent('/projects/cdk-testing/src/main.ts')).toContain('hammerjs');
expect(runner.tasks.some(t => t.name === 'node-package')).toBe(false);
expect(tree.readContent('/projects/cdk-testing/src/test.component.ts')).toContain(dedent`
import {HAMMER_GESTURE_CONFIG} from '@angular/platform-browser';
import {NgModule} from '@angular/core';
import {CustomGestureConfig} from "../gesture-config";

@NgModule({
providers: [
{
provide: HAMMER_GESTURE_CONFIG,
useClass: CustomGestureConfig
},
],
})
export class TestModule {}`);
});

it('should warn if hammer is used in template and references to Material gesture config ' +
'were detected', async () => {
writeFile('/projects/cdk-testing/src/test.component.ts', dedent`
import {HAMMER_GESTURE_CONFIG} from '@angular/platform-browser';
import {NgModule} from '@angular/core';
import {CustomGestureConfig} from "../gesture-config";

@NgModule({
providers: [
{
provide: HAMMER_GESTURE_CONFIG,
useClass: CustomGestureConfig
},
],
})
export class TestModule {}
`);

const subModuleFileContent = dedent`
import {NgModule} from '@angular/core';
import {HAMMER_GESTURE_CONFIG} from '@angular/platform-browser';
import {GestureConfig} from '@angular/material/core';

@NgModule({
providers: [
{
provide: HAMMER_GESTURE_CONFIG,
useClass: GestureConfig
},
],
})
export class SubModule {}
`;

writeFile('/projects/cdk-testing/src/sub.module.ts', subModuleFileContent);
writeFile('/projects/cdk-testing/src/app/app.component.html', `
<span (longpress)="onPress()"></span>
`);

const {logOutput} = await runMigration();

expect(logOutput).toContain(
'This target cannot be migrated completely. Please manually remove references ' +
'to the deprecated Angular Material gesture config.');
expect(runner.tasks.some(t => t.name === 'node-package')).toBe(false);
expect(tree.readContent('/projects/cdk-testing/src/main.ts')).toContain('hammerjs');
expect(tree.readContent('/projects/cdk-testing/src/test.component.ts')).toContain(dedent`
import {HAMMER_GESTURE_CONFIG} from '@angular/platform-browser';
import {NgModule} from '@angular/core';
import {CustomGestureConfig} from "../gesture-config";

@NgModule({
providers: [
{
provide: HAMMER_GESTURE_CONFIG,
useClass: CustomGestureConfig
},
],
})
export class TestModule {}`);
expect(tree.readContent('/projects/cdk-testing/src/sub.module.ts'))
.toBe(subModuleFileContent);
});

it('should not remove hammerjs if no usage could be detected', async () => {
writeFile('/projects/cdk-testing/src/test.component.ts', dedent`
import {HAMMER_GESTURE_CONFIG} from '@angular/platform-browser';
import {NgModule} from '@angular/core';
import {CustomGestureConfig} from "../gesture-config";

@NgModule({
providers: [
{
provide: HAMMER_GESTURE_CONFIG,
useClass: CustomGestureConfig
},
],
})
export class TestModule {}
`);

writeFile('/projects/cdk-testing/src/sub.component.ts', dedent`
import {HAMMER_GESTURE_CONFIG} from '@angular/platform-browser';
import {NgModule} from '@angular/core';
import {GestureConfig} from '@angular/material/core';

@NgModule({
providers: [
{
provide: HAMMER_GESTURE_CONFIG,
useClass: GestureConfig
},
],
})
export class SubModule {}
`);

const {logOutput} = await runMigration();

expect(logOutput).toContain(
'This target cannot be migrated completely, but all references to the ' +
'deprecated Angular Material gesture have been removed.');
expect(runner.tasks.some(t => t.name === 'node-package')).toBe(false);
expect(tree.readContent('/projects/cdk-testing/src/main.ts')).toContain('hammerjs');
expect(tree.readContent('/projects/cdk-testing/src/test.component.ts')).toContain(dedent`
import {HAMMER_GESTURE_CONFIG} from '@angular/platform-browser';
import {NgModule} from '@angular/core';
import {CustomGestureConfig} from "../gesture-config";

@NgModule({
providers: [
{
provide: HAMMER_GESTURE_CONFIG,
useClass: CustomGestureConfig
},
],
})
export class TestModule {}`);
expect(tree.readContent('/projects/cdk-testing/src/sub.component.ts')).toContain(dedent`
import {NgModule} from '@angular/core';

@NgModule({
providers: [
],
})
export class SubModule {}`);
});
});
});
Loading