fix(@ngtools/webpack): downlevel constructor parameter type information

[clydin]

The Angular JIT compiler uses the output of TypeScript's emitDecoratorMetadata to support dependency injection at runtime. This option stores type and DI token information so that the runtime JIT compiler can inject the proper provider. This option is not needed for AOT compilation and the metadata is indirectly stripped from the output due to all Angular decorators being removed during the compilation process.

Unfortunately, the emitDecoratorMetadata option does not support forward references (or more generally circular dependencies) within ES2015+ output due to the TypeScript compiler transforming the metadata into a direct reference of the yet to be defined class. This results in a runtime TDZ error. This is a known limitation of the emitDecoratorMetadata option (reference). This situation is not unique to Angular and can occur in any TypeScript code. As it is marked as a design limitation, it is considered that it cannot be fixed within the current architecture of TypeScript.

As a result, the change in this PR removes the need for the use of the option by storing the DI information in a static property function via the use of the tsickle ctorParameters transformation. By leveraging the existing functionality, no changes to the framework code is necessary. Also, minimal new code is required within the CLI, as the relevant tsickle code can be extracted and used with several local modifications.

For forward references to function in ES2015+, the emitDecoratorMetadata option must be disabled based on the current implementation. To support both the option and forward references, the metadata generated by TypeScript would need to be altered to remove the type information that would trigger a TDZ error. However, by doing so the metadata information is no longer useful to Angular and technically incorrect as it will not contain the information from the source code as described by the option.

Closes: #14424

[IgorMinar]

I really like this solution and especially the fact that we are reusing code from tsickle rather than writing stuff from scratch - this will align the external and google3 build stacks to work more alike.

notes from internal discussion:

• we considered also writing a migration to remove reflect metadata from dev polyfills in all projects, because at this point, there is no need for reflect-metadata in CLI projects unless the app code or other 3rd party libraries need it. This is however too risky during the RC so we'll defer this cleanup until v9
• we'll however document that reflect-metadata is no longer needed and will be removed from projects in v9 (in the meantime there is no prod impact because the code is never included in prod bundles).
• we will also update the polyfills docs and removed reflect-metadata from the list of required polyfills. the relevant doc is here: https://angular.io/guide/browser-support#core-es7-reflect
• lastly, we should update the new app/lib schematics to no longer bring reflect-metadata into new projects. we should tried to do this before 8.0 still since it's a low risk change with significant impact on new projects.

[alan-agius4]

//cc @alexeagle kindly note this change!

[alan-agius4]

From library/tsickle perspective this looks good to me.

[Splaktar]

This seems to be at least partially related to angular/angular#30586.

[trotyl]

@IgorMinar I believe angular/angular#25039 would be a better approach, which eliminate the need for emitDecoratorMetadata at semantic level, and will not result any inconsistency with or without using Angular CLI.

[IgorMinar]

EDIT: oops.. I copy pasted the wrong link, instead of angular/angular#25039 I meant angular/angular#30586. I updated the links below.

I agree that angular/angular#25039 angular/angular#30586 is better but it's not a change we can implement in 8.0. It's too late for that, we are in an RC and in risk-averse mode. We can however implement it in 8.1.

Keep in mind that while that solution is better, this PR is sufficient to prevent regressions in 8.0 - to ensure that things that worked prior to 8.0 still work with 8.0 and all the new defaults (e.g. differential loading). The proposal described by Paul enables something that never worked before - bazel+ssr. We should do this but again, not in 8.0.

So the least risky solution is to

• go ahead with this PR as it's low risk and unblocks the release
• improve upon this via a solution described in Feature: allow inject() to be used in constructor angular#25039 Libraries sometimes depend on tsickle decorator downleveling angular#30586 in 8.1

[IgorMinar]

With regards to angular/angular#25039 - it's solving a very different problem and while it could potentially also solve this one, it would only work for new code and not for any of the existing application/library code which would need to be migrated.

[evmar]

Does this mean we can remove the decorator downlevelling code from tsickle? When? (I think it is great that Angular owns this particular code because I never understood it well.)

[alexeagle]

@evmar I think that could happen already, tsickle is already an optional dependency from Angular ecosystem. It's really hard for us to be sure about the consequences without trying it though - there might be users like Lucidchart who still depend on that behavior

[angular-automatic-lock-bot]

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