fix(core): undecorated-classes-with-decorated-fields migration does not decorate derived classes #35339

devversion · 2020-02-11T15:37:46Z

The undecorated-classes-with-decorated-fields migration has been
introduced with 904a201, but misses logic for decorating derived classes
of undecorated classes which use Angular features. Example scenario:

export abstract class MyBaseClass {
  @input() someInput = true;
}

export abstract class BaseClassTwo extends MyBaseClass {}

@component(...)
export class MyButton extends BaseClassTwo {}

Both abstract classes would need to be migrated. Previously, the migration
only added @Directive() to MyBaseClass, but with this change, it also
decorates BaseClassTwo.

This is necessary because the Angular Compiler requires BaseClassTwo to
have a directive definition when it flattens the directive metadata for
MyButton in order to perform type checking. Technically, not decorating
BaseClassTwo does not break at runtime because the inherit definition feature
walks up the prototype chain. Though we want to enforce consistent use of
@Directive to simplify the mental model. See the migration guide for motivation.

Fixes #34376.

Note: It should be easier and less verbose to to review the commits individually.

crisbeto

A couple of nits, but otherwise LGTM.

...atics/migrations/undecorated-classes-with-decorated-fields/google3/tslint_update_recorder.ts

packages/core/schematics/migrations/undecorated-classes-with-decorated-fields/transform.ts

Moves the `findBaseClassDeclarations` method into the shared schematic utilities. This method will be useful for future migrations, and for planned changes to the `undecorated-classes-with-decorated-fields` migration.

The import manager has been created for both the `missing-injectable` and `undecorated-classes-with-di` migration. Both initial PRs brought in the manager class, so the manager is duplicated in the schematics. In order to reduce this duplication, and to expose the manager to other schematics/migrations, we move it into the shared schematic utils.

@input

…ot decorate derived classes The `undecorated-classes-with-decorated-fields` migration has been introduced with 904a201, but misses logic for decorating derived classes of undecorated classes which use Angular features. Example scenario: ```ts export abstract class MyBaseClass { @input() someInput = true; } export abstract class BaseClassTwo extends MyBaseClass {} @component(...) export class MyButton extends BaseClassTwo {} ``` Both abstract classes would need to be migrated. Previously, the migration only added `@Directive()` to `MyBaseClass`, but with this change, it also decorates `BaseClassTwo`. This is necessary because the Angular Compiler requires `BaseClassTwo` to have a directive definition when it flattens the directive metadata for `MyButton` in order to perform type checking. Technically, not decorating `BaseClassTwo` does not break at runtime. We basically want to enforce consistent use of `@Directive` to simplify the mental model. [See the migration guide](https://angular.io/guide/migration-undecorated-classes#migrating-classes-that-use-field-decorators). Fixes angular#34376.

…lds migration We don't have an integration test for the `undecorated-classes-with-decorated-fields migration. For consistency and to cover for the latest changes, we add it to the `ng update` integration test.

@directive

…act classes In version 10, undecorated base classes that use Angular features need to be decorated explicitly with `@Directive()`. Additionally, derived classes of abstract directives need to be decorated. The migration already handles this for undecorated classes that are not explicitly decorated, but since in V9, abstract directives can be used, we also need to handle this for explicitly decorated abstract directives. e.g. ``` @directive() export class Base {...} // needs to be decorated by migration when updating from v9 to v10 export class Wrapped extends Base {} @component(...) export class Cmp extends Wrapped {} ```

devversion · 2020-03-23T16:49:31Z

@alxhub A similar discussion came up for this in the ngcc channel. It looks like it's unclear whether intermediary classes (as shown in the PR description) need to be decorated or not. It might be good to have a look at that discussion, so that we can have a consistent solution for both ngcc and ng update.

josephperrott

LGTM

Reviewed-for: integration-tests

josephperrott

LGTM

Reviewed-for: dev-infra

devversion · 2020-03-31T10:47:07Z

Caretaker: Can you please run presubmit for this? I'm unsure whether schematic code is synced into g3. If it is not, then we should update the bot to exclude the folder.

kara · 2020-04-01T22:28:00Z

presubmit

The import manager has been created for both the `missing-injectable` and `undecorated-classes-with-di` migration. Both initial PRs brought in the manager class, so the manager is duplicated in the schematics. In order to reduce this duplication, and to expose the manager to other schematics/migrations, we move it into the shared schematic utils. PR Close #35339

@input

…ot decorate derived classes (#35339) The `undecorated-classes-with-decorated-fields` migration has been introduced with 904a201, but misses logic for decorating derived classes of undecorated classes which use Angular features. Example scenario: ```ts export abstract class MyBaseClass { @input() someInput = true; } export abstract class BaseClassTwo extends MyBaseClass {} @component(...) export class MyButton extends BaseClassTwo {} ``` Both abstract classes would need to be migrated. Previously, the migration only added `@Directive()` to `MyBaseClass`, but with this change, it also decorates `BaseClassTwo`. This is necessary because the Angular Compiler requires `BaseClassTwo` to have a directive definition when it flattens the directive metadata for `MyButton` in order to perform type checking. Technically, not decorating `BaseClassTwo` does not break at runtime. We basically want to enforce consistent use of `@Directive` to simplify the mental model. [See the migration guide](https://angular.io/guide/migration-undecorated-classes#migrating-classes-that-use-field-decorators). Fixes #34376. PR Close #35339

…lds migration (#35339) We don't have an integration test for the `undecorated-classes-with-decorated-fields migration. For consistency and to cover for the latest changes, we add it to the `ng update` integration test. PR Close #35339

@directive

…act classes (#35339) In version 10, undecorated base classes that use Angular features need to be decorated explicitly with `@Directive()`. Additionally, derived classes of abstract directives need to be decorated. The migration already handles this for undecorated classes that are not explicitly decorated, but since in V9, abstract directives can be used, we also need to handle this for explicitly decorated abstract directives. e.g. ``` @directive() export class Base {...} // needs to be decorated by migration when updating from v9 to v10 export class Wrapped extends Base {} @component(...) export class Cmp extends Wrapped {} ``` PR Close #35339

…35339) Moves the `findBaseClassDeclarations` method into the shared schematic utilities. This method will be useful for future migrations, and for planned changes to the `undecorated-classes-with-decorated-fields` migration. PR Close #35339

The import manager has been created for both the `missing-injectable` and `undecorated-classes-with-di` migration. Both initial PRs brought in the manager class, so the manager is duplicated in the schematics. In order to reduce this duplication, and to expose the manager to other schematics/migrations, we move it into the shared schematic utils. PR Close #35339

@input

…ot decorate derived classes (#35339) The `undecorated-classes-with-decorated-fields` migration has been introduced with 904a201, but misses logic for decorating derived classes of undecorated classes which use Angular features. Example scenario: ```ts export abstract class MyBaseClass { @input() someInput = true; } export abstract class BaseClassTwo extends MyBaseClass {} @component(...) export class MyButton extends BaseClassTwo {} ``` Both abstract classes would need to be migrated. Previously, the migration only added `@Directive()` to `MyBaseClass`, but with this change, it also decorates `BaseClassTwo`. This is necessary because the Angular Compiler requires `BaseClassTwo` to have a directive definition when it flattens the directive metadata for `MyButton` in order to perform type checking. Technically, not decorating `BaseClassTwo` does not break at runtime. We basically want to enforce consistent use of `@Directive` to simplify the mental model. [See the migration guide](https://angular.io/guide/migration-undecorated-classes#migrating-classes-that-use-field-decorators). Fixes #34376. PR Close #35339

…lds migration (#35339) We don't have an integration test for the `undecorated-classes-with-decorated-fields migration. For consistency and to cover for the latest changes, we add it to the `ng update` integration test. PR Close #35339

@directive

…act classes (#35339) In version 10, undecorated base classes that use Angular features need to be decorated explicitly with `@Directive()`. Additionally, derived classes of abstract directives need to be decorated. The migration already handles this for undecorated classes that are not explicitly decorated, but since in V9, abstract directives can be used, we also need to handle this for explicitly decorated abstract directives. e.g. ``` @directive() export class Base {...} // needs to be decorated by migration when updating from v9 to v10 export class Wrapped extends Base {} @component(...) export class Cmp extends Wrapped {} ``` PR Close #35339

angular-automatic-lock-bot · 2020-05-03T16:05:58Z

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.}

devversion added action: discuss action: review The PR is still awaiting reviews from at least one requested reviewer area: core Issues related to the framework runtime target: patch This PR is targeted for the next patch release labels Feb 11, 2020

devversion assigned kara and crisbeto Feb 11, 2020

ngbot bot added this to the needsTriage milestone Feb 11, 2020

pullapprove bot requested review from AndrewKushnir and IgorMinar February 11, 2020 15:37

googlebot added the cla: yes label Feb 11, 2020

devversion force-pushed the fix/migration-improvements branch from 3c485da to 2a6e547 Compare February 13, 2020 18:36

devversion marked this pull request as ready for review February 13, 2020 19:02

devversion removed the action: discuss label Feb 13, 2020

devversion force-pushed the fix/migration-improvements branch 3 times, most recently from 1c406ad to ea27239 Compare February 14, 2020 17:59

devversion removed request for IgorMinar and AndrewKushnir February 14, 2020 18:01

devversion assigned alxhub Feb 14, 2020

pullapprove bot requested review from AndrewKushnir and IgorMinar February 14, 2020 18:01

crisbeto approved these changes Feb 14, 2020

View reviewed changes

...atics/migrations/undecorated-classes-with-decorated-fields/google3/tslint_update_recorder.ts Show resolved Hide resolved

packages/core/schematics/migrations/undecorated-classes-with-decorated-fields/transform.ts Show resolved Hide resolved

devversion added 5 commits February 19, 2020 19:05

refactor(core): move schematic base classes logic into shared utils

58eb4f9

Moves the `findBaseClassDeclarations` method into the shared schematic utilities. This method will be useful for future migrations, and for planned changes to the `undecorated-classes-with-decorated-fields` migration.

test: add integration test for undecorated-classes-with-decorated-fie…

ead3327

…lds migration We don't have an integration test for the `undecorated-classes-with-decorated-fields migration. For consistency and to cover for the latest changes, we add it to the `ng update` integration test.

devversion force-pushed the fix/migration-improvements branch from ea27239 to 304c0df Compare February 19, 2020 18:05

alxhub approved these changes Mar 24, 2020

View reviewed changes

devversion requested review from josephperrott and removed request for AndrewKushnir March 24, 2020 22:53

devversion assigned josephperrott and unassigned kara, alxhub and crisbeto Mar 24, 2020

josephperrott approved these changes Mar 24, 2020

View reviewed changes

josephperrott removed the request for review from IgorMinar March 24, 2020 23:07

pullapprove bot requested a review from IgorMinar March 24, 2020 23:07

josephperrott approved these changes Mar 24, 2020

View reviewed changes

devversion added action: merge The PR is ready for merge by the caretaker and removed action: review The PR is still awaiting reviews from at least one requested reviewer labels Mar 25, 2020

devversion removed the request for review from IgorMinar March 25, 2020 14:34

devversion added action: presubmit The PR is in need of a google3 presubmit merge: caretaker note Alert the caretaker performing the merge to check the PR for an out of normal action needed or note labels Mar 30, 2020

kara removed the action: presubmit The PR is in need of a google3 presubmit label Apr 2, 2020

kara closed this in 8e55a11 Apr 2, 2020

angular-automatic-lock-bot bot locked and limited conversation to collaborators May 3, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

fix(core): undecorated-classes-with-decorated-fields migration does not decorate derived classes #35339

fix(core): undecorated-classes-with-decorated-fields migration does not decorate derived classes #35339

Uh oh!

devversion commented Feb 11, 2020 •

edited

Loading

Uh oh!

crisbeto left a comment

Uh oh!

Uh oh!

Uh oh!

devversion commented Mar 23, 2020

Uh oh!

josephperrott left a comment

Uh oh!

josephperrott left a comment

Uh oh!

devversion commented Mar 31, 2020

Uh oh!

kara commented Apr 1, 2020

Uh oh!

angular-automatic-lock-bot bot commented May 3, 2020

Uh oh!

Uh oh!

fix(core): undecorated-classes-with-decorated-fields migration does not decorate derived classes #35339

fix(core): undecorated-classes-with-decorated-fields migration does not decorate derived classes #35339

Uh oh!

Conversation

devversion commented Feb 11, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

crisbeto left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

devversion commented Mar 23, 2020

Uh oh!

josephperrott left a comment

Choose a reason for hiding this comment

Uh oh!

josephperrott left a comment

Choose a reason for hiding this comment

Uh oh!

devversion commented Mar 31, 2020

Uh oh!

kara commented Apr 1, 2020

Uh oh!

angular-automatic-lock-bot bot commented May 3, 2020

Uh oh!

Uh oh!

devversion commented Feb 11, 2020 •

edited

Loading