Repository navigation
Conversation
This comment has been minimized.
This comment has been minimized.
Closed
JoostK
force-pushed
the
ngcc-static-props
branch
from
September 11, 2019 20:31
5ab3ab5 to
aae916b
Compare
This comment has been minimized.
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
force-pushed
the
ngcc-static-props
branch
from
September 12, 2019 19:21
aae916b to
0a66bbe
Compare
petebacondarwin
approved these changes
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
|
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. |
This file contains hidden or 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
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.
fix(ngcc): correctly read static properties for aliased classes
In ESM2015 bundles, a class with decorators may be emitted as follows:
Such a class has two declarations: the publicly visible
let MyClassand the implementation
class MyClass {}node. In #32539 a refactoringtook 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 thestatic properties need to be obtained from the symbol corresponding with
the
let MyClassdeclaration, as that is where thedecoratorsproperty 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.tsfiles for UMD/CommonJS bundlesIn 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
ModuleWithProvidersimports that are declared in an external library.In that situation, ngtsc determines the type of the actual
NgModulethat is imported, by looking in the library's declaration files for the
generic type argument on
ModuleWithProviders. In this process, ngtscresolves the import for the
ModuleWithProvidersidentifier to verifythat it is indeed the
ModuleWithProviderstype from@angular/core.So, when the UMD reflection host was in use this resolution would fail,
therefore no
NgModuletype 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