fix(core): detect DI parameters in JIT mode for downleveled ES2015 classes #38500
Conversation
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
…asses
In the Angular Package Format, we always shipped UMD bundles and previously even ES5 module output.
With V10, we removed the ES5 module output but kept the UMD ES5 output.
For this, we were able to remove our second TypeScript transpilation. Instead we started only
building ES2015 output and then downleveled it to ES5 UMD for the NPM packages. This worked
as expected but unveiled an issue in the `@angular/core` reflection capabilities.
In JIT mode, Angular determines constructor parameters (for DI) using the `ReflectionCapabilities`. The
reflection capabilities basically read runtime metadata of classes to determine the DI parameters. Such
metadata can be either stored in static class properties like `ctorParameters` or within TypeScript's `design:params`.
If Angular comes across a class that does not have any parameter metadata, it tries to detect if the
given class is actually delegating to an inherited class. It does this naively in JIT by checking if the
stringified class (function in ES5) matches a certain pattern. e.g.
```js
function MatTable() {
var _this = _super.apply(this, arguments) || this;
```
These patterns are reluctant to changes of the class output. If a class is not recognized properly, the
DI parameters will be assumed empty and the class is **incorrectly** constructed without arguments.
This actually happened as part of v10 now. Since we downlevel ES2015 to ES5 (instead of previously
compiling sources directly to ES5), the class output changed slightly so that Angular no longer detects
it. e.g.
```js
var _this = _super.apply(this, __spread(arguments)) || this;
```
This happens because the ES2015 output will receive an auto-generated constructor if the class
defines class properties. This constructor is then already containing an explicit `super` call.
```js
export class MatTable extends CdkTable {
constructor() {
super(...arguments);
this.disabled = true;
}
}
```
If we then downlevel this file to ES5 with `--downlevelIteration`, TypeScript adjusts the `super` call so that
the spread operator is no longer used (not supported in ES5). The resulting super call is different to the
super call that would have been emitted if we would directly transpile to ES5. Ultimately, Angular no
longer detects such classes as having an delegate constructor -> and DI breaks.
We fix this by expanding the rather naive RegExp patterns used for the reflection capabilities
so that downleveled pass-through/delegate constructors are properly detected. There is a risk
of a false-positive as we cannot detect whether `__spread` is actually the TypeScript spread
helper, but given the reflection patterns already make lots of assumptions (e.g. that `super` is
actually the superclass, we should be fine making this assumption too. The false-positive would
not result in a broken app, but rather in unnecessary providers being injected (as a noop).
Fixes angular#38453
…S2015 classes Similarly to the change we landed in the `@angular/core` reflection capabilities, we need to make sure that ngcc can detect pass-through delegate constructors for classes using downleveled ES2015 output. More details can be found in the preceding commit, and in the issue outlining the problem: angular#38453. Fixes angular#38453.
…veled ES2015 classes Add better descriptions to helper methods; stylistic preference feedback
devversion
added
area: compiler
atscott
approved these changes
Aug 17, 2020
alxhub
approved these changes
Aug 17, 2020
atscott
pushed a commit
that referenced
this pull request
Aug 17, 2020
…asses (#38500) In the Angular Package Format, we always shipped UMD bundles and previously even ES5 module output. With V10, we removed the ES5 module output but kept the UMD ES5 output. For this, we were able to remove our second TypeScript transpilation. Instead we started only building ES2015 output and then downleveled it to ES5 UMD for the NPM packages. This worked as expected but unveiled an issue in the `@angular/core` reflection capabilities. In JIT mode, Angular determines constructor parameters (for DI) using the `ReflectionCapabilities`. The reflection capabilities basically read runtime metadata of classes to determine the DI parameters. Such metadata can be either stored in static class properties like `ctorParameters` or within TypeScript's `design:params`. If Angular comes across a class that does not have any parameter metadata, it tries to detect if the given class is actually delegating to an inherited class. It does this naively in JIT by checking if the stringified class (function in ES5) matches a certain pattern. e.g. ```js function MatTable() { var _this = _super.apply(this, arguments) || this; ``` These patterns are reluctant to changes of the class output. If a class is not recognized properly, the DI parameters will be assumed empty and the class is **incorrectly** constructed without arguments. This actually happened as part of v10 now. Since we downlevel ES2015 to ES5 (instead of previously compiling sources directly to ES5), the class output changed slightly so that Angular no longer detects it. e.g. ```js var _this = _super.apply(this, __spread(arguments)) || this; ``` This happens because the ES2015 output will receive an auto-generated constructor if the class defines class properties. This constructor is then already containing an explicit `super` call. ```js export class MatTable extends CdkTable { constructor() { super(...arguments); this.disabled = true; } } ``` If we then downlevel this file to ES5 with `--downlevelIteration`, TypeScript adjusts the `super` call so that the spread operator is no longer used (not supported in ES5). The resulting super call is different to the super call that would have been emitted if we would directly transpile to ES5. Ultimately, Angular no longer detects such classes as having an delegate constructor -> and DI breaks. We fix this by expanding the rather naive RegExp patterns used for the reflection capabilities so that downleveled pass-through/delegate constructors are properly detected. There is a risk of a false-positive as we cannot detect whether `__spread` is actually the TypeScript spread helper, but given the reflection patterns already make lots of assumptions (e.g. that `super` is actually the superclass, we should be fine making this assumption too. The false-positive would not result in a broken app, but rather in unnecessary providers being injected (as a noop). Fixes #38453 PR Close #38500
atscott
pushed a commit
that referenced
this pull request
Aug 17, 2020
…S2015 classes (#38500) Similarly to the change we landed in the `@angular/core` reflection capabilities, we need to make sure that ngcc can detect pass-through delegate constructors for classes using downleveled ES2015 output. More details can be found in the preceding commit, and in the issue outlining the problem: #38453. Fixes #38453. PR Close #38500
|
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. |
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.
Patch port of #38463