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

devversion · 2019-11-15T12:58:15Z

We currently do not properly handle the scenario where a project
uses a custom gesture config with event names matching the ones
provided by the deprecated Angular Material gesture config.

Prior to this commit, the migration would set up the Material
gesture config next to the custom one. This is incorrect, and
we should do nothing since a gesture config is already configured.

Though, if we detect that there are manual references to the
Angular Material gesture config, the detection is ambiguous
and the migration should warn that the developer should manually
remove the references to the deprecated Material gesture config.

Also improves messaging for the migration, adds a description
of the expected migration behavior into the migration rule and
adds safety checks for ts.Symbol#declaration (should never
be undefined according to typing, but for odd reasons it can be
undefined, and we do not want to throw accidentally).

devversion · 2019-11-15T13:40:10Z

Adding P1 label since this should go into the next possible RC.

…estures We currently do not properly handle the scenario where a project uses a custom gesture config with event names matching the ones provided by the deprecated Angular Material gesture config. Prior to this commit, the migration would set up the Material gesture config next to the custom one. This is incorrect, and we should do nothing since a gesture config is already configured. Though, if we detect that there are manual references to the Angular Material gesture config, the detection is ambiguous and the migration should warn that the developer should manually remove the references to the deprecated Material gesture config. Also improves messaging for the migration, adds a description of the expected migration behavior into the migration rule and adds safety checks for `ts.Symbol#declaration` (should never be undefined according to typing, but for odd reasons it can be undefined, and we do not want to throw accidentally).

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

Address feedback

andrewseguin

LGTM

Splaktar

Sorry for leaving some of the comments in the test instead of the code, but that's where I ran into the text first 😄

I think that we need to do more to avoid confusing and frustrating developers with this migration. I feel like we need a docs page for this migration similar to what angular.io has. There isn't enough actionable information in the current warnings and a docs page would allow us to provide that without having overly verbose ng update migration output.

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

Splaktar · 2019-11-17T00:29:27Z

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

+      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.');


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.

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.

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.

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

Splaktar · 2019-11-17T00:42:15Z

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

          'The HammerJS v9 migration for Angular components migrated the ' +
          'project to keep HammerJS installed, but detected ambiguous usage of HammerJS. Please ' +
-          'manually check if you can remove HammerJS from your application.'));
+          'manually check if you can remove HammerJS from your application.');


Is there a docs page/section that we can point them to? How will they know if they can remove HammerJS from their app?

Good question. I asked in Slack. Let's track that there.

Follow up feedback for the HammerJS v9 migration (based on comments by Michael in angular#17713)

Follow up feedback for the HammerJS v9 migration (based on comments by Michael in #17713)

angular-automatic-lock-bot · 2019-12-19T00:01:25Z

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

_{This action has been performed automatically by a bot.}

googlebot added the cla: yes PR author has agreed to Google's Contributor License Agreement label Nov 15, 2019

devversion force-pushed the refactor/hammer-v9-migration-do-not-setup-copied-config-if-custom-is-present branch from 570cee9 to 5ef3483 Compare November 15, 2019 13:18

devversion marked this pull request as ready for review November 15, 2019 13:27

devversion requested review from jelbourn and a team as code owners November 15, 2019 13:27

devversion assigned mmalerba and andrewseguin Nov 15, 2019

devversion added P2 The issue is important to a large percentage of users, with a workaround target: patch This PR is targeted for the next patch release labels Nov 15, 2019

devversion added this to the 9.0.0 milestone Nov 15, 2019

devversion added P1 Impacts a large percentage of users; if a workaround exists it is partial or overly painful and removed P2 The issue is important to a large percentage of users, with a workaround labels Nov 15, 2019

devversion force-pushed the refactor/hammer-v9-migration-do-not-setup-copied-config-if-custom-is-present branch from 5ef3483 to d8d7ea2 Compare November 15, 2019 13:53

Fix comment wording

7675e99

mmalerba approved these changes Nov 15, 2019

View reviewed changes

src/material/schematics/ng-update/upgrade-rules/hammer-gestures-v9/hammer-gestures-rule.ts Outdated Show resolved Hide resolved

src/material/schematics/ng-update/upgrade-rules/hammer-gestures-v9/hammer-gestures-rule.ts Outdated Show resolved Hide resolved

fixup! Fix comment wording

676a13d

Address feedback

mmalerba added pr: lgtm action: merge The PR is ready for merge by the caretaker labels Nov 15, 2019

andrewseguin approved these changes Nov 15, 2019

View reviewed changes

andrewseguin merged commit 4794c60 into angular:master Nov 15, 2019

Splaktar reviewed Nov 17, 2019

View reviewed changes

devversion added a commit to devversion/material2 that referenced this pull request Nov 18, 2019

refactor(ng-update): follow-up cleanup of hammer v9 migration

462746d

Follow up feedback for the HammerJS v9 migration (based on comments by Michael in angular#17713)

devversion mentioned this pull request Nov 18, 2019

refactor(ng-update): follow-up cleanup of hammer v9 migration #17739

Merged

jelbourn pushed a commit that referenced this pull request Nov 18, 2019

refactor(ng-update): follow-up cleanup of hammer v9 migration (#17739)

37015ac

Follow up feedback for the HammerJS v9 migration (based on comments by Michael in #17713)

angular-automatic-lock-bot bot locked and limited conversation to collaborators Dec 19, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

devversion commented Nov 15, 2019 •

edited

devversion commented Nov 15, 2019

andrewseguin left a comment

Splaktar left a comment

Splaktar Nov 17, 2019

devversion Nov 18, 2019

Splaktar Nov 18, 2019

Splaktar Nov 17, 2019

devversion Nov 18, 2019

angular-automatic-lock-bot bot commented Dec 19, 2019

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

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

Conversation

devversion commented Nov 15, 2019 • edited

devversion commented Nov 15, 2019

andrewseguin left a comment

Choose a reason for hiding this comment

Splaktar left a comment

Choose a reason for hiding this comment

Splaktar Nov 17, 2019

Choose a reason for hiding this comment

devversion Nov 18, 2019

Choose a reason for hiding this comment

Splaktar Nov 18, 2019

Choose a reason for hiding this comment

Splaktar Nov 17, 2019

Choose a reason for hiding this comment

devversion Nov 18, 2019

Choose a reason for hiding this comment

angular-automatic-lock-bot bot commented Dec 19, 2019

devversion commented Nov 15, 2019 •

edited