fix(ngcc): correctly associate decorators with aliased classes #33878
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
A testcase that was using a spy has shown itself to be brittle, and its assertions can easily be moved into a related test.
JoostK
added
type: bug/fix
action: review
JoostK
changed the title
In flat bundle formats, multiple classes that have the same name can be suffixed to become unique. In ES5-like bundles this results in the outer declaration from having a different name from the "implementation" declaration within the class' IIFE, as the implementation declaration may not have been suffixed. As an example, the following code would fail to have a `Directive` decorator as ngcc would search for `__decorate` calls that refer to `AliasedDirective$1` by name, whereas the `__decorate` call actually uses the `AliasedDirective` name. ```javascript var AliasedDirective$1 = /** @Class */ (function () { function AliasedDirective() {} AliasedDirective = tslib_1.__decorate([ Directive({ selector: '[someDirective]' }), ], AliasedDirective); return AliasedDirective; }()); ``` This commit fixes the problem by not relying on comparing names, but instead finding the declaration and matching it with both the outer and inner declaration.
petebacondarwin
approved these changes
Nov 16, 2019
|
|
||
| // The identifier corresponds with the class if its declaration is either the outer or inner | ||
| // declaration. | ||
| return decl.node === outerDeclaration || decl.node === innerDeclaration; |
There was a problem hiding this comment.
This can be simplified to
return decl !== null && (decl.node === outerDeclaration || decl.node === innerDeclaration);Or if we are sure that innerDeclaration and outerDeclaration are always defined then simply:
return decl.node === outerDeclaration || decl.node === innerDeclaration;Right?
There was a problem hiding this comment.
Your first suggestion is possible, but I deliberately choose the current logic. I like how it separates handling of the edge case where identifier could not be resolved to its declaration, vs the actual logic of comparing the declarations.
Your second suggestion is essentially what I have now, but the null guard is required because that is targeting decl, not outerDeclaration/innerDeclaration.
tl;dr; I'll keep this as is.
JoostK
added
action: merge
alxhub
pushed a commit
that referenced
this pull request
Nov 18, 2019
In flat bundle formats, multiple classes that have the same name can be suffixed to become unique. In ES5-like bundles this results in the outer declaration from having a different name from the "implementation" declaration within the class' IIFE, as the implementation declaration may not have been suffixed. As an example, the following code would fail to have a `Directive` decorator as ngcc would search for `__decorate` calls that refer to `AliasedDirective$1` by name, whereas the `__decorate` call actually uses the `AliasedDirective` name. ```javascript var AliasedDirective$1 = /** @Class */ (function () { function AliasedDirective() {} AliasedDirective = tslib_1.__decorate([ Directive({ selector: '[someDirective]' }), ], AliasedDirective); return AliasedDirective; }()); ``` This commit fixes the problem by not relying on comparing names, but instead finding the declaration and matching it with both the outer and inner declaration. PR Close #33878
alxhub
pushed a commit
that referenced
this pull request
Nov 18, 2019
A testcase that was using a spy has shown itself to be brittle, and its assertions can easily be moved into a related test. PR Close #33878
alxhub
pushed a commit
that referenced
this pull request
Nov 18, 2019
In flat bundle formats, multiple classes that have the same name can be suffixed to become unique. In ES5-like bundles this results in the outer declaration from having a different name from the "implementation" declaration within the class' IIFE, as the implementation declaration may not have been suffixed. As an example, the following code would fail to have a `Directive` decorator as ngcc would search for `__decorate` calls that refer to `AliasedDirective$1` by name, whereas the `__decorate` call actually uses the `AliasedDirective` name. ```javascript var AliasedDirective$1 = /** @Class */ (function () { function AliasedDirective() {} AliasedDirective = tslib_1.__decorate([ Directive({ selector: '[someDirective]' }), ], AliasedDirective); return AliasedDirective; }()); ``` This commit fixes the problem by not relying on comparing names, but instead finding the declaration and matching it with both the outer and inner declaration. PR Close #33878
|
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. |
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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.
In flat bundle formats, multiple classes that have the same name can be
suffixed to become unique. In ES5-like bundles this results in the outer
declaration from having a different name from the "implementation"
declaration within the class' IIFE, as the implementation declaration
may not have been suffixed.
As an example, the following code would fail to have a
Directivedecorator as ngcc would search for
__decoratecalls that refer toAliasedDirective$1by name, whereas the__decoratecall actuallyuses the
AliasedDirectivename.This commit fixes the problem by not relying on comparing names, but
instead finding the declaration and matching it with both the outer
and inner declaration.