From 66e6b932d818a161bd19c03e3e854a22552a7db2 Mon Sep 17 00:00:00 2001 From: Paul Gschwendtner Date: Fri, 12 Jun 2020 00:22:00 +0200 Subject: [PATCH] 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: 401ef71ae5b01be95d124184a0b6936fc453a5d4 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 | 10 +- .../downlevel_decorators_transform.ts | 26 ++++- .../compiler-cli/src/transformers/program.ts | 4 +- .../downlevel_decorators_transform_spec.ts | 95 ++++++++++++++++++- 4 files changed, 122 insertions(+), 13 deletions(-) diff --git a/packages/compiler-cli/src/tooling.ts b/packages/compiler-cli/src/tooling.ts index b986f0df95..01bfed3b80 100644 --- a/packages/compiler-cli/src/tooling.ts +++ b/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 { const typeChecker = program.getTypeChecker(); const reflectionHost = new TypeScriptReflectionHost(typeChecker); return getDownlevelDecoratorsTransform( typeChecker, reflectionHost, [], /* isCore */ false, - /* enableClosureCompiler */ false); + /* enableClosureCompiler */ false, /* skipClassDecorators */ true); } diff --git a/packages/compiler-cli/src/transformers/downlevel_decorators_transform.ts b/packages/compiler-cli/src/transformers/downlevel_decorators_transform.ts index ad19c49d44..aa30ce4da1 100644 --- a/packages/compiler-cli/src/transformers/downlevel_decorators_transform.ts +++ b/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 { + isCore: boolean, isClosureCompilerEnabled: boolean, + skipClassDecorators: boolean): ts.TransformerFactory { return (context: ts.TransformationContext) => { let referencedParameterTypes = new Set(); @@ -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)); } diff --git a/packages/compiler-cli/src/transformers/program.ts b/packages/compiler-cli/src/transformers/program.ts index fa3467e5e9..d8af2c9f62 100644 --- a/packages/compiler-cli/src/transformers/program.ts +++ b/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) { diff --git a/packages/compiler-cli/test/transformers/downlevel_decorators_transform_spec.ts b/packages/compiler-cli/test/transformers/downlevel_decorators_transform_spec.ts index 964ca8079a..d1200b92a0 100644 --- a/packages/compiler-cli/test/transformers/downlevel_decorators_transform_spec.ts +++ b/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. */