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): a couple of ngcc fixes #32619

Closed
wants to merge 2 commits into from

Conversation

@JoostK
Copy link
Member

commented Sep 11, 2019

fix(ngcc): correctly read static properties for aliased classes

In ESM2015 bundles, a class with decorators may be emitted as follows:

var MyClass_1;
let MyClass = MyClass_1 = class MyClass {};
MyClass.decorators = [/* here be decorators */];

Such a class has two declarations: the publicly visible let MyClass
and the implementation class MyClass {} node. In #32539 a refactoring
took place to handle such classes more consistently, however the logic
to find static properties was mistakenly kept identical to its broken
state before the refactor, by looking for static properties on the
implementation symbol (the one for class MyClass {}) whereas the
static properties need to be obtained from the symbol corresponding with
the let MyClass declaration, as that is where the decorators
property is assigned to in the example above.

This commit fixes the behavior by looking for static properties on the
public declaration symbol. This fixes an issue where decorators were not
found for classes that do in fact have decorators, therefore preventing
the classes from being compiled for Ivy.

Fixes #31791


fix(ngcc): resolve imports in .d.ts files for UMD/CommonJS bundles

In ngcc's reflection host for UMD and CommonJS bundles, custom logic is
present to resolve import details of an identifier. However, this custom
logic is unable to resolve an import for an identifier inside of
declaration files, as such files use the regular ESM import syntax.

As a consequence of this limitation, ngtsc is unable to resolve
ModuleWithProviders imports that are declared in an external library.
In that situation, ngtsc determines the type of the actual NgModule
that is imported, by looking in the library's declaration files for the
generic type argument on ModuleWithProviders. In this process, ngtsc
resolves the import for the ModuleWithProviders identifier to verify
that it is indeed the ModuleWithProviders type from @angular/core.
So, when the UMD reflection host was in use this resolution would fail,
therefore no NgModule type could be detected.

This commit fixes the bug by using the regular import resolution logic
in addition to the custom resolution logic that is required for UMD
and CommonJS bundles.

Fixes #31791

@JoostK

This comment was marked as outdated.

Copy link
Member Author

commented Sep 11, 2019

Blocked on #32539, only last commit to be reviewed.

@JoostK JoostK referenced this pull request Sep 11, 2019
@JoostK JoostK force-pushed the JoostK:ngcc-static-props branch from 5ab3ab5 to aae916b Sep 11, 2019
@JoostK

This comment was marked as outdated.

Copy link
Member Author

commented Sep 11, 2019

Actually marking this WIP for now, as #31791 also describes an error for UMD bundles which I'm now able to reproduce. Will take some time to look into resolving that tomorrow.

JoostK added 2 commits Sep 11, 2019
In ESM2015 bundles, a class with decorators may be emitted as follows:

```javascript
var MyClass_1;
let MyClass = MyClass_1 = class MyClass {};
MyClass.decorators = [/* here be decorators */];
```

Such a class has two declarations: the publicly visible `let MyClass`
and the implementation `class MyClass {}` node. In #32539 a refactoring
took place to handle such classes more consistently, however the logic
to find static properties was mistakenly kept identical to its broken
state before the refactor, by looking for static properties on the
implementation symbol (the one for `class MyClass {}`) whereas the
static properties need to be obtained from the symbol corresponding with
the `let MyClass` declaration, as that is where the `decorators`
property is assigned to in the example above.

This commit fixes the behavior by looking for static properties on the
public declaration symbol. This fixes an issue where decorators were not
found for classes that do in fact have decorators, therefore preventing
the classes from being compiled for Ivy.

Fixes #31791
In ngcc's reflection host for UMD and CommonJS bundles, custom logic is
present to resolve import details of an identifier. However, this custom
logic is unable to resolve an import for an identifier inside of
declaration files, as such files use the regular ESM import syntax.

As a consequence of this limitation, ngtsc is unable to resolve
`ModuleWithProviders` imports that are declared in an external library.
In that situation, ngtsc determines the type of the actual `NgModule`
that is imported, by looking in the library's declaration files for the
generic type argument on `ModuleWithProviders`. In this process, ngtsc
resolves the import for the `ModuleWithProviders` identifier to verify
that it is indeed the `ModuleWithProviders` type from `@angular/core`.
So, when the UMD reflection host was in use this resolution would fail,
therefore no `NgModule` type could be detected.

This commit fixes the bug by using the regular import resolution logic
in addition to the custom resolution logic that is required for UMD
and CommonJS bundles.

Fixes #31791
@JoostK JoostK force-pushed the JoostK:ngcc-static-props branch from aae916b to 0a66bbe Sep 12, 2019
@JoostK JoostK changed the title fix(ngcc): correctly read static properties for aliased classes fix(ngcc): a couple of ngcc fixes Sep 12, 2019
@JoostK JoostK removed the PR state: WIP label Sep 12, 2019
@JoostK JoostK removed the request for review from angular/fw-compiler Sep 12, 2019
@kara kara closed this in c4e039a Sep 12, 2019
kara added a commit that referenced this pull request Sep 12, 2019
…32619)

In ngcc's reflection host for UMD and CommonJS bundles, custom logic is
present to resolve import details of an identifier. However, this custom
logic is unable to resolve an import for an identifier inside of
declaration files, as such files use the regular ESM import syntax.

As a consequence of this limitation, ngtsc is unable to resolve
`ModuleWithProviders` imports that are declared in an external library.
In that situation, ngtsc determines the type of the actual `NgModule`
that is imported, by looking in the library's declaration files for the
generic type argument on `ModuleWithProviders`. In this process, ngtsc
resolves the import for the `ModuleWithProviders` identifier to verify
that it is indeed the `ModuleWithProviders` type from `@angular/core`.
So, when the UMD reflection host was in use this resolution would fail,
therefore no `NgModule` type could be detected.

This commit fixes the bug by using the regular import resolution logic
in addition to the custom resolution logic that is required for UMD
and CommonJS bundles.

Fixes #31791

PR Close #32619
arnehoek added a commit to arnehoek/angular that referenced this pull request Sep 26, 2019
…lar#32619)

In ESM2015 bundles, a class with decorators may be emitted as follows:

```javascript
var MyClass_1;
let MyClass = MyClass_1 = class MyClass {};
MyClass.decorators = [/* here be decorators */];
```

Such a class has two declarations: the publicly visible `let MyClass`
and the implementation `class MyClass {}` node. In angular#32539 a refactoring
took place to handle such classes more consistently, however the logic
to find static properties was mistakenly kept identical to its broken
state before the refactor, by looking for static properties on the
implementation symbol (the one for `class MyClass {}`) whereas the
static properties need to be obtained from the symbol corresponding with
the `let MyClass` declaration, as that is where the `decorators`
property is assigned to in the example above.

This commit fixes the behavior by looking for static properties on the
public declaration symbol. This fixes an issue where decorators were not
found for classes that do in fact have decorators, therefore preventing
the classes from being compiled for Ivy.

Fixes angular#31791

PR Close angular#32619
arnehoek added a commit to arnehoek/angular that referenced this pull request Sep 26, 2019
…ngular#32619)

In ngcc's reflection host for UMD and CommonJS bundles, custom logic is
present to resolve import details of an identifier. However, this custom
logic is unable to resolve an import for an identifier inside of
declaration files, as such files use the regular ESM import syntax.

As a consequence of this limitation, ngtsc is unable to resolve
`ModuleWithProviders` imports that are declared in an external library.
In that situation, ngtsc determines the type of the actual `NgModule`
that is imported, by looking in the library's declaration files for the
generic type argument on `ModuleWithProviders`. In this process, ngtsc
resolves the import for the `ModuleWithProviders` identifier to verify
that it is indeed the `ModuleWithProviders` type from `@angular/core`.
So, when the UMD reflection host was in use this resolution would fail,
therefore no `NgModule` type could be detected.

This commit fixes the bug by using the regular import resolution logic
in addition to the custom resolution logic that is required for UMD
and CommonJS bundles.

Fixes angular#31791

PR Close angular#32619
@angular-automatic-lock-bot

This comment has been minimized.

Copy link

commented Oct 13, 2019

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.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Oct 13, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.