-
Notifications
You must be signed in to change notification settings - Fork 24.8k
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.
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
fix(ivy): ngcc - several fixes related to decorators #31614
Conversation
b0b81c0
to
70c9acd
Compare
9bca1a9
to
8e41d0b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work @JoostK - some minor naming suggestions.
Any decorator information present in TypeScript is emitted into the generated JavaScript sources by means of `__decorate` call. This call contains both the decorators as they existed in the original source code, together with calls to `tslib` helpers that convey additional information on e.g. type information and parameter decorators. These different kinds of decorator calls were not previously distinguished on their own, but instead all treated as `Decorator` by themselves. The "decorators" that were actually `tslib` helper calls were conveniently filtered out because they were not imported from `@angular/core`, a characteristic that ngcc uses to drop certain decorators. Note that this posed an inconsistency in ngcc when it processes `@angular/core`'s UMD bundle, as the `tslib` helper functions have been inlined in said bundle. Because of the inlining, the `tslib` helpers appear to be from `@angular/core`, so ngcc would fail to drop those apparent "decorators". This inconsistency does not currently cause any issues, as ngtsc is specifically looking for decorators based on their name and any remaining decorators are simply ignored. This commit rewrites the decorator analysis of a class to occur all in a single phase, instead of all throughout the `ReflectionHost`. This allows to categorize the various decorate calls in a single sweep, instead of constantly needing to filter out undesired decorate calls on the go. As an added benefit, the computed decorator information is now cached per class, such that subsequent reflection queries that need decorator information can reuse the cached info.
An identifier may become repeated when bundling multiple source files into a single bundle, so bundlers have a strategy of suffixing non-unique identifiers with a suffix like $2. Since ngcc operates on such bundles, it needs to process potentially suffixed identifiers in their canonical form without the suffix. The "ngx-pagination" package was previously not compiled fully, as most decorators were not recognized. This commit ensures that identifiers are first canonicalized by removing the suffix, such that they are properly recognized and processed by ngcc. Fixes angular#31540
8e41d0b
to
305e0b9
Compare
@@ -951,23 +951,6 @@ exports.ExternalModule = ExternalModule; | |||
expect(decorators[0]).toEqual(jasmine.objectContaining({name: 'Directive'})); | |||
}); | |||
|
|||
it('should use `getImportOfIdentifier()` to retrieve import info', () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I decided to delete these tests as the refactor into a single analysis phase affects the interaction with the spy, making this test brittle and not really of added value, IMHO. Assertions around the import location of a decorator have been added to other tests, where necessary.
In angular#31426 a fix was implemented to render namespaced decorator imports correctly, however it turns out that the fix only worked when decorator information was extracted from static properties, not when using `__decorate` calls. This commit fixes the issue by creating the decorator metadata with the full decorator expression, instead of only its name. Closes angular#31394
8b1b0b1
to
3ddd6bc
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome PR @JoostK - the first commit is a thing of beauty.
Approved with a few minor nits.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💯
…ngular#31614) Any decorator information present in TypeScript is emitted into the generated JavaScript sources by means of `__decorate` call. This call contains both the decorators as they existed in the original source code, together with calls to `tslib` helpers that convey additional information on e.g. type information and parameter decorators. These different kinds of decorator calls were not previously distinguished on their own, but instead all treated as `Decorator` by themselves. The "decorators" that were actually `tslib` helper calls were conveniently filtered out because they were not imported from `@angular/core`, a characteristic that ngcc uses to drop certain decorators. Note that this posed an inconsistency in ngcc when it processes `@angular/core`'s UMD bundle, as the `tslib` helper functions have been inlined in said bundle. Because of the inlining, the `tslib` helpers appear to be from `@angular/core`, so ngcc would fail to drop those apparent "decorators". This inconsistency does not currently cause any issues, as ngtsc is specifically looking for decorators based on their name and any remaining decorators are simply ignored. This commit rewrites the decorator analysis of a class to occur all in a single phase, instead of all throughout the `ReflectionHost`. This allows to categorize the various decorate calls in a single sweep, instead of constantly needing to filter out undesired decorate calls on the go. As an added benefit, the computed decorator information is now cached per class, such that subsequent reflection queries that need decorator information can reuse the cached info. PR Close angular#31614
An identifier may become repeated when bundling multiple source files into a single bundle, so bundlers have a strategy of suffixing non-unique identifiers with a suffix like $2. Since ngcc operates on such bundles, it needs to process potentially suffixed identifiers in their canonical form without the suffix. The "ngx-pagination" package was previously not compiled fully, as most decorators were not recognized. This commit ensures that identifiers are first canonicalized by removing the suffix, such that they are properly recognized and processed by ngcc. Fixes angular#31540 PR Close angular#31614
…tly (angular#31614) In angular#31426 a fix was implemented to render namespaced decorator imports correctly, however it turns out that the fix only worked when decorator information was extracted from static properties, not when using `__decorate` calls. This commit fixes the issue by creating the decorator metadata with the full decorator expression, instead of only its name. Closes angular#31394 PR Close angular#31614
This issue has been automatically locked due to inactivity. Read more about our automatic conversation locking policy. This action has been performed automatically by a bot. |
refactor(ivy): ngcc - categorize the various decorate calls upfront
Any decorator information present in TypeScript is emitted into the
generated JavaScript sources by means of
__decorate
call. This callcontains both the decorators as they existed in the original source
code, together with calls to
tslib
helpers that convey additionalinformation on e.g. type information and parameter decorators. These
different kinds of decorator calls were not previously distinguished on
their own, but instead all treated as
Decorator
by themselves. The"decorators" that were actually
tslib
helper calls were convenientlyfiltered out because they were not imported from
@angular/core
, acharacteristic that ngcc uses to drop certain decorators.
Note that this posed an inconsistency in ngcc when it processes
@angular/core
's UMD bundle, as thetslib
helper functions have beeninlined in said bundle. Because of the inlining, the
tslib
helpersappear to be from
@angular/core
, so ngcc would fail to drop thoseapparent "decorators". This inconsistency does not currently cause any
issues, as ngtsc is specifically looking for decorators based on their
name and any remaining decorators are simply ignored.
This commit rewrites the decorator analysis of a class to occur all in a
single phase, instead of all throughout the
ReflectionHost
. Thisallows to categorize the various decorate calls in a single sweep,
instead of constantly needing to filter out undesired decorate calls on
the go. As an added benefit, the computed decorator information is now
cached per class, such that subsequent reflection queries that need
decorator information can reuse the cached info.
fix(ivy): ngcc - recognize suffixed tslib helpers
An identifier may become repeated when bundling multiple source files
into a single bundle, so bundlers have a strategy of suffixing non-unique
identifiers with a suffix like $2. Since ngcc operates on such bundles,
it needs to process potentially suffixed identifiers in their canonical
form without the suffix. The "ngx-pagination" package was previously not
compiled fully, as most decorators were not recognized.
This commit ensures that identifiers are first canonicalized by removing
the suffix, such that they are properly recognized and processed by ngcc.
Fixes #31540
fix(ivy): ngcc - render decorators in UMD and CommonJS bundles correctly
In #31426 a fix was implemented to render namespaced decorator imports
correctly, however it turns out that the fix only worked when decorator
information was extracted from static properties, not when using
__decorate
calls.This commit fixes the issue by creating the decorator metadata with the
full decorator expression, instead of only its name.
Closes #31394