Improve error handling for custom/duplicate decorators #54139
+189
−31
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
pmvald
added
action: review
pmvald
force-pushed
the
orphan-comps
branch
3 times, most recently
from
90cb601
to
1f3280a
Compare
pmvald
changed the title
MeftunH
reviewed
Jan 29, 2024
Comment on lines
+754
to
+747
| if (isCore) { | ||
| return decorator.name === 'Injectable'; | ||
| } else if (decorator.import !== null && decorator.import.from === '@angular/core') { | ||
| return decorator.import.name === 'Injectable'; | ||
| } |
There was a problem hiding this comment.
I think this is unclear usage. So, it makes harder to find changes for next versions
There was a problem hiding this comment.
I'm not sure I follow. Can you elaborate please?
Comment on lines
+1170
to
1185
| semanticDepGraphUpdater, this.adapter, isCore, !!this.options.forbidOrphanComponents); | ||
|
|
There was a problem hiding this comment.
why we did use magic operator in here?
devversion
reviewed
Jan 29, 2024
| // reason for both cases is that the `DepsTracker` is not able to understand components mutated | ||
| // by decorators. Once the `DepsTracker` is fixed then these issues will be resolved. This issue | ||
| // is to be fixed soon (b/320536434). | ||
| const unmatchedDecoratorsInLocalMode = |
There was a problem hiding this comment.
Suggested change
| const unmatchedDecoratorsInLocalMode = | |
| const unmatchedDecorators = |
Regarding "fixing deps tracker". There is no fix we are landing right? except hoping for ES decorators at some point. The comment sound like deps tracker needs a fix.
There was a problem hiding this comment.
Done!
It is possible to fix this issue. It requires some emit shenanigans to somehow connect a decorated class to its pre-decorated version. I have a solution in mind. We can do this if the number of use cases increases. Right now I'm aware of one app using custom decorators for components (and even that one marked the decorator as deprecated as plan to remove).
There was a problem hiding this comment.
I was just curious because of the comment sounding like we intend to allow this in the future again. Maybe remove?
Comment on lines
+754
to
+747
| if (isCore) { | ||
| return decorator.name === 'Injectable'; | ||
| } else if (decorator.import !== null && decorator.import.from === '@angular/core') { | ||
| return decorator.import.name === 'Injectable'; | ||
| } |
There was a problem hiding this comment.
I'm not sure I follow. Can you elaborate please?
packages/compiler-cli/src/ngtsc/transform/test/compilation_spec.ts
Outdated
Show resolved
Hide resolved
…in local compilation diagnostics For cases like this: ``` @component({...}) @component({...}) export class SomeComp { } ``` The `DecoratorHandler.detect` apparantly matches only one of the `@Component` decorator, leaving the other undetected which will be transformed by TS decorator helper and that breaks local compilation runtimes. But the error message only mentioned "custom" decorator, while in this case it is a "duplicate Angular" decorator. The respective error message is updated thus.
devversion
approved these changes
Feb 1, 2024
…classes in local compilation mode Custom/duplicate decorators break the deps tracker in local mode. But deps tracker only deals with non-injectable classes. So applying custom/duplicate decorators to `@Injectable` only classes does not disturb deps tracker and local compilation in general. There are also ~ 100 such cases in g3 which cannot be cleaned up.
…bidOrphanComponents` is set The deps tracker which is responsible to track orphan components does not work for classes mutated by custom decorator. Some work needed to make this happen (tracked in b/320536434). As a result, with option `forbidOrphanComponents` being true the deps tracker will falsely report any component as orphan if it or its NgModule have custom/duplicate decorators. So it is unsafe to use this option in the presence of custom/duplicate decorator and we disable it until it is made compatible. Note that applying custom/duplicate decorators to `@Injectable` classes is ok since these classes never make it into the deps tracker. So we excempt them.
pmvald
added
action: merge
jessicajaniuk
pushed a commit
that referenced
this pull request
Feb 5, 2024
…classes in local compilation mode (#54139) Custom/duplicate decorators break the deps tracker in local mode. But deps tracker only deals with non-injectable classes. So applying custom/duplicate decorators to `@Injectable` only classes does not disturb deps tracker and local compilation in general. There are also ~ 100 such cases in g3 which cannot be cleaned up. PR Close #54139
jessicajaniuk
pushed a commit
that referenced
this pull request
Feb 5, 2024
…bidOrphanComponents` is set (#54139) The deps tracker which is responsible to track orphan components does not work for classes mutated by custom decorator. Some work needed to make this happen (tracked in b/320536434). As a result, with option `forbidOrphanComponents` being true the deps tracker will falsely report any component as orphan if it or its NgModule have custom/duplicate decorators. So it is unsafe to use this option in the presence of custom/duplicate decorator and we disable it until it is made compatible. Note that applying custom/duplicate decorators to `@Injectable` classes is ok since these classes never make it into the deps tracker. So we excempt them. PR Close #54139
|
This PR was merged into the repository by commit 96bcf4f. |
jessicajaniuk
pushed a commit
to jessicajaniuk/angular
that referenced
this pull request
Feb 5, 2024
…ion `forbidOrphanComponents` is set (angular#54139)" This reverts commit 96bcf4f.
jessicajaniuk
pushed a commit
to jessicajaniuk/angular
that referenced
this pull request
Feb 5, 2024
…ectable classes in local compilation mode (angular#54139)" This reverts commit a592904.
jessicajaniuk
pushed a commit
to jessicajaniuk/angular
that referenced
this pull request
Feb 5, 2024
…orators in local compilation diagnostics (angular#54139)" This reverts commit 4b1d948.
jessicajaniuk
pushed a commit
that referenced
this pull request
Feb 5, 2024
jessicajaniuk
pushed a commit
that referenced
this pull request
Feb 5, 2024
jessicajaniuk
pushed a commit
that referenced
this pull request
Feb 5, 2024
|
@pmvald This caused a breakage in G3 and has been reverted. |
jessicajaniuk
added
state: blocked
action: global presubmit
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
PR Checklist
Please check if your PR fulfills the following requirements:
PR Type
What kind of change does this PR introduce?
What is the current behavior?
Issue Number: N/A
What is the new behavior?
Does this PR introduce a breaking change?
Other information