Skip to content

fix(ngcc): a couple of ngcc fixes - #32619

Closed
JoostK wants to merge 2 commits into
angular:masterfrom
JoostK:ngcc-static-props
Closed

JoostK wants to merge 2 commits into
angular:masterfrom
JoostK:ngcc-static-props

Conversation

@JoostK

@JoostK JoostK commented Sep 11, 2019 •

Copy link
Copy Markdown
Member

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 JoostK added type: bug/fix state: blocked action: review The PR is still awaiting reviews from at least one requested reviewer effort1: hours workaround4: none freq2: medium severity3: broken target: major This PR is targeted for the next major release risk: low labels Sep 11, 2019
@JoostK
JoostK requested review from a team September 11, 2019 20:10
@ngbot ngbot Bot modified the milestone: Backlog Sep 11, 2019
@JoostK

This comment has been minimized.

@JoostK

This comment has been minimized.

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
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
@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 state: WIP label Sep 12, 2019
@JoostK
JoostK removed the request for review from a team September 12, 2019 19:27
@petebacondarwin petebacondarwin added action: merge The PR is ready for merge by the caretaker and removed action: review The PR is still awaiting reviews from at least one requested reviewer labels Sep 12, 2019
@kara kara closed this in c4e039a Sep 12, 2019
kara pushed 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
@angular-automatic-lock-bot

Copy link
Copy Markdown

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.

Labels

action: merge The PR is ready for merge by the caretaker cla: yes effort1: hours freq2: medium risk: low target: major This PR is targeted for the next major release type: bug/fix workaround4: none

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Ivy fails with NGXS

3 participants