fix(ivy): ngcc - several fixes related to decorators by JoostK · Pull Request #31614 · angular/angular

JoostK · 2019-07-17T20:12:15Z

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

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

petebacondarwin

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

JoostK · 2019-07-26T21:31:26Z

          expect(decorators[0]).toEqual(jasmine.objectContaining({name: 'Directive'}));
        });

-        it('should use `getImportOfIdentifier()` to retrieve import info', () => {


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

petebacondarwin

Awesome PR @JoostK - the first commit is a thing of beauty.
Approved with a few minor nits.

…front

gkalpak

💯

… correctly

…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

angular-automatic-lock-bot · 2019-09-15T08:52:37Z

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

JoostK added type: bug/fix action: review The PR is still awaiting reviews from at least one requested reviewer effort1: hours workaround4: none freq1: low severity3: broken target: major This PR is targeted for the next major release risk: low labels Jul 17, 2019

JoostK requested review from a team July 17, 2019 20:12

ngbot Bot modified the milestone: Backlog Jul 17, 2019

googlebot added the cla: yes label Jul 17, 2019

JoostK force-pushed the ngcc-suffixed-helpers branch from b0b81c0 to 70c9acd Compare July 17, 2019 20:14

petebacondarwin reviewed Jul 18, 2019

View reviewed changes

Comment thread packages/compiler-cli/ngcc/src/host/esm2015_host.ts Resolved

JoostK force-pushed the ngcc-suffixed-helpers branch 4 times, most recently from 9bca1a9 to 8e41d0b Compare July 21, 2019 20:05

JoostK requested a review from petebacondarwin July 22, 2019 05:25

petebacondarwin approved these changes Jul 22, 2019

View reviewed changes

Comment thread packages/compiler-cli/ngcc/src/host/esm2015_host.ts Resolved

Comment thread packages/compiler-cli/ngcc/src/host/esm2015_host.ts Resolved

Comment thread packages/compiler-cli/ngcc/test/helpers/utils.ts Resolved

petebacondarwin added the action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews label Jul 22, 2019

JoostK added 2 commits July 26, 2019 23:26

JoostK force-pushed the ngcc-suffixed-helpers branch from 8e41d0b to 305e0b9 Compare July 26, 2019 21:27

JoostK commented Jul 26, 2019

View reviewed changes

JoostK removed the action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews label Jul 26, 2019

JoostK requested review from a team and petebacondarwin July 27, 2019 09:32

JoostK changed the title ~~fix(ivy): ngcc - recognize suffixed tslib helpers~~ fix(ivy): ngcc - several fixes related to decorators Jul 27, 2019

JoostK mentioned this pull request Jul 27, 2019

ivy-ngcc doesn't transform UMD bundles correctly #31394

Closed

JoostK force-pushed the ngcc-suffixed-helpers branch from 8b1b0b1 to 3ddd6bc Compare July 27, 2019 14:04

petebacondarwin approved these changes Jul 27, 2019

View reviewed changes

petebacondarwin added action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews and removed action: review The PR is still awaiting reviews from at least one requested reviewer labels Jul 27, 2019

JoostK added 2 commits July 27, 2019 23:17

fixup! refactor(ivy): ngcc - categorize the various decorate calls up…

21554ae

…front

fixup! fix(ivy): ngcc - recognize suffixed tslib helpers

cf92848

JoostK removed the action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews label Jul 27, 2019

petebacondarwin approved these changes Jul 28, 2019

View reviewed changes

petebacondarwin removed the request for review from a team July 28, 2019 06:33

petebacondarwin added the action: merge The PR is ready for merge by the caretaker label Jul 28, 2019

gkalpak approved these changes Jul 29, 2019

View reviewed changes

Comment thread packages/compiler-cli/ngcc/src/host/esm2015_host.ts Resolved

Comment thread packages/compiler-cli/ngcc/src/host/esm2015_host.ts Resolved

Comment thread integration/ngcc/test.sh Resolved

Comment thread packages/compiler-cli/ngcc/test/helpers/utils.ts Resolved

mhevery approved these changes Jul 29, 2019

View reviewed changes

JoostK added 2 commits July 29, 2019 17:54

fixup! fix(ivy): ngcc - recognize suffixed tslib helpers

2b989ec

fixup! fix(ivy): ngcc - render decorators in UMD and CommonJS bundles…

39cb495

… correctly

JoostK added merge: caretaker note Alert the caretaker performing the merge to check the PR for an out of normal action needed or note and removed merge: caretaker note Alert the caretaker performing the merge to check the PR for an out of normal action needed or note labels Jul 29, 2019

AndrewKushnir closed this in 5e5be43 Jul 29, 2019

JoostK mentioned this pull request Aug 24, 2019

Ivy + angular2-text-mask: runtime error #31513

Closed

angular-automatic-lock-bot Bot locked and limited conversation to collaborators Sep 15, 2019

Navigation Menu

Repository navigation

fix(ivy): ngcc - several fixes related to decorators - #31614

fix(ivy): ngcc - several fixes related to decorators#31614
JoostK wants to merge 7 commits into
angular:masterfrom
JoostK:ngcc-suffixed-helpers

JoostK commented Jul 17, 2019 •

edited

Loading

Uh oh!

Uh oh!

petebacondarwin left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

JoostK Jul 26, 2019

Uh oh!

Uh oh!

petebacondarwin left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

gkalpak left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

angular-automatic-lock-bot Bot commented Sep 15, 2019

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Conversation

JoostK commented Jul 17, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

petebacondarwin left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

JoostK Jul 26, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

petebacondarwin left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

gkalpak left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

angular-automatic-lock-bot Bot commented Sep 15, 2019

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

JoostK commented Jul 17, 2019 •

edited

Loading