refactor(compiler-cli): skip class decorators in tooling constructor parameters transform (#37545)

We recently added a transformer to NGC that is responsible for downleveling Angular
decorators and constructor parameter types. The primary goal was to mitigate a
TypeScript limitation/issue that surfaces in Angular projects due to the heavy
reliance on type metadata being captured for DI. Additionally this is a pre-requisite
of making `tsickle` optional in the Angular bazel toolchain.

See: 401ef71ae5 for more context on this.

Another (less important) goal was to make sure that the CLI can re-use
this transformer for its JIT mode compilation. The CLI (as outlined in
the commit mentioned above), already has a transformer for downleveling
constructor parameters. We want to avoid this duplication and exported
the transform through the tooling-private compiler entry-point.

Early experiments in using this transformer over the current one, highlighted
that in JIT, class decorators cannot be downleveled. Angular relies on those
to be invoked immediately for JIT (so that factories etc. are generated upon loading)

The transformer we exposed, always downlevels such class decorators
though, so that would break CLI's JIT mode. We can address the CLI's
needs by adding another flag to skip class decorators. This will allow
us to continue with the goal of de-duplication.

PR Close #37545

packages/compiler-cli/src/tooling.ts

@@ -35,15 +35,15 @@ export const GLOBAL_DEFS_FOR_TERSER_WITH_AOT = {
 /**
  * Transform for downleveling Angular decorators and Angular-decorated class constructor
  * parameters for dependency injection. This transform can be used by the CLI for JIT-mode
- * compilation where decorators should be preserved, but downleveled so that apps are not
- * exposed to the ES2015 temporal dead zone limitation in TypeScript's metadata.
- * See https://github.com/angular/angular-cli/pull/14473 for more details.
+ * compilation where constructor parameters and associated Angular decorators should be
+ * downleveled so that apps are not exposed to the ES2015 temporal dead zone limitation
+ * in TypeScript. See https://github.com/angular/angular-cli/pull/14473 for more details.
  */
-export function decoratorDownlevelTransformerFactory(program: ts.Program):
+export function constructorParametersDownlevelTransform(program: ts.Program):
     ts.TransformerFactory<ts.SourceFile> {
   const typeChecker = program.getTypeChecker();
   const reflectionHost = new TypeScriptReflectionHost(typeChecker);
   return getDownlevelDecoratorsTransform(
       typeChecker, reflectionHost, [], /* isCore */ false,
-      /* enableClosureCompiler */ false);
+      /* enableClosureCompiler */ false, /* skipClassDecorators */ true);
 }

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

@@ -327,10 +327,17 @@ interface ParameterDecorationInfo {
  * @param diagnostics List which will be populated with diagnostics if any.
  * @param isCore Whether the current TypeScript program is for the `@angular/core` package.
  * @param isClosureCompilerEnabled Whether closure annotations need to be added where needed.
+ * @param skipClassDecorators Whether class decorators should be skipped from downleveling.
+ *   This is useful for JIT mode where class decorators should be preserved as they could rely
+ *   on immediate execution. e.g. downleveling `@Injectable` means that the injectable factory
+ *   is not created, and injecting the token will not work. If this decorator would not be
+ *   downleveled, the `Injectable` decorator will execute immediately on file load, and
+ *   Angular will generate the corresponding injectable factory.
  */
 export function getDownlevelDecoratorsTransform(
     typeChecker: ts.TypeChecker, host: ReflectionHost, diagnostics: ts.Diagnostic[],
-    isCore: boolean, isClosureCompilerEnabled: boolean): ts.TransformerFactory<ts.SourceFile> {
+    isCore: boolean, isClosureCompilerEnabled: boolean,
+    skipClassDecorators: boolean): ts.TransformerFactory<ts.SourceFile> {
   return (context: ts.TransformationContext) => {
     let referencedParameterTypes = new Set<ts.Declaration>();
 
@@ -517,13 +524,22 @@ export function getDownlevelDecoratorsTransform(
       }
       const decorators = host.getDecoratorsOfDeclaration(classDecl) || [];
 
+      let hasAngularDecorator = false;
       const decoratorsToLower = [];
       const decoratorsToKeep: ts.Decorator[] = [];
       for (const decorator of decorators) {
         // We only deal with concrete nodes in TypeScript sources, so we don't
         // need to handle synthetically created decorators.
         const decoratorNode = decorator.node! as ts.Decorator;
-        if (isAngularDecorator(decorator, isCore)) {
+        const isNgDecorator = isAngularDecorator(decorator, isCore);
+
+        // Keep track if we come across an Angular class decorator. This is used
+        // for to determine whether constructor parameters should be captured or not.
+        if (isNgDecorator) {
+          hasAngularDecorator = true;
+        }
+
+        if (isNgDecorator && !skipClassDecorators) {
           decoratorsToLower.push(extractMetadataFromSingleDecorator(decoratorNode, diagnostics));
         } else {
           decoratorsToKeep.push(decoratorNode);
@@ -536,9 +552,9 @@ export function getDownlevelDecoratorsTransform(
         newMembers.push(createDecoratorClassProperty(decoratorsToLower));
       }
       if (classParameters) {
-        if ((decoratorsToLower.length) || classParameters.some(p => !!p.decorators.length)) {
-          // emit ctorParameters if the class was decoratored at all, or if any of its ctors
-          // were classParameters
+        if (hasAngularDecorator || classParameters.some(p => !!p.decorators.length)) {
+          // Capture constructor parameters if the class has Angular decorator applied,
+          // or if any of the parameters has decorators applied directly.
           newMembers.push(createCtorParametersClassProperty(
               diagnostics, entityNameToExpression, classParameters, isClosureCompilerEnabled));
         }

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

@@ -535,8 +535,8 @@ class AngularCompilerProgram implements Program {
       // ignore diagnostics that have been collected by the transformer. These are
       // non-significant failures that shouldn't prevent apps from compiling.
       beforeTs.push(getDownlevelDecoratorsTransform(
-          typeChecker, reflectionHost, [], this.isCompilingAngularCore,
-          annotateForClosureCompiler));
+          typeChecker, reflectionHost, [], this.isCompilingAngularCore, annotateForClosureCompiler,
+          /* skipClassDecorators */ false));
     }
 
     if (metadataTransforms.length > 0) {

packages/compiler-cli/test/transformers/downlevel_decorators_transform_spec.ts

@@ -21,6 +21,7 @@ describe('downlevel decorator transform', () => {
   let context: MockAotContext;
   let diagnostics: ts.Diagnostic[];
   let isClosureEnabled: boolean;
+  let skipClassDecorators: boolean;
 
   beforeEach(() => {
     diagnostics = [];
@@ -32,6 +33,7 @@ describe('downlevel decorator transform', () => {
     });
     host = new MockCompilerHost(context);
     isClosureEnabled = false;
+    skipClassDecorators = false;
   });
 
   function transform(
@@ -58,7 +60,7 @@ describe('downlevel decorator transform', () => {
         ...preTransformers,
         getDownlevelDecoratorsTransform(
             program.getTypeChecker(), reflectionHost, diagnostics,
-            /* isCore */ false, isClosureEnabled)
+            /* isCore */ false, isClosureEnabled, skipClassDecorators)
       ]
     };
     let output: string|null = null;
@@ -606,6 +608,97 @@ describe('downlevel decorator transform', () => {
     expect(output).not.toContain('MyDir.ctorParameters');
     expect(output).not.toContain('tslib');
   });
+
+  describe('class decorators skipped', () => {
+    beforeEach(() => skipClassDecorators = true);
+
+    it('should not downlevel Angular class decorators', () => {
+      const {output} = transform(`
+        import {Injectable} from '@angular/core';
+  
+        @Injectable()
+        export class MyService {}
+      `);
+
+      expect(diagnostics.length).toBe(0);
+      expect(output).not.toContain('MyService.decorators');
+      expect(output).toContain(dedent`
+        MyService = tslib_1.__decorate([
+          core_1.Injectable()
+        ], MyService);
+      `);
+    });
+
+    it('should downlevel constructor parameters', () => {
+      const {output} = transform(`
+        import {Injectable} from '@angular/core';
+ 
+        @Injectable()
+        export class InjectClass {}
+ 
+        @Injectable()
+        export class MyService {
+          constructor(dep: InjectClass) {}
+        }
+      `);
+
+      expect(diagnostics.length).toBe(0);
+      expect(output).not.toContain('MyService.decorators');
+      expect(output).toContain('MyService.ctorParameters');
+      expect(output).toContain(dedent`
+        MyService.ctorParameters = () => [
+          { type: InjectClass }
+        ];
+        MyService = tslib_1.__decorate([
+          core_1.Injectable()
+        ], MyService);
+      `);
+    });
+
+    it('should downlevel constructor parameter decorators', () => {
+      const {output} = transform(`
+        import {Injectable, Inject} from '@angular/core';
+ 
+        @Injectable()
+        export class InjectClass {}
+ 
+        @Injectable()
+        export class MyService {
+          constructor(@Inject('test') dep: InjectClass) {}
+        }
+      `);
+
+      expect(diagnostics.length).toBe(0);
+      expect(output).not.toContain('MyService.decorators');
+      expect(output).toContain('MyService.ctorParameters');
+      expect(output).toContain(dedent`
+        MyService.ctorParameters = () => [
+          { type: InjectClass, decorators: [{ type: core_1.Inject, args: ['test',] }] }
+        ];
+        MyService = tslib_1.__decorate([
+          core_1.Injectable()
+        ], MyService);
+      `);
+    });
+
+    it('should downlevel class member Angular decorators', () => {
+      const {output} = transform(`
+        import {Injectable, Input} from '@angular/core';
+ 
+        export class MyService {
+          @Input() disabled: boolean;
+        }
+      `);
+
+      expect(diagnostics.length).toBe(0);
+      expect(output).not.toContain('tslib');
+      expect(output).toContain(dedent`
+        MyService.propDecorators = {
+          disabled: [{ type: core_1.Input }]
+        };
+      `);
+    });
+  });
 });
 
 /** Template string function that can be used to dedent a given string literal. */