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): consistently use outer declaration for classes #32539

Closed
wants to merge 3 commits into from

Conversation

@JoostK
Copy link
Member

commented Sep 7, 2019

In ngcc's reflection hosts for compiled JS bundles, such as ESM2015,
special care needs to be taken for classes as there may be an outer
declaration (referred to as "declaration") and an inner declaration
(referred to as "implementation") for a given class. Therefore, there
will also be two ts.Symbols bound per class, and ngcc needs to switch
between those declarations and symbols depending on where certain
information can be found.

Prior to this commit, the NgccReflectionHost interface had methods
getClassSymbol and findClassSymbols that would return a ts.Symbol.
These class symbols would be used to kick off compilation of components
using ngtsc, so it is important for these symbols to correspond with the
publicly visible outer declaration of the class. However, the ESM2015
reflection host used to return the ts.Symbol for the inner
declaration, if the class was declared as follows:

var MyClass = class MyClass {};

For the above code, Esm2015ReflectionHost.getClassSymbol would return
the ts.Symbol corresponding with the class MyClass {} declaration,
whereas it should have corresponded with the var MyClass declaration.
As a consequence, no NgModule could be resolved for the component, so
no components/directives would be in scope for the component. This
resulted in errors during runtime.

This commit resolves the issue by introducing a NgccClassSymbol that
contains references to both the outer and inner ts.Symbol, instead of
just a single ts.Symbol. This avoids the unclarity of whether a
ts.Symbol corresponds with the outer or inner declaration.

More details can be found here: https://hackmd.io/7nkgWOFWQlSRAuIW_8KPPw

Fixes #32078
Closes FW-1507

@ngbot ngbot bot modified the milestone: Backlog Sep 7, 2019
@googlebot googlebot added the cla: yes label Sep 7, 2019
@JoostK JoostK marked this pull request as ready for review Sep 7, 2019
@JoostK JoostK requested review from angular/fw-compiler as code owners Sep 7, 2019
@@ -14,7 +14,7 @@ import {getDeclaration} from '../../../src/ngtsc/testing';
import {loadFakeCore, loadTestFiles} from '../../../test/helpers';
import {CommonJsReflectionHost} from '../../src/host/commonjs_host';
import {Esm2015ReflectionHost} from '../../src/host/esm2015_host';
import {getIifeBody} from '../../src/host/esm5_host';
import {Esm5ReflectionHost, getIifeBody} from '../../src/host/esm5_host';

This comment has been minimized.

Copy link
@gkalpak

gkalpak Sep 9, 2019

Member

Both Esm2015ReflectionHost and Esm5ReflectionHost seem to be unused here.

@@ -13,7 +13,7 @@ import {isFromDtsFile} from '../../../src/ngtsc/util/src/typescript';
import {getNameText, hasNameIdentifier, stripDollarSuffix} from '../utils';

import {Esm2015ReflectionHost, ParamInfo, getPropertyValueFromSymbol, isAssignmentStatement} from './esm2015_host';
import {NgccClassSymbol} from './ngcc_host';
import {ClassSymbol, NgccClassSymbol} from './ngcc_host';

This comment has been minimized.

Copy link
@gkalpak

gkalpak Sep 9, 2019

Member

ClassSymbol (as well as CtorParameter and isFromDtsFile above) seem to be unused.


return {
name: declarationSymbol.name,
declaration: declarationSymbol as ClassSymbol,

This comment has been minimized.

Copy link
@gkalpak

gkalpak Sep 9, 2019

Member

I generally find type casting easier to "manually verify", if it is as close as possible to the variable declaration. Consider:

-const declarationSymbol = this.checker.getSymbolAtLocation(outerDeclaration.name);
+const declarationSymbol = this.checker.getSymbolAtLocation(outerDeclaration.name) as ClassSymbol | undefined;
 ...
 return {
   name: declarationSymbol.name,
-  declaration: declarationSymbol as ClassSymbol,
+  declaration: declarationSymbol,
@petebacondarwin petebacondarwin self-assigned this Sep 10, 2019
Copy link
Member

left a comment

Great work @JoostK both on this PR and the design work that went into it.
A few nits and I agree with @gkalpak's comments.

* @returns the symbol for the node or `undefined` if it is not a "class" or has no symbol.
*/
protected getClassSymbolFromOuterDeclaration(declaration: ts.Node): NgccClassSymbol|undefined {
const superSymbol = super.getClassSymbolFromOuterDeclaration(declaration);

This comment has been minimized.

Copy link
@petebacondarwin

petebacondarwin Sep 10, 2019

Member

superSymbol seems a strange name here. It implies (to me) that we are getting a symbol for the super class declaration. But in reality we are asking the super host for the symbol that represents the class. Perhaps classSymbol or es2015ClassSymbol would be preferable?

This comment has been minimized.

Copy link
@JoostK

JoostK Sep 10, 2019

Author Member

Ah, I see how this can cause confusion. I don't personally like es2015ClassSymbol, as it could as well be the TypeScript symbol, so went with classSymbol instead 👍

*
* The returned declaration is either the class declaration (from the typings file) or the outer
* variable declaration.
* This method extracts a `NgccClassSymbol` if `node` is the function declaration inside the IIFE.

This comment has been minimized.

Copy link
@petebacondarwin

petebacondarwin Sep 10, 2019

Member

typo: change node to declaration here

* - The declaration of the outer variable, which is assigned the result of the IIFE.
* - The function declaration inside the IIFE, which is eventually returned and assigned to the
* outer variable.
* This method extracts a `NgccClassSymbol` if `node` is the outer variable which is assigned the

This comment has been minimized.

Copy link
@petebacondarwin

petebacondarwin Sep 10, 2019

Member

typo: change node to declaration here

const superDeclaration = super.getClassDeclaration(node);
if (superDeclaration) return superDeclaration;
protected getClassSymbolFromInnerDeclaration(declaration: ts.Node): NgccClassSymbol|undefined {
const superSymbol = super.getClassSymbolFromInnerDeclaration(declaration);

This comment has been minimized.

Copy link
@petebacondarwin

petebacondarwin Sep 10, 2019

Member

superSymbol => classSymbol ?

JoostK added 3 commits Sep 1, 2019
In ngcc's reflection hosts for compiled JS bundles, such as ESM2015,
special care needs to be taken for classes as there may be an outer
declaration (referred to as "declaration") and an inner declaration
(referred to as "implementation") for a given class. Therefore, there
will also be two `ts.Symbol`s bound per class, and ngcc needs to switch
between those declarations and symbols depending on where certain
information can be found.

Prior to this commit, the `NgccReflectionHost` interface had methods
`getClassSymbol` and `findClassSymbols` that would return a `ts.Symbol`.
These class symbols would be used to kick off compilation of components
using ngtsc, so it is important for these symbols to correspond with the
publicly visible outer declaration of the class. However, the ESM2015
reflection host used to return the `ts.Symbol` for the inner
declaration, if the class was declared as follows:

```javascript
var MyClass = class MyClass {};
```

For the above code, `Esm2015ReflectionHost.getClassSymbol` would return
the `ts.Symbol` corresponding with the `class MyClass {}` declaration,
whereas it should have corresponded with the `var MyClass` declaration.
As a consequence, no `NgModule` could be resolved for the component, so
no components/directives would be in scope for the component. This
resulted in errors during runtime.

This commit resolves the issue by introducing a `NgccClassSymbol` that
contains references to both the outer and inner `ts.Symbol`, instead of
just a single `ts.Symbol`. This avoids the unclarity of whether a
`ts.Symbol` corresponds with the outer or inner declaration.

More details can be found here: https://hackmd.io/7nkgWOFWQlSRAuIW_8KPPw

Fixes #32078
Closes FW-1507
@JoostK JoostK referenced this pull request Sep 10, 2019
1 of 1 task complete
JoostK added a commit to JoostK/angular that referenced this pull request 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 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
JoostK added a commit to JoostK/angular that referenced this pull request 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 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
@alxhub
alxhub approved these changes Sep 11, 2019
@kara

This comment has been minimized.

Copy link
Contributor

commented Sep 12, 2019

@kara kara unassigned alxhub Sep 12, 2019
@kara kara closed this in 2279cb8 Sep 12, 2019
kara added a commit that referenced this pull request Sep 12, 2019
In ngcc's reflection hosts for compiled JS bundles, such as ESM2015,
special care needs to be taken for classes as there may be an outer
declaration (referred to as "declaration") and an inner declaration
(referred to as "implementation") for a given class. Therefore, there
will also be two `ts.Symbol`s bound per class, and ngcc needs to switch
between those declarations and symbols depending on where certain
information can be found.

Prior to this commit, the `NgccReflectionHost` interface had methods
`getClassSymbol` and `findClassSymbols` that would return a `ts.Symbol`.
These class symbols would be used to kick off compilation of components
using ngtsc, so it is important for these symbols to correspond with the
publicly visible outer declaration of the class. However, the ESM2015
reflection host used to return the `ts.Symbol` for the inner
declaration, if the class was declared as follows:

```javascript
var MyClass = class MyClass {};
```

For the above code, `Esm2015ReflectionHost.getClassSymbol` would return
the `ts.Symbol` corresponding with the `class MyClass {}` declaration,
whereas it should have corresponded with the `var MyClass` declaration.
As a consequence, no `NgModule` could be resolved for the component, so
no components/directives would be in scope for the component. This
resulted in errors during runtime.

This commit resolves the issue by introducing a `NgccClassSymbol` that
contains references to both the outer and inner `ts.Symbol`, instead of
just a single `ts.Symbol`. This avoids the unclarity of whether a
`ts.Symbol` corresponds with the outer or inner declaration.

More details can be found here: https://hackmd.io/7nkgWOFWQlSRAuIW_8KPPw

Fixes #32078
Closes FW-1507

PR Close #32539
JoostK added a commit to JoostK/angular that referenced this pull request Sep 12, 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 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
kara added a commit that referenced this pull request Sep 12, 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

PR Close #32619
@JoostK JoostK deleted the JoostK:ngcc-class-symbol branch Sep 15, 2019
arnehoek added a commit to arnehoek/angular that referenced this pull request Sep 26, 2019
arnehoek added a commit to arnehoek/angular that referenced this pull request Sep 26, 2019
)

In ngcc's reflection hosts for compiled JS bundles, such as ESM2015,
special care needs to be taken for classes as there may be an outer
declaration (referred to as "declaration") and an inner declaration
(referred to as "implementation") for a given class. Therefore, there
will also be two `ts.Symbol`s bound per class, and ngcc needs to switch
between those declarations and symbols depending on where certain
information can be found.

Prior to this commit, the `NgccReflectionHost` interface had methods
`getClassSymbol` and `findClassSymbols` that would return a `ts.Symbol`.
These class symbols would be used to kick off compilation of components
using ngtsc, so it is important for these symbols to correspond with the
publicly visible outer declaration of the class. However, the ESM2015
reflection host used to return the `ts.Symbol` for the inner
declaration, if the class was declared as follows:

```javascript
var MyClass = class MyClass {};
```

For the above code, `Esm2015ReflectionHost.getClassSymbol` would return
the `ts.Symbol` corresponding with the `class MyClass {}` declaration,
whereas it should have corresponded with the `var MyClass` declaration.
As a consequence, no `NgModule` could be resolved for the component, so
no components/directives would be in scope for the component. This
resulted in errors during runtime.

This commit resolves the issue by introducing a `NgccClassSymbol` that
contains references to both the outer and inner `ts.Symbol`, instead of
just a single `ts.Symbol`. This avoids the unclarity of whether a
`ts.Symbol` corresponds with the outer or inner declaration.

More details can be found here: https://hackmd.io/7nkgWOFWQlSRAuIW_8KPPw

Fixes angular#32078
Closes FW-1507

PR Close angular#32539
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
@angular-automatic-lock-bot

This comment has been minimized.

Copy link

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