fix(compiler-cli): downlevel angular decorators to static properties #37382

devversion · 2020-06-01T20:45:52Z

fix(compiler-cli): downlevel angular decorators to static properties

In v7 of Angular we removed tsickle from the default ngc pipeline.
This had the negative potential of breaking ES2015 output and SSR due
to a limitation in TypeScript.

TypeScript by default preserves type information for decorated constructor
parameters when emitDecoratorMetadata is enabled. For example,
consider this snippet below:

@Directive()
export class MyDirective {
  constructor(button: MyButton) {}
}

export class MyButton {}

TypeScript would generate metadata for the MyDirective class it has
a decorator applied. This metadata would be needed in JIT mode, or
for libraries that provide MyDirective through NPM. The metadata would
look as followed:

let MyDirective = class MyDir {}

MyDirective = __decorate([
  Directive(),
  __metadata("design:paramtypes", [MyButton]),
], MyDirective);

let MyButton = class MyButton {}

Notice that TypeScript generated calls to __decorate and
__metadata. These calls are needed so that the Angular compiler
is able to determine whether MyDirective is actually an directive,
and what types are needed for dependency injection.

The limitation surfaces in this concrete example because MyButton
is declared after the __metadata(..) call, while __metadata
actually directly references MyButton. This is illegal though because
MyButton has not been declared at this point. This is due to the
so-called temporal dead zone in JavaScript. Errors like followed will
be reported at runtime when such file/code evaluates:

Uncaught ReferenceError: Cannot access 'MyButton' before initialization

As noted, this is a TypeScript limitation because ideally TypeScript
shouldn't evaluate __metadata/reference MyButton immediately.
Instead, it should defer the reference until MyButton is actually
declared. This limitation will not be fixed by the TypeScript team
though because it's a limitation as per current design and they will
only revisit this once the tc39 decorator proposal is finalized
(currently stage-2 at time of writing).

Given this wontfix on the TypeScript side, and our heavy reliance on
this metadata in libraries (and for JIT mode), we intend to fix this
from within the Angular compiler by downleveling decorators to static
properties that don't need to evaluate directly. For example:

MyDirective.ctorParameters = () => [MyButton];

With this snippet above, MyButton is not referenced directly. Only
lazily when the Angular runtime needs it. This mitigates the temporal
dead zone issue caused by a limitation in TypeScript's decorator
metadata output. See: microsoft/TypeScript#27519.

In the past (as noted; before version 7), the Angular compiler by
default used tsickle that already performed this transformation. We
moved the transformation to the CLI for JIT and ng-packager, but now
we realize that we can move this all to a single place in the compiler
so that standalone ngc consumers can benefit too, and that we can
disable tsickle in our Bazel ngc-wrapped pipeline (that currently
still relies on tsickle to perform this decorator processing).

This transformation also has another positive side-effect of making
Angular application/library code more compatible with server-side
rendering. In principle, TypeScript would also preserve type information
for decorated class members (similar to how it did that for constructor
parameters) at runtime. This becomes an issue when your application
relies on native DOM globals for decorated class member types. e.g.

@Input() panelElement: HTMLElement;

Your application code would then reference HTMLElement directly
whenever the source file is loaded in NodeJS for SSR. HTMLElement
does not exist on the server though, so that will become an invalid
reference. One could work around this by providing global mocks for
these DOM symbols, but that doesn't match up with other places where
dependency injection is used for mocking DOM/browser specific symbols.

More context in this issue: #30586. The TL;DR here is that the Angular
compiler does not care about types for these class members, so it won't
ever reference HTMLElement at runtime.

Fixes #30106. Fixes #30586. Fixes #30141.
Resolves FW-2196. Resolves FW-2199.

goldens/size-tracking/aio-payloads.json

devversion · 2020-06-02T23:09:38Z

@alxhub @IgorMinar I marked this as master-only because I think it's not critical to go into V10 given that we have re-enabled tsickle in ng_package and standalone ngc standalone has been broken for quite some while. Though, we surely could get it into v10 as a fix if desired 😄

alan-agius4 · 2020-06-03T08:25:15Z

I think it’s best that this goes in v10, because otherwise we’ll be unable to remove this logic from ng-packagr and CLI until v11, that’s because ng-packagr and CLI are typically compatible for a particular major version of the framework.

Note: the transformer needs to be added as part of the public api, because in the CLI this transformer is only used when using JIT mode, which in CLI happens via TSC.

devversion · 2020-06-03T08:58:50Z

@alan-agius4 Yeah, we chatted about that being exposed. I wonder how we can do that as compiler code never is public API as per PUBLIC_API.md. How has it been done in the past for similar things?

alan-agius4 · 2020-06-03T09:17:38Z

Since the compiler code is not bundled we can opt to use deep imports in this case.

alan-agius4

LGTM.

IgorMinar

Overall this looks really really good @devversion!

I left a few comments and I'm going to rely on @alxhub's review of the transformer code itself(I skipped that part during this review pass).

The biggest sticking point for me is emitDecoratorMetadata- see my longer comment about that.

With regards to your comment about "master-only" vs "master&patch". I think that we should get this in v10 to enable simplification of the downstream tooling as soon as possible. So please relabel the PR.

Lastly, and I'm very hesitant to even suggest it this late in the RC, but I do have to bring it up because otherwise we'll have to go through more churn and migrations: how about we rename .decorators and ctorParams to ɵdec and ɵctor (or something similar prefixed with ɵ). The goal of this would be to ensure that these props are clearly marked as private APIs and that we follow the same conventions for these apis as we do elsewhere.

The property name change needs more discussion, so please do not act on it, but think about it and we can discuss it during the FW sync meeting.

(one known downstream change that would need to happen as a result of this renaming would be to update the CLI/build-optimizer passes to look for both the old and new property names, @alan-agius4 could weigh in on how difficult that would be and if there is more to it than an extra property lookup in AST)

packages/bazel/src/ngc-wrapped/index.ts

packages/compiler-cli/integrationtest/tsconfig-build.json

packages/compiler-cli/src/transformers/program.ts

packages/compiler-cli/test/ngc_spec.ts

IgorMinar · 2020-06-04T16:21:54Z

The presubmit in google3 seems to be failing, @alxhub can confirm but at the surface it seems that g3 expects that ngc-wrapped enables emitDecoratorMetadata imperatively and doesn't require that setting in tsconfig.json.

example error:

ERROR - [JSC_UNDEFINED_VARIABLE] variable raw_diff_url_service_1 is undeclared
  83|     { type: raw_diff_url_service_1.RawDiffUrlService, decorators: [{ type: core_1.Optional }] },
                  ^^^^^^^^^^^^^^^^^^^^^^

The error comes from the closure compiler that tries to process JS code emitted by ngc (VE).

The error looks like an issue with the emit that causes TS to emit code while ommitting some of the imports needed for it. I recall seeing an issue about this tsickle or typescript issue tracker but I can't find it right now. I believe that turning on the emitDecoratorMetadata within ngc-wrapped will fix this problem.

(the failing presubmit)

IgorMinar · 2020-06-04T16:39:43Z

Thinking more about the property renaming. I think it's too risky at this point. Let's not get distracted with that and schedule it for v11.

alan-agius4 · 2020-06-04T16:42:39Z

Renaming the props is not a big change from the CLI POV, I think the framework will be impacted more, because the compiler & ngcc will need to be aware of these new annotations for decorators.

Currently the compiler/cli/ngcc can process/reflect decorators that are either emitted either by tsickle or TypeScript.

packages/bazel/src/ngc-wrapped/index.ts

packages/compiler-cli/test/ngc_spec.ts

IgorMinar

LGTM! Thank you!

Reviewed-for: global-approvers

devversion · 2020-06-09T10:52:11Z

packages/bazel/test/ng_package/example_package.golden

@@ -611,7 +611,7 @@ let MyService = /** @class */ (() => {
    return MyService;
 })();
 export { MyService };
-//# sourceMappingURL=data:application/json;base64,eyJ2ZXJzaW9uIjozLCJmaWxlIjoicHVibGljLWFwaS5qcyIsInNvdXJjZVJvb3QiOiIiLCJzb3VyY2VzIjpbIi4uLy4uLy4uLy4uLy4uLy4uLy4uLy4uLy4uL3BhY2thZ2VzL2JhemVsL3Rlc3QvbmdfcGFja2FnZS9leGFtcGxlL2ltcG9ydHMvcHVibGljLWFwaS50cyJdLCJuYW1lcyI6W10sIm1hcHBpbmdzIjoiQUFBQTs7Ozs7O0dBTUc7QUFFSCxPQUFPLEVBQUMsVUFBVSxFQUFDLE1BQU0sZUFBZSxDQUFDO0FBQ3pDLE9BQU8sRUFBQyxlQUFlLEVBQUMsTUFBTSxVQUFVLENBQUM7OztBQUV6QztJQUFBLE1BQ2EsU0FBUztRQUNwQixZQUFtQixhQUE4QjtZQUE5QixrQkFBYSxHQUFiLGFBQWEsQ0FBaUI7UUFBRyxDQUFDOzs7O2dCQUZ0RCxVQUFVLFNBQUMsRUFBQyxVQUFVLEVBQUUsTUFBTSxFQUFDOzs7Z0JBRUksZUFBZTs7b0JBYm5EO0tBY0M7U0FGWSxTQUFTIiwic291cmNlc0NvbnRlbnQiOlsiLyoqXG4gKiBAbGljZW5zZVxuICogQ29weXJpZ2h0IEdvb2dsZSBMTEMgQWxsIFJpZ2h0cyBSZXNlcnZlZC5cbiAqXG4gKiBVc2Ugb2YgdGhpcyBzb3VyY2UgY29kZSBpcyBnb3Zlcm5lZCBieSBhbiBNSVQtc3R5bGUgbGljZW5zZSB0aGF0IGNhbiBiZVxuICogZm91bmQgaW4gdGhlIExJQ0VOU0UgZmlsZSBhdCBodHRwczovL2FuZ3VsYXIuaW8vbGljZW5zZVxuICovXG5cbmltcG9ydCB7SW5qZWN0YWJsZX0gZnJvbSAnQGFuZ3VsYXIvY29yZSc7XG5pbXBvcnQge015U2Vjb25kU2VydmljZX0gZnJvbSAnLi9zZWNvbmQnO1xuXG5ASW5qZWN0YWJsZSh7cHJvdmlkZWRJbjogJ3Jvb3QnfSlcbmV4cG9ydCBjbGFzcyBNeVNlcnZpY2Uge1xuICBjb25zdHJ1Y3RvcihwdWJsaWMgc2Vjb25kU2VydmljZTogTXlTZWNvbmRTZXJ2aWNlKSB7fVxufVxuIl19
+//# sourceMappingURL=data:application/json;base64,eyJ2ZXJzaW9uIjozLCJmaWxlIjoicHVibGljLWFwaS5qcyIsInNvdXJjZVJvb3QiOiIiLCJzb3VyY2VzIjpbIi4uLy4uLy4uLy4uLy4uLy4uLy4uLy4uLy4uL3BhY2thZ2VzL2JhemVsL3Rlc3QvbmdfcGFja2FnZS9leGFtcGxlL2ltcG9ydHMvcHVibGljLWFwaS50cyJdLCJuYW1lcyI6W10sIm1hcHBpbmdzIjoiQUFBQTs7Ozs7O0dBTUc7QUFFSCxPQUFPLEVBQUMsVUFBVSxFQUFDLE1BQU0sZUFBZSxDQUFDO0FBQ3pDLE9BQU8sRUFBQyxlQUFlLEVBQUMsTUFBTSxVQUFVLENBQUM7OztBQUV6QztJQUFBLE1BQ2EsU0FBUztRQUNwQixZQUFtQixhQUE4QjtZQUE5QixrQkFBYSxHQUFiLGFBQWEsQ0FBaUI7UUFBRyxDQUFDOzs7O2dCQUZ0RCxVQUFVLFNBQUMsRUFBQyxVQUFVLEVBQUUsTUFBTSxFQUFDOzs7Z0JBRnhCLGVBQWU7O29CQVR2QjtLQWNDO1NBRlksU0FBUyIsInNvdXJjZXNDb250ZW50IjpbIi8qKlxuICogQGxpY2Vuc2VcbiAqIENvcHlyaWdodCBHb29nbGUgTExDIEFsbCBSaWdodHMgUmVzZXJ2ZWQuXG4gKlxuICogVXNlIG9mIHRoaXMgc291cmNlIGNvZGUgaXMgZ292ZXJuZWQgYnkgYW4gTUlULXN0eWxlIGxpY2Vuc2UgdGhhdCBjYW4gYmVcbiAqIGZvdW5kIGluIHRoZSBMSUNFTlNFIGZpbGUgYXQgaHR0cHM6Ly9hbmd1bGFyLmlvL2xpY2Vuc2VcbiAqL1xuXG5pbXBvcnQge0luamVjdGFibGV9IGZyb20gJ0Bhbmd1bGFyL2NvcmUnO1xuaW1wb3J0IHtNeVNlY29uZFNlcnZpY2V9IGZyb20gJy4vc2Vjb25kJztcblxuQEluamVjdGFibGUoe3Byb3ZpZGVkSW46ICdyb290J30pXG5leHBvcnQgY2xhc3MgTXlTZXJ2aWNlIHtcbiAgY29uc3RydWN0b3IocHVibGljIHNlY29uZFNlcnZpY2U6IE15U2Vjb25kU2VydmljZSkge31cbn1cbiJdfQ==


Note for reviewer: You might wonder why this commit changed the source map. This is because before this commit, the actual parameter type identifier was used for ctorParameters. Now since we use the import alias though for ctorParameters, the source map will change back to what it was with tsickle too (except for the move of the factory and removal of @nocollapse).

alxhub

LGTM with the fix for aliased constructor parameter names.

alxhub · 2020-06-09T17:26:11Z

Presubmit

alxhub · 2020-06-09T19:23:28Z

And again

@directive

In v7 of Angular we removed `tsickle` from the default `ngc` pipeline. This had the negative potential of breaking ES2015 output and SSR due to a limitation in TypeScript. TypeScript by default preserves type information for decorated constructor parameters when `emitDecoratorMetadata` is enabled. For example, consider this snippet below: ``` @directive() export class MyDirective { constructor(button: MyButton) {} } export class MyButton {} ``` TypeScript would generate metadata for the `MyDirective` class it has a decorator applied. This metadata would be needed in JIT mode, or for libraries that provide `MyDirective` through NPM. The metadata would look as followed: ``` let MyDirective = class MyDir {} MyDirective = __decorate([ Directive(), __metadata("design:paramtypes", [MyButton]), ], MyDirective); let MyButton = class MyButton {} ``` Notice that TypeScript generated calls to `__decorate` and `__metadata`. These calls are needed so that the Angular compiler is able to determine whether `MyDirective` is actually an directive, and what types are needed for dependency injection. The limitation surfaces in this concrete example because `MyButton` is declared after the `__metadata(..)` call, while `__metadata` actually directly references `MyButton`. This is illegal though because `MyButton` has not been declared at this point. This is due to the so-called temporal dead zone in JavaScript. Errors like followed will be reported at runtime when such file/code evaluates: ``` Uncaught ReferenceError: Cannot access 'MyButton' before initialization ``` As noted, this is a TypeScript limitation because ideally TypeScript shouldn't evaluate `__metadata`/reference `MyButton` immediately. Instead, it should defer the reference until `MyButton` is actually declared. This limitation will not be fixed by the TypeScript team though because it's a limitation as per current design and they will only revisit this once the tc39 decorator proposal is finalized (currently stage-2 at time of writing). Given this wontfix on the TypeScript side, and our heavy reliance on this metadata in libraries (and for JIT mode), we intend to fix this from within the Angular compiler by downleveling decorators to static properties that don't need to evaluate directly. For example: ``` MyDirective.ctorParameters = () => [MyButton]; ``` With this snippet above, `MyButton` is not referenced directly. Only lazily when the Angular runtime needs it. This mitigates the temporal dead zone issue caused by a limitation in TypeScript's decorator metadata output. See: microsoft/TypeScript#27519. In the past (as noted; before version 7), the Angular compiler by default used tsickle that already performed this transformation. We moved the transformation to the CLI for JIT and `ng-packager`, but now we realize that we can move this all to a single place in the compiler so that standalone ngc consumers can benefit too, and that we can disable tsickle in our Bazel `ngc-wrapped` pipeline (that currently still relies on tsickle to perform this decorator processing). This transformation also has another positive side-effect of making Angular application/library code more compatible with server-side rendering. In principle, TypeScript would also preserve type information for decorated class members (similar to how it did that for constructor parameters) at runtime. This becomes an issue when your application relies on native DOM globals for decorated class member types. e.g. ``` @input() panelElement: HTMLElement; ``` Your application code would then reference `HTMLElement` directly whenever the source file is loaded in NodeJS for SSR. `HTMLElement` does not exist on the server though, so that will become an invalid reference. One could work around this by providing global mocks for these DOM symbols, but that doesn't match up with other places where dependency injection is used for mocking DOM/browser specific symbols. More context in this issue: angular#30586. The TL;DR here is that the Angular compiler does not care about types for these class members, so it won't ever reference `HTMLElement` at runtime. Fixes angular#30106. Fixes angular#30586. Fixes angular#30141. Resolves FW-2196. Resolves FW-2199.

…erties Address feedback

…erties Use barrel imports

…erties Fix incorrect import reference with conflicting local parameter name

…erties Properly handle namespace imports resolving to non-value

…erties

alxhub · 2020-06-09T22:09:52Z

Here we go again

@directive

…37382) In v7 of Angular we removed `tsickle` from the default `ngc` pipeline. This had the negative potential of breaking ES2015 output and SSR due to a limitation in TypeScript. TypeScript by default preserves type information for decorated constructor parameters when `emitDecoratorMetadata` is enabled. For example, consider this snippet below: ``` @directive() export class MyDirective { constructor(button: MyButton) {} } export class MyButton {} ``` TypeScript would generate metadata for the `MyDirective` class it has a decorator applied. This metadata would be needed in JIT mode, or for libraries that provide `MyDirective` through NPM. The metadata would look as followed: ``` let MyDirective = class MyDir {} MyDirective = __decorate([ Directive(), __metadata("design:paramtypes", [MyButton]), ], MyDirective); let MyButton = class MyButton {} ``` Notice that TypeScript generated calls to `__decorate` and `__metadata`. These calls are needed so that the Angular compiler is able to determine whether `MyDirective` is actually an directive, and what types are needed for dependency injection. The limitation surfaces in this concrete example because `MyButton` is declared after the `__metadata(..)` call, while `__metadata` actually directly references `MyButton`. This is illegal though because `MyButton` has not been declared at this point. This is due to the so-called temporal dead zone in JavaScript. Errors like followed will be reported at runtime when such file/code evaluates: ``` Uncaught ReferenceError: Cannot access 'MyButton' before initialization ``` As noted, this is a TypeScript limitation because ideally TypeScript shouldn't evaluate `__metadata`/reference `MyButton` immediately. Instead, it should defer the reference until `MyButton` is actually declared. This limitation will not be fixed by the TypeScript team though because it's a limitation as per current design and they will only revisit this once the tc39 decorator proposal is finalized (currently stage-2 at time of writing). Given this wontfix on the TypeScript side, and our heavy reliance on this metadata in libraries (and for JIT mode), we intend to fix this from within the Angular compiler by downleveling decorators to static properties that don't need to evaluate directly. For example: ``` MyDirective.ctorParameters = () => [MyButton]; ``` With this snippet above, `MyButton` is not referenced directly. Only lazily when the Angular runtime needs it. This mitigates the temporal dead zone issue caused by a limitation in TypeScript's decorator metadata output. See: microsoft/TypeScript#27519. In the past (as noted; before version 7), the Angular compiler by default used tsickle that already performed this transformation. We moved the transformation to the CLI for JIT and `ng-packager`, but now we realize that we can move this all to a single place in the compiler so that standalone ngc consumers can benefit too, and that we can disable tsickle in our Bazel `ngc-wrapped` pipeline (that currently still relies on tsickle to perform this decorator processing). This transformation also has another positive side-effect of making Angular application/library code more compatible with server-side rendering. In principle, TypeScript would also preserve type information for decorated class members (similar to how it did that for constructor parameters) at runtime. This becomes an issue when your application relies on native DOM globals for decorated class member types. e.g. ``` @input() panelElement: HTMLElement; ``` Your application code would then reference `HTMLElement` directly whenever the source file is loaded in NodeJS for SSR. `HTMLElement` does not exist on the server though, so that will become an invalid reference. One could work around this by providing global mocks for these DOM symbols, but that doesn't match up with other places where dependency injection is used for mocking DOM/browser specific symbols. More context in this issue: #30586. The TL;DR here is that the Angular compiler does not care about types for these class members, so it won't ever reference `HTMLElement` at runtime. Fixes #30106. Fixes #30586. Fixes #30141. Resolves FW-2196. Resolves FW-2199. PR Close #37382

@directive

…ngular#37382) In v7 of Angular we removed `tsickle` from the default `ngc` pipeline. This had the negative potential of breaking ES2015 output and SSR due to a limitation in TypeScript. TypeScript by default preserves type information for decorated constructor parameters when `emitDecoratorMetadata` is enabled. For example, consider this snippet below: ``` @directive() export class MyDirective { constructor(button: MyButton) {} } export class MyButton {} ``` TypeScript would generate metadata for the `MyDirective` class it has a decorator applied. This metadata would be needed in JIT mode, or for libraries that provide `MyDirective` through NPM. The metadata would look as followed: ``` let MyDirective = class MyDir {} MyDirective = __decorate([ Directive(), __metadata("design:paramtypes", [MyButton]), ], MyDirective); let MyButton = class MyButton {} ``` Notice that TypeScript generated calls to `__decorate` and `__metadata`. These calls are needed so that the Angular compiler is able to determine whether `MyDirective` is actually an directive, and what types are needed for dependency injection. The limitation surfaces in this concrete example because `MyButton` is declared after the `__metadata(..)` call, while `__metadata` actually directly references `MyButton`. This is illegal though because `MyButton` has not been declared at this point. This is due to the so-called temporal dead zone in JavaScript. Errors like followed will be reported at runtime when such file/code evaluates: ``` Uncaught ReferenceError: Cannot access 'MyButton' before initialization ``` As noted, this is a TypeScript limitation because ideally TypeScript shouldn't evaluate `__metadata`/reference `MyButton` immediately. Instead, it should defer the reference until `MyButton` is actually declared. This limitation will not be fixed by the TypeScript team though because it's a limitation as per current design and they will only revisit this once the tc39 decorator proposal is finalized (currently stage-2 at time of writing). Given this wontfix on the TypeScript side, and our heavy reliance on this metadata in libraries (and for JIT mode), we intend to fix this from within the Angular compiler by downleveling decorators to static properties that don't need to evaluate directly. For example: ``` MyDirective.ctorParameters = () => [MyButton]; ``` With this snippet above, `MyButton` is not referenced directly. Only lazily when the Angular runtime needs it. This mitigates the temporal dead zone issue caused by a limitation in TypeScript's decorator metadata output. See: microsoft/TypeScript#27519. In the past (as noted; before version 7), the Angular compiler by default used tsickle that already performed this transformation. We moved the transformation to the CLI for JIT and `ng-packager`, but now we realize that we can move this all to a single place in the compiler so that standalone ngc consumers can benefit too, and that we can disable tsickle in our Bazel `ngc-wrapped` pipeline (that currently still relies on tsickle to perform this decorator processing). This transformation also has another positive side-effect of making Angular application/library code more compatible with server-side rendering. In principle, TypeScript would also preserve type information for decorated class members (similar to how it did that for constructor parameters) at runtime. This becomes an issue when your application relies on native DOM globals for decorated class member types. e.g. ``` @input() panelElement: HTMLElement; ``` Your application code would then reference `HTMLElement` directly whenever the source file is loaded in NodeJS for SSR. `HTMLElement` does not exist on the server though, so that will become an invalid reference. One could work around this by providing global mocks for these DOM symbols, but that doesn't match up with other places where dependency injection is used for mocking DOM/browser specific symbols. More context in this issue: angular#30586. The TL;DR here is that the Angular compiler does not care about types for these class members, so it won't ever reference `HTMLElement` at runtime. Fixes angular#30106. Fixes angular#30586. Fixes angular#30141. Resolves FW-2196. Resolves FW-2199. PR Close angular#37382

angular-automatic-lock-bot · 2020-07-11T16:13:32Z

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.}

@directive

…ngular#37382) In v7 of Angular we removed `tsickle` from the default `ngc` pipeline. This had the negative potential of breaking ES2015 output and SSR due to a limitation in TypeScript. TypeScript by default preserves type information for decorated constructor parameters when `emitDecoratorMetadata` is enabled. For example, consider this snippet below: ``` @directive() export class MyDirective { constructor(button: MyButton) {} } export class MyButton {} ``` TypeScript would generate metadata for the `MyDirective` class it has a decorator applied. This metadata would be needed in JIT mode, or for libraries that provide `MyDirective` through NPM. The metadata would look as followed: ``` let MyDirective = class MyDir {} MyDirective = __decorate([ Directive(), __metadata("design:paramtypes", [MyButton]), ], MyDirective); let MyButton = class MyButton {} ``` Notice that TypeScript generated calls to `__decorate` and `__metadata`. These calls are needed so that the Angular compiler is able to determine whether `MyDirective` is actually an directive, and what types are needed for dependency injection. The limitation surfaces in this concrete example because `MyButton` is declared after the `__metadata(..)` call, while `__metadata` actually directly references `MyButton`. This is illegal though because `MyButton` has not been declared at this point. This is due to the so-called temporal dead zone in JavaScript. Errors like followed will be reported at runtime when such file/code evaluates: ``` Uncaught ReferenceError: Cannot access 'MyButton' before initialization ``` As noted, this is a TypeScript limitation because ideally TypeScript shouldn't evaluate `__metadata`/reference `MyButton` immediately. Instead, it should defer the reference until `MyButton` is actually declared. This limitation will not be fixed by the TypeScript team though because it's a limitation as per current design and they will only revisit this once the tc39 decorator proposal is finalized (currently stage-2 at time of writing). Given this wontfix on the TypeScript side, and our heavy reliance on this metadata in libraries (and for JIT mode), we intend to fix this from within the Angular compiler by downleveling decorators to static properties that don't need to evaluate directly. For example: ``` MyDirective.ctorParameters = () => [MyButton]; ``` With this snippet above, `MyButton` is not referenced directly. Only lazily when the Angular runtime needs it. This mitigates the temporal dead zone issue caused by a limitation in TypeScript's decorator metadata output. See: microsoft/TypeScript#27519. In the past (as noted; before version 7), the Angular compiler by default used tsickle that already performed this transformation. We moved the transformation to the CLI for JIT and `ng-packager`, but now we realize that we can move this all to a single place in the compiler so that standalone ngc consumers can benefit too, and that we can disable tsickle in our Bazel `ngc-wrapped` pipeline (that currently still relies on tsickle to perform this decorator processing). This transformation also has another positive side-effect of making Angular application/library code more compatible with server-side rendering. In principle, TypeScript would also preserve type information for decorated class members (similar to how it did that for constructor parameters) at runtime. This becomes an issue when your application relies on native DOM globals for decorated class member types. e.g. ``` @input() panelElement: HTMLElement; ``` Your application code would then reference `HTMLElement` directly whenever the source file is loaded in NodeJS for SSR. `HTMLElement` does not exist on the server though, so that will become an invalid reference. One could work around this by providing global mocks for these DOM symbols, but that doesn't match up with other places where dependency injection is used for mocking DOM/browser specific symbols. More context in this issue: angular#30586. The TL;DR here is that the Angular compiler does not care about types for these class members, so it won't ever reference `HTMLElement` at runtime. Fixes angular#30106. Fixes angular#30586. Fixes angular#30141. Resolves FW-2196. Resolves FW-2199. PR Close angular#37382

googlebot added the cla: yes label Jun 1, 2020

devversion force-pushed the fix/ngc-strip-angular-decorators-and-downlevel branch 2 times, most recently from 20b80f2 to 787a42e Compare Jun 1, 2020

devversion reviewed Jun 1, 2020

View changes

goldens/size-tracking/aio-payloads.json Outdated Show resolved Hide resolved

devversion force-pushed the fix/ngc-strip-angular-decorators-and-downlevel branch 5 times, most recently from 9fcff60 to f533ee8 Compare Jun 2, 2020

devversion added comp: compiler action: review target: major labels Jun 2, 2020

ngbot bot added this to the needsTriage milestone Jun 2, 2020

devversion requested review from alxhub, IgorMinar and alan-agius4 Jun 2, 2020

devversion marked this pull request as ready for review Jun 2, 2020

devversion force-pushed the fix/ngc-strip-angular-decorators-and-downlevel branch from f533ee8 to d884e61 Compare Jun 2, 2020

alan-agius4 approved these changes Jun 4, 2020

View changes

IgorMinar suggested changes Jun 4, 2020

View changes

devversion added target: patch and removed target: major labels Jun 4, 2020

alxhub reviewed Jun 4, 2020

View changes

packages/bazel/src/ngc-wrapped/index.ts Outdated Show resolved Hide resolved

packages/compiler-cli/test/ngc_spec.ts Outdated Show resolved Hide resolved

pullapprove bot requested review from IgorMinar and kyliau Jun 6, 2020

JohannesHuster mentioned this pull request Jun 6, 2020

Library decorator metadata ReferenceError with SSR #37472

Closed

devversion force-pushed the fix/ngc-strip-angular-decorators-and-downlevel branch from a1d86d7 to d158731 Compare Jun 6, 2020

IgorMinar approved these changes Jun 8, 2020

View changes

IgorMinar removed request for kyliau, alan-agius4 and clydin Jun 8, 2020

devversion force-pushed the fix/ngc-strip-angular-decorators-and-downlevel branch 2 times, most recently from cdfd61e to 9d6c343 Compare Jun 9, 2020

devversion reviewed Jun 9, 2020

View changes

alxhub approved these changes Jun 9, 2020

View changes

devversion force-pushed the fix/ngc-strip-angular-decorators-and-downlevel branch 2 times, most recently from b76ec5f to 7170363 Compare Jun 9, 2020

devversion added 6 commits Jun 9, 2020

fixup! fix(compiler-cli): downlevel angular decorators to static prop…

6debd51

…erties Address feedback

fixup! fix(compiler-cli): downlevel angular decorators to static prop…

d24c603

…erties Use barrel imports

fixup! fix(compiler-cli): downlevel angular decorators to static prop…

fb5128a

…erties Fix incorrect import reference with conflicting local parameter name

fixup! fix(compiler-cli): downlevel angular decorators to static prop…

a4cd62b

…erties Properly handle namespace imports resolving to non-value

fixup! fix(compiler-cli): downlevel angular decorators to static prop…

f871ec3

…erties

devversion force-pushed the fix/ngc-strip-angular-decorators-and-downlevel branch from 7170363 to f871ec3 Compare Jun 9, 2020

atscott closed this in angular/angular@401ef71 Jun 10, 2020

angular-automatic-lock-bot bot locked and limited conversation to collaborators Jul 11, 2020

Feb	MAR	Apr
	22
2021	2022	2023

angular / angular Public

fix(compiler-cli): downlevel angular decorators to static properties #37382

fix(compiler-cli): downlevel angular decorators to static properties #37382

devversion commented Jun 1, 2020 •

edited

devversion commented Jun 2, 2020 •

edited

alan-agius4 commented Jun 3, 2020

devversion commented Jun 3, 2020

alan-agius4 commented Jun 3, 2020

alan-agius4 left a comment •

edited

IgorMinar left a comment

IgorMinar commented Jun 4, 2020 •

edited

IgorMinar commented Jun 4, 2020

alan-agius4 commented Jun 4, 2020

IgorMinar left a comment

devversion Jun 9, 2020 •

edited

alxhub left a comment

alxhub commented Jun 9, 2020

alxhub commented Jun 9, 2020

alxhub commented Jun 9, 2020

angular-automatic-lock-bot bot commented Jul 11, 2020

angular / angular Public

fix(compiler-cli): downlevel angular decorators to static properties #37382

fix(compiler-cli): downlevel angular decorators to static properties #37382

Conversation

devversion commented Jun 1, 2020 • edited

devversion commented Jun 2, 2020 • edited

alan-agius4 commented Jun 3, 2020

devversion commented Jun 3, 2020

alan-agius4 commented Jun 3, 2020

alan-agius4 left a comment • edited

IgorMinar left a comment

IgorMinar commented Jun 4, 2020 • edited

IgorMinar commented Jun 4, 2020

alan-agius4 commented Jun 4, 2020

IgorMinar left a comment

devversion Jun 9, 2020 • edited

Choose a reason for hiding this comment

alxhub left a comment

alxhub commented Jun 9, 2020

alxhub commented Jun 9, 2020

alxhub commented Jun 9, 2020

angular-automatic-lock-bot bot commented Jul 11, 2020

devversion commented Jun 1, 2020 •

edited

devversion commented Jun 2, 2020 •

edited

alan-agius4 left a comment •

edited

IgorMinar commented Jun 4, 2020 •

edited

devversion Jun 9, 2020 •

edited