Skip to content
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

Jump to bottom

feat(compiler-cli): report error if undecorated class with Angular features is discovered #36921

Closed
wants to merge 6 commits into from
Closed

feat(compiler-cli): report error if undecorated class with Angular features is discovered #36921

wants to merge 6 commits into from

Conversation

devversion
Copy link
Member

  • See individual commits.

@devversion devversion force-pushed the v10/backsliding-prevention branch 8 times, most recently from cf2cf15 to f3802bb Compare May 4, 2020 21:35
@devversion devversion mentioned this pull request May 4, 2020
Merged
@devversion devversion force-pushed the v10/backsliding-prevention branch 4 times, most recently from f838fca to 357b52c Compare May 5, 2020 14:48
@devversion devversion added area: compiler Issues related to `ngc`, Angular's template compiler area: core Issues related to the framework runtime action: review The PR is still awaiting reviews from at least one requested reviewer target: patch This PR is targeted for the next patch release labels May 5, 2020
@ngbot ngbot bot modified the milestone: needsTriage May 5, 2020
@devversion devversion marked this pull request as ready for review May 5, 2020 15:27
@devversion
Copy link
Member Author

Note: The components repo unit test job currently fails as a new compiler diagnostic has been introduced in this PR and errors would need to be solved in the components repo.

Regardless of that failing job though, it would be good to receive feedback on the PR.

@alxhub alxhub mentioned this pull request May 6, 2020
Closed
@devversion
feat(core): undecorated-classes-with-decorated-fields migration shoul…
81f1c4e 
…d handle classes with lifecycle hooks

As of v10, undecorated classes using Angular features are no longer
supported. In v10, we plan on removing the undecorated classes
compatibility code in ngtsc. This means that old patterns for
undecorated classes will result in compilation errors.

We had a migration for this in v9 already, but it looks like
the migration does not handle cases where classes uses lifecycle
hooks. This is handled in the ngtsc compatibility code, and we
should handle it similarly in migrations too.

This has not been outlined in the migration plan initially,
but an appendix has been added for v10 to the plan document.

https://hackmd.io/vuQfavzfRG6KUCtU7oK_EA?both.

Note: The migration is unable to determine whether a given undecorated
class that only defines `ngOnDestroy` is a directive or an actual
service. This means that in some cases the migration cannot do
more than adding a TODO and printing an failure.

Certainly there are more ways to determine the type of such classes,
but it would involve metadata and NgModule analysis. This is out of
scope for this migration.
@devversion
feat(compiler-cli): report error if undecorated class with Angular fe…
46ac4d9 
…atures is discovered

Previously in v9, we deprecated the pattern of undecorated base classes
that rely on Angular features. We ran a migration for this in version 9
and will run the same on in version 10 again.

To ensure that projects do not regress and start using the unsupported
pattern again, we report an error in ngtsc if such undecorated classes
are discovered.

We keep the compatibility code enabled in ngcc so that libraries
can be still be consumed, even if they have not been migrated yet.

Resolves FW-2130.
@devversion
refactor: enable ng update migrations for v10
c7aeec0 
Enables the `ng update` migrations for v10. Status for individual
migrations:

**undecorated-classes-with-di**.

This migration dealt exlusively with inherited constructors and
cases where a derived component was undecorated. In those cases,
the migration added `@Directive()` or copied the inherited decorator
to the derived class.

We don't need to run this migration again because ngtsc throws if
constructor is inherited from an undecorated class. Also ngtsc will
throw if a NgModule references an undecorated class in the declarations.

***undecorated-classes-with-decorated-fields***

This migration exclusively deals with undecorated classes that use
Angular features but are not decorated. Angular features include
the use of lifecycle hooks or class fields with Angular decorators,
such as `@Input()`.

We want to re-run this migration in v10 as we will disable the
compatibility code in ngtsc that detects such undecorated classes
as `@Directive`.

**module-with-providers**:

This migration adds an explicit generic type to `ModuleWithProviders`.
As of v10, the generic type is required, so we need to re-run the
migration again.

**renderer-to-renderer2**:

We don't need to re-run that migration again as the
renderer has been already removed in v9.

**missing-injectable**:

This migration is exclusively concerned with undecorated
providers referenced in an `NgModule`. We should re-run
that migration again as we don't have proper backsliding
prevention for this yet. We can consider adding an error
in ngtsc for v10, or v11. In either way, we should re-run
the migration.

**dynamic-queries**:

We ran this one in v9 to reduce code complexity in projects. Instead
of explicitly passing `static: false`, not passing any object literal
has the same semantics. We don't need to re-run the migration again
since there is no good way to prevent backsliding and we cannot always
run this migration for future versions (as some apps might actually
intentionally use the explicit `static: false` option).
@devversion
refactor: fix undecorated classes with angular features in repo
72782b8 
A few instances of undecorated classes with Angular features
have been discovered in the framework repo. This commit fixes
those.
@devversion
build(docs-infra): do not require abstract directives to declare a @N…
2430bd2 
…gModule

Abstract directives cannot be part of a `@NgModule`, but the AIO dgeni
setup currently enforces this. This commit updates the logic so that
abstract directives are skipped.
@devversion
ci: update components-repo-unit-tests job commit
1fcd932 
Updates the `components-repo-unit-tests` job to
angular/components@d3a9ac6.

We need to update since we added a new diagnostic in ngtsc, and
the given commit in the components repo fixes failures caused by
the new diagnostic.

Note: This commit currently points to a PR as it's unlikely that
this fix lands soon, but we want to move forward. There is no
downside to doing that as the PR is based on top of the latest
components repo `master`.
@alxhub alxhub added target: major This PR is targeted for the next major release and removed target: patch This PR is targeted for the next patch release labels May 6, 2020
@alxhub alxhub closed this in c6ecdc9 May 6, 2020
alxhub pushed a commit that referenced this pull request May 6, 2020
@devversion @alxhub
feat(compiler-cli): report error if undecorated class with Angular fe…
4c92cf4 
…atures is discovered (#36921)

Previously in v9, we deprecated the pattern of undecorated base classes
that rely on Angular features. We ran a migration for this in version 9
and will run the same on in version 10 again.

To ensure that projects do not regress and start using the unsupported
pattern again, we report an error in ngtsc if such undecorated classes
are discovered.

We keep the compatibility code enabled in ngcc so that libraries
can be still be consumed, even if they have not been migrated yet.

Resolves FW-2130.

PR Close #36921
alxhub pushed a commit that referenced this pull request May 6, 2020
@devversion @alxhub
refactor: enable ng update migrations for v10 (#36921)
0577bf0 
Enables the `ng update` migrations for v10. Status for individual
migrations:

**undecorated-classes-with-di**.

This migration dealt exlusively with inherited constructors and
cases where a derived component was undecorated. In those cases,
the migration added `@Directive()` or copied the inherited decorator
to the derived class.

We don't need to run this migration again because ngtsc throws if
constructor is inherited from an undecorated class. Also ngtsc will
throw if a NgModule references an undecorated class in the declarations.

***undecorated-classes-with-decorated-fields***

This migration exclusively deals with undecorated classes that use
Angular features but are not decorated. Angular features include
the use of lifecycle hooks or class fields with Angular decorators,
such as `@Input()`.

We want to re-run this migration in v10 as we will disable the
compatibility code in ngtsc that detects such undecorated classes
as `@Directive`.

**module-with-providers**:

This migration adds an explicit generic type to `ModuleWithProviders`.
As of v10, the generic type is required, so we need to re-run the
migration again.

**renderer-to-renderer2**:

We don't need to re-run that migration again as the
renderer has been already removed in v9.

**missing-injectable**:

This migration is exclusively concerned with undecorated
providers referenced in an `NgModule`. We should re-run
that migration again as we don't have proper backsliding
prevention for this yet. We can consider adding an error
in ngtsc for v10, or v11. In either way, we should re-run
the migration.

**dynamic-queries**:

We ran this one in v9 to reduce code complexity in projects. Instead
of explicitly passing `static: false`, not passing any object literal
has the same semantics. We don't need to re-run the migration again
since there is no good way to prevent backsliding and we cannot always
run this migration for future versions (as some apps might actually
intentionally use the explicit `static: false` option).

PR Close #36921
alxhub pushed a commit that referenced this pull request May 6, 2020
@devversion @alxhub
refactor: fix undecorated classes with angular features in repo (#36921)
e17fe90 
A few instances of undecorated classes with Angular features
have been discovered in the framework repo. This commit fixes
those.

PR Close #36921
alxhub pushed a commit that referenced this pull request May 6, 2020
@devversion @alxhub
build(docs-infra): do not require abstract directives to declare a @N…
854bd7d 
…gModule (#36921)

Abstract directives cannot be part of a `@NgModule`, but the AIO dgeni
setup currently enforces this. This commit updates the logic so that
abstract directives are skipped.

PR Close #36921
alxhub pushed a commit that referenced this pull request May 6, 2020
@devversion @alxhub
ci: update components-repo-unit-tests job commit (#36921)
f930e75 
Updates the `components-repo-unit-tests` job to
angular/components@d3a9ac6.

We need to update since we added a new diagnostic in ngtsc, and
the given commit in the components repo fixes failures caused by
the new diagnostic.

Note: This commit currently points to a PR as it's unlikely that
this fix lands soon, but we want to move forward. There is no
downside to doing that as the PR is based on top of the latest
components repo `master`.

PR Close #36921
@IgorMinar IgorMinar mentioned this pull request May 16, 2020
Closed
3 tasks
devversion added a commit to devversion/angular that referenced this pull request May 18, 2020
@devversion
refactor(core): better error for undecorated classes using Angular fe…
acbb0d3 
…atures

In angular#36921, we landed a new diagnostic that is reported whenever the
directive handler discovers undecorated class declarations that use
Angular features.

We landed this before feature freeze, but wanted to follow-up on this
with better DX by providing a more helpful diagnostic. We want to
recommend applying `@Directive` if the class is guaranteed to be a
directive. For cases where it's not clear, we'll recommend that any
arbitrary class Angular decorator works (such as `@Injectable`).

Additionally we move the diagnostic to the class member that has been
detected, and attach diagnostic related information that points to the
class which would need the determined decorator applied.

See:
angular#36921 (comment).
devversion added a commit to devversion/angular that referenced this pull request May 19, 2020
@devversion
refactor(core): better error for undecorated classes using Angular fe…
3f7a4c9 
…atures

In angular#36921, we landed a new diagnostic that is reported whenever the
directive handler discovers undecorated class declarations that use
Angular features.

We landed this before feature freeze, but wanted to follow-up on this
with better DX by providing a more helpful diagnostic. We want to
recommend applying `@Directive` if the class is guaranteed to be a
directive. For cases where it's not clear, we'll recommend that any
arbitrary class Angular decorator works (such as `@Injectable`).

Additionally we move the diagnostic to the class member that has been
detected, and attach diagnostic related information that points to the
class which would need the determined decorator applied.

See:
angular#36921 (comment).
devversion added a commit to devversion/angular that referenced this pull request May 19, 2020
@devversion
refactor(core): better error for undecorated classes using Angular fe…
470b9f3 
…atures

In angular#36921, we landed a new diagnostic that is reported whenever the
directive handler discovers undecorated class declarations that use
Angular features.

We landed this before feature freeze, but wanted to follow-up on this
with better DX by providing a more helpful diagnostic. We want to
recommend applying `@Directive` if the class is guaranteed to be a
directive. For cases where it's not clear, we'll recommend that any
arbitrary class Angular decorator works (such as `@Injectable`).

Additionally we move the diagnostic to the class member that has been
detected, and attach diagnostic related information that points to the
class which would need the determined decorator applied.

See:
angular#36921 (comment).
@angular-automatic-lock-bot
Copy link

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.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Jun 6, 2020
profanis pushed a commit to profanis/angular that referenced this pull request Sep 5, 2020
@devversion @profanis
feat(core): undecorated-classes-with-decorated-fields migration shoul…
d901f14 
…d handle classes with lifecycle hooks (angular#36921)

As of v10, undecorated classes using Angular features are no longer
supported. In v10, we plan on removing the undecorated classes
compatibility code in ngtsc. This means that old patterns for
undecorated classes will result in compilation errors.

We had a migration for this in v9 already, but it looks like
the migration does not handle cases where classes uses lifecycle
hooks. This is handled in the ngtsc compatibility code, and we
should handle it similarly in migrations too.

This has not been outlined in the migration plan initially,
but an appendix has been added for v10 to the plan document.

https://hackmd.io/vuQfavzfRG6KUCtU7oK_EA?both.

Note: The migration is unable to determine whether a given undecorated
class that only defines `ngOnDestroy` is a directive or an actual
service. This means that in some cases the migration cannot do
more than adding a TODO and printing an failure.

Certainly there are more ways to determine the type of such classes,
but it would involve metadata and NgModule analysis. This is out of
scope for this migration.

PR Close angular#36921
profanis pushed a commit to profanis/angular that referenced this pull request Sep 5, 2020
@devversion @profanis
feat(compiler-cli): report error if undecorated class with Angular fe…
884d8b6 
…atures is discovered (angular#36921)

Previously in v9, we deprecated the pattern of undecorated base classes
that rely on Angular features. We ran a migration for this in version 9
and will run the same on in version 10 again.

To ensure that projects do not regress and start using the unsupported
pattern again, we report an error in ngtsc if such undecorated classes
are discovered.

We keep the compatibility code enabled in ngcc so that libraries
can be still be consumed, even if they have not been migrated yet.

Resolves FW-2130.

PR Close angular#36921
profanis pushed a commit to profanis/angular that referenced this pull request Sep 5, 2020
@devversion @profanis
refactor: enable ng update migrations for v10 (angular#36921)
75eefa4 
Enables the `ng update` migrations for v10. Status for individual
migrations:

**undecorated-classes-with-di**.

This migration dealt exlusively with inherited constructors and
cases where a derived component was undecorated. In those cases,
the migration added `@Directive()` or copied the inherited decorator
to the derived class.

We don't need to run this migration again because ngtsc throws if
constructor is inherited from an undecorated class. Also ngtsc will
throw if a NgModule references an undecorated class in the declarations.

***undecorated-classes-with-decorated-fields***

This migration exclusively deals with undecorated classes that use
Angular features but are not decorated. Angular features include
the use of lifecycle hooks or class fields with Angular decorators,
such as `@Input()`.

We want to re-run this migration in v10 as we will disable the
compatibility code in ngtsc that detects such undecorated classes
as `@Directive`.

**module-with-providers**:

This migration adds an explicit generic type to `ModuleWithProviders`.
As of v10, the generic type is required, so we need to re-run the
migration again.

**renderer-to-renderer2**:

We don't need to re-run that migration again as the
renderer has been already removed in v9.

**missing-injectable**:

This migration is exclusively concerned with undecorated
providers referenced in an `NgModule`. We should re-run
that migration again as we don't have proper backsliding
prevention for this yet. We can consider adding an error
in ngtsc for v10, or v11. In either way, we should re-run
the migration.

**dynamic-queries**:

We ran this one in v9 to reduce code complexity in projects. Instead
of explicitly passing `static: false`, not passing any object literal
has the same semantics. We don't need to re-run the migration again
since there is no good way to prevent backsliding and we cannot always
run this migration for future versions (as some apps might actually
intentionally use the explicit `static: false` option).

PR Close angular#36921
profanis pushed a commit to profanis/angular that referenced this pull request Sep 5, 2020
@devversion @profanis
refactor: fix undecorated classes with angular features in repo (angu…
4b1122a 
…lar#36921)

A few instances of undecorated classes with Angular features
have been discovered in the framework repo. This commit fixes
those.

PR Close angular#36921
profanis pushed a commit to profanis/angular that referenced this pull request Sep 5, 2020
@devversion @profanis
build(docs-infra): do not require abstract directives to declare a @N…
64eafaa 
…gModule (angular#36921)

Abstract directives cannot be part of a `@NgModule`, but the AIO dgeni
setup currently enforces this. This commit updates the logic so that
abstract directives are skipped.

PR Close angular#36921
profanis pushed a commit to profanis/angular that referenced this pull request Sep 5, 2020
@devversion @profanis
ci: update components-repo-unit-tests job commit (angular#36921)
9415336 
Updates the `components-repo-unit-tests` job to
angular/components@d3a9ac6.

We need to update since we added a new diagnostic in ngtsc, and
the given commit in the components repo fixes failures caused by
the new diagnostic.

Note: This commit currently points to a PR as it's unlikely that
this fix lands soon, but we want to move forward. There is no
downside to doing that as the PR is based on top of the latest
components repo `master`.

PR Close angular#36921
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@petebacondarwin petebacondarwin petebacondarwin approved these changes

@IgorMinar IgorMinar IgorMinar approved these changes

@atscott atscott atscott approved these changes

@kara kara kara approved these changes

@alxhub alxhub alxhub approved these changes

@AndrewKushnir AndrewKushnir AndrewKushnir approved these changes

Assignees
No one assigned
Labels
action: merge The PR is ready for merge by the caretaker action: presubmit The PR is in need of a google3 presubmit area: compiler Issues related to `ngc`, Angular's template compiler area: core Issues related to the framework runtime cla: yes target: major This PR is targeted for the next major release
Projects
None yet
Milestone
needsTriage
Development

Successfully merging this pull request may close these issues.

None yet
8 participants
@devversion @petebacondarwin @IgorMinar @atscott @kara @alxhub @AndrewKushnir @googlebot