From c1242868557a95d79a7bb5a108153496e1f964c3 Mon Sep 17 00:00:00 2001 From: Ahn Date: Thu, 14 Jan 2021 12:33:31 +0100 Subject: [PATCH] refactor(transformers): update downlevel ctor transformer (#730) Follow the fix https://github.com/angular/angular/pull/40374 --- src/transformers/downlevel-ctor.ts | 139 ++--------------- .../patch-alias-reference-resolution.ts | 140 ++++++++++++++++++ 2 files changed, 152 insertions(+), 127 deletions(-) create mode 100644 src/transformers/patch-alias-reference-resolution.ts diff --git a/src/transformers/downlevel-ctor.ts b/src/transformers/downlevel-ctor.ts index c8de798fa2..25900c9ad0 100644 --- a/src/transformers/downlevel-ctor.ts +++ b/src/transformers/downlevel-ctor.ts @@ -11,107 +11,7 @@ import { Decorator, ReflectionHost, TypeScriptReflectionHost } from '@angular/compiler-cli/src/ngtsc/reflection'; import ts from 'typescript'; -/** - * Describes a TypeScript transformation context with the internal emit - * resolver exposed. There are requests upstream in TypeScript to expose - * that as public API: https://github.com/microsoft/TypeScript/issues/17516.. - */ -interface TransformationContextWithResolver extends ts.TransformationContext { - getEmitResolver: () => EmitResolver; -} - -/** Describes a subset of the TypeScript internal emit resolver. */ -interface EmitResolver { - isReferencedAliasDeclaration?(node: ts.Node, checkChildren?: boolean): void; -} - -/** - * Patches the alias declaration reference resolution for a given transformation context - * so that TypeScript knows about the specified alias declarations being referenced. - * - * This exists because TypeScript performs analysis of import usage before transformers - * run and doesn't refresh its state after transformations. This means that imports - * for symbols used as constructor types are elided due to their original type-only usage. - * - * In reality though, since we downlevel decorators and constructor parameters, we want - * these symbols to be retained in the JavaScript output as they will be used as values - * at runtime. We can instruct TypeScript to preserve imports for such identifiers by - * creating a mutable clone of a given import specifier/clause or namespace, but that - * has the downside of preserving the full import in the JS output. See: - * https://github.com/microsoft/TypeScript/blob/3eaa7c65f6f076a08a5f7f1946fd0df7c7430259/src/compiler/transformers/ts.ts#L242-L250. - * - * This is a trick the CLI used in the past for constructor parameter downleveling in JIT: - * https://github.com/angular/angular-cli/blob/b3f84cc5184337666ce61c07b7b9df418030106f/packages/ngtools/webpack/src/transformers/ctor-parameters.ts#L323-L325 - * The trick is not ideal though as it preserves the full import (as outlined before), and it - * results in a slow-down due to the type checker being involved multiple times. The CLI - * worked around this import preserving issue by having another complex post-process step that - * detects and elides unused imports. Note that these unused imports could cause unused chunks - * being generated by Webpack if the application or library is not marked as side-effect free. - * - * This is not ideal though, as we basically re-implement the complex import usage resolution - * from TypeScript. We can do better by letting TypeScript do the import eliding, but providing - * information about the alias declarations (e.g. import specifiers) that should not be elided - * because they are actually referenced (as they will now appear in static properties). - * - * More information about these limitations with transformers can be found in: - * 1. https://github.com/Microsoft/TypeScript/issues/17552. - * 2. https://github.com/microsoft/TypeScript/issues/17516. - * 3. https://github.com/angular/tsickle/issues/635. - * - * The patch we apply to tell TypeScript about actual referenced aliases (i.e. imported symbols), - * matches conceptually with the logic that runs internally in TypeScript when the - * `emitDecoratorMetadata` flag is enabled. TypeScript basically surfaces the same problem and - * solves it conceptually the same way, but obviously doesn't need to access an `@internal` API. - * - * See below. Note that this uses sourcegraph as the TypeScript checker file doesn't display on - * Github. - * https://sourcegraph.com/github.com/microsoft/TypeScript@3eaa7c65f6f076a08a5f7f1946fd0df7c7430259/-/blob/src/compiler/checker.ts#L31219-31257 - */ -function patchAliasReferenceResolutionOrDie( - context: ts.TransformationContext, - referencedAliases: Set, -): void { - // If the `getEmitResolver` method is not available, TS most likely changed the - // internal structure of the transformation context. We will abort gracefully. - if (!isTransformationContextWithEmitResolver(context)) { - throwIncompatibleTransformationContextError(); - - return; - } - const emitResolver = context.getEmitResolver(); - // eslint-disable-next-line @typescript-eslint/unbound-method - const originalReferenceResolution = emitResolver.isReferencedAliasDeclaration; - // If the emit resolver does not have a function called `isReferencedAliasDeclaration`, then - // we abort gracefully as most likely TS changed the internal structure of the emit resolver. - if (originalReferenceResolution === undefined) { - throwIncompatibleTransformationContextError(); - - return; - } - emitResolver.isReferencedAliasDeclaration = function (node, ...args) { - if (isAliasImportDeclaration(node) && referencedAliases.has(node)) { - return true; - } - - return originalReferenceResolution.call(emitResolver, node, ...args); - }; -} - -/** Whether the transformation context exposes its emit resolver. */ -function isTransformationContextWithEmitResolver( - context: ts.TransformationContext, -): context is TransformationContextWithResolver { - return (context as Partial).getEmitResolver !== undefined; -} - -/** - * Gets whether a given node corresponds to an import alias declaration. Alias - * declarations can be import specifiers, namespace imports or import clauses - * as these do not declare an actual symbol but just point to a target declaration. - */ -function isAliasImportDeclaration(node: ts.Node): node is ts.ImportSpecifier | ts.NamespaceImport | ts.ImportClause { - return ts.isImportSpecifier(node) || ts.isNamespaceImport(node) || ts.isImportClause(node); -} +import { isAliasImportDeclaration, loadIsReferencedAliasDeclarationPatch } from './patch-alias-reference-resolution'; /** * Whether a given decorator should be treated as an Angular decorator. @@ -532,7 +432,12 @@ function getDownlevelDecoratorsTransform( skipClassDecorators: boolean, ): ts.TransformerFactory { return (context: ts.TransformationContext) => { - const referencedParameterTypes = new Set(); + // Ensure that referenced type symbols are not elided by TypeScript. Imports for + // such parameter type symbols previously could be type-only, but now might be also + // used in the `ctorParameters` static property as a value. We want to make sure + // that TypeScript does not elide imports for such type references. Read more + // about this in the description for `loadIsReferencedAliasDeclarationPatch`. + const referencedParameterTypes = loadIsReferencedAliasDeclarationPatch(context); /** * Converts an EntityName (from a type annotation) to an expression (accessing a value). @@ -627,7 +532,7 @@ function getDownlevelDecoratorsTransform( return [undefined, element, []]; } - const name = element.name.text; + const name = (element.name as ts.Identifier).text; const mutable = ts.getMutableClone(element); // eslint-disable-next-line @typescript-eslint/no-explicit-any (mutable as any).decorators = decoratorsToKeep.length @@ -663,14 +568,16 @@ function getDownlevelDecoratorsTransform( decoratorsToKeep.push(decoratorNode); continue; } - paramInfo.decorators.push(decoratorNode); + // eslint-disable-next-line @typescript-eslint/no-non-null-assertion + paramInfo!.decorators.push(decoratorNode); } if (param.type) { // param has a type provided, e.g. "foo: Bar". // The type will be emitted as a value expression in entityNameToExpression, which takes // care not to emit anything for types that cannot be expressed as a value (e.g. // interfaces). - paramInfo.type = param.type; + // eslint-disable-next-line @typescript-eslint/no-non-null-assertion + paramInfo!.type = param.type; } parametersInfo.push(paramInfo); const newParam = ts.updateParameter( @@ -815,13 +722,6 @@ function getDownlevelDecoratorsTransform( } return (sf: ts.SourceFile) => { - // Ensure that referenced type symbols are not elided by TypeScript. Imports for - // such parameter type symbols previously could be type-only, but now might be also - // used in the `ctorParameters` static property as a value. We want to make sure - // that TypeScript does not elide imports for such type references. Read more - // about this in the description for `patchAliasReferenceResolution`. - patchAliasReferenceResolutionOrDie(context, referencedParameterTypes); - // Downlevel decorators and constructor parameter types. We will keep track of all // referenced constructor parameter types so that we can instruct TypeScript to // not elide their imports if they previously were only type-only. @@ -830,21 +730,6 @@ function getDownlevelDecoratorsTransform( }; } -/** - * Throws an error about an incompatible TypeScript version for which the alias - * declaration reference resolution could not be monkey-patched. The error will - * also propose potential solutions that can be applied by developers. - */ -function throwIncompatibleTransformationContextError() { - throw Error( - 'Unable to downlevel Angular decorators due to an incompatible TypeScript ' + - 'version.\nIf you recently updated TypeScript and this issue surfaces now, consider ' + - 'downgrading.\n\n' + - 'Please report an issue on the Angular repositories when this issue ' + - 'surfaces and you are using a supposedly compatible TypeScript version.', - ); -} - /** * 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 diff --git a/src/transformers/patch-alias-reference-resolution.ts b/src/transformers/patch-alias-reference-resolution.ts new file mode 100644 index 0000000000..988b6b98ed --- /dev/null +++ b/src/transformers/patch-alias-reference-resolution.ts @@ -0,0 +1,140 @@ +/** + * @license + * Copyright Google LLC All Rights Reserved. + * + * Use of this source code is governed by an MIT-style license that can be + * found in the LICENSE file at https://angular.io/license + */ + +import ts from 'typescript'; + +/** + * Describes a TypeScript transformation context with the internal emit + * resolver exposed. There are requests upstream in TypeScript to expose + * that as public API: https://github.com/microsoft/TypeScript/issues/17516.. + */ +interface TransformationContextWithResolver extends ts.TransformationContext { + getEmitResolver: () => EmitResolver; +} + +const patchedReferencedAliasesSymbol = Symbol('patchedReferencedAliases'); + +/** Describes a subset of the TypeScript internal emit resolver. */ +interface EmitResolver { + isReferencedAliasDeclaration?(node: ts.Node, ...args: unknown[]): void; + [patchedReferencedAliasesSymbol]?: Set; +} + +/** + * Patches the alias declaration reference resolution for a given transformation context + * so that TypeScript knows about the specified alias declarations being referenced. + * + * This exists because TypeScript performs analysis of import usage before transformers + * run and doesn't refresh its state after transformations. This means that imports + * for symbols used as constructor types are elided due to their original type-only usage. + * + * In reality though, since we downlevel decorators and constructor parameters, we want + * these symbols to be retained in the JavaScript output as they will be used as values + * at runtime. We can instruct TypeScript to preserve imports for such identifiers by + * creating a mutable clone of a given import specifier/clause or namespace, but that + * has the downside of preserving the full import in the JS output. See: + * https://github.com/microsoft/TypeScript/blob/3eaa7c65f6f076a08a5f7f1946fd0df7c7430259/src/compiler/transformers/ts.ts#L242-L250. + * + * This is a trick the CLI used in the past for constructor parameter downleveling in JIT: + * https://github.com/angular/angular-cli/blob/b3f84cc5184337666ce61c07b7b9df418030106f/packages/ngtools/webpack/src/transformers/ctor-parameters.ts#L323-L325 + * The trick is not ideal though as it preserves the full import (as outlined before), and it + * results in a slow-down due to the type checker being involved multiple times. The CLI worked + * around this import preserving issue by having another complex post-process step that detects and + * elides unused imports. Note that these unused imports could cause unused chunks being generated + * by Webpack if the application or library is not marked as side-effect free. + * + * This is not ideal though, as we basically re-implement the complex import usage resolution + * from TypeScript. We can do better by letting TypeScript do the import eliding, but providing + * information about the alias declarations (e.g. import specifiers) that should not be elided + * because they are actually referenced (as they will now appear in static properties). + * + * More information about these limitations with transformers can be found in: + * 1. https://github.com/Microsoft/TypeScript/issues/17552. + * 2. https://github.com/microsoft/TypeScript/issues/17516. + * 3. https://github.com/angular/tsickle/issues/635. + * + * The patch we apply to tell TypeScript about actual referenced aliases (i.e. imported symbols), + * matches conceptually with the logic that runs internally in TypeScript when the + * `emitDecoratorMetadata` flag is enabled. TypeScript basically surfaces the same problem and + * solves it conceptually the same way, but obviously doesn't need to access an `@internal` API. + * + * The set that is returned by this function is meant to be filled with import declaration nodes + * that have been referenced in a value-position by the transform, such the the installed patch can + * ensure that those import declarations are not elided. + * + * See below. Note that this uses sourcegraph as the TypeScript checker file doesn't display on + * Github. + * https://sourcegraph.com/github.com/microsoft/TypeScript@3eaa7c65f6f076a08a5f7f1946fd0df7c7430259/-/blob/src/compiler/checker.ts#L31219-31257 + */ +export function loadIsReferencedAliasDeclarationPatch(context: ts.TransformationContext): Set { + // If the `getEmitResolver` method is not available, TS most likely changed the + // internal structure of the transformation context. We will abort gracefully. + if (!isTransformationContextWithEmitResolver(context)) { + throwIncompatibleTransformationContextError(); + } + const emitResolver = context.getEmitResolver(); + + // The emit resolver may have been patched already, in which case we return the set of referenced + // aliases that was created when the patch was first applied. + // See https://github.com/angular/angular/issues/40276. + const existingReferencedAliases = emitResolver[patchedReferencedAliasesSymbol]; + if (existingReferencedAliases !== undefined) { + return existingReferencedAliases; + } + + const originalIsReferencedAliasDeclaration = emitResolver.isReferencedAliasDeclaration; + // If the emit resolver does not have a function called `isReferencedAliasDeclaration`, then + // we abort gracefully as most likely TS changed the internal structure of the emit resolver. + if (originalIsReferencedAliasDeclaration === undefined) { + throwIncompatibleTransformationContextError(); + } + + const referencedAliases = new Set(); + emitResolver.isReferencedAliasDeclaration = function (node, ...args) { + if (isAliasImportDeclaration(node) && referencedAliases.has(node)) { + return true; + } + + return originalIsReferencedAliasDeclaration.call(emitResolver, node, ...args); + }; + + return (emitResolver[patchedReferencedAliasesSymbol] = referencedAliases); +} + +/** + * Gets whether a given node corresponds to an import alias declaration. Alias + * declarations can be import specifiers, namespace imports or import clauses + * as these do not declare an actual symbol but just point to a target declaration. + */ +export function isAliasImportDeclaration( + node: ts.Node, +): node is ts.ImportSpecifier | ts.NamespaceImport | ts.ImportClause { + return ts.isImportSpecifier(node) || ts.isNamespaceImport(node) || ts.isImportClause(node); +} + +/** Whether the transformation context exposes its emit resolver. */ +function isTransformationContextWithEmitResolver( + context: ts.TransformationContext, +): context is TransformationContextWithResolver { + return (context as Partial).getEmitResolver !== undefined; +} + +/** + * Throws an error about an incompatible TypeScript version for which the alias + * declaration reference resolution could not be monkey-patched. The error will + * also propose potential solutions that can be applied by developers. + */ +function throwIncompatibleTransformationContextError(): never { + throw Error( + 'Unable to downlevel Angular decorators due to an incompatible TypeScript ' + + 'version.\nIf you recently updated TypeScript and this issue surfaces now, consider ' + + 'downgrading.\n\n' + + 'Please report an issue on the Angular repositories when this issue ' + + 'surfaces and you are using a supposedly compatible TypeScript version.', + ); +}