diff --git a/packages/compiler-cli/src/transformers/downlevel_decorators_transform.ts b/packages/compiler-cli/src/transformers/downlevel_decorators_transform.ts index 1e27e784fa..a5533f38a1 100644 --- a/packages/compiler-cli/src/transformers/downlevel_decorators_transform.ts +++ b/packages/compiler-cli/src/transformers/downlevel_decorators_transform.ts @@ -8,7 +8,7 @@ import * as ts from 'typescript'; import {Decorator, ReflectionHost} from '../ngtsc/reflection'; -import {isAliasImportDeclaration, patchAliasReferenceResolutionOrDie} from './patch_alias_reference_resolution'; +import {isAliasImportDeclaration, loadIsReferencedAliasDeclarationPatch} from './patch_alias_reference_resolution'; /** * Whether a given decorator should be treated as an Angular decorator. @@ -347,7 +347,12 @@ export function getDownlevelDecoratorsTransform( isCore: boolean, isClosureCompilerEnabled: boolean, skipClassDecorators: boolean): ts.TransformerFactory { return (context: ts.TransformationContext) => { - let 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). @@ -595,12 +600,6 @@ export 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. diff --git a/packages/compiler-cli/src/transformers/patch_alias_reference_resolution.ts b/packages/compiler-cli/src/transformers/patch_alias_reference_resolution.ts index f1ccea42d8..7f632a315d 100644 --- a/packages/compiler-cli/src/transformers/patch_alias_reference_resolution.ts +++ b/packages/compiler-cli/src/transformers/patch_alias_reference_resolution.ts @@ -17,9 +17,12 @@ 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, checkChildren?: boolean): void; + isReferencedAliasDeclaration?(node: ts.Node, ...args: unknown[]): void; + [patchedReferencedAliasesSymbol]?: Set; } /** @@ -40,10 +43,10 @@ interface EmitResolver { * 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. + * 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 @@ -60,32 +63,46 @@ interface EmitResolver { * `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 patchAliasReferenceResolutionOrDie( - context: ts.TransformationContext, referencedAliases: Set): void { +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(); - return; } const emitResolver = context.getEmitResolver(); - const originalReferenceResolution = emitResolver.isReferencedAliasDeclaration; + + // 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 (originalReferenceResolution === undefined) { + if (originalIsReferencedAliasDeclaration === undefined) { throwIncompatibleTransformationContextError(); - return; } + + const referencedAliases = new Set(); emitResolver.isReferencedAliasDeclaration = function(node, ...args) { if (isAliasImportDeclaration(node) && referencedAliases.has(node)) { return true; } - return originalReferenceResolution.call(emitResolver, node, ...args); + return originalIsReferencedAliasDeclaration.call(emitResolver, node, ...args); }; + return emitResolver[patchedReferencedAliasesSymbol] = referencedAliases; } /** @@ -110,7 +127,7 @@ function isTransformationContextWithEmitResolver(context: ts.TransformationConte * declaration reference resolution could not be monkey-patched. The error will * also propose potential solutions that can be applied by developers. */ -function throwIncompatibleTransformationContextError() { +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 ' + 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 28fc1708a9..7aa942901c 100644 --- a/packages/compiler-cli/test/transformers/downlevel_decorators_transform_spec.ts +++ b/packages/compiler-cli/test/transformers/downlevel_decorators_transform_spec.ts @@ -740,6 +740,107 @@ describe('downlevel decorator transform', () => { `); }); }); + + describe('transforming multiple files', () => { + it('should work correctly for multiple files that import distinct declarations', () => { + context.writeFile('foo_service.d.ts', ` + export declare class Foo {}; + `); + context.writeFile('foo.ts', ` + import {Injectable} from '@angular/core'; + import {Foo} from './foo_service'; + + @Injectable() + export class MyService { + constructor(foo: Foo) {} + } + `); + + context.writeFile('bar_service.d.ts', ` + export declare class Bar {}; + `); + context.writeFile('bar.ts', ` + import {Injectable} from '@angular/core'; + import {Bar} from './bar_service'; + + @Injectable() + export class MyService { + constructor(bar: Bar) {} + } + `); + + const {program, transformers} = createProgramWithTransform(['/foo.ts', '/bar.ts']); + program.emit(undefined, undefined, undefined, undefined, transformers); + + expect(context.readFile('/foo.js')).toContain(`import { Foo } from './foo_service';`); + expect(context.readFile('/bar.js')).toContain(`import { Bar } from './bar_service';`); + }); + + it('should not result in a stack overflow for a large number of files', () => { + // The decorators transform used to patch `ts.EmitResolver.isReferencedAliasDeclaration` + // repeatedly for each source file in the program, causing a stack overflow once a large + // number of source files was reached. This test verifies that emit succeeds even when there's + // lots of source files. See https://github.com/angular/angular/issues/40276. + context.writeFile('foo.d.ts', ` + export declare class Foo {}; + `); + + // A somewhat minimal number of source files that used to trigger a stack overflow. + const numberOfTestFiles = 6500; + const files: string[] = []; + for (let i = 0; i < numberOfTestFiles; i++) { + const file = `/${i}.ts`; + files.push(file); + context.writeFile(file, ` + import {Injectable} from '@angular/core'; + import {Foo} from './foo'; + + @Injectable() + export class MyService { + constructor(foo: Foo) {} + } + `); + } + + const {program, transformers} = createProgramWithTransform(files); + + let written = 0; + program.emit(undefined, (fileName, outputText) => { + written++; + + // The below assertion throws an explicit error instead of using a Jasmine expectation, + // as we want to abort on the first failure, if any. This avoids as many as `numberOfFiles` + // expectation failures, which would bloat the test output. + if (!outputText.includes(`import { Foo } from './foo';`)) { + throw new Error(`Transform failed to preserve the import in ${fileName}:\n${outputText}`); + } + }, undefined, undefined, transformers); + expect(written).toBe(numberOfTestFiles); + }); + + function createProgramWithTransform(files: string[]) { + const program = ts.createProgram( + files, { + moduleResolution: ts.ModuleResolutionKind.NodeJs, + importHelpers: true, + lib: [], + module: ts.ModuleKind.ESNext, + target: ts.ScriptTarget.Latest, + declaration: false, + experimentalDecorators: true, + emitDecoratorMetadata: false, + }, + host); + const typeChecker = program.getTypeChecker(); + const reflectionHost = new TypeScriptReflectionHost(typeChecker); + const transformers: ts.CustomTransformers = { + before: [getDownlevelDecoratorsTransform( + program.getTypeChecker(), reflectionHost, diagnostics, + /* isCore */ false, isClosureEnabled, skipClassDecorators)] + }; + return {program, transformers}; + } + }); }); /** Template string function that can be used to dedent a given string literal. */