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.

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(ngcc): correctly associate decorators with aliased classes #33878

Closed
wants to merge 2 commits into from

Conversation

@JoostK
Copy link
Member

JoostK commented Nov 16, 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.

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.

A testcase that was using a spy has shown itself to be brittle, and its
assertions can easily be moved into a related test.
@ngbot ngbot bot added this to the needsTriage milestone Nov 16, 2019
@googlebot googlebot added the cla: yes label Nov 16, 2019
@JoostK JoostK changed the title test(ngcc): correctly associate decorators with aliased classes fix(ngcc): correctly associate decorators with aliased classes Nov 16, 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.
@JoostK JoostK force-pushed the JoostK:ngcc-aliases branch from a4cec59 to 07e7701 Nov 16, 2019
@JoostK JoostK marked this pull request as ready for review Nov 16, 2019
@JoostK JoostK requested a review from angular/fw-ngcc as a code owner 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;

This comment has been minimized.

Copy link
@petebacondarwin

petebacondarwin Nov 16, 2019

Member

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?

This comment has been minimized.

Copy link
@JoostK

JoostK Nov 17, 2019

Author Member

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.

@alxhub alxhub closed this in 19a6c15 Nov 18, 2019
alxhub added 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 added 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 added 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.